Uh oh!
There was an error while loading. Please reload this page.
resolve: Turn the "non-empty glob must import something" error into a lint - #65539
Conversation
rust-highfive
commented
Oct 18, 2019
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. |
fpoli
commented
Oct 18, 2019
To update the test I think you only need to replace the |
petrochenkov
commented
Oct 18, 2019
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. |
traxys
commented
Oct 18, 2019
I will need to check this evening, When running rustc I see a warning but the test does pass if I just remove the |
traxys
commented
Oct 18, 2019
I fixed the test but because the tests in import are run with |
rust-highfive
commented
Oct 18, 2019
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 |
petrochenkov
commented
Oct 18, 2019
Yep, that's the right thing to do. |
Uh oh!
There was an error while loading. Please reload this page.
petrochenkov
commented
Oct 18, 2019
This probably doesn't need a full FCP, but I'm still going to ping the lang team. |
rust-highfive
commented
Oct 18, 2019
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 |
Centril
commented
Oct 19, 2019
Nominated. :) We'll discuss the aspect of FCP/not & what we think. Having the rationale ready by then would be great. |
petrochenkov
commented
Oct 19, 2019
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 - |
Uh oh!
There was an error while loading. Please reload this page.
nikomatsakis
commented
Oct 24, 2019
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? |
nikomatsakis
left a comment
There was a problem hiding this comment.
One minor nit (pre-existing, but hey)
Uh oh!
There was an error while loading. Please reload this page.
traxys
commented
Oct 24, 2019
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" |
petrochenkov
commented
Oct 26, 2019
Yes, it doesn't apply to globs. Also, this error was introduced together with mod m {}use m::*;fnmain(){} |
petrochenkov
commented
Oct 26, 2019
Disregard all my comments about progress, I misremembered what this error is about. |
rust-highfive
commented
Oct 28, 2019
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 |
rust-highfive
commented
Oct 28, 2019
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 |
nikomatsakis
commented
Oct 28, 2019
@traxys the usual technique is to use |
eddyb
commented
Oct 28, 2019
You can use |
petrochenkov
commented
Oct 29, 2019
@bors r+ |
bors
commented
Oct 29, 2019
📌 Commit ed8585f has been approved by |
resolve: Turn the "non-empty glob must import something" error into a lint This fixesrust-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 fixesrust-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
pnkfelix
commented
Oct 31, 2019
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