Handling of primary vs. secondary spans in checkers

[dpc]

rustc output must have changed, and now every time I compile my code and call a method with a wrong number of arguments, Vim will jump to the definition of the function that is called incorrectly, and not the place where it is being called. There's no easy way jump to the actual code that I need to fix, which is slowing me down a lot: I have to manually inspect the compiler output in another terminal, and manually go to the right place.

It seems to me that the output of the compiler is wrong in the first place: the main error file:line message should point to the broken code, not the definition of the function that is called (which is useful, but secondary). Ideally I'd like rust.vim to be able to display both in the compilation error list.

[estebank]

The plugin should point at the first primary span, not at the first span, as sometimes diagnostics will have a secondary span before the main span.

[da-x]

@dpc, is this still relevant with Rust 1.27? I vaguely remember seeing this myself (I work with ALE as a checker) but I wasn't able to create a small working demo. The linked playground from the rust-lang/rust issue shows a single error at the correct place (the call).

[dpc]

It still happens here with neomake

rustc 1.28.0-nightly (f28c7aef7 2018-06-19)

   Compiling brainwiki v0.1.0 (file:///home/dpc/lab/brainwiki)
error[E0061]: this function takes 1 parameter but 2 parameters were supplied                                                                       
  --> src/main.rs:78:19
   |
78 |     state.write().insert_from_dir(&opts.data_dir, 1).unwrap();
   |                   ^^^^^^^^^^^^^^^ expected 1 parameter
   | 
  ::: src/data.rs:78:5
   |
78 |     pub fn insert_from_dir(&mut self, dir_path: &Path) -> Result<()> {                                                                        
   |     ---------------------------------------------------------------- defined here   

I don't quite understand why, since the right place is first. I have a neomake set to do the check on write.

[da-x]

Thanks - I managed to reproduce the issue - this problem also afflicts ALE.

Thinking of it, it affects all checkers that sort by line number and don't really do anything with the is_primary boolean in the error message JSON. I am not sure what we can do about Syntastic and Neomake right now, but I have already issued a PR in ALE:

https://github.com/w0rp/ale/pull/1696

Ignoring the secondary spans is one way to solve it. Unfortunately ALE sorts by line numbers too - if we don't want to ignore the secondary spans we will have teach it to sort by an additional key. @w0rp what do you think?

[w0rp]

I don't know what you mean about sorting by an additional key. ALE sorts items by buffer or filename, then by line, then by column.

[w0rp]

Why is there an error message showing where the function is defined at all? Who cares? There's no fault at the function if a function is called incorrectly. That's like blaming Ford if someone crashes a car because they were speeding.

[estebank]

@w0rp it's not about blame, it's about context. We show you the method definition so you have the signature front and center so you can fix your code without having to search for it. If we don't have the span (which we don't for external crates), we show only the compiler's knowledge of the signature (only method name and types) as a note.

We also use secondary spans for many things that can be considered "irrelevant", but that are useful to try and keep head-scratching to a minimum. If you do not care at all about these, you can use the short error format instead (https://github.com/rust-lang/rust/pull/49546) if you're calling rustc from the terminal. For tools such as your's, we differentiate the types of spans precisely so you can ignore the secondary labels if you need to.

The compiler team is making a balancing act between avoiding overwhelming newcomers to the language, not being too obscure for them and not spamming experienced programmers. Because of this the long term plan is to support 3 levels of verbosity: short which may as well have been called oneline (opt-in, linked above), the default which is what you currently see where the compiler has error messages with hints and maybe a short explanation, and teach (opt-in, https://github.com/rust-lang/rust/pull/47652) to provide a ton of information for people that are just starting, comparable to the text in the error code index.

[dpc]

This problem still exists, and it is still being a painful thorn in the Rust in Vim experience.

I think Rust should at very list a flag that or something that would make all the existing software work OK.~ (If that's the --short one, @w0rp please just use that if you don't agree with the original PR).

[w0rp]

The trouble I have with the primary and second spans is that it's difficult to write code for associating secondary spans with primary spans. The Cargo format seems to be roughly like this.

[
   {"spans": [
        {"is_primary": true},
        {"is_primary": false},
        {"is_primary": false},
        {"is_primary": true}
   ]},
   {"spans": [
        {"is_primary": true},
        {"is_primary": false},
        {"is_primary": false},
        {"is_primary": true}
   ]}
]

There are multiple spans for each item, and the location information is in the spans. There can be multiple primary spans for each message. From that it's difficult to figure out which secondary spans might be related to particular primary spans. It's hard to parse "the function call failed here, and here is where the function is defined."

Does the Cargo JSON API guarantee that secondary spans will always appear after primary spans in the Arrays? If it does, then you could figure out how to associate secondary spans with primary spans by remembering what the last primary span was.

APIs like the Language Server Protocol API present diagnostics like so instead.

[
  {
    relatedInformation: [{}, {}]
  },
  {
    relatedInformation: [{}, {}]
  }
]

Each item in the outer array represents one problem in your file, and the objects in the optional relatedInformation arrays can reference other locations. From that parsing "this function call failed, and here's where the function was defined" is easy.

[dpc]

Can there be more than one primary? Oh. I was hopping there is always one and only one primary span. Just like you know ... primary index on the database etc.

This is indeed much more complicated than a typical gcc output or something like that. Maybe there is some existing documentation on how to interpret it, and what is the recommended handling approach for tooling like error checkers.

[estebank]

@dpc the change in #2076 should be a very good heuristic for now. Rustc can emit multiple primary spans for a single error, but in those cases using the first primary spans is a good approximation for the editor to use for annotating, as I'm pretty sure in most real cases all the primary spans are very close to each other, like in this case https://github.com/rust-lang/rust/blob/master/src/test/ui/async-fn-multiple-lifetimes.stderr

I'm without my laptop until next week, but I can help with figuring out how we can improve the situation. We won't be emitting less detailed information, as we want to allow external tools to be have the full context to rebuild the cli output with the json output, but if we can make it easier to do so, there's no reason for us not to change it.