Use MultiSpan for unused imports

[fhahn]

This is an initial PR that uses MultiSpan introduced by @mitaa to make the unused imports lint less nosiy (#16132).

However there are still a few open questions about how the lint system should use MultiSpans. At the moment each lint added by add_lint is tied to a single AST node. If we want to use a multispan to bundle a few spans together for a lint , we would probably want to call add_lint with a MultiSpan. But when we bundle multipe lint messages together, there also are multiple AST ids.

The unused imports lint should illustrate the problem quite well. Consider the exmaple from the issue. Ideally we would use a multispan to highlight all unused imports in a single line. This could be done by grouping the lint spans and passing a MultiSpan to add_lint. But as the lint system works at the moment, lints are tied to a single AST node they originated from. What I do in the initial version of the PR is just picking the AST id of the first node, but that's not good.

#![crate_type = "lib"]

use foo::{Foo, Bar, Baz, Qux};

mod foo {
    pub struct Foo;
    pub struct Bar;
    pub struct Baz;
    pub struct Qux;
}

It also breaks down when you use #[allow()] attributes on imports, like

#[allow(unused_imports)]
use foo::{Foo, Bar}; use foo::{Baz, Qux};

It seems like we would have to have use the allow information when grouping spans, but this is currently done when the lint is emitted, not when it's added. Pushing the grouping to the stage where the lints are emitted would have quite an impact on the code, because we would have first collect all instances of a lint that should be emitted, and then group them if it makes sense.

Any ideas/preferences? I think @nrc handled the PR that added MultiSpan, so I hope r? @nrc makes sense.

[mitaa]

It seems like we would have to have use the allow information when grouping spans, but this is currently done when the lint is emitted, not when it's added. Pushing the grouping to the stage where the lints are emitted would have quite an impact on the code, because we would have first collect all instances of a lint that should be emitted, and then group them if it makes sense.

Yes, that I think that would make sense. (I've experimented with a similar approach a few days ago here, though I don't really understand the lint system too well)

One question I have here is how important is it that reported spans are lexically continuous? A MultiSpan does not have to be limited to a single line (you'd probably be forced to change CodeMap::group_spans to do that) and it might be suboptimal if a MultiSpan lint reports a span at line 720 and the next lint reports a span at line 40. Or multiple different lints reported on the same NodeId being far apart of each other.

This isn't currently an issue due to the NodeId visitor and there only being a single span per report, but I'd like to know what the desired or acceptable behaviour would be here for multispans.

[nrc]

Ideally we would use a multispan to highlight all unused imports in a single line

I think this is starting on the wrong foot - a multispan should be used where a single error affects multiple spans in the source code. Each unused import is a separate error, so should be reported separately. To make this concrete, if I fix one of the unused imports, the others are still valid. For a multispan to make sense, I think fixing a single error should make the whole thing go away.

[fhahn]

Yeah that makes sense. I still think using a multispan for unused imports would be nice (making the message more compact), but I guess there are not too many other error messages where using multispans would make sense remotely.

Anyhow, this seems more like a discussion whether #16132 is still valid or should be closed as wont fix. Maybe we should move the discussion there?

[nrc]

OK, looking again, I see a bit more what the original motivation was here. I think it is fine to group the lints for a single import into a single error, and then use the multispan for each item in the import. In which case, we should add the lint for the ast node for the import (which maybe is what we do today anyway?). This feels like it should be possible and maybe even easier than what you have at the moment - just need to group the lints by AST node id.

[fhahn]

I've pushed an updated version of the commit. It now uses the AST node id of the import (previously it used the ids of the path items). At the moment the grouping is done in the unused import lint. This means add_lint has to accept mutlispans. The easiest way to do this (and that's what the PR is doing) is just storing multispans instead of spans in the lint array. But this adds some overhead (because multispans use a Vec internally). Alternatively we could just add multiple unused import lints with plain spans for the same node id and convert them to multispans when emitting the actual lint message.

What do you think?

[nrc]

The failing tests are due to not updating the compile-fail tests which check for unused imports.

[nrc]

Could you give an example of what the error output looks like with this PR please?

[nrc]

can we take spans by reference to avoid the new clones?

[nrc]

I think this approach looks good. I don't think there is too much advantage to grouping spans later. Do you think it could be significant?

[fhahn]

Yes I do not think that grouping spans later would be an advantage either, because I think the right grouping will depend on the particular lints.

For the following program

#![crate_type = "lib"]

//#[allow(unused_imports)]
use foo::{Foo, Bar};
use foo::{Baz,
          Qux};

mod foo {
    pub struct Foo;
    pub struct Bar;
    pub struct Baz;
    pub struct Qux;
}

this output is produced:

test.rs:4:15: 4:23 warning: unused imports, #[warn(unused_imports)] on by default
test.rs:4     use foo::{Foo, Bar};
                                   ^~~  ^~~
test.rs:5:15: 6:18 warning: unused imports, #[warn(unused_imports)] on by default
test.rs:5     use foo::{Baz,
                                   ^~~
test.rs:6               Qux};
                             ^~~

[bors]

☔ The latest upstream changes (presumably #32496) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!

[nrc]

cc @nikomatsakis use of MultiSpan

[nikomatsakis]

Should be easy enough to port this over to the new system. This use case was definitely something we had in mind.