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

Handling of primary vs. secondary spans in checkers #174

Open
dpc opened this issue Sep 16, 2017 · 11 comments
Open

Handling of primary vs. secondary spans in checkers #174

dpc opened this issue Sep 16, 2017 · 11 comments
Assignees
Labels
checkers Pertains to Syntastic, ALE, Neomake, RLS

Comments

@dpc
Copy link

dpc commented Sep 16, 2017

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

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 da-x self-assigned this Jun 30, 2018
@da-x
Copy link
Member

da-x commented Jun 30, 2018

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

dpc commented Jul 1, 2018

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

da-x commented Jul 4, 2018

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:

dense-analysis/ale#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
Copy link

w0rp commented Jul 4, 2018

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

w0rp commented Jul 4, 2018

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.

@da-x da-x changed the title this function takes 1 parameter but 3 parameters were supplied: defined here is killing me :) Handling of primary vs. secondary spans in checkers Jul 4, 2018
@da-x da-x added the checkers Pertains to Syntastic, ALE, Neomake, RLS label Jul 4, 2018
@estebank
Copy link

estebank commented Jul 4, 2018

@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 (rust-lang/rust#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, rust-lang/rust#47652) to provide a ton of information for people that are just starting, comparable to the text in the error code index.

@dpc
Copy link
Author

dpc commented Sep 6, 2018

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).

dpc added a commit to dpc/neomake that referenced this issue Sep 6, 2018
This would be great improvement to the current situation, where the
first span is used, which is quite often not the right one.

See rust-lang/rust.vim#174

Related to: neomake#2075
dpc added a commit to dpc/neomake that referenced this issue Sep 6, 2018
This would be great improvement to the current situation, where the
first span is used, which is quite often not the right one.

See rust-lang/rust.vim#174

Related to: neomake#2075
@w0rp
Copy link

w0rp commented Sep 6, 2018

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

dpc commented Sep 6, 2018

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

estebank commented Sep 7, 2018

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

blueyed pushed a commit to dpc/neomake that referenced this issue Sep 9, 2018
This would be great improvement to the current situation, where the
first span is used, which is quite often not the right one.

See rust-lang/rust.vim#174

Related to: neomake#2075
blueyed pushed a commit to dpc/neomake that referenced this issue Sep 9, 2018
This would be great improvement to the current situation, where the
first span is used, which is quite often not the right one.

See rust-lang/rust.vim#174

Related to: neomake#2075
okhin pushed a commit to okhin/neomake that referenced this issue Sep 11, 2018
This would be great improvement to the current situation, where the
first span is used, which is quite often not the right one.

See rust-lang/rust.vim#174

Related to: neomake#2075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checkers Pertains to Syntastic, ALE, Neomake, RLS
Projects
None yet
Development

No branches or pull requests

4 participants