-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
resolve: Partially convert ambiguous_glob_imports lint into a hard error
#149195
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
base: main
Are you sure you want to change the base?
Conversation
|
Some changes occurred in compiler/rustc_codegen_gcc |
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
resolve: Convert `ambiguous_glob_imports` lint into a hard error
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
This comment was marked as resolved.
This comment was marked as resolved.
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
@petrochenkov: seems like a lot of regressions :( Should I review this? |
Not yet. |
|
Reminder, once the PR becomes ready for a review, use |
|
Correct me if I'm wrong on this one @rustbot author |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot p=11 |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
7e2e10d to
6d83649
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6d83649 to
ab8342a
Compare
|
These commits modify Please ensure that if you've changed the output:
|
ab8342a to
682fe41
Compare
This comment has been minimized.
This comment has been minimized.
682fe41 to
ec57a75
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
resolve: Partially convert `ambiguous_glob_imports` lint into a hard error
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no opinion one way or the other on this PR, just noting that I found the change to rustdoc JSON surprising and felt it might be a correctness risk to downstream tools.
|
|
||
| //@ set m1 = "$.index[?(@.name == 'm1' && @.inner.module)].id" | ||
| //@ is "$.index[?(@.name == 'm1')].inner.module.items" [] | ||
| //@ is "$.index[?(@.name == 'm1')].inner.module.items" [0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a surprising change and possibly like a regression to me.
A few things I don't quite understand:
- why
m1claims to be stripped but still returns the item - why
m2has different behavior, not returning its item despite looking identical tom1
Based on the current rustdoc here (and ignoring that we run with private and hidden items documented), cargo-semver-checks and similar tools would determine that m1::f is pub and importable without ambiguity, since m2::f is not shown and therefore there's nothing to indicate ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that needed to go together with #147984 actually, independently from this PR.
Ambiguous glob imports can now be used from other crates (even if under a lint), so effective visibilities tables need to mark them as reachable (otherwise you get various kinds of issues, e.g. MIR for those items not being encoded).
Rustdoc uses the same effective visibility tables too, so the new items appear in rustdoc output as well.
ec57a75 to
a14b1f8
Compare
|
@bors try |
resolve: Partially convert `ambiguous_glob_imports` lint into a hard error
|
⌛ Trying commit a14b1f8 with merge aea72be… (The previously running try build was automatically cancelled.) To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/20396523752 |
I'm tired of this logic interfering with any attempts to fix or refactor glob imports.
The lint was implemented 2.3 years ago, and made deny-by-default and report-in-dependencies 4 months ago.
The removal is a bit over-eager because of one piece that wasn't implemented correctly (#113099 (comment)), but I want to look at the crater results.
Part of #114095.