Make suggestion include the line number #42904

estebank · 2017-06-25T20:35:40Z

When there're more than one suggestions in the same diagnostic, they are
displayed in their own block, instead of inline. In order to reduce
confusion, those blocks now display the line number.

New output:

error[E0308]: mismatched types
  --> ../../src/test/ui/block-result/unexpected-return-on-unit.rs:19:5
   |
19 |     foo()
   |     ^^^^^ expected (), found usize
   |
   = note: expected type `()`
              found type `usize`
help: did you mean to add a semicolon here?
   |
19 |     foo();
   |          ^
help: possibly return type missing here?
   |
18 | fn bar() -> usize {
   |          ^^^^^^^^

error: aborting due to previous error(s)

Fix #39152.

rust-highfive · 2017-06-25T20:35:43Z

r? @eddyb

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

eddyb · 2017-06-25T21:15:33Z

r? @nikomatsakis

killercup · 2017-06-26T08:10:33Z

The output could use a bit of spacing, the line numbers aligned with help: seems weird to me. I don't have a good layout suggestions right now, though.

It might also be a good idea to test this with clippy's lints, which have a bunch more suggestions (and there are almost certainly also lints that render multiple suggestions).

cc @mcarton (who has just yesterday reacted to some of my clippy suggestions issues, and I didn't want to tag every clippy maintainer)

nikomatsakis · 2017-06-26T17:52:37Z

Well, r=me on the code anyway, very simple. I think before I chose to exclude the line numbers because it did not correspond to actual source, and I was afraid it might seem confusing, but I guess it's confusing either way, particularly with multiple suggestions.

nikomatsakis · 2017-06-26T17:53:01Z

I agree that somehow the multiple suggestions above look more crowded.

estebank · 2017-06-26T19:08:45Z

@nikomatsakis @killercup would something like the following look better?

error[E0308]: mismatched types
  --> ../../src/test/ui/block-result/unexpected-return-on-unit.rs:19:5
   |
19 |     foo()
   |     ^^^^^ expected (), found usize
   |
   = note: expected type `()`
              found type `usize`
help: did you mean to add a semicolon here?
   |
19 |     foo();
   |          ^
help: possibly return type missing here?
   |
18 | fn bar() -> usize {
   |          ^^^^^^^^
error: aborting due to previous error(s)

nikomatsakis · 2017-06-27T17:14:39Z

That does look better to me.

estebank · 2017-06-28T06:18:37Z

@nikomatsakis new output

killercup · 2017-06-28T19:23:30Z

@estebank looks great!

nikomatsakis · 2017-06-28T20:18:53Z

@estebank that's pretty awesome

nikomatsakis · 2017-06-28T20:20:17Z

src/test/ui/resolve/issue-21221-1.stderr

+11 | use mul1::Mul;
+11 | use mul2::Mul;
+11 | use mul3::Mul;
+11 | use mul4::Mul;


this looks a bit surprising to me -- I wonder if we want to disable line numbers sometimes? or..maybe it's ok. Kind of helps convey that these are distinct alternatives.

I think it might take a second look to recognize the line number is always the same. I'm not sure what scenario you want to optimize for. Many suggestions will probably look like your screenshot.

What do you think about something like:

help: possible candidates are found in other modules, you can import them into scope | 1) | use mul1::Mul; 2) | use mul2::Mul; 3) | use mul3::Mul; 4) | use mul4::Mul; |

Actually, let's think of a more complex example to see where this layout breaks: How does a diagnostic message look that

has multiple alternative,

that span multiple lines,

and each has a highlight/underline?

The underline only happens if there's only one suggestion that is on a single line (didn't want to have fancy multiline markings in the suggestions).

A multiline span suggestion looks the following way at the moment:

error: `<` is interpreted as a start of generic arguments for `usize`, not a comparison --> ../../src/test/ui/issue-22644.rs:20:14 | 19 | < | - not interpreted as comparison 20 | b); | ^ interpreted as generic argument | help: if you want to compare the casted value then write: | 16 | println!("{}", (a 17 | as 18 | usize) |

Two other possible ways to present multiple candidates are

error[E0412]: cannot find type `Mul` in this scope --> $DIR/issue-21221-1.rs:72:16 | 72 | fn getMul() -> Mul { | ^^^ not found in this scope | help: possible candidates are found in other modules, you can import them into scope | 11 | use mul1::Mul; | 11 | use mul2::Mul; | 11 | use mul3::Mul; | 11 | use mul4::Mul; | and 2 other candidates

and

error[E0412]: cannot find type `Mul` in this scope --> $DIR/issue-21221-1.rs:72:16 | 72 | fn getMul() -> Mul { | ^^^ not found in this scope | help: possible candidates are found in other modules, you can import them into scope | 11 | use mul1::Mul; | ^^^^^^^^^^^^^^ 11 | use mul2::Mul; | ^^^^^^^^^^^^^^ 11 | use mul3::Mul; | ^^^^^^^^^^^^^^ 11 | use mul4::Mul; | ^^^^^^^^^^^^^^ and 2 other candidates

Multiple suggestions that span multiple lines would look terrible with the current state of this PR:

error[E0412]: cannot find type `Mul` in this scope --> $DIR/issue-21221-1.rs:72:16 | 72 | fn getMul() -> Mul { | ^^^ not found in this scope | help: possible candidates are found in other modules, you can import them into scope | 11 | use 12 | mul1::Mul; 11 | use 12 | mul2::Mul; 11 | use 12 | mul3::Mul; 11 | use 12 | mul4::Mul; and 2 other candidates

I like the spaced version, with ^^^ iff it's not spanning the full suggestion.

oli-obk · 2017-06-30T08:03:53Z

src/test/ui/resolve/enums-are-namespaced-xc.stderr

-   | use namespaced_enums::Foo::B;
+   |
+12 | use namespaced_enums::Foo::B;
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


if the entire span is the suggestion, I don't think it should put the ^^^ under it

estebank · 2017-07-06T17:57:31Z

@nikomatsakis updated. There're some stylistically dubious lines, but the actual output should be sound.

nikomatsakis · 2017-07-06T18:30:33Z

@bors r+

bors · 2017-07-06T18:30:33Z

📌 Commit 4bffe1f has been approved by nikomatsakis

bors · 2017-07-06T19:51:34Z

🔒 Merge conflict

When there're more than one suggestions in the same diagnostic, they are displayed in their own block, instead of inline. In order to reduce confusion, those blocks now display the line number.

estebank · 2017-07-06T21:37:14Z

@bors r=nikomatsakis

bors · 2017-07-06T21:37:15Z

📌 Commit 697c85a has been approved by nikomatsakis

Make suggestion include the line number When there're more than one suggestions in the same diagnostic, they are displayed in their own block, instead of inline. In order to reduce confusion, those blocks now display the line number. New output: ``` error[E0308]: mismatched types --> ../../src/test/ui/block-result/unexpected-return-on-unit.rs:19:5 | 19 | foo() | ^^^^^ expected (), found usize | = note: expected type `()` found type `usize` help: did you mean to add a semicolon here? | 19 | foo(); | ^ help: possibly return type missing here? | 18 | fn bar() -> usize { | ^^^^^^^^ error: aborting due to previous error(s) ``` Fix #39152.

bors · 2017-07-06T22:22:29Z

⌛ Testing commit 697c85a with merge d2ebb12...

bors · 2017-07-07T00:58:11Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d2ebb12 to master...

rust-highfive assigned eddyb Jun 25, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Jun 25, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2017

estebank force-pushed the number-suggestions branch 2 times, most recently from e797c31 to 89de943 Compare June 26, 2017 18:09

arielb1 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 Jun 27, 2017

nikomatsakis reviewed Jun 28, 2017

View reviewed changes

oli-obk reviewed Jun 30, 2017

View reviewed changes

estebank force-pushed the number-suggestions branch 3 times, most recently from 262daf9 to 4bffe1f Compare July 6, 2017 01:25

nikomatsakis approved these changes Jul 6, 2017

View reviewed changes

estebank added 2 commits July 6, 2017 14:36

Make suggestion include the line number

7c84914

When there're more than one suggestions in the same diagnostic, they are displayed in their own block, instead of inline. In order to reduce confusion, those blocks now display the line number.

Add extra whitespace for suggestions

eb478e2

Only underline suggestion if it is not the only code being shown

697c85a

estebank force-pushed the number-suggestions branch from 4bffe1f to 697c85a Compare July 6, 2017 21:36

bors merged commit 697c85a into rust-lang:master Jul 7, 2017

bors mentioned this pull request Jul 7, 2017

syntax: Apply x as usize < y recovery to type ascription as well #43060

Merged

estebank mentioned this pull request Aug 10, 2017

use suggestions corrupted w/ nearby code when inside items #43769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make suggestion include the line number #42904

Make suggestion include the line number #42904

estebank commented Jun 25, 2017 •

edited

Loading

rust-highfive commented Jun 25, 2017

eddyb commented Jun 25, 2017

killercup commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

estebank commented Jun 26, 2017

nikomatsakis commented Jun 27, 2017

estebank commented Jun 28, 2017

killercup commented Jun 28, 2017

nikomatsakis commented Jun 28, 2017

nikomatsakis Jun 28, 2017

killercup Jun 28, 2017

estebank Jun 28, 2017

oli-obk Jun 30, 2017

oli-obk Jun 30, 2017

estebank Jul 5, 2017

estebank commented Jul 6, 2017

nikomatsakis commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 6, 2017

estebank commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 7, 2017

Make suggestion include the line number #42904

Make suggestion include the line number #42904

Conversation

estebank commented Jun 25, 2017 • edited Loading

rust-highfive commented Jun 25, 2017

eddyb commented Jun 25, 2017

killercup commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

estebank commented Jun 26, 2017

nikomatsakis commented Jun 27, 2017

estebank commented Jun 28, 2017

killercup commented Jun 28, 2017

nikomatsakis commented Jun 28, 2017

nikomatsakis Jun 28, 2017

Choose a reason for hiding this comment

killercup Jun 28, 2017

Choose a reason for hiding this comment

estebank Jun 28, 2017

Choose a reason for hiding this comment

oli-obk Jun 30, 2017

Choose a reason for hiding this comment

oli-obk Jun 30, 2017

Choose a reason for hiding this comment

estebank Jul 5, 2017

Choose a reason for hiding this comment

estebank commented Jul 6, 2017

nikomatsakis commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 6, 2017

estebank commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 6, 2017

bors commented Jul 7, 2017

estebank commented Jun 25, 2017 •

edited

Loading