-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Suggest crate keyword when unresolved path contains current crate's name
#138529
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
Conversation
Prioritizing the added check for the crate's name at the top of `report_path_resolution_error()` seems to account for differences in Rust 2015 and 2018's crate/module path systems, allowing the same suggestion to show up for Rust 2015.
(The `help` annotation was missing due to an issue that, admittedly, resulted from a typo I stared at and didn't catch.)
|
r? compiler |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
r? compiler |
jieyouxu
left a comment
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.
Thanks for the PR. However, I'm hesitant about making this suggestion at all because of potential for a lot of incorrect/misleading suggestions, even if the suggestion is MaybeIncorrect (as in, I'm hesitant about the signal-to-noise ratio).
Some examples:
- There might be modules in other places of various visibilities that shares the same name as the root crate name, and it's not clear to me that the root crate is necessarily frequently the right answer.
- It's not clear to me that just because the current crate name happens to be the same as the unresolved module/linked crate, that it is the right suggestion. It is possible that even though the root crate name is
foo, you can still have an extern crate of the same name! E.g. you can provide an extern crate candidate viarustc foo.rs --extern foo=libbar.rlib, andextern crate foo;anduse foo::{...}infoo.rsroot module would not be "itself".
| if ident.name == self.tcx().crate_name(0_usize.into()) { | ||
| ( | ||
| format!("use of unresolved module or unlinked crate `{ident}`"), | ||
| Some(( | ||
| vec![(ident.span, String::from("crate"))], | ||
| format!( | ||
| "the current crate name can not be used in a path, use the keyword `crate` instead" | ||
| ), | ||
| Applicability::MaybeIncorrect, | ||
| )), | ||
| ) |
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.
Problem: I don't think this is right. Counter-example:
// foo.rs
mod bar {
pub fn baz() {}
}
use crate::foo::baz;
fn main() {}error[E0432]: unresolved import `crate::foo`
--> foo.rs:5:12
|
5 | use crate::foo::baz;
| ^^^ use of unresolved module or unlinked crate `foo`
|
help: the current crate name can not be used in a path, use the keyword `crate` instead
|
5 - use crate::foo::baz;
5 + use crate::crate::baz;
|
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0432`.
| _ => None, | ||
| }; | ||
| if module_res == self.graph_root.res() { | ||
| if ident.name == self.tcx().crate_name(0_usize.into()) { |
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.
Problem: I think this condition is too lax. Even if we constrain this suggestion to only be made if
- Unresolved module/crate is the first path segment of a non-1-length path
- Not a global path, e.g.
::foo
I think the possibility of false positives is still quite high, even if the applicability is MaybeIncorrect.
|
Thanks for the review! I definitely agree with your comments. In light of your hesitancy about trying to add the suggestion at all, should I go ahead and close the PR? |
|
Yeah, I think for now we should close this PR. I'd be open to revisiting some heuristic that'd provide a higher signal-to-noise ratio, but I can't immediately think of a combination right now. Thanks for the exploration anyway! |
Related to #138312.
Unsure about the second "help" message suggested by the issue writer, but for at least the main part of the issue, I've added a check to emit a suggestion replacing the current crate's name in a path with
crate. Let me know if there's a more elegant way to do this.