Minimize single span suggestions into a label #40851

oli-obk · 2017-03-27T12:07:04Z

changes

14 |     println!("☃{}", tup[0]);
   |                     ^^^^^^
   |
help: to access tuple elements, use tuple indexing syntax as shown
   |     println!("☃{}", tup.0);

into

14 |     println!("☃{}", tup[0]);
   |                     ^^^^^^ to access tuple elements, use `tup.0`

Also makes suggestions explicit in the backend in preparation of adding multiple suggestions to a single diagnostic. Currently that's already possible, but results in a full help message + modified code snippet per suggestion, and has no rate limit (might show 100+ suggestions).

rust-highfive · 2017-03-27T12:07:08Z

r? @arielb1

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

oli-obk · 2017-03-27T12:07:51Z

r? @nrc

cc @jonathandturner
cc @petrochenkov

nrc · 2017-03-27T18:45:54Z

cc @rust-lang/tools

nrc · 2017-03-27T18:50:55Z

This seems like a good idea because it puts the suggestion closer to the problem and because it is using less space. However, I have two worries:

how does it look when there is already a label on a span? I feel that it could be confusing having two labels for different things on the same (or similar) spans
I think we should be clear about the status of what the compiler is telling us. I.e., what is a problem that must be fixed, and what is a suggestion that doesn't have to be followed. That might be as simple as keeping the note: prefix for the label (and making it green like the out-of-line helps are). But perhaps we can do better than that?

nrc · 2017-03-27T18:53:40Z

src/librustc_errors/diagnostic.rs

-                 })));
+    /// See `diagnostic::CodeSuggestion` for more information.
+    pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
+        assert!(self.suggestion.is_none());


It surprises me that we never have multiple suggestions, but I guess part of your plan is to do this properly in the long run?

nrc

Code looks fine, I'd like to wait and hear from others (in particular @jonathandturner ) about the UX of this change before r+ing

nrc · 2017-03-27T18:54:22Z

src/librustc_errors/diagnostic.rs

-                     substitutes: vec![suggestion],
-                 })));
+    /// See `diagnostic::CodeSuggestion` for more information.
+    pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {


I think it is worth keeping the Into<MultiSpan> stuff for consistency with the other methods, unless there is a specific reason to remove it.

The reason is that using a multispan will panic in the original emitter code

nrc · 2017-03-27T18:56:07Z

src/librustc_errors/diagnostic.rs

@@ -11,7 +11,6 @@
 use CodeSuggestion;
 use Level;
 use RenderSpan;
-use RenderSpan::Suggestion;


Can we remove RenderSpan::Suggestion now or is it still used elsewhere?

I left it in for keeping the diff small. I convert to it in the emitter and json impls. Wanted to get feedback on a readable diff first

sophiajt · 2017-03-27T18:58:39Z

Yeah I like the general idea, though I have the same question as @nrc. What does this look like where there's already a primary label?

petrochenkov · 2017-03-27T22:31:47Z

src/test/ui/span/issue-39018.stderr

@@ -2,11 +2,9 @@ error[E0369]: binary operation `+` cannot be applied to type `&'static str`
  --> $DIR/issue-39018.rs:12:13
   |
 12 |     let x = "Hello " + "World!";
-   |             ^^^^^^^^
+   |             ^^^^^^^^ `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left: `"Hello ".to_owned()`


Labels, in general, look better then they are short, maybe as long as this

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

, but no longer if possible.
Maybe long labels can be shortened somehow, and super long ones splitted?

Totally agreed and good catch. My rule of thumb has been "6 words, only rarely more"

Yea I was considering also doing a length check. Would it be entirely too confusing to add such an arbitrary limit in the emitter? I mean, someone using span_suggestion might be trying to get their suggestion into a label, but the rule is pretty hidden, even if added to the docs

In this specific case I'd like to move the detailed description to the error code description and just write "you can fix this by allocating the string on the left: "Hello".to_owned()"

oli-obk · 2017-03-28T06:10:09Z

Span_label + span_suggestion looks like span_label twice. So you get vertical lines always and ------ if the spans aren't equal.

I wanted to move the note in the string addition error to a label, because it would make more sense there, but that's ugly, since the suggestion works on a different span, and the label isn't behind the ^^^^^^^.

--- a/src/test/ui/span/issue-39018.stderr
+++ b/src/test/ui/span/issue-39018.stderr
@@ -2,9 +2,10 @@ error[E0369]: binary operation `+` cannot be applied to type `&'static str`
   --> $DIR/issue-39018.rs:12:13
    |
 12 |     let x = "Hello " + "World!";
-   |             ^^^^^^^^ `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left: `"Hello ".to_owned()`
-   |
-   = note: `+` can't be used to concatenate two `&str` strings
+   |             ^^^^^^^^-----------
+   |             |
+   |             `+` can't be used to concatenate two `&str` strings
+   |             `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left. `"Hello ".to_owned()`
 
 error[E0369]: binary operation `+` cannot be applied to type `World`
   --> $DIR/issue-39018.rs:17:13

oli-obk · 2017-03-28T07:46:28Z

I think we should be clear about the status of what the compiler is telling us. I.e., what is a problem that must be fixed, and what is a suggestion that doesn't have to be followed. That might be as simple as keeping the note: prefix for the label (and making it green like the out-of-line helps are). But perhaps we can do better than that?

I believe it is up to the diagnostic author to produce a good message that makes it obvious that it's a suggestion and not a problem. Something like did you mean, try, maybe you meant, ...

nrc · 2017-03-28T20:04:44Z

I believe it is up to the diagnostic author to produce a good message that makes it obvious that it's a suggestion and not a problem. Something like did you mean, try, maybe you meant, ...

I think this is too subtle for communicating to the author and relying too much on future devs doing the right thing - it is a bit like designing an API where you are assuming the client will write bug-free code rather than coding defensively in the API.

oli-obk · 2017-03-29T07:17:18Z

I think this is too subtle for communicating to the author and relying too much on future devs doing the right thing - it is a bit like designing an API where you are assuming the client will write bug-free code rather than coding defensively in the API.

Makes total sense. The extreme variant would be to create an enum, where the suggestion author can choose between

did you mean
try
maybe you meant
Custom(&'static str)

which will be prepended before the code snippet. The author can then choose to leave out the main message, or include the main message if it contains some sort of extra information other than "hey this will fix your code" (like an explanation or sth).

By requiring such an enum to be passed to span_suggestion noone can accidentally forget to pass some form of suggestion text.

bors · 2017-03-30T09:29:17Z

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

oli-obk · 2017-03-31T07:46:29Z

rebased. The only open question is how to ensure that the API enforces that the message will clearly be a suggestion

Possible solutions:

add a new enum so the lint author can choose a message
always add suggestion: before the label
something else?

nrc · 2017-04-03T23:38:34Z

always add suggestion: before the label

works for me - it is nice and simple and (IMO) clear to the user. I'd also be happy to use help:, I don't think that has caused confusion in the past and is shorter.

carols10cents · 2017-04-17T12:14:59Z

Friendly ping for @oli-obk! This PR misses you! ❤️

shepmaster · 2017-04-21T14:31:51Z

Friendly pings for @oli-obk all around! Do you still plan on being able to address the last bit of suggestions?

oli-obk · 2017-04-21T14:59:36Z

Yes. But not before next week

oli-obk · 2017-04-25T11:38:51Z

I added the help: prefix before all suggestions that are rendered as labels

carols10cents · 2017-04-25T13:20:47Z

Reassigning to @jonathandturner for re-review since nrc is out with 👶

sophiajt · 2017-04-27T01:09:31Z

Trying to remember this one. IIRC - there was some question about how the label handles larger notes. Do we check for the size of the note before we turn it into a label?

oli-obk · 2017-04-27T06:38:16Z

src/librustc_errors/emitter.rs

+            // don't display multispans as labels
+            if sugg.substitutes.len() == 1 &&
+               // don't display long messages as labels
+               sugg.msg.split_whitespace().count() < 10 &&


The rules about message length ate defined here. Currently only single line messages with fewer than 10 words are accepted

oli-obk · 2017-04-27T11:36:38Z

The travis failure is bogus. One worker simply didn't start.

carols10cents · 2017-04-27T13:29:30Z

Logged that failure in our infrastructure tracking of spurious failures and i've restarted the travis build.

sophiajt · 2017-04-30T23:10:20Z

I'm 👍 on how this looks. Seems like an overall improvement.

Are all the code questions ironed out? If we're good to go, I'll approve.

oli-obk · 2017-05-01T08:18:32Z

There's a cleanup scheduled for a future PR and multiple suggestions also come later. I went through all comments again and can't find any open questions

sophiajt · 2017-05-01T23:17:29Z

@bors r+

bors · 2017-05-01T23:17:30Z

📌 Commit d64af4a has been approved by jonathandturner

bors · 2017-05-02T01:04:32Z

⌛ Testing commit d64af4a with merge 33535af...

Minimize single span suggestions into a label changes ``` 14 | println!("☃{}", tup[0]); | ^^^^^^ | help: to access tuple elements, use tuple indexing syntax as shown | println!("☃{}", tup.0); ``` into ``` 14 | println!("☃{}", tup[0]); | ^^^^^^ to access tuple elements, use `tup.0` ``` Also makes suggestions explicit in the backend in preparation of adding multiple suggestions to a single diagnostic. Currently that's already possible, but results in a full help message + modified code snippet per suggestion, and has no rate limit (might show 100+ suggestions).

bors · 2017-05-02T03:26:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jonathandturner
Pushing 33535af to master...

rust-highfive assigned arielb1 Mar 27, 2017

rust-highfive assigned nrc and unassigned arielb1 Mar 27, 2017

nrc reviewed Mar 27, 2017

View reviewed changes

petrochenkov reviewed Mar 27, 2017

View reviewed changes

oli-obk force-pushed the multisugg branch from 8fa7017 to ff763e3 Compare March 29, 2017 10:38

oli-obk force-pushed the multisugg branch from ff763e3 to 36cd9ac Compare March 30, 2017 11:15

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

oli-obk added 6 commits April 25, 2017 11:04

Minimize single span suggestions into a note

0e920fd

Update affected tests

b857a1a

Simplify a suggestion for str addition

ca701d7

Address PR comments

3a5567b

Improve E0178 suggestion placement

f4b1e2a

Fix a parse-fail test

e0655a0

Rebase and address comments

d64af4a

oli-obk force-pushed the multisugg branch from 36cd9ac to d64af4a Compare April 25, 2017 11:36

carols10cents 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 Apr 25, 2017

carols10cents assigned sophiajt and unassigned nrc Apr 25, 2017

oli-obk commented Apr 27, 2017

View reviewed changes

bors merged commit d64af4a into rust-lang:master May 2, 2017

This was referenced May 2, 2017

Consider changing to & for let bindings #40402 #41640

Merged

Reorder span_suggestions to appear below main labels #40851 #41698

Closed

ishitatsuyuki mentioned this pull request May 2, 2017

Compilation breakage: rustc_errors::CodeSuggestion rust-lang/rust-clippy#1714

Closed

oli-obk deleted the multisugg branch May 17, 2017 06:50

estebank mentioned this pull request Jun 25, 2017

Make suggestion include the line number #42904

Merged

Minimize single span suggestions into a label #40851

Minimize single span suggestions into a label #40851

Conversation

oli-obk commented Mar 27, 2017

rust-highfive commented Mar 27, 2017

oli-obk commented Mar 27, 2017

nrc commented Mar 27, 2017

nrc commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc left a 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

sophiajt commented Mar 27, 2017

Choose a reason for hiding this comment

sophiajt Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 28, 2017 • edited Loading

oli-obk commented Mar 28, 2017

nrc commented Mar 28, 2017

oli-obk commented Mar 29, 2017

bors commented Mar 30, 2017

oli-obk commented Mar 31, 2017

nrc commented Apr 3, 2017

carols10cents commented Apr 17, 2017

shepmaster commented Apr 21, 2017

oli-obk commented Apr 21, 2017

oli-obk commented Apr 25, 2017

carols10cents commented Apr 25, 2017

sophiajt commented Apr 27, 2017

Choose a reason for hiding this comment

oli-obk commented Apr 27, 2017

carols10cents commented Apr 27, 2017

sophiajt commented Apr 30, 2017

oli-obk commented May 1, 2017

sophiajt commented May 1, 2017

bors commented May 1, 2017

bors commented May 2, 2017

bors commented May 2, 2017

sophiajt Mar 27, 2017 •

edited

Loading

oli-obk commented Mar 28, 2017 •

edited

Loading