-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Do not suggest importing inaccessible items #88838
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
r? @JohnTitor (randomly picked someone from wg-diagnostics, feel free to re-assign) |
Seems good but I'm not part of t-compiler, r? @estebank could you take a look? |
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 think the changes are an improvement already, but I have a few ideas on how we could improve them further. I'm r=me for the code changes as they are, but I would like to continue tweaking them so the output is nicer. Would you have time to make them perfect, @FabianWolff?
Thanks for the review! Yes, I'll try to implement your suggestions (though I won't get around to it today). |
☔ The latest upstream changes (presumably #89037) made this pull request unmergeable. Please resolve the merge conflicts. |
One last thing: if the |
434c694
to
82e54ab
Compare
@estebank I have now implemented your suggestions! I did not get around to your proposal in #88838 (comment); instead, I have opened #89057 as a follow-up, as you have suggested (I might come back to it if I have time in the next few days). |
☔ The latest upstream changes (presumably #89262) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
r=me after rebasing
I'm curious whether this will fix #88636 too |
@crlf0710 it's very likely it will, good catch! |
82e54ab
to
750018e
Compare
@estebank Rebased & ready. |
@bors r+ |
📌 Commit 750018e has been approved by |
⌛ Testing commit 750018e with merge 93468f041e3a0ab409c395d2cbf7a790e4c8ff8c... |
💥 Test timed out |
@bors retry |
…arth Rollup of 7 pull requests Successful merges: - rust-lang#88838 (Do not suggest importing inaccessible items) - rust-lang#89251 (Detect when negative literal indices are used and suggest appropriate code) - rust-lang#89321 (Rebase resume argument projections during state transform) - rust-lang#89327 (Pick one possible lifetime in case there are multiple choices) - rust-lang#89344 (Cleanup lower_generics_mut and make span be the bound itself) - rust-lang#89397 (Update `llvm` submodule to fix function name mangling on x86 Windows) - rust-lang#89412 (Add regression test for issues rust-lang#88969 and rust-lang#89119 ) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Do not mention a reexported item if it's private Fixes rust-lang#90113 The _actual_ regression was introduced in rust-lang#73652, then rust-lang#88838 made it worse. This fixes the issue by not counting such an import as a candidate.
Fixes #88472. For this example:
rustc currently emits:
this is incorrect, as applying this suggestion leads to
With my changes, I get:
As for the wildcard mentioned in #88472, I would argue that the warning is actually correct, since the import is unused. I think the real issue is the wrong suggestion, which I have fixed here.