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

"snippet" for "did you mean x" is not so helpful #36164

Closed
nikomatsakis opened this issue Aug 31, 2016 · 12 comments
Closed

"snippet" for "did you mean x" is not so helpful #36164

nikomatsakis opened this issue Aug 31, 2016 · 12 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 31, 2016

On current beta, at least, I see:

error: attempted access of field `test_results` on type `TestResult`, but no field with that name was
 found
   --> src/main.rs:196:25
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: did you mean `results`?
   --> src/main.rs:196:37
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                                     ^^^^^^^^^^^^

But I feel like I'd rather see:

error: attempted access of field `test_results` on type `TestResult`, but no field with that name was
 found
   --> src/main.rs:196:25
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                                     ^^^^^^^^^^^^
    =  help: did you mean `results`?

Come to think of it, we could shorten the main message and move the suggestion into the label, as I think you've done elsewhere:

error: no field `test_results` on type `TestResult`
   --> src/main.rs:196:25
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                                     ^^^^^^^^^^^^ did you mean `results`?
    |

Standalone example:

https://is.gd/8STXMd

cc @jonathandturner

@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2016
@durka
Copy link
Contributor

durka commented Aug 31, 2016

Another possibility is to actually replace it

error: attempted access of field `test_results` on type `TestResult`, but no field with that name was
 found
   --> src/main.rs:196:25
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: did you mean `results`?
   --> src/main.rs:196:37
    |
196 |         tests_passed += normal_test.results.iter().filter(|t| t.status == "ok").count();
    |                                     ^^^^^^^

But if it's only a guess, that might not be a good idea.

@nikomatsakis
Copy link
Contributor Author

@durka my inclination is that the extra 5 lines of output aren't valuable enough in this case, it's not likely it's tricky to figure out how to apply the change

@sophiajt
Copy link
Contributor

Or maybe even...

error: attempted access of field `test_results` on type `TestResult`, but no field with that name was
 found
   --> src/main.rs:196:25
    |
196 |         tests_passed += normal_test.test_results.iter().filter(|t| t.status == "ok").count();
    |                                     ^^^^^^^^^^^^ did you mean `results`?

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Sep 1, 2016

@jonathandturner how is that different from my 3rd example? (Except for using a longer "error' message)

@sophiajt
Copy link
Contributor

sophiajt commented Sep 1, 2016

@nikomatsakis - lol are you sure you didn't add it after I commented?

Just kidding, I just didn't see it the first time through.

@sophiajt
Copy link
Contributor

sophiajt commented Sep 1, 2016

My vote is your # 3 ;)

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Sep 2, 2016
@nikomatsakis
Copy link
Contributor Author

I'm happy to mentor the change. It seems like it would be fairly self-contained. The error is reported by this code in src/librustc_typeck/check/mod.rs -- just search for "attempted access of field" and you ought to find it.

The idea would be to

  • change the main message there to something shorter, like "no field {} in type {}"
  • change the expr.span in self.type_error_struct(expr.span to field.span, so that we highlight just the field name itself
  • change the suggest_field_names function so that instead of calling err.span_help it calls err.span_label

I think that will basically do it!

@nikomatsakis
Copy link
Contributor Author

One wrinkle I am not sure about: the changes that I suggested above will, iirc, cause a ^^^ with no suggestion to appear in the case that we don't have a suggested field name to offer. I feel like we wanted to move away from those, which might suggest that we ought to offer a generic label like "this field here" or something? <-- @jonathandturner ?

@sophiajt
Copy link
Contributor

sophiajt commented Sep 2, 2016

Yes, a default label would be great when we don't know the name. Maybe something like "^^^^ unknown field" when we don't know of a good suggestion

@aravind-pg
Copy link
Contributor

aravind-pg commented Sep 2, 2016

I'd love to try my hand at this. The instructions posted seem sufficient for now, but I'll come back if I have any questions.

@KiChjang
Copy link
Member

@aravind-pg Are you still working on this?

@aravind-pg
Copy link
Contributor

Sorry, I just haven't found time for this unfortunately. You can go ahead and un-assign me.

0xmohit added a commit to 0xmohit/rust that referenced this issue Sep 29, 2016
gavinb added a commit to gavinb/rust that referenced this issue Oct 2, 2016
- Fixes rust-lang#36164
- Part of rust-lang#35233
- handles unknown fields
- uses UI-style tests
- update all related tests (cfail, ui, incremental)
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2016
Improve error message and snippet for "did you mean `x`"

- Fixes rust-lang#36164
- Part of rust-lang#35233

Based on the standalone example https://is.gd/8STXMd posted by @nikomatsakis and using the third formatting option mentioned in rust-lang#36164 and agreed by @jonathandturner.

Note however this does not address the question of [how to handle an empty or unknown suggestion](rust-lang#36164 (comment)). @nikomatsakis any suggestions on how best to address that part?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants