Unused imports are too conservative

[alexcrichton]

use foo::Bar;

mod foo {
    pub type Bar = i32;
}

fn baz() -> Bar { 3 }

fn main() {
    use foo::Bar; // this should trigger an unused import warning
    let _a: Bar = 3;
    baz();
}

Right now that program has no unused import warnings, but the second one can be considered an unused import. The reason it's not unused is because it brings a name into the current scope, which is indeed then used. The catch is that the name it brings into scope is the exact same as what it was before.

It'd be nice if resolve caught this and basically didn't bring in any metadata about the inner use foo::Bar import (only because it's already in scope by a previous import).

Not critical, but it would help us cut down on imports!

[reem]

Triage: This has not changed status, but would be very useful.

[steveklabnik]

Triage: no change, other than using usize.

[frewsxcv]

Triage: no change.

[orium]

I would rather have a "redundant import" warning, because the import is technically used. Something like:

warning: redundant import: `foo::Bar`
  --> main.rs:10:9
   |
10 |     use foo::Bar;
   |         ^^^^^^^^
   |
   = note: `foo::Bar` was already in scope.

[fabric-and-ink]

Started working on this.

[petrochenkov]

@fabric-and-ink
As orium said, the import is actually used (via let _a: Bar), so we should detect redundant imports rather than unused imports that are already detected.

Before looking into implementation we need to understand what it means for an import to be redundant from the language point of view.
(To be continued...)

[petrochenkov]

First, we assume that the import is used (otherwise it would already be reported).

Used import is redundant if all its uses will reroute to some other name introduction (definition, import, --extern name, whatever) with the same definition (Def) if the import is removed.

Import foo can be used in three ways:

1. Module-relative use (prefix::foo).
2. Use from the current scope (simply foo without any prefix::).
3. pub use foo; defined in a normal module (not a nameless block) is effectively used because it can be used from other crates.

Right now we don't track whether items are used "module-relatively" or from current scope.
We have to track this (at least partially, enum ModuleRelativeUse { Yes, No, Maybe }) if we want to report redundant imports because module-relatively used imports can never be redundant.

Note however, that imports in nameless blocks can never be used module-relatively.
So, if we want a more conservative lint that would cover only imports in nameless blocks (like in the original example in #10178 (comment)), then we don't need to track the kinds of use.

If the import is not pub-in-mod and definitely not module-relatively used, then we only need to cover the case 2..
(To be continued...)

[petrochenkov]

An import foo is redundant during in-scope resolution if every its use will resolve to the same Def introduced in some outer scope if the import is removed or somehow blacklisted.

Instead of "every use" we can invent a single use located in the same position as the import in question and attempt to resolve it with foo blacklisted and check whether the result is the same Def.

In fact we already have this blacklisting mechanism (Resolver::blacklisted_binding), so to implement the lint we need to:

• Go to fn finalize_import.
• Set the blacklisted binding.
• Call early_resolve_ident_in_lexical_scope with the name introduced by the import and same ParentScope that the import has.
• Check the result and report the lint if its Def is the same.
• The check needs to be performed in all three namespaces.

[petrochenkov]

Caveat: something::Specific in

use something::*;
use something::Specific;

is not always a redundant import due to "glob vs glob" ambiguities and restricted shadowing, and it's hard to detect the cases where it's actually not redundant.
Perhaps we need to extend blacklisted_binding to support multiple bindings and blacklist both the current import and its shadowed_glob.