Reduce visual clutter of multiline start when possible #41245

estebank · 2017-04-12T09:19:22Z

When a span starts on a line with nothing but whitespace to the left,
and there are no other annotations in that line, simplify the visual
representation of the span.

Go from:

error[E0072]: recursive type `A` has infinite size
 --> file2.rs:1:1
  |
1 |   struct A {
  |  _^ starting here...
2 | |     a: A,
3 | | }
  | |_^ ...ending here: recursive type has infinite size
  |

To:

error[E0072]: recursive type `A` has infinite size
 --> file2.rs:1:1
  |
1 | / struct A {
2 | |     a: A,
3 | | }
  | |_^ recursive type has infinite size

Re: #38246.

r? @nikomatsakis CC @jonathandturner

estebank · 2017-04-12T09:20:24Z

instead of

nikomatsakis · 2017-04-12T12:14:48Z

Seems better. I would still like to make progress on only highlighting the struct name etc, though I know that's a deeper edit.

nikomatsakis · 2017-04-12T12:15:10Z

I wonder if the ...ending here: feels confusing without the starting here...?

estebank · 2017-04-12T18:19:32Z

I would still like to make progress on only highlighting the struct name etc, though I know that's a deeper edit.

#41214 is an attempt at doing the smallest patch possible that would accomplish that.

I wonder if the ...ending here: feels confusing without the starting here...?

Would it make sense to remove the starting here.../...ending here completely, @nikomatsakis? I sometimes feel like they might be overpowering the actual messages...

sophiajt · 2017-04-12T19:06:25Z

@nikomatsakis - seconded on "can we underline just the name". I feel like often, but not always, that's what we want to do.

nikomatsakis · 2017-04-13T18:19:58Z

Would it make sense to remove the starting here.../...ending here completely, @nikomatsakis? I sometimes feel like they might be overpowering the actual messages...

I've wondered the same. I think quite likely yes.

Mark-Simulacrum · 2017-04-14T04:04:29Z

Looks like tests need to be fixed here.

nikomatsakis · 2017-04-14T19:29:47Z

@estebank I like it. One thing I've also wondered: would it be hard to put the label text at the top rather than the bottom? I usually find that a bit more intuitive.

estebank · 2017-04-14T23:09:19Z

@nikomatsakis it wouldn't be hard to do it, but I've been thinking about doing the following

2 |   fn foo() { {
  |  ____^       -
  | | ___________|
  | ||
3 | ||
4 | ||  } }
  | ||__- ^
  | |_____|

instead of

2 |   fn foo() { {
  |   ___^_______-
  |  |___|
  | ||
3 | ||
4 | ||  } }
  | ||  - ^
  | ||__|_|
  |  |__|

which wouldn't work with labels (regardless of on top or the bottom). (Then again, it doesn't work with them in the bottom with that change either.)

I'll add the change to do move the labels on the top to this PR.

estebank · 2017-04-15T00:02:05Z

Also just realized, while moving the labels to the beginning, this PR's change is unworkable, as the you need the line for the label after the first line of the span to be able to show the text. Keeping it in the end, either always or only when the multiline span is the only thing being shown, would be necessary to decrease the vertical space waste.

nikomatsakis · 2017-04-17T23:40:56Z

@estebank

Also just realized, while moving the labels to the beginning, this PR's change is unworkable, as the you need the line for the label after the first line of the span to be able to show the text.

Oh, hmm, good point! You could preserve the total number of lines like so:

error[E0072]: recursive type `A` has infinite size
 --> file2.rs:1:1
  |
1 |   struct A {
  |  _^ starting here...
2 | |     a: A,
3 | \ }
  |

But I imagine that part of the point here is also just not to interject stuff into the middle.

nikomatsakis · 2017-04-18T19:52:23Z

@estebank ok well let's leave labels at the end for now. PR seems good to me. Did you plan to make more changes?

estebank · 2017-04-18T20:56:01Z

@nikomatsakis I just need to clean up the last commit. Shall we merge this with the removal of the starting here.../...ending here text once I've fixed the PR?

nikomatsakis · 2017-04-19T20:46:14Z

Shall we merge this with the removal of the starting here.../...ending here text once I've fixed the PR?

👍 from me

nikomatsakis · 2017-04-20T16:42:29Z

@estebank

Shall we merge this with the removal of the starting here.../...ending here text once I've fixed the PR?

👍

estebank · 2017-04-20T16:51:18Z

@nikomatsakis, I'll r=you once I've fixed the current output discrepancy:

[00:58:15] expected output:
[00:58:15] ------
[00:58:15] error: foo
[00:58:15]  --> test.rs:3:3
[00:58:15]   |
[00:58:15] 3 |      X0 Y0
[00:58:15]   |  ____^__-
[00:58:15]   | | ___|
[00:58:15] 4 | ||   Y1 X1
[00:58:15]   | ||____-__^ `X` is a good letter
[00:58:15]   | |_____|
[00:58:15]   |       `Y` is a good letter too
[00:58:15] 
[00:58:15] ------
[00:58:15] actual output:
[00:58:15] ------
[00:58:15] error: foo
[00:58:15]  --> test.rs:3:3
[00:58:15]   |
[00:58:15] 3 |      X0 Y0
[00:58:15]   |  ____^__-
[00:58:15] 4 | ||   Y1 X1
[00:58:15]   | ||____-__^ `X` is a good letter
[00:58:15]   | |_____|
[00:58:15]   |       `Y` is a good letter too

Is there a way to run only these tests using x.py? I guess "./build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--features" "syntax:0.0.0" "-p" "syntax_pos:0.0.0" "-p" "syntax_ext:0.0.0" "--" should work, right?

Also, I'll have to rebase to fix tests that have changed since I created this PR.

nikomatsakis · 2017-04-20T18:58:05Z

@estebank what test is it? A useful option is --on-fail, so something like this:

./x.py ... --on-fail bash

will put you into a shell with all the environment and stuff setup. It'll print the last command (which failed). So you can copy-and-paste that command and then re-run to your heart's content.

estebank · 2017-04-20T19:27:38Z

@nikomatsakis it's src/libsyntax/test_snippet.rs, but compile-fail-fulldeps/proc-macro fails on my environment, so I wasn't able to test locally. I had tried ./x.py test src/libsyntax --stage 1 -j4 but didn't seem to run any tests, but trying it again it actually worked, weird. Must have made a mistake. Thanks for the tip, that'll will come in handy.

When a span starts on a line with nothing but whitespace to the left, and there are no other annotations in that line, simplify the visual representation of the span. Go from: ```rust error[E0072]: recursive type `A` has infinite size --> file2.rs:1:1 | 1 | struct A { | _^ starting here... 2 | | a: A, 3 | | } | |_^ ...ending here: recursive type has infinite size | ``` To: ```rust error[E0072]: recursive type `A` has infinite size --> file2.rs:1:1 | 1 | / struct A { 2 | | a: A, 3 | | } | |_^ recursive type has infinite size ``` Remove `starting here...`/`...ending here` labels from all multiline diagnostics.

estebank · 2017-04-21T05:05:44Z

@bors r=nikomatsakis

bors · 2017-04-21T05:05:45Z

📌 Commit cc07c35 has been approved by nikomatsakis

bors · 2017-04-21T06:59:30Z

⌛ Testing commit cc07c35 with merge 4ed9500...

@nikomatsakis

Reduce visual clutter of multiline start when possible When a span starts on a line with nothing but whitespace to the left, and there are no other annotations in that line, simplify the visual representation of the span. Go from: ```rust error[E0072]: recursive type `A` has infinite size --> file2.rs:1:1 | 1 | struct A { | _^ starting here... 2 | | a: A, 3 | | } | |_^ ...ending here: recursive type has infinite size | ``` To: ```rust error[E0072]: recursive type `A` has infinite size --> file2.rs:1:1 | 1 | / struct A { 2 | | a: A, 3 | | } | |_^ recursive type has infinite size ``` Re: #38246. r? @nikomatsakis CC @jonathandturner

bors · 2017-04-21T09:44:47Z

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

rust-highfive assigned nikomatsakis Apr 12, 2017

estebank force-pushed the multiline-trim branch from e06dacf to a651d5e Compare April 12, 2017 18:23

estebank force-pushed the multiline-trim branch 2 times, most recently from 4586517 to f52e1df Compare April 12, 2017 22:54

estebank force-pushed the multiline-trim branch from f52e1df to 2d119ff Compare April 13, 2017 23:02

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

estebank force-pushed the multiline-trim branch from 2d119ff to db3c5ff Compare April 14, 2017 15:24

estebank force-pushed the multiline-trim branch from db3c5ff to b3458c9 Compare April 14, 2017 23:05

estebank force-pushed the multiline-trim branch 6 times, most recently from e096cfa to cbbe6a6 Compare April 14, 2017 23:59

estebank force-pushed the multiline-trim branch 2 times, most recently from 44f36f0 to b6da758 Compare April 15, 2017 09:59

nikomatsakis approved these changes Apr 18, 2017

View reviewed changes

estebank force-pushed the multiline-trim branch from b6da758 to ff72621 Compare April 19, 2017 00:17

estebank mentioned this pull request Apr 19, 2017

Add a way to get shorter spans until char for pointing at defs #41214

Merged

estebank force-pushed the multiline-trim branch 2 times, most recently from 6639fd0 to 152a74c Compare April 20, 2017 01:15

estebank force-pushed the multiline-trim branch 2 times, most recently from 8f5afc7 to 0ca2234 Compare April 20, 2017 18:53

estebank force-pushed the multiline-trim branch 2 times, most recently from 4c79c51 to b13eb7c Compare April 20, 2017 22:14

estebank force-pushed the multiline-trim branch from b13eb7c to cc07c35 Compare April 21, 2017 00:31

bors merged commit cc07c35 into rust-lang:master Apr 21, 2017

mcarton mentioned this pull request Apr 23, 2017

Update tests to new format of multiline errors rust-lang/rust-clippy#1692

Merged

kennytm mentioned this pull request May 1, 2017

Fix issue #41652 #41668

Merged

estebank deleted the multiline-trim branch November 9, 2023 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce visual clutter of multiline start when possible #41245

Reduce visual clutter of multiline start when possible #41245

estebank commented Apr 12, 2017 •

edited

Loading

estebank commented Apr 12, 2017

nikomatsakis commented Apr 12, 2017

nikomatsakis commented Apr 12, 2017

estebank commented Apr 12, 2017

sophiajt commented Apr 12, 2017

nikomatsakis commented Apr 13, 2017

Mark-Simulacrum commented Apr 14, 2017

nikomatsakis commented Apr 14, 2017

estebank commented Apr 14, 2017 •

edited

Loading

estebank commented Apr 15, 2017

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 18, 2017

estebank commented Apr 18, 2017

nikomatsakis commented Apr 19, 2017

nikomatsakis commented Apr 20, 2017

estebank commented Apr 20, 2017 •

edited

Loading

nikomatsakis commented Apr 20, 2017

estebank commented Apr 20, 2017

estebank commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

Reduce visual clutter of multiline start when possible #41245

Reduce visual clutter of multiline start when possible #41245

Conversation

estebank commented Apr 12, 2017 • edited Loading

estebank commented Apr 12, 2017

nikomatsakis commented Apr 12, 2017

nikomatsakis commented Apr 12, 2017

estebank commented Apr 12, 2017

sophiajt commented Apr 12, 2017

nikomatsakis commented Apr 13, 2017

Mark-Simulacrum commented Apr 14, 2017

nikomatsakis commented Apr 14, 2017

estebank commented Apr 14, 2017 • edited Loading

estebank commented Apr 15, 2017

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 18, 2017

estebank commented Apr 18, 2017

nikomatsakis commented Apr 19, 2017

nikomatsakis commented Apr 20, 2017

estebank commented Apr 20, 2017 • edited Loading

nikomatsakis commented Apr 20, 2017

estebank commented Apr 20, 2017

estebank commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

estebank commented Apr 12, 2017 •

edited

Loading

estebank commented Apr 14, 2017 •

edited

Loading

estebank commented Apr 20, 2017 •

edited

Loading