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

False positive caused by cross-crate pub use ... as .... #573

Open
jswrenn opened this issue Oct 30, 2023 · 7 comments
Open

False positive caused by cross-crate pub use ... as .... #573

jswrenn opened this issue Oct 30, 2023 · 7 comments
Labels
C-bug Category: doesn't meet expectations

Comments

@jswrenn
Copy link

jswrenn commented Oct 30, 2023

Steps to reproduce the bug with the above code

In google/zerocopy@10cb29b (in PR google/zerocopy#562), we are attempting to simplify our re-export of core (which we do so that our macros work across a variety of Rust editions and versions). Previously, we defined a module core_reexport that re-exported a sub-set of core. Then, we realized we could just use-rename core as core_reexport:

-pub mod core_reexport {
-    pub use core::*;
-}
+pub use core as core_reexport;

Actual Behaviour

This causes a semver check failure:

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.24.1/src/lints/module_missing.ron

Expected Behaviour

I think this isn't a semver breaking change.

Generated System Information

Software version

cargo-semver-checks 0.24.2

Operating system

Linux 5.13.0-1031-aws

Command-line

/home/ubuntu/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.73.0 (9c4383fb5 2023-08-26)

Compile time information

  • Profile: release
  • Target triple: x86_64-unknown-linux-gnu
  • Family: unix
  • OS: linux
  • Architecture: x86_64
  • Pointer width: 64
  • Endian: little
  • CPU features: fxsr,sse,sse2
  • Host: x86_64-unknown-linux-gnu

Build Configuration

No response

Additional Context

No response

@jswrenn jswrenn added the C-bug Category: doesn't meet expectations label Oct 30, 2023
@obi1kenobi
Copy link
Owner

I agree, this looks like a false positive. Sorry about that! I'm looking into it. I'm also in the middle of moving to a new apartment, so it might take me a few days longer than usual 😅

@obi1kenobi
Copy link
Owner

Ah wait, the core there is Rust's built-in core, right?

cargo-semver-checks can currently only do local analysis, which means that it doesn't see items from foreign crates at all. This is due to several reasons. The biggest one is that there's currently no reliable way to determine which specific (possibly among multiple major versions) other crate an item came from: rust-lang/compiler-team#635

Once that's resolved, we're hoping for an update to the rustdoc JSON format to include the identifiers mentioned in that link, and we'll then need to generate multiple crates' rustdoc JSON (possibly on-the-fly) and combine them in the query engine. In the meantime, cargo-semver-checks is oblivious to foreign crates' items, including their local re-exports.

So long story short, both "bug" and "works as intended" at the same time for the moment 😅 Resolving this will require a lot of work (not just from me but also from folks working on rustdoc JSON and the folks in the linked issue) so the best way to make this work happen faster is for someone on the zerocopy team to take a look at our GitHub Sponsors accounts :)

@obi1kenobi obi1kenobi changed the title Possible false positive caused by pub use ... as .... False positive caused by cross-crate pub use ... as .... Oct 31, 2023
@joshlf
Copy link

joshlf commented Nov 2, 2023

Makes sense! Is there any way to instruct cargo-semver-checks to skip a particular symbol? Either a CLI flag or an #[allow(...)] or something?

@obi1kenobi
Copy link
Owner

Unfortunately not at the moment. CLI flag per symbol is really clunky, and #[allow(...)] would require cargo-semver-checks to either be a part of Rust itself or be a dependency of the program. We have an open issue to support configuring it via package.metadata, for which I'd happily be a mentor if someone wants to take it up: #537

Thankfully, since cargo-semver-checks only compares to the previous release, this kind of problem disappears when the next release is made. If you make a new release (ignoring cargo-semver-checks), that release becomes the baseline for future checks and you won't see this false-positive anymore.

@joshlf
Copy link

joshlf commented Nov 2, 2023

Okay gotcha, thanks! It's not particularly high priority for us, but good to know there's an option if we decide to put more engineering effort towards it!

@obi1kenobi
Copy link
Owner

In general if you decide to allocate some engineering effort toward cargo-semver-checks, I'd be happy to chat about how to get maximum bang for the buck and help folks get up to speed. It's a big project and I could certainly use all the help I can get!

@joshlf
Copy link

joshlf commented Nov 2, 2023

Sounds good! Realistically, I think it's likely that we'll have higher priorities for at least the next few months, but if/when priorities change or we have more engineering hours to throw around, cargo-semver-checks will definitely be on the list!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations
Projects
None yet
Development

No branches or pull requests

3 participants