Distinguish guesses from suggestions #39458

oli-obk · 2017-02-02T09:54:00Z

fixes #33691
cc #37085
fixes #39254

oli-obk · 2017-02-02T09:54:24Z

cc @Manishearth @llogiq @mcarton

rust-highfive · 2017-02-02T09:54:31Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

llogiq · 2017-02-02T10:54:43Z

cc @killercup

oli-obk · 2017-02-02T11:35:45Z

failures are legit, but just related to spans in ui tests. I'm fixing them now.

petrochenkov · 2017-02-02T11:36:50Z

@oli-obk
You have regenerate the ui tests as well.
(It's also interesting how this patch changes diagnostics visually.)

It looks like you've made //~ GUESS annotations in cfail tests mandatory, I think this needs to be reverted.
Extended diagnostics are mostly tested by ui tests now, and requiring it for cfail tests checking for things like "this should just fail" is nuisance.

oli-obk · 2017-02-02T11:40:15Z

src/tools/compiletest/src/runtest.rs

@@ -1135,6 +1135,7 @@ actual:\n\
            Some(ErrorKind::Error) => true,
            Some(ErrorKind::Warning) => true,
            Some(ErrorKind::Suggestion) => false,
+            Some(ErrorKind::Guess) => false,


guesses are not mandatory

You have just added these annotations to many tests, so I thought they are mandatory now.

oli-obk · 2017-02-02T11:42:39Z

It's also interesting how this patch changes diagnostics visually.

yes, about that... I'd like suggestions and guesses to show their help message just like notes are shown. There's no advantage to showing a full help.

petrochenkov · 2017-02-02T13:52:26Z

src/test/ui/resolve/issue-14254.stderr

+   |         ^^^
+   |
+help: did you intend to call a method of the same name?
+   |         self.baz();


Suggestions from resolve were intentionally reported "in place" as labels.
See #38154, especially @jonathandturner's comments.
This is a pretty big regression IMO.

bjorn3 · 2017-02-02T17:03:16Z

src/librustc_errors/diagnostic.rs

+        self
+    }
+
+    pub fn guess(&mut self, sp: Span, msg: &str, guess: String) -> &mut Self {


I thinks span_guess is a better name, because all other functions taking a span have span in the name. ( for exanple span_suggestion )

bjorn3 · 2017-02-02T17:06:59Z

src/librustc_resolve/lib.rs

-                            err.span_label(span, &format!("did you mean `self.{}(...)`?",
-                                                           path_str));
+                            err.guess(span, "did you intend to call a method of the same name?",
+                                      format!("self.{}", path_str));


I prefer self.{}(...) over self.{}

for foo(5) that would give the suggestion self.foo(...)(5)

I though that the span was for the entire call, instead of just the self.{} part

bjorn3 · 2017-02-02T17:11:17Z

src/librustc_typeck/check/method/suggest.rs

+                                                span,
+                                                &format!("did you mean to call the function \
+                                                          stored in the `{}` field?", item_name),
+                                                format!("({}.{})", expr_string, item_name),


I prefer ({}.{})(...)

nrc · 2017-02-03T00:44:20Z

I'm a bit dubious about adding yet another category of error to the compiler. Why do we need guess rather than using a help or note?

nrc · 2017-02-03T00:44:36Z

cc @rust-lang/compiler @rust-lang/tools

Manishearth · 2017-02-03T01:02:03Z

because the suggestion API outputs a copyable snippet of code with context, and help doesn't, because there's usually no need to.

nrc · 2017-02-03T01:14:26Z

If we want the user to copy, then why not use a suggestion?

Manishearth · 2017-02-03T01:17:43Z

Because it may have placeholders or be otherwise something that can't directly be copied. We have different types of notes:

note: additional info
help: textual help
guess: code suggestion, can't be applied automatically (because of placeholders or whatever)
suggestion: code suggestion, can be applied automatically

oli-obk · 2017-02-03T12:19:56Z

One thing about guesses is that there can be multiple guesses. E.g. let x: Iter; currently gives

  = help: you can import several candidates into scope (`use ...;`):
  = help:   `std::collections::binary_heap::Iter`
  = help:   `std::collections::btree_map::Iter`
  = help:   `std::collections::btree_set::Iter`
  = help:   `std::collections::hash_map::Iter`
  = help:   and 8 other candidates

My proposal is to move this to guesses in json and keep it the same in textual output. The guesses should supply the entire use std::whatever; and suggest to place it in the closest parent scope where it can be placed.

bors · 2017-02-04T01:36:54Z

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

nrc · 2017-02-07T04:31:06Z

I'm not persuaded that it is necessary to separate guesses and suggestions. My work flow for 'automatic application' is that the user opts in to doing this in an IDE or rustfix tool, possibly with some further input. I don't think there is a need or expectation that after such an application the resulting code is guaranteed to be logically correct or even to compile. So, I think a suggestion with placeholders would be fine as would a suggestion with multiple choices. In other words, I'd rather make suggestions more flexible than add another category of error.

I strongly dislike the idea that JSON and plain text error messages might be different (JSON might have more info, e.g., show all choices rather than the first 8, or contain full text where the human text elides some source code with elipses, etc.). I think it would be confusing for users to get (semantically) different kinds of message depending on how they view the errors.

nrc · 2017-02-07T04:34:12Z

I'm not persuaded that it is necessary to separate guesses and suggestions. My work flow for 'automatic application' is that the user opts in to doing this in an IDE or rustfix tool, possibly with some further input. I don't think there is a need or expectation that after such an application the resulting code is guaranteed to be logically correct or even to compile. So, I think a suggestion with placeholders would be fine as would a suggestion with multiple choices. In other words, I'd rather make suggestions more flexible than add another category of error.

I strongly dislike the idea that JSON and plain text error messages might be different (JSON might have more info, e.g., show all choices rather than the first 8, or contain full text where the human text elides some source code with elipses, etc.). I think it would be confusing for users to get (semantically) different kinds of message depending on how they view the errors.

Manishearth · 2017-02-07T04:36:42Z

I don't think there is a need or expectation that after such an application the resulting code is guaranteed to be logically correct or even to compile

Clippy tries pretty hard to make suggestions that will just compile. There's always the small chance it won't, but there's a big difference between "probably will compile" (and if it doesn't it's a clippy bug) and "probably won't compile" (because we knew it wouldn't beforehand). The former is something you can tie up to an automated tool, the latter needs a more interactive UI. I would love to be able to auto-rustfix the vast majority of clippy errors, compile, inspect, and commit, and then go ahead to more manually rustfix the remaining suggestions by applying them one by one.

I would represent guesses and suggestions as the same thing in JSON, with an extra "guess" param.

nrc · 2017-02-07T04:52:07Z

I would love to be able to auto-rustfix the vast majority of clippy errors

I guess I just don't think this is realistic, or importantly something we should encourage. Even more so with compiler errors.

Let me also expound on the other side of the trade-off: although the taxonomy in #39458 (comment) is sensible, I think it is not reliably adhered to currently and together with errors and warnings all these categories are confusing and poorly used by compiler devs (especially since improving error messages is a prime area for new contributors who often don't have a strong notion of the distinctions). It is also detrimental for users who get a mutli-coloured, chaotic mix of different terms without clear distinctions. I think adding another category will make things worse in both the code and the user experience. I'm not saying we must never add anything, but I do think there should be a really strong case.

Manishearth · 2017-02-07T05:05:39Z

I think from the user's POV it would just be displayed as a suggestion; this is mostly for tools to consume.

(This is why I originally proposed it as an additional boolean param that gets reflected in the json)

nikomatsakis · 2017-02-07T05:07:10Z

I haven't made up my mind on the overall distinction, but some of what @nrc is saying is resonating with me. There is definitely a balance to be struck regarding the number of distinctions, colors, and so forth in our output.

I also think that "guaranteed to be correct" (from the wording in the code) is an impossibly high standard for a suggestion to meet: e.g., if you see an unused mut warning, is it "correct" to remove it? Maybe, but about 50% of the time for me that indicates a bug in my code (wait, I meant to mutate that...). The other 50% of the time it's because of some refactoring and the mut is no longer needed. I suppose what is meant is something weaker like "guaranteed not to cause compilation errors" -- that seems closer to what @Manishearth is saying. In some cases that is certainly something we can know (e.g., removing an unused mut or import).

I also agree that moving the "Did you mean foo?" suggestions into their own help: feels like a step backward. This is obviously subjective: but I think that in general we were trying to move away from having "big sections" in the output unless there is a strong reason to do so, and doing more of the hints in line.

Manishearth · 2017-02-07T05:12:20Z

@nikomatsakis note that I'm not suggesting this display differently in the regular Rust UI

I think part of the issue comes from the fact that I'm mostly thinking about clippy lints, whereas you're thinking about compiler lints. Clippy do tend to have "one correct solution" much more often because they're often dealing with tidying up code. This is just like how we don't worry about the transformations rustfmt applies to our code.

With clippy I envision a workflow where you just pipe it through rustfix. Maybe check the diff. Maybe not. Meh.

With rustc I envision that you'd pipe it through rustfix, and then definitely check the diff. I'd absolutely love to have a tool like goimports that fixed my imports, for example.

nikomatsakis · 2017-02-07T05:48:09Z

@Manishearth

Clippy do tend to have "one correct solution" much more often because they're often dealing with tidying up code.

Yeah, I hear you, although I chose my example very deliberately. Note that "unused mut" might be seen as "just tidying up code", but it often signals an actual bug in the underlying code. But I guess that there are many style bugs where that is far less likely to be the case.

oli-obk · 2017-02-07T08:04:48Z

I strongly dislike the idea that JSON and plain text error messages might be different (JSON might have more info, e.g., show all choices rather than the first 8, or contain full text where the human text elides some source code with elipses, etc.). I think it would be confusing for users to get (semantically) different kinds of message depending on how they view the errors.

That's not what I meant. I mean that guesses are always displayed the way "they" are now. So a single guess is just a note that directly includes the text, e.g.:

29 |         baz();
   |         ^^^ did you mean `self.baz(...)`?

Multiple guesses are rendered as

  = help: you can import several candidates into scope (`use ...;`):
  = help:   `std::collections::binary_heap::Iter`
  = help:   `std::collections::btree_map::Iter`
  = help:   `std::collections::btree_set::Iter`
  = help:   `std::collections::hash_map::Iter`
  = help:   and 8 other candidates

But json displays them uniformly as an array of guesses. So there's no semantic difference, since both suggest the same thing, but are displayed in a readable form or show with enough metadata (json) to be automatically applicable.

I think it is not reliably adhered to currently

This can be tested by running rustfix over the test suite and checking whether run-pass still passes. This could be something done when nightly moves to beta, so it doesn't slow down the regular travis.

and together with errors and warnings all these categories are confusing and poorly used by compiler devs (especially since improving error messages is a prime area for new contributors who often don't have a strong notion of the distinctions).

I believe that adding guesses actually improves this. Right now we have many notes and helps which contain code snippets. Having an actual place to put them would probably help.

It is also detrimental for users who get a mutli-coloured, chaotic mix of different terms without clear distinctions. I think adding another category will make things worse in both the code and the user experience. I'm not saying we must never add anything, but I do think there should be a really strong case.

That's why I suggest no not show guesses to users as anything new. Just to introduce a structured way to write guesses.

nrc · 2017-02-07T22:45:28Z

.... But json displays them uniformly as an array

OK, sorry I msunderstood, this sounds good.

This can be tested by running rustfix over the test suite

The other distinctions can't be tested like this though, and I worry about making a muddy picture worse as much as creating mud in the first place.

petrochenkov · 2017-02-07T23:07:50Z

src/test/ui/did_you_mean/issue-36798.stderr

@@ -2,7 +2,7 @@ error: no field `baz` on type `Foo`
  --> $DIR/issue-36798.rs:17:7
   |
 17 |     f.baz;
-   |       ^^^ did you mean `bar`?
+   |       ^^^ did you mean `bar`


Questions have disappeared in many places.

petrochenkov · 2017-02-07T23:08:43Z

src/test/ui/resolve/issue-2356.stderr

-   |     |
-   |     did you mean `self.whiskers`?
-   |     `self` value is only available in methods with `self` parameter
+   |     ^^^^^^^^ did you intend to access a struct field? `self.whiskers`


All the diagnostics with 2 labels lose one of the labels

petrochenkov · 2017-02-07T23:09:53Z

src/test/ui/resolve/suggest-path-instead-of-mod-dot-item.stderr

-   |     ^ did you mean `a::I`?
+   |     ^^^
+   |     |
+   |     did you mean `a::I`


extraneous

| ^^^ | |

This might be caused by the same span being used as primary and secondary. The primary span didn't have any text, but the secondary did. In terminal output the ^^^ would be red and the did you (...) would be blue. (This is an educated guess.)

petrochenkov · 2017-02-07T23:13:09Z

I look at the diagnostics changes and I can't say I like what I see (even after a5965ab).

Can this PR be redone in some way to not affect user-visible behavior at all, just to emit some semantic markup into json for tools to use?

mrhota · 2017-03-28T03:10:06Z

@oli-obk any updates?

oli-obk · 2017-03-28T06:02:23Z

Waiting for the rfc

alexcrichton · 2017-04-01T22:56:53Z

Closing in favor of the associated RFC (while we wait on that)

@petrochenkov

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

@petrochenkov

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

@petrochenkov

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

@petrochenkov

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in #39458

oli-obk added 2 commits February 2, 2017 10:52

distinguish guesses from suggestions

aea71bd

test guesses

e46c459

rust-highfive assigned nrc Feb 2, 2017

oli-obk mentioned this pull request Feb 2, 2017

use help rather than span note for no method error; a slight rephrasing #39441

Closed

oli-obk commented Feb 2, 2017

View reviewed changes

oli-obk added 2 commits February 2, 2017 14:37

fix fallout and some span bugs

7e5bca7

parse fail test fallout

89fb0e0

oli-obk force-pushed the guess branch from ad3c6e2 to 89fb0e0 Compare February 2, 2017 13:50

petrochenkov reviewed Feb 2, 2017

View reviewed changes

bjorn3 requested changes Feb 2, 2017

View reviewed changes

undo error regressions

a5965ab

petrochenkov reviewed Feb 7, 2017

View reviewed changes

oli-obk mentioned this pull request Feb 20, 2017

Refactor suggestions into a structured form internally #39973

Closed

oli-obk mentioned this pull request Mar 3, 2017

Guess diagnostics rust-lang/rfcs#1941

Closed

alexcrichton closed this Apr 1, 2017

oli-obk mentioned this pull request May 16, 2017

Change some notes into suggestions #42033

Merged

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 23, 2017

Rollup merge of rust-lang#42033 - oli-obk:suggestions, r=petrochenkov

5960da2

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 23, 2017

Rollup merge of rust-lang#42033 - oli-obk:suggestions, r=petrochenkov

c20a157

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017

Rollup merge of rust-lang#42033 - oli-obk:suggestions, r=petrochenkov

2fc3f28

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in rust-lang#39458

bors added a commit that referenced this pull request Jul 17, 2017

Auto merge of #42033 - oli-obk:suggestions, r=petrochenkov

5803f99

Change some notes into suggestions r? @petrochenkov since you commented on the same edits in #39458

oli-obk deleted the guess branch June 15, 2020 15:25

Distinguish guesses from suggestions #39458

Distinguish guesses from suggestions #39458

Conversation

oli-obk commented Feb 2, 2017

oli-obk commented Feb 2, 2017

rust-highfive commented Feb 2, 2017

llogiq commented Feb 2, 2017

oli-obk commented Feb 2, 2017 • edited Loading

petrochenkov commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Feb 3, 2017

nrc commented Feb 3, 2017

Manishearth commented Feb 3, 2017

nrc commented Feb 3, 2017

Manishearth commented Feb 3, 2017

oli-obk commented Feb 3, 2017

bors commented Feb 4, 2017

nrc commented Feb 7, 2017

nrc commented Feb 7, 2017

Manishearth commented Feb 7, 2017

nrc commented Feb 7, 2017

Manishearth commented Feb 7, 2017

nikomatsakis commented Feb 7, 2017

Manishearth commented Feb 7, 2017

nikomatsakis commented Feb 7, 2017

oli-obk commented Feb 7, 2017

nrc commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Feb 7, 2017

mrhota commented Mar 28, 2017

oli-obk commented Mar 28, 2017

alexcrichton commented Apr 1, 2017

oli-obk commented Feb 2, 2017 •

edited

Loading