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

Improve typo suggestions #91579

Closed

Conversation

camsteffen
Copy link
Contributor

The goal is to give a message that the user can parse and respond to more quickly. For example...

Before:

a local variable with a similar name exists: `x`

After:

did you mean `x`? (a similarly named local variable)

I believe in most cases the user will see that the suggestion is correct and won't even read the rest of the message.

I also added span_suggestion_hide_inline so that it doesn't print : `x` at the end, but will show the snippet if the message is too long to be shown inline.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2021
@rust-log-analyzer

This comment has been minimized.

@euclio
Copy link
Contributor

euclio commented Dec 6, 2021

To add a some context, in the past, we've tried to remove "did you mean" questions from suggestions. See the the first bullet point of the suggestion style guide.

I personally prefer avoiding questions in the output, but if we end up changing the style please make sure to update the style guide as well.

@matthiaskrgr
Copy link
Member

Hmm imo the previous msg had the advantage that the eye can immediately jump to the end of the line to see what the compiler suggests. Now I have to read the line more carefully to check where and what the suggestion is.

@camsteffen
Copy link
Contributor Author

To add a some context, in the past, we've tried to remove "did you mean" questions from suggestions. See the the first bullet point of the suggestion style guide.

Thanks, I wasn't aware of that. That point in the style guide seems to assume that if you ask a question then you are not explaining the reasoning behind the suggestion. This is not the case here: I put the "why" in parentheses (a comma could also be used).

Come to think of it, it makes sense that questions should be avoided by default, but I think typo suggestions could be an exception to the rule. Typo suggestions are by nature not as confident as other suggestions and "did you mean...?" conveys this lesser confidence in a human-like way. Other suggestions are more confident because we know that it will make the code syntactically correct or typeck-correct. A typo suggestion on the other hand could have the completely wrong type, or the "word closeness" algorithm top result could be the wrong one.

Another thing I don't like about the current message is that it is indirect. Good suggestions tell the coder what to do (e.g. "use `collect`...", "try adding a semicolon...", etc.).

So if we don't want a question, but want to make the suggestion more direct, it could be:

try `x`, a similarly named local variable

This is a viable option, but to me "did you mean..." is more appropriate because of the lesser confidence explained above.

Hmm imo the previous msg had the advantage that the eye can immediately jump to the end of the line to see what the compiler suggests. Now I have to read the line more carefully to check where and what the suggestion is.

Maybe my brain works differently but I find it hard to relate to this. I read messages from left to right. Only after reading a few words I might decide I can start to skim because I've seen this kind of message before. If I see "did you mean..." I will definitely read the next word without skipping ahead.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@jackh726
Copy link
Member

Tbh I don't really like this change, but that's just preference. I think the only improvement here would be to maybe always show the variable name inline.

Anyways, I'm going to r? rust-lang/diagnostics for a second opinion.

@rust-highfive rust-highfive assigned estebank and unassigned jackh726 Dec 14, 2021

error[E0609]: no field `principial` on type `U`
--> $DIR/union-suggest-field.rs:17:15
|
LL | let w = u.principial;
| ^^^^^^^^^^ help: a field with a similar name exists: `principal`
Copy link
Member

@camelid camelid Dec 14, 2021

Choose a reason for hiding this comment

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

Just a suggestion: To make the old version a bit shorter, we could do a similarly named field exists instead. That'd be a smaller change and perhaps less controversial.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would go in the opposite direction and make the typo suggestions always verbose (so they appear in their own subdiagnostic window) to make them stand out more, with the current wording. I need to simmer on it to see if it is just familiarity (and the fact I likely wrote the original wording ^_^') or an actual "reason" I prefer it.

The proposed output has one problem though, which is that you now do have to read the whole thing. When I see ^^^ help: ... I usually skip to the end to see what the suggested code is. The new message has a question (that we've tried to move away from) and a parenthesis that explains what's going on. It feels to me as adding verbosity without a gain. (But again, I need to argue with myself to see if I can come around to seeing it a different way.)

Copy link
Contributor Author

@camsteffen camsteffen Dec 16, 2021

Choose a reason for hiding this comment

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

I would go in the opposite direction and make the typo suggestions always verbose (so they appear in their own subdiagnostic window)

I like the inline suggestion because you can see the misspelling and the suggestion next to each other.

When I see ^^^ help: ... I usually skip to the end to see what the suggested code is.

I already responded to this concern in a previous comment, but a couple more thoughts. For your brain to not even see the next three words before skipping to the end - this sounds like the experience of someone who spends a LOT of time with rustc and therefore has highly developed heuristics for skimming its output. Also consider that not every help: message has a suggestion at the end.

It feels to me as adding verbosity without a gain.

I would say the gain is that it makes the message follow the usual convention of

  1. Tell what to do
  2. Explain why (if needed)
  3. Show the code

Most suggestions start with telling you what to do: remove this comma, try `<some code>`, try adding a semicolon. The current message does not tell what to do but skips right into "why" and then "show" and this feels surprising to me. Admittedly the question is asking rather than telling, but it still has "what to do".


One purpose of this PR is to make the messages consistent. So, even if some aspects of this PR are declined, I think we should agree on a message format. Probably one of the following:

  1. a local variable with a similar name exists: `x`
  2. a similarly named local variable exists: `x`
  3. there is a local variable with a similar name: `x`
  4. Did you mean `x`? (a local variable with a similar name)
  5. Did you mean `x`? (a similarly named local variable)
  6. Did you mean `x`, a local variable with a similar name?
  7. Did you mean `x`, a similarly named local variable?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, error messages are nearly always (and IIUC supposed to be) lowercase, so I don't think we should capitalize.

@bors
Copy link
Contributor

bors commented Jan 21, 2022

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

@estebank
Copy link
Contributor

Apologies for the delay. Thank you for taking the time to take a look at this. I will be closing this PR as I think that changing the wording in this direction isn't the right call. I think that there are things we should do here, particularly what you proposed to make the messaging more consistent.

@estebank estebank closed this Feb 17, 2022
@camsteffen camsteffen deleted the did-you-mean-i-bet-you-did branch February 17, 2022 18:21
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.

None yet

10 participants