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

Generating rustdoc with --all-features exposes APIs in an unstable feature #321

Closed
sdroege opened this issue Jan 24, 2023 · 13 comments · Fixed by #426
Closed

Generating rustdoc with --all-features exposes APIs in an unstable feature #321

sdroege opened this issue Jan 24, 2023 · 13 comments · Fixed by #426
Labels
C-enhancement Category: raise the bar on expectations

Comments

@sdroege
Copy link

sdroege commented Jan 24, 2023

Steps to reproduce the bug with the above code

Running on https://github.com/sdroege/ebur128 gives the following error

Actual Behaviour

$ cargo semver-checks check-release
    Updating index
     Parsing ebur128 v0.1.7 (current)
     Parsing ebur128 v0.1.7 (baseline)
    Checking ebur128 v0.1.7 -> v0.1.7 (no change)
   Completed [   0.117s] 40 checks; 39 passed, 1 failed, 0 unnecessary

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.16.1/src/queries/struct_missing.ron

Failed in:
  struct ebur128::interp::Interp2F, previously in file ~/.cargo/registry/src/github.com-1ecc6299db9ec823/ebur128-0.1.7/src/interp.rs:161
  struct ebur128::interp::Interp4F, previously in file ~/.cargo/registry/src/github.com-1ecc6299db9ec823/ebur128-0.1.7/src/interp.rs:162
       Final [   0.118s] semver requires new major version: 1 major and 0 minor checks failed

Expected Behaviour

This should pass because the type is not exported from the crate

Generated System Information

Software version

cargo-semver-checks 0.16.1

Operating system

Linux 6.0.18-300.fc37.x86_64

Command-line

~/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.66.0 (d65d197ad 2022-11-15)

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

@sdroege sdroege added the C-bug Category: doesn't meet expectations label Jan 24, 2023
@obi1kenobi
Copy link
Owner

Sorry about the false-positive error, and thanks for reporting it.

I'm already in the middle of a rework of our import-handling system to make renaming re-exports and glob re-exports work, so I'll make sure this case is correctly handled as part of that as well: obi1kenobi/trustfall-rustdoc-adapter#34

We'll also add your crate to our list of crates that we use for integration tests, so that we never have this issue again.

@obi1kenobi obi1kenobi added the A-lint Area: new or existing lint label Jan 24, 2023
@sdroege
Copy link
Author

sdroege commented Jan 25, 2023

No worries and thanks for the fast response :)

@obi1kenobi
Copy link
Owner

obi1kenobi commented Jan 28, 2023

I just had a chance to look into this, and unfortunately the ebur128::interp::Interp2F and ebur128::interp::Interp4F types are detected as being public API because of this feature-gated public re-export.

cargo-semver-checks runs API checks with --all-features enabled. This is because cargo doesn't yet support unstable features (rust-lang/cargo#10881) so all of a crate's features are considered public and stable API.

This means that for rustdoc and cargo-semver-checks, those types are part of the public API, so this is not a false-positive.

#315 is describes the feature for only enabling a user-specified set of features while checking. If you agree that's the underlying cause here, I'd like to consolidate this issue into that one. Is that okay?

In the meantime, a workaround is available: cargo-semver-checks uses RUSTFLAGS="-Z unstable-options --document-private-items --document-hidden-items --output-format=json" as an env var when running cargo doc to generate rustdoc JSON (which is unstable, and requires nightly Rust barring some hackery I wouldn't recommend here. You should be able to use that invocation to manually generate rustdoc JSON that only enables your selected features, and then pass the generated JSON files to cargo-semver-checks via the --baseline-rustdoc and --current-rustdoc options. Checking will then happen normally and should work as expected only over the features you selected.

@obi1kenobi obi1kenobi removed A-lint Area: new or existing lint C-bug Category: doesn't meet expectations labels Jan 28, 2023
@obi1kenobi obi1kenobi changed the title Considers pub struct in a pub(crate) module as public API Generating rustdoc with --all-features exposes APIs in an unstable feature Jan 28, 2023
@obi1kenobi obi1kenobi added the C-enhancement Category: raise the bar on expectations label Jan 28, 2023
@sdroege
Copy link
Author

sdroege commented Jan 28, 2023

Ah I see, that makes a lot of sense. These features are "internal" features that are only enabled for testing purposes. I saw that other crates are prefixing such features with underscores. Maybe for the time being that could also be used as a hint to cargo-semver-checks that this feature should not be activated?

Otherwise yes, this would be a duplicate of #315 and/or the missing feature in cargo.

@obi1kenobi
Copy link
Owner

If we weren't looking to merge cargo-semver-checks into cargo soonish, I'd feel absolutely fine with adding the "ignore features starting with __" heuristic.

But with the merge coming up, I don't feel comfortable making that kind of decision unilaterally without consulting the cargo maintainers first. @epage what do you think?

@epage
Copy link
Collaborator

epage commented Jan 28, 2023

When we last talked about _ prefixed features (since rustdoc ignores them) we decided we favored making the relationship more explicit, see rust-lang/cargo#10882 (which is different than the aforementioned issue)

However, that feature isn't implemented yet and there is a gap between semver-checks being merged, so it might be ok to implement support for it now and then we rip it out pre-merge (adding that removal to the merge task list).

I've gone ahead and added the two related cargo issues to the "nice to have" list in #61. @obi1kenobi do you know of any other relevant cargo improvements?

@obi1kenobi
Copy link
Owner

It would be great if cargo-semver-checks could somehow get cargo to generate rustdoc for a yanked version. Right now, generating rustdoc for a crate version requires making a lockfile, which isn't allowed for yanked versions (relevant: #275, rust-lang/cargo#4225).

This is a bit unfortunate because e.g. it's hampering @tonowak's research into how often crates get yanked due to semver violations and for which reasons — having this real-world data would help us prioritize which lints to build next.

@obi1kenobi
Copy link
Owner

obi1kenobi commented Jan 31, 2023

Related: #181. Perhaps worth closing this issue in favor of that one, I attempted to summarize the discussion here: #181 (comment)

@kangalio
Copy link

kangalio commented Feb 5, 2023

I've hit this on the serenity crate. In the latest version, a feature was removed and replaced with a compile_error!(): serenity-rs/serenity#2246. This (among other reasons) is why serenity has its own full feature, which enables all other features that are currently usable. Because cargo-semver-checks is hardcoded to --all-features, it also hits this compile_error!().

@obi1kenobi
Copy link
Owner

Thanks for bringing this up.

Out of curiosity, why not remove the feature altogether? It's already non-additive, which cargo doesn't like, and both removing it and leaving compile_error!() there would cause a compile error. I've never had to deprecate and remove a feature before so I just haven't thought through the tradeoffs and I'm hoping to learn something :)

@kangalio
Copy link

kangalio commented Feb 5, 2023

Valid point. I added the compile_error to point migrating users directly to the new API, so they don't have to search around in the changelog. But I guess I could also add a feature-gated deprecation warning to a part of the API that the user definitely calls, so the migration message is seen

EDIT: Hm... Is this really better? kangalio/serenity@7a3f1c1

@obi1kenobi
Copy link
Owner

I think that would probably make both users and tooling happier!

@obi1kenobi
Copy link
Owner

This will be part of the next release, sometime in the next week or so — just need to finish the docs for the new feature flags. Sorry for the wait!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants