Rust checker: allow to ignore secondary spans

[da-x]

No description provided.

[w0rp]

Are all secondary spans those that point to where the definition of something is? I would like to ignore those types of spans by default. Those should't be shown as error messages, because they aren't errors. Is there maybe a type for the span like 'suggestion' or 'info'? Add tests for this too.

[da-x]

I am not sure it's always a definition. There are borrow checker messages that may have secondaries that are not relating to where stuff is defined, but where things are first borrowed. I couldn't extract it easily from rustc's source. Maybe @estebank can tell more about the types of secondaries?

[w0rp]

Have a look at the JSON object in :ALEInfo and see what information is there. If there's no other information, we can always look at the message string. I would hope there's something like an error type.

[estebank]

A bunch of comments:

• There can be multiple primary spans in any given error (see #47481 for example).
• The secondary spans do not have extra semantic meaning embedded in the output. They can point to the definition, add necessary context ("can't assign closure bound borrow to external binding"/"external binding defined here"/"this is the closure the borrow is valid for"). Lifetime errors are probably the ones that will be the hardest to deal with and present nicely when inline in an editor, specially if you want to distill it to a single line.
• From a cursory look, you only use the gutter and the lower status line for output. This should be enough for the great majority of the errors, but for things like lifetime errors it'd be nice if you could open a secondary pane with the full text (which is sometimes needed to be able to understand the error).
• It is conceivable that there are errors that point to the place with a problem using a secondary span, while maybe pointing at something less specific with the primary span (as it was before the above referenced PR, for example). This should get better over time, but it is a caveat to be aware of. If you see some like this, file a ticket.
• We want to cater for single line output errors, going as far as having a flag for it, but I don't feel the support is perfect yet.

[w0rp]

@estebank Cool, thank you for all of the information. That's very helpful.

[da-x]

Thanks @estebank!

@w0rp I think it's your call on what to pick between the options: sort, ignore-always, ignore-optionally (the PR at its present state), open-special-window-for-secondaries, or do-nothing. Or if you more ideas that's also great.

[w0rp]

Having read everything, filter out secondary spans completely, then put text from them in the detail key, so you can see extra information with :ALEDetail. The secondary spans aren't errors worth flagging in files, and only seem to be useful for getting some additional information, and that's what :ALEDetail is for.

[w0rp]

I don't understand how multiple primary spans are supposed to work. It makes sense to return a list of errors, and then some additional information about each error. How can an error exist in two locations at the same time?

[estebank]

@w0rp a case off the top of my mind:

error: multiple unused imports
LL | use foo::{bar, qux};
   |           ^^^  ^^^

Before it was

error: unused import
LL | use foo::{bar, qux};
   |           ^^^

error: unused import
LL | use foo::{bar, qux};
   |                ^^^

[w0rp]

I would count those as two errors, not one error with two "spans". If spans are mixed together in one error object, how do you know which secondary spans are associated with which primary spans?

[estebank]

We purposely avoid having cases where different secondary spans refer to different primary spans.

[w0rp]

Could you provide an example, using the JSON format, of how you expect someone to use the JSON information? What should you do if you just want to get an error, and stuff additional information into a string someone can view if they want to?

[w0rp]

Closing due to inactivity.

[dpc]

😞

[w0rp]

@da-x We can do this for now, as the secondary spans are too hard to figure out. Can you add some tests for this?

[w0rp]

Use ale#Var here so the buffer option works.

[da-x]

@w0rp should tests include actual Cargo project execution and letting ALE check it, or something more whitebox-y to drive the JSON parsing code? Some reference to similar tests will be helpful.

[w0rp]

The tests won't execute Cargo itself. Look at test/handler/test_rust_handler.vader. You can provide some output for the handler there. You can Save and restore the setting, and you can pass in the same test data with secondary spans.

[RyanSquared]

@da-x do you mind if I push some commits containing test?

[dpc]

@da-x do you mind if I push some commits containing test?

Go for it. Whatever it takes to push this forward. :)

[da-x]

@RyanSquared Go ahead, every help is appreciated!

[w0rp]

@RyanSquared Feel free to merge this one after some decent tests have been added.

[RyanSquared]

I found a potential issue. JSON dump below:

{
  "message": "multiple unused formatting arguments",
  "code": null,
  "level": "error",
  "spans": [
    {
      "file_name": "<anon>",
      "byte_start": 21,
      "byte_end": 27,
      "line_start": 1,
      "line_end": 1,
      "column_start": 22,
      "column_end": 28,
      "is_primary": false,
      "text": [
        {
          "text": "fn main() { println!(\"Test\", 1, 2) }",
          "highlight_start": 22,
          "highlight_end": 28
        }
      ],
      "label": "multiple missing formatting specifiers",
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "expansion": {
        "span": {
          "file_name": "<println macros>",
          "byte_start": 120,
          "byte_end": 154,
          "line_start": 4,
          "line_end": 4,
          "column_start": 29,
          "column_end": 63,
          "is_primary": false,
          "text": [
            {
              "text": "( $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ) ; } # [",
              "highlight_start": 29,
              "highlight_end": 63
            }
          ],
          "label": null,
          "suggested_replacement": null,
          "suggestion_applicability": null,
          "expansion": {
            "span": {
              "file_name": "<anon>",
              "byte_start": 12,
              "byte_end": 34,
              "line_start": 1,
              "line_end": 1,
              "column_start": 13,
              "column_end": 35,
              "is_primary": false,
              "text": [
                {
                  "text": "fn main() { println!(\"Test\", 1, 2) }",
                  "highlight_start": 13,
                  "highlight_end": 35
                }
              ],
              "label": null,
              "suggested_replacement": null,
              "suggestion_applicability": null,
              "expansion": null
            },
            "macro_decl_name": "println!",
            "def_site_span": {
              "file_name": "<println macros>",
              "byte_start": 0,
              "byte_end": 244,
              "line_start": 1,
              "line_end": 5,
              "column_start": 1,
              "column_end": 78,
              "is_primary": false,
              "text": [
                {
                  "text": "(  ) => ( print ! ( \"\\n\" ) ) ; ( $ ( $ arg : tt ) * ) => (",
                  "highlight_start": 1,
                  "highlight_end": 59
                },
                {
                  "text": "{",
                  "highlight_start": 1,
                  "highlight_end": 2
                },
                {
                  "text": "# [ cfg ( not ( stage0 ) ) ] {",
                  "highlight_start": 1,
                  "highlight_end": 31
                },
                {
                  "text": "( $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ) ; } # [",
                  "highlight_start": 1,
                  "highlight_end": 75
                },
                {
                  "text": "cfg ( stage0 ) ] { print ! ( \"{}\\n\" , format_args ! ( $ ( $ arg ) * ) ) } } )",
                  "highlight_start": 1,
                  "highlight_end": 78
                }
              ],
              "label": null,
              "suggested_replacement": null,
              "suggestion_applicability": null,
              "expansion": null
            }
          }
        },
        "macro_decl_name": "format_args_nl!",
        "def_site_span": null
      }
    },
    {
      "file_name": "<anon>",
      "byte_start": 29,
      "byte_end": 30,
      "line_start": 1,
      "line_end": 1,
      "column_start": 30,
      "column_end": 31,
      "is_primary": true,
      "text": [
        {
          "text": "fn main() { println!(\"Test\", 1, 2) }",
          "highlight_start": 30,
          "highlight_end": 31
        }
      ],
      "label": null,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "expansion": null
    },
    {
      "file_name": "<anon>",
      "byte_start": 32,
      "byte_end": 33,
      "line_start": 1,
      "line_end": 1,
      "column_start": 33,
      "column_end": 34,
      "is_primary": true,
      "text": [
        {
          "text": "fn main() { println!(\"Test\", 1, 2) }",
          "highlight_start": 33,
          "highlight_end": 34
        }
      ],
      "label": null,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "expansion": null
    }
  ],
  "children": [],
  "rendered": "error: multiple unused formatting arguments\n --> <anon>:1:30\n  |\n1 | fn main() { println!(\"Test\", 1, 2) }\n  |                      ------  ^  ^\n  |                      |\n  |                      multiple missing formatting specifiers\n\n"
}
{
  "message": "aborting due to previous error",
  "code": null,
  "level": "error",
  "spans": [],
  "children": [],
  "rendered": "error: aborting due to previous error\n\n"
}

As is shown in the above JSON, there is a case where an is_primary: true value does not have a label, but a secondary one does. In the current PR, it is possible for the message to be completely unnoticed due to the lack of a label.

This could possibly be an issue with Rust. Either way, for now, I am putting my development on the tests on hold due to this issue (and this is why we do tests!!!).

I think, if this is intended behavior, then we should use a "candidate" system, which could unfortunately result in only one span being highlighted per section. If no label exist, the message is updated regardless of primary-ness. If a label does exist, and the new span is primary, the message is updated. I'm not sure how I would deal with multiple primary spans.

[estebank]

@RyanSquared that can be considered a bug in the Rust output. The primary spans weren't given a label as that would have added too much clutter:

error: multiple unused formatting arguments
 --> src/main.rs:1:30
  |
1 | fn main() { println!("Test", 1, 2) }
  |                      ------  ^  ^
  |                      |
  |                      multiple missing formatting specifiers

but in this case we could switch the spans around and make the string literal the primary span (which makes sense, it's what needs to be fixed half of the time). This would reverse the emphasis of the error but not the spirit of it (you have a mismatch between formatting specifiers and arguments).

Created rust-lang/rust#55350 as a follow up.

[RyanSquared]

Tests that broke are not relevant to the current changes, and I think this should merge cleanly into master. I tested it out locally and it works fine. @w0rp looks good?

[w0rp]

Cheers! 🍻