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

Clippy vs. RUST_NEW_ERROR_FORMAT=true #907

Closed
3 tasks
killercup opened this issue May 8, 2016 · 14 comments
Closed
3 tasks

Clippy vs. RUST_NEW_ERROR_FORMAT=true #907

killercup opened this issue May 8, 2016 · 14 comments

Comments

@killercup
Copy link
Member

killercup commented May 8, 2016

Finally, there is a nightly that contains rust-lang/rust#32756! Clippy works mostly fine with it, but there are some minor issues. I posted an example output (PDF) on this internals.r-l.o thread, if you are interested in how this new style looks.

I noticed:

  • Some help messages contain line breaks with file names, e.g.,

    help: instead of prefixing all patterns with `&`, you can dereference the expression
    src/result.rs: match *self { .. }
    help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats
    
  • Some warnings only underline one character in the line of code, e.g.

    80 |>     match self {
       |>     ^
    
  • It is my understanding that the new format allows showing multiple annotations in the same code block, e.g. to show the highlighted spans (warning + note) in lints like "this match has identical arm bodies" (or "cannot borrow as mutable more than once at a time" as in the original example screenshot) next to each other. Clippy should do this!

@mcarton
Copy link
Member

mcarton commented May 8, 2016

(I took the liberty of fixing the link)

Some warnings only underline one character in the line of code

And I noticed in #906 that some spans used to be 0 bytes long, which used to be accepted apparently :(

What version of Clippy is this? There is an error in a link with DOC_MARKDOWN which should not be there.

@killercup
Copy link
Member Author

What version of Clippy is this? There is an error in a link with DOC_MARKDOWN which should not be there.

0.0.65, which was needed to run on the nightly that contains the new error formatting. Should I reopen my issue from last week? :)

@Manishearth
Copy link
Member

We probably should have a flag to hide the wiki links btw

@Manishearth
Copy link
Member

I think the ability to use MultiSpan was always there, btw, we just never used it.

Also, does this break compiletest?

@killercup
Copy link
Member Author

Also, does this break compiletest?

RUST_NEW_ERROR_FORMAT is not set by default, so compiletest should continue to work. (Also, compiletest should switch to rustc's JSON output before adapting to this new human-readable version.)

@mcarton
Copy link
Member

mcarton commented May 8, 2016

I think the ability to use MultiSpan was always there, btw, we just never used it.

IIRC LintContext uses Span, not Into<MultiSpan>.

@Manishearth
Copy link
Member

Oh, I see.

@mcarton
Copy link
Member

mcarton commented May 8, 2016

What version of Clippy is this? There is an error in a link with DOC_MARKDOWN which should not be there.

0.0.65, which was needed to run on the nightly that contains the new error formatting. Should I reopen my issue from last week? :)

I see, that’s an actual bug. On it.

@mcarton
Copy link
Member

mcarton commented May 8, 2016

Some warnings only underline one character in the line of code, e.g.

At least that is not limited to Clippy. I just had

error: missing field `line` in initializer of `doc::check_doc::Iter<_, _>` [--explain E0063]
   --> src/doc.rs:145:16
145 |>     let iter = Iter {
    |>                ^

with rustc itself.

@mcarton
Copy link
Member

mcarton commented May 8, 2016

As for

Some help messages contain line breaks with file names, e.g.,

this is also rustc:

$ cat a.rs
trait Foo{}

fn main() {
    &42 as Foo;
}
$ RUST_NEW_ERROR_FORMAT=true rustc a.rs
error: cast to unsized type: `&_` as `Foo`
 --> a.rs:4:5
4 |>     &42 as Foo;
  |>     ^^^^^^^^^^
help: try casting to a reference instead:
a.rs:      &42 as &Foo;

error: aborting due to previous error

We are using rustc’s span_suggestion function (which rustc uses incredibly few).

@mcarton
Copy link
Member

mcarton commented May 8, 2016

IIRC LintContext uses Span, not Into<MultiSpan>.

Yes, but we don’t really care. We also use DiagnosticBuilder which has the Into<MultiSpan> functions.

@killercup
Copy link
Member Author

By the way:

Some warnings only underline one character in the line of code, e.g.

At least that is not limited to Clippy. I just had […]

Niko writes that

This is the result of a deliberate change we made, which was to convert multi-line spans into a single ^ at the start. This was made to lighten the display of multi-line spans

Assuming this behaviour should stay this way, it may be an option to try to not use multi-line spans and use more additional 'help' spans.

@killercup
Copy link
Member Author

Seeing the great effort of improving rustc's error messages in rust-lang/rust#35233, I'm wondering if it'd make sense to do something similar for clippy as well – even if just to hop onto the "nicer errors" train and add some trivial explanations to some spans. (Stuff like annotating the NaN part in cmp_nan's error message.)

@llogiq
Copy link
Contributor

llogiq commented Jan 15, 2017

I think the new error format is pretty established now within both rustc and clippy, so I'll close this.

@llogiq llogiq closed this as completed Jan 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants