-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Detect likely .
-> ..
typo in method calls
#105765
Conversation
r? @lcnr (rustbot has picked a reviewer for you, use r? to override) |
r? @oli-obk |
let suggestion = | ||
if res.is_none() { this.report_missing_type_error(path) } else { None }; | ||
let suggestion = if let Some((start, end)) = this.diagnostic_metadata.in_range | ||
&& path[0].ident.span.lo() == end.span.lo() |
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 didn't look too much into this, but does this check that this is a method?
We should also probably double check this is a path with one segment.
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.
It doesn't, but during resolve we don't have enough data to do that (although now that I'm tracking start
I might be able to cobble something together).
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'm comfortable with the weasel wording on this one suggestion: it is a guess, and we'd have to perform more extensive surgery on resolve to track more state, and it is already quite a beast.
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.
My only concern is that this triggers when we have any resolution error on the RHS of a range, e.g.
fn foo() {
a..b;
}
Results in:
error[E0425]: cannot find value `a` in this scope
--> /home/ubuntu/test.rs:2:5
|
2 | a..b;
| ^ not found in this scope
error[E0425]: cannot find value `b` in this scope
--> /home/ubuntu/test.rs:2:8
|
2 | a..b;
| ^ not found in this scope
|
help: you might have meant to write a method call instead of a range
|
2 - a..b;
2 + a.b;
I'd much prefer if the wording was made to not mention methods in specific, but focus on the .
itself -- something like "you might've meant to write the .
operator instead of a range"?
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.
What do you think of "you might have meant to write .
instead of ..
"
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.
Yeah, that works fine.
Just would prefer it to not mention "method" unless we can verify it's actually a method, but since this code is in resolve, I totally understand how that's hard, haha.
@rustbot author |
r=me -- preferably with the resolve diagnostic's wording made more generic, but I guess I won't hold you to it |
@bors r=compiler-errors |
…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#103718 (More inference-friendly API for lazy) - rust-lang#105765 (Detect likely `.` -> `..` typo in method calls) - rust-lang#105852 (Suggest rewriting a malformed hex literal if we expect a float) - rust-lang#105965 (Provide local extern function arg names) - rust-lang#106064 (Partially fix `explicit_outlives_requirements` lint in macros) - rust-lang#106179 (Fix a formatting error in Iterator::for_each docs) - rust-lang#106181 (Fix doc comment parsing description in book) - rust-lang#106187 (Update the documentation of `Vec` to use `extend(array)` instead of `extend(array.iter().copied())`) - rust-lang#106189 (Fix UnsafeCell Documentation Spelling Error) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fix #65015.