Multiline highlights in diagnostic get noisy quickly

[killercup]

This code (playground)

trait Foo<'a, 'b> {
    type Bar;
}

struct Baz {}

struct Dolor<'c, 'd> {
    sit: &'d &'c str,
}

impl<'a, 'b> Foo<'a, 'b> for Baz where 'a: 'b {
    type Bar = &'a Dolor<
        'a,
        'b
    >;
}

fn main() {}

generates these diagnostic messages:

rustc 1.15.0-nightly (daf8c1dfc 2016-12-05)
error[E0491]: in type `&'d &'c str`, reference has a longer lifetime than the data it references
 --> <anon>:8:5
  |
8 |     sit: &'d &'c str,
  |     ^^^^^^^^^^^^^^^^
  |
note: the pointer is valid for the lifetime 'd as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 |   struct Dolor<'c, 'd> {
  |  _^ starting here...
8 | |     sit: &'d &'c str,
9 | | }
  | |_^ ...ending here
note: but the referenced data is only valid for the lifetime 'c as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 |   struct Dolor<'c, 'd> {
  |  _^ starting here...
8 | |     sit: &'d &'c str,
9 | | }
  | |_^ ...ending here

error[E0491]: in type `&'a Dolor<'a, 'b>`, reference has a longer lifetime than the data it references
  --> <anon>:12:5
   |
12 |       type Bar = &'a Dolor<
   |  _____^ starting here...
13 | |         'a,
14 | |         'b
15 | |     >;
   | |______^ ...ending here
   |
   = note: the pointer is valid for the lifetime 'a as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))
   = note: but the referenced data is only valid for the lifetime 'b as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))

error: aborting due to 2 previous errors

Especially the markers on the struct are making it pretty weird to read/recognize the code. (Colored output makes it a bit easier to differentiate the code symbols from the diagnostics ascii art though! 😄)

There probably are some opportunities to make concrete errors highlight more specific parts of the code. Sadly, this only gets worse for less straightforward code. Here's an example from an error I recently saw while fiddling with a macro in diesel:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
   --> src/macros/insertable.rs:473:13
    |
473 |               type Values = (
    |  _____________^ starting here...
474 | |                 ColumnInsertValue<
475 | |                     posts::tags,
476 | |                     AsExpr<&'insert &'a [&'a str], posts::tags>,
477 | |                 >,
478 | |             );
    | |______________^ ...ending here
    |
    = note: first, the lifetime cannot outlive the lifetime 'insert as defined on unknown free region bounded by scope CodeExtent(19922/DestructionScope(NodeId(398)))...
note: ...so that types are compatible (expected expression::AsExpression<pg::types::sql_types::Array<types::Text>>, found expression::AsExpression<pg::types::sql_types::Array<types::Text>>)

This would require highlighting both 'as, right?

cc @estebank who worked on #37369

[estebank]

I understand that there're some cases where highlighting the first char of the span as it was done before would be less noisy. I think that those cases can still be identified and select the desired output span (only the first line always) specifically and leave multiline output by default.

I wonder if the starting here/ending here messages are contributing quite a bit to making these messages too clunky. With those messages removed these look a bit easier on the eyes without the highlighting (that would be a very small change):

rustc 1.15.0-nightly (daf8c1dfc 2016-12-05)
error[E0491]: in type `&'d &'c str`, reference has a longer lifetime than the data it references
 --> <anon>:8:5
  |
8 |     sit: &'d &'c str,
  |     ^^^^^^^^^^^^^^^^
  |
note: the pointer is valid for the lifetime 'd as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 |   struct Dolor<'c, 'd> {
  |  _^
8 | |     sit: &'d &'c str,
9 | | }
  | |_^
note: but the referenced data is only valid for the lifetime 'c as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 |   struct Dolor<'c, 'd> {
  |  _^
8 | |     sit: &'d &'c str,
9 | | }
  | |_^

error[E0491]: in type `&'a Dolor<'a, 'b>`, reference has a longer lifetime than the data it references
  --> <anon>:12:5
   |
12 |       type Bar = &'a Dolor<
   |  _____^
13 | |         'a,
14 | |         'b
15 | |     >;
   | |______^
   |
   = note: the pointer is valid for the lifetime 'a as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))
   = note: but the referenced data is only valid for the lifetime 'b as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))

I'm also thinking that situations where the span starts and ends in the first char of the line (or that have only whitespace before the start of the span) could lend themselves to something like:

rustc 1.15.0-nightly (daf8c1dfc 2016-12-05)
error[E0491]: in type `&'d &'c str`, reference has a longer lifetime than the data it references
 --> <anon>:8:5
  |
8 |     sit: &'d &'c str,
  |     ^^^^^^^^^^^^^^^^
  |
note: the pointer is valid for the lifetime 'd as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 | / struct Dolor<'c, 'd> {
8 | |     sit: &'d &'c str,
9 | | }
  | |_^
note: but the referenced data is only valid for the lifetime 'c as defined on the struct at 7:0
 --> <anon>:7:1
  |
7 | / struct Dolor<'c, 'd> {
8 | |     sit: &'d &'c str,
9 | | }
  | |_^

error[E0491]: in type `&'a Dolor<'a, 'b>`, reference has a longer lifetime than the data it references
  --> <anon>:12:5
   |
12 | /     type Bar = &'a Dolor<
13 | |         'a,
14 | |         'b
15 | |     >;
   | |______^
   |
   = note: the pointer is valid for the lifetime 'a as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))
   = note: but the referenced data is only valid for the lifetime 'b as defined on unknown free region bounded by scope CodeExtent(15/DestructionScope(NodeId(33)))

/cc @nikomatsakis @jonathandturner

[killercup]

@estebank, I think the highlights as they are used currently are great when the beginning and end of the highlight are important; but they are less useful when whole blocks are being highlighted, and the error/warning does not refer to a scope.

For example, when the dead code lint marks a whole function as unused, it is currently shown as this:

warning: function is never used: `mk_naive_date`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:131:1
    |
131 |   pub fn mk_naive_date(days: u32) -> NaiveDate {
    |  _^ starting here...
132 | |     let earliest_pg_date = NaiveDate::from_ymd(-4713, 11, 24);
133 | |     let latest_chrono_date = date::MAX;
134 | |     let num_days_representable = (latest_chrono_date - earliest_pg_date).num_days();
135 | |     earliest_pg_date + Duration::days(days as i64 % num_days_representable)
136 | | }
    | |_^ ...ending here

Or, for a one-liner function:

warning: function is never used: `mk_datetime`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:127:1
    |
127 |   pub fn mk_datetime(data: (i64, u32)) -> DateTime<UTC> {
    |  _^ starting here...
128 | |     DateTime::from_utc(mk_naive_datetime(data), UTC)
129 | | }
    | |_^ ...ending here

I think it might make sense to just highlight the lines, like (please bikeshed):

warning: function is never used: `mk_naive_date`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:131:1
    |
131 |>  pub fn mk_naive_date(days: u32) -> NaiveDate {
132 |>      let earliest_pg_date = NaiveDate::from_ymd(-4713, 11, 24);
133 |>      let latest_chrono_date = date::MAX;
134 |>      let num_days_representable = (latest_chrono_date - earliest_pg_date).num_days();
135 |>      earliest_pg_date + Duration::days(days as i64 % num_days_representable)
136 |>  }

I can see that it might be difficult to differentiate when to show "scope markers" and when not to.

[estebank]

For the cases of function never used, I'd prefer to see something like

warning: function is never used: `mk_naive_date`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:131:1
    |
131 | pub fn mk_naive_date(days: u32) -> NaiveDate {
    |        ^^^^^^^^^^^^^

For cases where the entire multispan is actually needed, I wonder if we can just forego the highlighting and do the following, maybe changing the color of the | separating the column number from the code

warning: function is never used: `mk_naive_date`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:131:1
    |
131 | pub fn mk_naive_date(days: u32) -> NaiveDate {
132 |     let earliest_pg_date = NaiveDate::from_ymd(-4713, 11, 24);
133 |     let latest_chrono_date = date::MAX;
134 |     let num_days_representable = (latest_chrono_date - earliest_pg_date).num_days();
135 |     earliest_pg_date + Duration::days(days as i64 % num_days_representable)
136 | }
    ^ this block's help text here

or maybe, avoiding overloading meaning and reducing the special case presentation:

warning: function is never used: `mk_naive_date`, #[warn(dead_code)] on by default
   --> tests/types_roundtrip.rs:131:1
    |
131 | >  pub fn mk_naive_date(days: u32) -> NaiveDate {
132 | |      let earliest_pg_date = NaiveDate::from_ymd(-4713, 11, 24);
133 | |      let latest_chrono_date = date::MAX;
134 | |      let num_days_representable = (latest_chrono_date - earliest_pg_date).num_days();
135 | |      earliest_pg_date + Duration::days(days as i64 % num_days_representable)
136 | >  }
    | ^ this block's help text here

This would only be done when there's only 1 span, it is a multiline span and there's only whitespace before the beginning of the fist line and after the end of the last line.

Thoughts?

[killercup]

@estebank great ideas! Actually just highlighting the method name will probably suffice. Many of these cases will probably work far better when highlighting much less, but more specific code :)

[sophiajt]

Yup, definitely 👍 for highlighting just the name if you mean the whole thing.

There's another case that came up recently where we show the mismatch in the call and the definition. In that case we may where we want to show the function signature, we could also highlight the name and the parameters.

I think either of those requires adding some more span information to the declarations.

[estebank]

@jonathandturner do you think it is better to just add a Span field to the struct called something along the lines of name_span, or just replacing the current span with the name (as I did in #38328) is enough (at least for now). Is there any case where the complete span for a fn/struct/enum block would be needed?