Span markers in diagnostics can be confusing if text above wraps

[crumblingstatue]

Code:

use std::rc::Rc;
use std::cell::RefCell;

struct MyType;

fn exec_closure<F: Fn() + 'static>(f: F) {}

fn just_do_it(my_arg_1: Rc<RefCell<MyType>>, my_arg_2: &MyType, my_arg_3: Rc<RefCell<MyType>>, my_arg_4: Rc<RefCell<MyType>>){
    exec_closure(move || {
        my_arg_1;
    });
}

fn main() {}

This is how the error is displayed on a 80 column terminal

error[E0507]: cannot move out of captured outer variable in an `Fn` closure
  --> foo.rs:10:9
   |
8  | fn just_do_it(my_arg_1: Rc<RefCell<MyType>>, my_arg_2: &MyType, my_arg_3: R
c<RefCell<MyType>>, my_arg_4: Rc<RefCell<MyType>>){ % Wrapped line %
   |               -------- captured outer variable % Makes it look like it's
                                                      Pointing to my_arg_4 %
9  |     exec_closure(move || {
10 |         my_arg_1;
   |         ^^^^^^^^ cannot move out of captured outer variable in an `Fn` clos
ure

It's even worse in the real-life scenario that inspired this:

The second span marker just lazily points to the top of the closure.

Possible improvements:

• Perhaps the irrelevant part could be omitted:

  error[E0507]: cannot move out of captured outer variable in an `Fn` closure
    --> foo.rs:10:9
     |
  8  | fn just_do_it(my_arg_1: Rc<RefCell<MyType>>, ...
     |               -------- captured outer variable 
     |
  9  |     exec_closure(move || {
  10 |         my_arg_1;
     |         ^^^^^^^^ cannot move out of captured outer variable in an `Fn` clos
  ure
  
  

  An additional advantage of this method is that there is less spam overall. The less irrelevant information there is, the more comprehensible the diagnostic becomes. It can also be applied in both directions. For example, if the relevant arg is in the middle, it could be displayed like fn just_do_it(..., relevant_arg, ...).

• If the overflowing part should be kept for some reason, maybe it could be modified to make
  the span marker fit in somehow.

  error[E0507]: cannot move out of captured outer variable in an `Fn` closure
    --> foo.rs:10:9
     |
  8  | fn just_do_it(my_arg_1: Rc<RefCell<MyType>>, my_arg_2: &MyType, my_arg_3: R
  c<RefCell<MyType>>,-------- my_arg_4: Rc<RefCell<MyType>>){ % Wrapped line %
     |               | captured outer variable
     |
  9  |     exec_closure(move || {
  10 |         my_arg_1;
     |         ^^^^^^^^ cannot move out of captured outer variable in an `Fn` clos
  ure
  
  

• my_arg_1 could be highlighted directly. This would require the use of color or making the font bold.

[estebank]

@nikomatsakis, @GuillaumeGomez, @arielb1, @jonathandturner should we go back to highlighting the part of the code being underlined? If so, should it be highlighted with the same color as the underline, or just use bolding? I can post mentoring instructions if we wish to implement that change.

[GuillaumeGomez]

Well, I like how it is right now. :-/

[estebank]

@GuillaumeGomez what do you think of the following output?

With the code from above:

It only highlights the primary span when it isn't multiline. Looking at that output it wouldn't fix the original issue unless I added the highlighting for secondary spans too.

[GuillaumeGomez]

I have to admit that it looks pretty nice. So for me it's ok.

[estebank]

Other alternatives:

1. All labels highlighted bold, almost same as above, helps with my_arg_1
2. All labels highlighted with the label's color, primary spans override secondary spans
3. All labels highlighted with the label's color, coloring of code follows the underlines approach (smaller spans within larger spans retain the smaller span's color)

Examples for 2:

[nikomatsakis]

I also wonder -- we've talked from time to time about using a pager (like git does) when running rustc from the command line. If we did so, we could also avoid wrapping at 80 terminals.

[GuillaumeGomez]

I don't like this idea. :-/

[estebank]

If we did so, we could also avoid wrapping at 80 terminals.

I don't know if we need to go that far. There're ways of getting the size of the terminal and truncate the output accordingly, within reason (removing indentation, using shorter labels, moving labels to always be below the underline, cutting off code to the right of the span that wouldn't fit, etc.), but don't know if we want to start introducing that kind of behavior in the compiler (and if we do it should have extensive tests).

For the original case presented here, the options presented above could look like this (these would not take care of spans that would fall outside of the printed "window"):

error[E0507]: cannot move out of captured o
uter variable in an `Fn` closure
  --> foo.rs:10:9
   |
8  | fn just_do_it(my_arg_1: Rc<RefCell<...
   |               --------
   |               |
   |               captured outer variable
9  |     exec_closure(move || {
10 |         my_arg_1;
   |         ^^^^^^^^
   |         |
   |         cannot move out of captured ou
ter variable in an `Fn` closure

[nikomatsakis]

@estebank hmm I don't know. I've long wanted some kind of "wrapping" for long labels, but that output you showed doesn't look all that clear to me, and it seems like a shame to invest a lot of effort into it.

That said, some kind of minimal effort where we just truncate a long line (perhaps even to a hardcoded width of 80) if the highlight doesn't include that part might be ok.

error[E0507]: cannot move out of captured outer variable in an `Fn` closure
  --> foo.rs:10:9
   |
8  | fn just_do_it(my_arg_1: Rc<RefCell<MyType>>, ...
   |               -------- captured outer variable 
   |
9  |     exec_closure(move || {
10 |         my_arg_1;
   |         ^^^^^^^^ cannot move out of captured outer variable in an `Fn` clos
ure

That said, I'm curious why people are so negative on the idea of a pager. To me, it's always been a great feature of git that it doesn't spew tons of output out by default. Moreover, the early errors in the compiler are the most useful -- so if you run the compiler on the command line, then you either have to pipe through less yourself (which @crumblingstatue might consider as an interim step :) or scroll back up. The latter is annoying and I can tell you from looking over people's shoulders that they frequently accidentally scroll backup in the messages from previous compilations, thus getting themselves confused. I don't really see why this is such a great experience. If you really like the raw output, of course, you could always do rustc ... | cat or something (or --no-pager...).