-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Suggest to use . instead of :: when accessing a method of an object #101975
Conversation
r? @TaKO8Ki (rust-highfive has picked a reviewer for you, use r? to override) |
104f081
to
aff89a6
Compare
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.
Sorry for the late review. I left a comment.
LL | println!("{}", rect1::area()); | ||
| ^^^^^ use of undeclared crate or module `rect1` | ||
| | ||
help: `rect1` is not a crate or module, maybe you meant to call instance method |
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 this suggestion is emitted if rect1 does not have area
method. Is it possible to check if rect has it before creating suggestion?
Given the following code:
// run-rustfix
struct Rectangle {
width: i32,
height: i32,
}
impl Rectangle {
fn new(width: i32, height: i32) -> Self {
Self { width, height }
}
}
fn main() {
let width = 3;
let height = 4;
let rect1 = Rectangle::new(width, height);
println!("{}", rect1::area());
//~^ ERROR failed to resolve: use of undeclared crate or module `rect1`
}
Current output is:
error[E0433]: failed to resolve: use of undeclared crate or module `rect1`
--> src/main.rs:27:20
|
27 | println!("{}", rect1::area());
| ^^^^^ use of undeclared crate or module `rect1`
|
help: `rect1` is not a crate or module, maybe you meant to call instance method
|
27 | println!("{}", rect1.area());
| ~~~~~~
For more information about this error, try `rustc --explain E0433`.
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.
You are right.
This need to delay the suggestion to be emit in type checking phase, I will have a try.
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.
Spent some time to trying this, I think it's complicated to make it work. We need to find the local binding of and ty
of the ident, then to check whether it has the method.
See my branch:
chenyukang@9cf2a60
Not sure whether worth it for a corner case of suggestion.
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.
Adding a field to Resolver
like the following for storing impl items might help you.
rust/compiler/rustc_resolve/src/lib.rs
Line 1026 in 4891d57
trait_impls: FxIndexMap<DefId, Vec<LocalDefId>>, |
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 continue with my previous solution, delay diagnostic to checking method in type checking phase, now it works by checking whether the method name is valid (may need more tweak about the arguments checking).
I have a question for my current code, how can we find the binding according to a ident
(then find out the type
of it),
My code for this part is:
https://github.com/rust-lang/rust/pull/101975/files#diff-8dea1c54efc225f1b3a27454cbf2fc50f016073f51319b27d1b834bee7c5419eR1435
It's a little bit complicated to use a visitor
for this, another issue is it can only handle the let
expression, I want our suggestion works also for other scenarios, such as the binding is introduced by function arguments.
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.
@compiler-errors do you know any better solution for this?
6ade4e8
to
3096d8e
Compare
3096d8e
to
92fbb59
Compare
This comment has been minimized.
This comment has been minimized.
92fbb59
to
76c5988
Compare
9f1685f
to
bccd912
Compare
This comment has been minimized.
This comment has been minimized.
18a110d
to
7a20d12
Compare
☔ The latest upstream changes (presumably #103978) made this pull request unmergeable. Please resolve the merge conflicts. |
7a20d12
to
54b58fc
Compare
Hi @TaKO8Ki, |
☔ The latest upstream changes (presumably #104655) made this pull request unmergeable. Please resolve the merge conflicts. |
let Ok(snippet) = sm.span_to_snippet(span) && | ||
snippet.starts_with("::") && snippet.matches("::").count() == 1 { | ||
let local_span = *self.pat_span_map.get(&local_id).unwrap(); | ||
let mut err = self.session.struct_span_err( |
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.
Why are you making a new error here? Is it possible to pass this down to where the original error (use of undeclared crate or module
) is being created?
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 because I need to stash it, and if the method checking failed, I want cancel it, but the previous one use of undeclared crate or module
should always emit?
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.
Yes, but there should not be two errors -- only one, and you can append a suggestion to it.
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.
Let' me try to stash use of undeclared crate or module
, and only add help suggestion when method checking succeed.
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.
oops, seems need many more changes, since report_path_resolution_error
only report the error message and suggestions.
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, you probably need to pass it down in the return statement and use it where that error is being created.
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.
maybe this function is not the proper position to add this check...😛
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.
Maybe so... but I would prefer if we did it the correct way. Is it possible to investigate the correct way of doing it?
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.
Yes, let me try it.
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.
Code updated!
There are some test case result changed, since the diagnostic order change.
let parent = self.tcx.hir().get_parent_node(seg1.hir_id); | ||
if let Some(Node::Expr(call_expr)) = self.tcx.hir().find(parent) && | ||
let Some(expr) = visitor.result { | ||
let self_ty = self.check_expr(expr); |
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 checking an expression that should have already been checked. There should be a way of using the TypeckResults
(or something else) to find the Ty
of a local variable...
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.
Use node_ty
to get Ty
now.
Sorry for taking so long to review this even though I was |
54b58fc
to
2ad07ea
Compare
94f7c7e
to
2a3edb5
Compare
@chenyukang make sure to do: @rustbot ready When you're ready to review. Otherwise I might not see this, haha. |
@compiler-errors thanks for reminder! 😁 |
let sm = self.infcx.tcx.sess.source_map(); | ||
diag.span_suggestion_verbose( | ||
sm.span_extend_while(seg1.ident.span.shrink_to_hi(), |c| c == ':').unwrap(), | ||
"maybe you meant to call instance method", |
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.
"maybe you meant to call instance method", | |
"you may have meant to call an instance method", |
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, this looks pretty good. I will give one last review once you fix the typo. Please ping me if I forget.
2a3edb5
to
fa83004
Compare
fa83004
to
fb004e9
Compare
Updated, thanks! |
@bors r+ |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#101975 (Suggest to use . instead of :: when accessing a method of an object) - rust-lang#105141 (Fix ICE on invalid variable declarations in macro calls) - rust-lang#105224 (Properly substitute inherent associated types.) - rust-lang#105236 (Add regression test for rust-lang#47814) - rust-lang#105247 (Use parent function WfCheckingContext to check RPITIT.) - rust-lang#105253 (Update a couple of rustbuild deps) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #101749
Fixes #101542