-
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
Show a note when closure field is called as method #18346
Conversation
I don't know how to test this. Any ideas? |
Also, this is my first contribution to |
|
||
// Returns true if the type is a struct and contains a field with | ||
// the same name as the not-found method | ||
let is_field = || { |
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 would actually be clearer as a match expression.
let is_field = match ty::get(rcvr_ty).sty {
ty_struct(did, _) =>
ty::lookup_struct_fields(cx, did)
.iter()
.any(|f| f.name.user_string(cx) == method_ustring),
_ => false
};
@aochagavia Thanks for the PR! I added comments inline. In terms of testing, you'll need a small self-contained Rust program that is supposed to produce this diagnostic and annotate it accordingly with the diagnostic messages you expect it to produce. See the files in src/test/compile-fail/ for examples. Also, the wiki has some information about this. |
I think in the future we may want this to also extend to fields that can be reached via an arbitrary number of autoderefs so that the note also shows up for structs contained in smart pointers and similar. But this is great for now! Also, I wonder if there's any particular reason why a.f() doesn't work for closures in fields or if it just hasn't been implemented yet. |
This code will show the helping note even when the field of the struct is not a function/closure. Do you think that is ok? Maybe I could add some extra checks to see if the field is indeed a function/closure. How complex would it be? Is it worth it? |
}, | ||
rcvr_ty, | ||
None); | ||
|
||
// If the method has the name of a field, give a help note | ||
if is_field { | ||
cx.sess.span_note(span, |
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 have used span_note
because the meaning seems to be the same and I can use NOTE
to check if the message is correct (see the test)
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 can also check for HELP in tests and I think we're moving towards HELP for "this is how you fix the error" kind of messages but don't worry about it, whoever drives by this part of code can fix it next time!
Looks like there's some tidy issues (trailing whitespace) |
@alexcrichton thanks! I have fixed it |
It may still be there in the test case (travis failures) |
Not anymore :) |
Closes #18343