-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Fix ICE with use clippy::a::b;
#83336
Conversation
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
error[E0433]: failed to resolve: maybe a missing crate `clippy`? | ||
--> $DIR/tool-mod-child.rs:1:5 | ||
| | ||
LL | use clippy::a::b; | ||
| ^^^^^^ maybe a missing crate `clippy`? |
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 inconsistent with the error for use clippy::a;
:
error[E0432]: unresolved import `clippy`
--> src/lib.rs:1:5
|
1 | use clippy::a;
| ^^^^^^ `clippy` is a tool module, not a module
How should I change this code to get that error?
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.
Interestingly, the clippy::a::b
error is the same as the error for rustdoc::a::b
.
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.
Right, they're both tool lints.
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.
Before this change clippy::a
and rustdoc::a
gave different errors, but now they're the same... weird.
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.
Rustdoc isn't marked as a tool module in resolve: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/question.20about.20tool.20module.20import.20errors
} | ||
// The error was already reported earlier. | ||
return None; | ||
} |
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 it should be
PathResult::NonModule(path_res) => {
if no_ambiguity {
assert!(import.imported_module.get().is_none());
}
// The error was already reported earlier.
return None;
}
Res::Err
was used to discern between reachable cases and (seemingly) unreachable cases.
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.
By the way, why do clippy::a
and clippy::a::b
seem to go through very different code paths? clippy::a
didn't trigger the ICE, only clippy::a::b
.
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.
Not sure what exactly happens there, but both imports and tool attributes are "special" in some regards.
- Import paths are split into two parts - module (everything except for the last segment, type namespace) and the final segment resolved multiple times in all namespaces.
- Tool attributes are also split into two parts but in a different way - the first segment resolved as a tool module, and all other segments that are not even resolved (because no definitions for them exist in source code), just automatically assumed to be a tool attribute. (Perhaps the implementation is taking a shortcut here and it could be done better, at least for diagnostics.)
When you combine these two special cases together you get some unique code paths.
Could you make a different PR for orthogonal diagnostic changes? They may cause a large diff in tests, depending on the scope. |
The issue I was talking about is that
But if you think the errors are okay, they're fine for me as well. |
This comment has been minimized.
This comment has been minimized.
@bors r+ |
📌 Commit bfae41d has been approved by |
Rollup of 9 pull requests Successful merges: - rust-lang#80193 (stabilize `feature(osstring_ascii)`) - rust-lang#80771 (Make NonNull::as_ref (and friends) return refs with unbound lifetimes) - rust-lang#81607 (Implement TrustedLen and TrustedRandomAccess for Range<integer>, array::IntoIter, VecDequeue's iterators) - rust-lang#82554 (Fix invalid slice access in String::retain) - rust-lang#82686 (Move `std::sys::unix::platform` to `std::sys::unix::ext`) - rust-lang#82771 (slice: Stabilize IterMut::as_slice.) - rust-lang#83329 (Cleanup LLVM debuginfo module docs) - rust-lang#83336 (Fix ICE with `use clippy::a::b;`) - rust-lang#83350 (Download a more recent LLVM version if `src/version` is modified) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #83317.