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

Reorder error message parts for single source lines #9

Open
rpjohnst opened this issue Jul 12, 2017 · 8 comments
Open

Reorder error message parts for single source lines #9

rpjohnst opened this issue Jul 12, 2017 · 8 comments

Comments

@rpjohnst
Copy link

I don't know that this is the level of detail that matters yet, but here goes.

The single-line errors don't follow the narrative structure in the order that they're displayed:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^
   |     | |    write to `x` occurs here, while borrow is still in active use
   |     | borrow is later used here, during the call
   |     `x` borrowed here

It would be easier to understand if the vertical lines connecting the spans to the messages could be tweaked somehow to allow the messages to be vertically ordered to match the code's execution order.

@nikomatsakis
Copy link
Owner

This is a good comment. I'm not sure how best to handle the formatting. I think it's mildly orthogonal from the RFC, but still worthy of consideration in general.

@estebank may have thoughts here!

@estebank
Copy link

I see a few solutions to this, none of them exactly nice:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |            ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
   = note: `x` borrowed here
   |
 3 |     x.push(x.pop().unwrap());
   |     ^
   = note: borrow is later used here, during the call
   |
 3 |     x.push(x.pop().unwrap());
   |       ^^^^ 
error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |     `x` borrowed here
   |     | borrow is later used here, during the call
   |     | |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
   |     | |
   |     `x` borrowed here
   |       |
   |       borrow is later used here, during the call    
error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
   |     | |________________
   |     `x` borrowed here |
   |                       |
   |                       borrow is later used here, during the call

Of all of these, I'm partial to the first one, as it is the easiest to read. Diagnostics could have a flag stating that the readability is heavily reliant on the order of the spans. If it is set and the span labels would be rendered out of order, use that presentation, so that we get the more compact representation for:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     this_is_a_long_variable_name.push(x.pop().unwrap());
   |     ---------------------------- ---- ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
   |     |                            |
   |      `x` borrowed here           borrow is later used here, during the call

@nikomatsakis
Copy link
Owner

Of all of these, I'm partial to the first one, as it is the easiest to read.

Funny, this is what we used to do, but we moved away, because it was kind of hard to read -- it felt easier and lighterweight to see the labels "in context".

@Nashenas88
Copy link
Contributor

What about numbering them?

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^
   |     | |    3: write to `x` occurs here, while borrow is still in active use
   |     | 2: borrow is later used here, during the call
   |     1: `x` borrowed here

In that case my vote would be for something like this, where both the numbering and ordering of the text make it very clear (at least to me) of how to read the text:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |     1: `x` borrowed here
   |     | 2: borrow is later used here, during the call
   |     | |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^ 3: write to `x` occurs here, while borrow is still in active use

@rpjohnst
Copy link
Author

Isn't that the wrong ordering for the narrative structure? It should be:

  1. x borrowed here
  2. write to x occurs here, while borrow is still in active use
  3. borrow is later used here, during the call

@nikomatsakis
Copy link
Owner

nikomatsakis commented Aug 30, 2017

Numbering is an interesting idea, though:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^
   |     | |    [2] then, write to `x` occurs here, while borrow is still in active use
   |     | [3] later, borrow is used here, during the call
   |     [1] first, `x` is borrowed here

Note that this particular case -- a function call -- is the "worst case" in terms of violating expectations. The others work more smoothly.

@estebank
Copy link

estebank commented Sep 1, 2017

Coming back to this, I don't think the following looks too bad:

error[E0506]: cannot write to `x` while borrowed
 --> <anon>:4:5
   |
   |     `x` borrowed here
   |     |
 3 |     x.push(x.pop().unwrap());
   |     - ---- ^^^^^^^^^^^^^^^^ write to `x` occurs here, while borrow is still in active use
   |       |
   |       borrow is later used here, during the call

@rpjohnst
Copy link
Author

rpjohnst commented Sep 1, 2017

IMO that one combined with numbering would be the easiest to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants