Proposal for multiline span comments

compiler
1

Prompted by #24626 and #28011, I created PR 36371 to include the type of missing trait items in errors. While working on this it became obvious that there were two ways of presenting the signature of missing methods:

  • show a pretty printed or one lined version of the signatures for the missing methods
  • show the full span for the method definitions

These two approaches have different problems. For the former, it means reimplementing (or generalizing) the code from rustdoc's AST pretty printer. The second one as the compiler stands now is deficient for method definitions that span multiple lines, as you’d get only the first line presented to you, and you’re back where we started, with incomplete information to fully implement the missing method.

My proposal is to use the actual code as it exists (if the compiler has access to it), and start supporting multiline spans. This support could be limited to X lines, in order to avoid spitting out hundreds of unnecessary lines, in those cases doing what we’re currently doing, point at the first character of the first line.

My proposal for how this would look like one completed, allowing for composability of multiple multispans sharing lines is as follows:

error[E0046]: not all trait items implemented, missing: `bar`, `bay`
  --> file.rs:27:1
   |
15 |       fn bar();
   |       --------- `bar` from trait
...
16 | ┌     fn    bay<'lifetime,
   | â•Ž           ^^^ showing off an error
17 | â•Ž                                      TypeParameterA
18 | â•Ž
19 | ╎ ┌             >(  a   : usize,
   | â•Ž â•Ž                 ----------- error
20 | â•Ž â•Ž                 b: u8  );
   | └╴┼╴╴╴╴^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ some error
   |   └╴╴╴╴╴╴╴╴╴╴╴╴╴---------------- overlapped errors to show off how it could look like
...    
27 |     impl X<usize> for A {
   |     ^ missing `bar`, `bay` in implementation

Other possible ways to present this have been mentioned in the previously mentioned PR:

Right bracketing:

  --> missing-impls.rs:11:5
   |
11 |     fn    bay<                      â•® `bay` from trait
12 |        'lifetime,    TypeParameterA |
13 |            >(  a   : usize,         |
14 |                b: u8  );            â•Ż

Similar to the previous one:

foo(1 + bar(x,  <| wrong type
            y), <|
    z);

Fully enclosing the code (I feel this one as cool as it is, it might be too confusing):

        +-------+
        v       |
foo(1 + bar(x,  | wrong type
            y), |
            ^   |
            +-+
    z);

The other option is to just present the recreated signature from the AST (or even from the actual code, as lifetime information/labels seems to be discarded in the AST), but since there’s already a great way of presenting code and where it comes from, I feel that the presented option is better.

Thoughts?

2

One way of getting around the problem of not showing the actual start of the span in my proposal (which can lead to confusing errors, as pointed out in the PR) could be doing the following:

error[E0046]: not all trait items implemented, missing: `bar`, `bay`
  --> file.rs:27:1
   |
15 |       fn bar();
   |       --------- `bar` from trait
...
16 |       fn    bay<'lifetime,
   |  _____^     ^^^ showing off an error
17 | â•Ž                                      TypeParameterA
18 | â•Ž
19 | â•Ž               >(  a   : usize,
   | â•Ž  _____________-   ----------- error
20 | â•Ž â•Ž                 b: u8  );
   | |_|___^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ some error
   |   |_____________---------------- overlapped errors to show off how it could look like
...    
27 |     impl X<usize> for A {
   |     ^ missing `bar`, `bay` in implementation

In the simple example it could look like:

x |   foo(1 + bar(x,
  |  _________^
x | |             y),
x | |_________^^^^^^^ wrong type
x |       z);

or

x |   foo(1 + bar(x,
  |  _________^
x | |             y),
x | |______________^ wrong type
x |       z);
3

Honestly all of these examples are pretty intimidating to my eyes. I wonder if it would be best to take the text and “scrub it” by removing whitespace, comments, and so forth, and just present that? Maybe even run it through rustfmt =)

2 Likes
4

(I am pretty skeptical of pretty-printing.)

5

Pretty printing using rustfmt is not a bad choice, but I see value on pointing at the actual code that you’d find if you go digging for the sources. Keep in mind that busy example above is busier than we’re ever likely to encounter in real life :slight_smile:

I’ve seen cases of multiline spans where the it is maybe 2 or three lines long, and showing the full code would not be overly verbose. Maybe for the original intent that sent me in this wild goose chase about showing the full signature of missing trait items pretty printing is a better choice, but for the general case, I’d like to be able to show the full span.

I’m currently putting together a proof of concept to see how it’d actually look like the terminal with the last option I presented:

x |   foo(1 + bar(x,
  |  _________^
x | |             y),
  | |______________^ wrong type
x |       z);
6

I got some (far from production ready) code to implement this proposal, and this are the file I tested with and a screenshot of the output:

fn foo(a: u32, b: u32) {
    a + b;
}

fn bar(a: u32, b: u32) {
    a + b;
}

fn main() {
    let x = 1;
    let y = 2;
    let z = 3;
    foo(1 + bar(x,
            y),
        z)
}

Screen Shot 2016-10-21 at 22.33.21.png1115Ă—514 51.7 KB

Looking at it now, adding some label like "starting here..." at the beginning and "..ending here:" at the end might make it easier to read and understand. Coloring to the end of the lines to every single line in between can be done, but that might include comments (wouldn't that be wrong?).

I haven't been able to come up with code that would have two overlapping multiline spans in the same error. If you have any ideas of one let me know so I can use it when testing this out.

7

After getting the code in a better shape for a PR, this is the new look:

Screen Shot 2016-10-23 at 17.18.55.png1305Ă—507 51.7 KB

Do people think there's value in this kind of presentation?

1 Like
8

It would be good to see what this error looks like side-by-side with the current system, and the old system.

9

The current behaviour for multiline spans is to point at the first character of the span. For the example given above, it looks like this:

error[E0277]: the trait bound `{integer}: std::ops::Add<()>` is not satisfied
  --> <anon>:13:9
   |
13 |     foo(1 + bar(x,
   |         ^ trait `{integer}: std::ops::Add<()>` not satisfied
   |
10

I’m really sad to see that this discussion about multiline spans had stopped. The fact that rustc reports ambiguous spans is just unacceptable. This is especially annoying where the long multi-line method chains are involved. I remember hitting that wall myself a few times, especially when the error was “type-annotation required”. I’ve also seen somebody on users forum in similar situation.

I think that anything will be better than status quo, if the error message will not require users to do a guesswork. It may won’t look pretty, but we may fix the appearance afterwards.

The last version proposed by @ekuber looks quite clean. The only thing I’d change would be to make the whole span red (to be consistent with the short spans) (and then we can maybe get rid of “starting/ending here” annotations). This won’t work with overlapping erros, but do we actually have these?

Alternatively, if we don’t want to show the whole multiline span, we can do one simple change change to solve the ambiguity problem! Instead marking the beginning of span, mark the last characted! This will resolve the most common source of ambiguity – method chains. There would be two exceptions to that rule:

  • For the binary operators, mark the operator itself.
  • For the prefix unary operators (and also if and match), mark the first character of span, just as we’re doing now.
11

I find the latest example pretty attractive; I wonder how it will compose when there are multiple such spans at play? I guess we can make something work. As @krdln says, in such cases, perhaps people will be able to sort it out.

I definitely agree (again, with @krdln) we should make some efforts to avoid multiline spans whereever possible. This was going to be our original strategy but I don’t think we’ve followed up on it very well =(

It seems like the ambiguity problem is biggest on expressions: perhaps the thing to do is instead of just using expr.span, to have some helper function best_span(expr) that can try to pick a best span more intelligently?

12

I find the latest example pretty attractive; I wonder how it will compose when there are multiple such spans at play? I guess we can make something work. As @krdln says, in such cases, perhaps people will be able to sort it out.

The PR has some code to find overlapping multiline spans and put them on their own vertically, as such:

x |   foo(1 + bar(x,
  |  _____^ starting here...
x | |             qux(a,
  | | ____________^ starting here...
x | ||                b),
  | ||_________________^ ...ending here: wrong type
x | |             z),
  | |______________^ ...ending here: wrong type
x |       z);

but it is not extensively tested as I couldn't think of a source file that would cause overlapping multiline spans. If you can think of a test case for this, please let me know.

I definitely agree (again, with @krdln) we should make some efforts to avoid multiline spans whereever possible. This was going to be our original strategy but I don't think we've followed up on it very well =(

The PR simply checks the line length and (arbitrarily) keeps the current behavior for spans of more than 8 lines, using the new lengthier display for any shorter spans. We could also show the first and last line for longer spans:

  x |   foo(1 + bar(x,
    |  _____^ starting here...
... | |
  x | |             z),
    | |______________^ ...ending here: wrong type
  x |       z);

Alternatively, if we don't want to show the whole multiline span, we can do one simple change change to solve the ambiguity problem! Instead marking the beginning of span, mark the last characted! This will resolve the most common source of ambiguity – method chains.

Showing the first and last line using the multiline style outlined above would take care of this too :slight_smile:

13

Cool =)

I guess with some experimentation we could come up with some. But in any case the highlighting code was intended to be unit-testable, so we don't have to gin up code examples, but can just take some source and add some arbitrary spans with labels to see what it renders like in various extreme situations. I'd be interested to see what happens with non-nested examples, like this one (mostly just to make sure it does something sensible):

      X1
      Y1
      X2
      Y2
14

Huh, why does it ever make sense to keep the current behavior?

15

Sometimes the compiler points at a span for an entire trait, which can potentially be thousands of lines long. The current implementation points at the first char, my PR doesn't handle (yet?) snipping the middle to make it less verbose, so I went for the "lets keep the current behavior until a better idea comes up".

It should be presenting something like:

x |   foo(1 + bar(x,
  |  _____^ starting here...
x | |             qux(a,
  | | ____________^ starting here...
x | ||                b),
x | ||            z),
  | ||_____________^ ...ending here: wrong type
x |  |    z);
  |  |_____^ ...ending here: wrong type

It'll probably need testing to make sure it actually does that.

16

When would this ever happen, though? You’re overstepping expression boundaries. I can see it happening only with ambiguous expressions involving operators (which shouldn’t happen in Rust), not functions. :?

17

If this multiline span code were to be used for lifetimes (not that I think that is a good idea), you could have partially overlapping spans. In all other cases I can think of, one span would be included in another.

18

Ah, I see -- I misread this as "we also show", but you're saying you could do this as a future extension (hence the max lines). Seems good.

19

This feature has hit nightly a few days ago, and it is already available in https://play.rust-lang.org/! :grinning:

2 Likes
20

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.