Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inferno 0.11.8 removed sealed CollapsePrivate trait from public API #264

Closed
str4d opened this issue Sep 28, 2022 · 1 comment · Fixed by #265
Closed

inferno 0.11.8 removed sealed CollapsePrivate trait from public API #264

str4d opened this issue Sep 28, 2022 · 1 comment · Fixed by #265

Comments

@str4d
Copy link
Contributor

str4d commented Sep 28, 2022

We use pprof for benchmark profiling, which depends on inferno. The most recent update caused this failure in our CI, while inferno 0.11.7 compiles fine:

❯ rustc --version
rustc 1.56.1 (59eed8a2a 2021-11-01)

❯ cargo update -p inferno --precise 0.11.7
    Updating crates.io index
    Updating inferno v0.11.8 -> v0.11.7

❯ cargo check --tests
    Checking tinytemplate v1.2.1
    Checking inferno v0.11.7
    Checking criterion v0.3.6
    Checking pprof v0.8.0
    Checking halo2_proofs v0.2.0 (/path/to/halo2/halo2_proofs)
    Checking halo2_gadgets v0.2.0 (/path/to/halo2/halo2_gadgets)
    Finished dev [unoptimized + debuginfo] target(s) in 4.35s

❯ cargo update -p inferno
    Updating crates.io index
    Updating inferno v0.11.7 -> v0.11.8

❯ cargo check --tests
    Checking inferno v0.11.8
error[E0445]: crate-private trait `CollapsePrivate` in public interface
   --> /path/to/.cargo/registry/src/github.com-1ecc6299db9ec823/inferno-0.11.8/src/collapse/mod.rs:119:1
    |
119 | / impl<T> Collapse for T
120 | | where
121 | |     T: CollapsePrivate,
122 | | {
...   |
133 | |     }
134 | | }
    | |_^ can't leak crate-private trait
    |
   ::: /path/to/.cargo/registry/src/github.com-1ecc6299db9ec823/inferno-0.11.8/src/collapse/common.rs:54:1
    |
54  |   pub(crate) trait CollapsePrivate: Send + Sized {
    |   ---------------------------------------------- `CollapsePrivate` declared as crate-private

For more information about this error, try `rustc --explain E0445`.
error: could not compile `inferno` due to previous error
@str4d
Copy link
Contributor Author

str4d commented Sep 28, 2022

Looks like this was caused by b8f9260 which reduced the public API surface. Sadly blanket trait impls on public traits are themselves public, regardless of the visibility of the bounds.

The solution here (as described in rust-lang/rust#34537 among other places) is to treat CollapsePrivate as a sealed trait, where it is public but in a private module (and thus cannot be implemented by downstream library users because it cannot be named). It looks like inferno::collapse::common is crate-private, so reverting the s/pub/pub(crate) should be sufficient to resolve this issue.

@str4d str4d changed the title inferno 0.11.8 leaks crate-private trait CollapsePrivate inferno 0.11.8 removed sealed traits from public API Sep 28, 2022
str4d added a commit to str4d/inferno that referenced this issue Sep 28, 2022
It needs to be in the public API due to the blanket implementation of
`Collapse` for `T: CollapsePrivate`. The `inferno::collapse::common`
module is crate-private, ensuring that `CollapsePrivate` and
`Occurrences` cannot be named by downstream library users.

Closes jonhoo#264.
@str4d str4d changed the title inferno 0.11.8 removed sealed traits from public API inferno 0.11.8 removed sealed CollapsePrivate trait from public API Sep 28, 2022
bcmyers pushed a commit to bcmyers/inferno that referenced this issue Oct 10, 2022
It needs to be in the public API due to the blanket implementation of
`Collapse` for `T: CollapsePrivate`. The `inferno::collapse::common`
module is crate-private, ensuring that `CollapsePrivate` and
`Occurrences` cannot be named by downstream library users.

Closes jonhoo#264.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant