Spans for unused declarations don't need to be whole declaration

[sophiajt]

Currently, the spans for unused declarations, like enums and fns, span the whole declaration. Instead of spanning the whole declaration, we now put a single ^ at the beginning to say "it starts here".

But, if you compare unused enums/fns with unused variables an imports, it feels like one is pointing at the beginning and one is pointing at the name:

warning: enum is never used: `R0`, #[warn(dead_code)] on by default
  --> src/main.rs:10:1
   |>
10 |> enum R0 {
   |> ^

warning: function is never used: `bob`, #[warn(dead_code)] on by default
  --> src/main.rs:14:1
   |>
14 |> fn bob() {
   |> ^

warning: unused variable: `program`, #[warn(unused_variables)] on by default
  --> src/main.rs:24:9
   |>
24 |>     let program = Add (box Read, box Neg(box Int(8)));
   |>         ^^^^^^^

warning: unused import, #[warn(unused_imports)] on by default
  --> src/main.rs:19:9
   |>
19 |>     use R0::*;
   |>         ^^^^^^

Shouldn't we instead just underline the name in all cases (assuming there is one)?

Giving us:

warning: enum is never used: `R0`, #[warn(dead_code)] on by default
  --> src/main.rs:10:1
   |>
10 |> enum R0 {
   |>      ^^

warning: function is never used: `bob`, #[warn(dead_code)] on by default
  --> src/main.rs:14:1
   |>
14 |> fn bob() {
   |>    ^^^

warning: unused variable: `program`, #[warn(unused_variables)] on by default
  --> src/main.rs:24:9
   |>
24 |>     let program = Add (box Read, box Neg(box Int(8)));
   |>         ^^^^^^^

warning: unused import, #[warn(unused_imports)] on by default
  --> src/main.rs:19:9
   |>
19 |>     use R0::*;
   |>         ^^^^^^

[nagisa]

To me it feels like we should span either the whole definition or the name but not the “starts” here.

I personally prefer showing the full definition still, because most of the time the cause of the error is in-progress work where I have most of the structure prepared in advance but I haven’t implemented the code which would use it. Seeing the variants/fields etc helps me with remembering why the enum/struct is not used yet and exists in the first place and helps deciding whether I should remove it or not.

[sophiajt]

We've tried doing the whole declaration before (in fact, I think that was the default in the old error style before the new error work). Trouble is, you end up with a lot of underlined area, which makes errors hard to read, especially if you have a few errors in a row like that.

I agree the "starts" here pointer isn't all that helpful here, since it doesn't give your eyes much to go find.

[nagisa]

Trouble is, you end up with a lot of underlined area

Do not underline, then? We did not underline multiline spans before and it was fine.

---

Also, oh the opportunity missed with using |> for all lines rather than just those which contain the span:

   |
10 |> enum R0 {
11 |>     Variant1,
   |      ^~~~~~~~ This variant is not nice
12 |>     Variant2,
13 |> }
    ^ multiline span?

Now you could play the sliding game.

[sophiajt]

cc @nikomatsakis

[sophiajt]

@nagisa - I'm not sure what you're suggesting with "Do not underline, then? We did not underline multiline spans before and it was fine."

We played around some with the |> having other meanings but it ends up often being a bit too subtle. For example, I'm unclear on what the multiline span is in your example. Ideally, having something that can be understood "at a glance" seems to be the best general guideline, though that sometimes take a few tries to get to.

It sounds like we agree that underlining the name is fine.

[nagisa]

What I’m saying that before we didn’t underline any multi-line spans, only printed it out as is. What I’m trying to say that not underlining the whole definition is fine.

[nikomatsakis]

@nagisa we could potentially carry on with that before. However, it only works if there is exactly one span in the set of spans (and that span is multi-line). I guess we could detect this case, but we still need a policy for the more general case where there are additional spans (e.g., borrowck errors).

[nikomatsakis]

@jonathandturner

Shouldn't we instead just underline the name in all cases (assuming there is one)?

Yeah, I think we always assumed that we'd go and alter a lot of these multi-line spans to just highlight the method name, or some other more-limited case -- basically that multi-line spans would be the exception.

[sophiajt]

@nikomatsakis 👍