- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
Suggest to use . instead of :: when accessing a method of an object #101975
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
| 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.
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