add unnecessary_self_imports lint

[ebobrow]

fixes #6552

changelog: add unnecessary_self_imports lint

[rust-highfive]

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

[flip1995]

The functions you implement to check if something is in scope seem really resource intensive to me. Have you looked in rustc if there is a query to determine if a item is in scope?

[flip1995]

TL;DR: Implement this lint on the AST, not the HIR. It'll be way easier.

I just noticed that you can't really implement this lint on the HIR, since the information if a self was involved in an import isn't available there anymore. It just gets lowered to

use path::to::module::{}; 
use path::to::module;

However on the AST you should be able to determine if it is an import of just self and if it has a as something pretty easily

AST `use` expansion

        Item {
            attrs: [],
            id: NodeId(4294967040),
            span: main.rs:5:1: 5:15 (#0),
            vis: Visibility {
                kind: Inherited,
                span: main.rs:5:1: 5:1 (#0),
                tokens: None,
            },
            ident: #0,
            kind: Use(
                UseTree {
                    prefix: Path {
                        span: main.rs:5:5: 5:6 (#0),
                        segments: [
                            PathSegment {
                                ident: a#0,
                                id: NodeId(4294967040),
                                args: None,
                            },
                        ],
                        tokens: None,
                    },
                    kind: Nested(
                        [
                            (
                                UseTree {
                                    prefix: Path {
                                        span: main.rs:5:9: 5:13 (#0),
                                        segments: [
                                            PathSegment {
                                                ident: self#0,
                                                id: NodeId(4294967040),
                                                args: None,
                                            },
                                        ],
                                        tokens: None,
                                    },
                                    kind: Simple(
                                        None,
                                        NodeId(4294967040),
                                        NodeId(4294967040),
                                    ),
                                    span: main.rs:5:9: 5:13 (#0),
                                },
                                NodeId(4294967040),
                            ),
                        ],
                    ),
                    span: main.rs:5:5: 5:14 (#0),
                },
            ),
            tokens: None,
        },

UseTree Documentation

You'll then have the problem to determine if the imported module is already there in form of another item (at least I think that is what you try to do with the other 2 functions?). But this isn't really a problem. rustc will already error if it is. And if you have an alias, you can just remove the self and use use path::to::module as alias directly.

[camsteffen]

It's a bit more complicated.

• use a::b::{self} -> import the module a::b
• use a::b; -> import the module and/or item a::b

So changing to the latter may cause "b is defined multiple times". (playground)

I think a late pass will be necessary to detect this. And UseKind::ListStem can maybe be used to detect ::{self}? Not sure. Seems hacky.

[flip1995]

* `use a::b;` -> import the module and/or item `a::b`

Checking another case which shouldn't be a concern for this lint: Playground

So you only have to check which items are loaded in the current module. So instead of checking every use item you can just check on module level and walk over all items and lint on use items iff they aren't in scope in another form.

(That is on the AST. I really don't think we can implement this lint on the HIR reliably)

[ebobrow]

Couldn't self still be removed if an item with the same name exists in scope but not in the module? I'm not sure how to check modules in the AST.

[flip1995]

Oh yeah, your right. The item could still be in scope from the surrounding scope... I'm starting to think that there is no feasible way to implement this. Your method works, but I hope that you agree that going over all defined items multiple times is too performance heavy.

Determining what would happen if we change code without recompiling is just really hard in general.

I see two ways going forward:

1. Not adding this lint. I'm not sure how helpful this lint is in the first place. When someone uses ::{self} it is either intentional or just a refactor artifact. It doesn't hurt to have it. An expensive lint for code style isn't justified IMO.
2. Ignore that the item could already be in scope and implement it as a restriction lint with a statement in the documentation that this lint isn't meant to be permanently enabled, but only to quickly check, e.g. after refactoring, if there are artifacts like this in the code.

[camsteffen]

if an item with the same name exists in scope but not in the module

I'm not following...

[ebobrow]

Playground

I think @flip1995 is right. On the HIR, this lint is too resource intensive and it might not be possible on the AST. I don't know if it's worthwhile to have.

[camsteffen]

Hrrrmmm. Yeeahh. mrr

[flip1995]

if an item with the same name exists in scope but not in the module

I'm not following...

Or maybe not... Playground

Anyway, this is way too scuffed to figure out what works and what doesn't when the ::{self} might get removed to make it FP free.

[camsteffen]

Yes too scuffed! I can imagine the restriction lint begin useful as in "if removing ::{self} breaks something, you're doing it wrong."

[ebobrow]

Good point. I'll change this to a restriction lint and put a warning in the documentation.

[camsteffen]

I think we should also

diag.span_note("this will slightly change semantics; any non-module items at the same path will also be imported");

[camsteffen]

It's not necessarily wrong to enable the lint for a project.

[camsteffen]

Just some nits.

[camsteffen]

One more minor suggestion that you can take or leave. Otherwise looks good!

[ebobrow]

Thanks! I like that.

[flip1995]

LGTM now. @camsteffen if you're also ok with this PR, please go ahead and merge it.

@ebobrow Thanks for all your work on it, even though you had to delete most of it again. But now we learned what information about imports are available where, which will help us advising on issues about imports in the future,, which is definitely a win.

[camsteffen]

@bors r+

[bors]

📌 Commit 224881b has been approved by camsteffen

[bors]

⌛ Testing commit 224881b with merge 79b9eb5...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 79b9eb5 to master...