-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
resolve: Turn the "non-empty glob must import something" error into a lint #65539
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
To update the test I think you only need to replace the |
If the warning is not reported due to compilation being stopped too early, then the test can be split into two tests - one with errors and another with warnings only. |
I will need to check this evening, When running rustc I see a warning but the test does pass if I just remove the |
I fixed the test but because the tests in import are run with |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Yep, that's the right thing to do. |
This probably doesn't need a full FCP, but I'm still going to ping the lang team. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Nominated. :) We'll discuss the aspect of FCP/not & what we think. Having the rationale ready by then would be great. |
So, I couldn't find where exactly we discussed this with jseyfried, but the logic behind this is that
TLDR: The error is reported by analogy with single imports. However, this analogy doesn't give as much, and it also may be expected from single and glob imports to behave differently by analogy with e.g. regexps - |
So we discussed this in the @rust-lang/lang meeting and felt generally 👍 on warning and not error, except that we were also kind of confused about exactly what scenario we're even talking about. This is specifically referring to the case where:
This all seems fine to us. However, there was one question we wanted clarification on, @petrochenkov. You wrote that the error in the case of an explicit import is helpful for ensuring convergence:
Presumably, this doesn't apply to globs? |
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.
One minor nit (pre-existing, but hey)
The only thing for wich I have not seen any response is the concern about the warning message, if it should not be changed to "all items are too private" or "no item is public enough" |
Yes, it doesn't apply to globs. Also, this error was introduced together with mod m {}
use m::*;
fn main() {} |
Disregard all my comments about progress, I misremembered what this error is about. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@traxys the usual technique is to use |
You can use |
@bors r+ |
📌 Commit ed8585f has been approved by |
resolve: Turn the "non-empty glob must import something" error into a lint This fixes rust-lang#62334 by changing the error to a lint warning the glob. I changed the test but I'm very unsure of what I did as I do not know how to correctly check for the warning
resolve: Turn the "non-empty glob must import something" error into a lint This fixes rust-lang#62334 by changing the error to a lint warning the glob. I changed the test but I'm very unsure of what I did as I do not know how to correctly check for the warning
Rollup of 12 pull requests Successful merges: - #65405 (Create new error E0742 and add long error explanation) - #65539 (resolve: Turn the "non-empty glob must import something" error into a lint) - #65724 (ci: refactor pr tools job skipping) - #65741 (Prevent help popup to disappear when clicking on it) - #65832 (Re-enable Emscripten's exception handling support) - #65843 (Enable dist for MIPS64 musl targets) - #65898 (add basic HermitCore support within libtest) - #65900 (proc_macro: clean up bridge::client::__run_expand{1,2} a bit.) - #65906 (Update mdbook to 0.3.3) - #65920 (Use rustc-workspace-hack for rustbook) - #65930 (doc: use new feature gate for c_void type) - #65936 (save-analysis: Account for async desugaring in async fn return types) Failed merges: - #65434 (Add long error explanation for E0577) r? @ghost
discussed in T-compiler meeting. Accepted for beta backport. |
This fixes #62334 by changing the error to a lint warning the glob. I changed the test but I'm very unsure of what I did as I do not know how to correctly check for the warning