Skip to content
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

Make E0599's label more clear for field which is used like a method. #127193

Closed
wants to merge 1 commit into from

Conversation

surechen
Copy link
Contributor

@surechen surechen commented Jul 1, 2024

fixes #127178

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 1, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rewording "field, not method" to be a bit more detailed is useful (though I think the "in scope" part is redundant imo), but this doesn't fix the issue linked, which is expressing confusion about why something is being called a field when it's a method -- just because it's a method that's out of scope.

compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
@lolbinarycat
Copy link
Contributor

additionally, the most confusing part of issue is the help messages getting interleaved, which is completely unaddressed.

@surechen
Copy link
Contributor Author

surechen commented Jul 3, 2024

additionally, the most confusing part of issue is the help messages getting interleaved, which is completely unaddressed.

Thanks for the additional explanation.
In my opinion, the suggestions here are not getting interleaved, because first of all, it is a field, so it is recommended that the user remove the surrounding brackets first, and then try to find traits that can provide this method. The user will only be prompted after finding some satisfied traits.
I initially thought that what might cause user confusion was that field, not a method was too simple, especially when it was not known whether the user wanted a field or a method.
I feel that there is no need to move the probe logic in fn suggest_traits_to_import to the time when the message field, not a method is issued, because it does not seem to make much sense to know it in advance. I prefer to simply improve the information field, not a method here.
Hope to get your reply, thank you. @compiler-errors @lolbinarycat

@lolbinarycat
Copy link
Contributor

In my opinion, the suggestions here are not getting interleaved

the three help messages that get printed appear in this order:

  1. help: items from traits can only be used if the trait is implemented and in scope
  2. help: remove the arguments
  3. help: trait GetX which provides x is implemented but not in scope; perhaps you want to import it

1 and 3 deal with the fact that you could fix the error by importing a trait, while 2 deals with the fact you could fix the error by turning a method call into a field access. the similar fixes should be grouped together.

@surechen
Copy link
Contributor Author

surechen commented Jul 8, 2024

In my opinion, the suggestions here are not getting interleaved

the three help messages that get printed appear in this order:

  1. help: items from traits can only be used if the trait is implemented and in scope
  2. help: remove the arguments
  3. help: trait GetX which provides x is implemented but not in scope; perhaps you want to import it

1 and 3 deal with the fact that you could fix the error by importing a trait, while 2 deals with the fact you could fix the error by turning a method call into a field access. the similar fixes should be grouped together.

Thank you for your reply. I will take some time to process it as soon as possible

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2024
@surechen
Copy link
Contributor Author

The original help message "items from traits can only be used if the trait is implemented and in scope" may not be displayed in the order of the code due to the diagnostic's mechanism, so here I try to use span_help to make the help message sorting comply with the order of the code.
Please help review again. Thank you. @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2024
@rust-log-analyzer

This comment has been minimized.

@@ -3680,7 +3687,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
_ => None,
};
if !trait_missing_method {
err.help(if param_type.is_some() {
err.span_help(span, if param_type.is_some() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this. I don't think this improves the quality of the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this. I don't think this improves the quality of the error message.

Done. Thank you.

@bors
Copy link
Contributor

bors commented Aug 17, 2024

☔ The latest upstream changes (presumably #128786) made this pull request unmergeable. Please resolve the merge conflicts.

@surechen surechen closed this Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

confusing error when trying to call a not-in-scope method that shares a name with a field
7 participants