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

Multiline highlights in diagnostic get noisy quickly #38246

Closed
killercup opened this issue Dec 8, 2016 · 7 comments
Closed

Multiline highlights in diagnostic get noisy quickly #38246

killercup opened this issue Dec 8, 2016 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@killercup
Copy link
Member

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
Copy link
Contributor

estebank commented Dec 8, 2016

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
Copy link
Member Author

@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
Copy link
Contributor

estebank commented Dec 9, 2016

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Contributor

@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?

@steveklabnik steveklabnik added the A-diagnostics Area: Messages for errors, warnings, and lints label Dec 14, 2016
@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 20, 2017
…nkov

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

```rust
error[E0072]: recursive type `X` has infinite size
  --> file.rs:10:1
   |
10 | struct X {
   | ^^^^^^^^ recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `X` representable
```

vs

```rust
error[E0072]: recursive type `X` has infinite size
  --> file.rs:10:1
   |
10 |   struct X {
   |  _^ starting here...
11 | |     x: X,
12 | | }
   | |_^ ...ending here: recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `X` representable
```

Re: rust-lang#35965,  rust-lang#38246. Follow up to rust-lang#38328.

r? @jonathandturner
bors added a commit that referenced this issue Apr 21, 2017
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
@Mark-Simulacrum
Copy link
Member

Closing as per #41214 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants