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

Spurious unused import warning with trait used transitively via glob #45268

Closed
jturner314 opened this issue Oct 14, 2017 · 16 comments · Fixed by #60415
Closed

Spurious unused import warning with trait used transitively via glob #45268

jturner314 opened this issue Oct 14, 2017 · 16 comments · Fixed by #60415
Labels

Comments

@jturner314
Copy link

@jturner314 jturner314 commented Oct 14, 2017

Under certain circumstances involving pub(crate) use Trait, the compiler issues an "unused import" warning even though the import is actually being used.

This is an example (playground link):

mod foo {
    pub(crate) use std::ascii::AsciiExt;
}

mod bar {
    use foo::*;
    
    pub fn bar() -> bool {
        "bar".is_ascii()
    }
}

fn main() {
    println!("{}", bar::bar());
}

Since the body of bar::bar() uses the AsciiExt trait (AsciiExt provides .is_ascii()), there shouldn't be a warning.

Instead, this happened:

warning: unused import: `std::ascii::AsciiExt`
 --> src/main.rs:2:20
  |
2 |     pub(crate) use std::ascii::AsciiExt;
  |                    ^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

Applying any of the following changes makes the warning go away:

  • change pub(crate) use std::ascii::AsciiExt to pub use std::ascii::AsciiExt
  • change use foo::* to use foo::AsciiExt
  • change "bar".is_ascii() to AsciiExt::is_ascii("bar")

Meta

This occurs for all versions of the compiler on the playground:

Also, on my machine, rustc --version --verbose:

rustc 1.21.0 (3b72af97e 2017-10-09)
binary: rustc
commit-hash: 3b72af97e42989b2fe104d8edbaee123cdf7c58f
commit-date: 2017-10-09
host: x86_64-unknown-linux-gnu
release: 1.21.0
LLVM version: 4.0
@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Nov 4, 2017

Just ran into this myself today. I'm surprised I haven't run into it earlier.

My own findings are aligned with the OP's:

// build with cargo test

mod tr {
    pub trait Trait: Sized { fn frob(self); }
    
    impl Trait for () { fn frob(self) {} }
}

#[cfg(test)]
mod c {
    use tr::Trait;
    mod c {
        use super::*;
        #[test] fn lol() { ().frob(); }
    }
}
  • replacing use tr::Trait with a copy of contents of mod tr suppresses the warning (so the warning only applies to imports of traits, not traits themselves)
  • unglobbing the glob import suppresses the warning

It has been around since the introduction of item-like imports. Or at least since 1.15, which is the first version in which the snippet compiles.

   Compiling unused v0.1.0 (file:///home/lampam/cpp/throwaway/unused)
warning: unused import: `tr::Trait`, #[warn(unused_imports)] on by default
  --> src/lib.rs:12:9
   |
12 |     use tr::Trait;
   |         ^^^^^^^^^

    Finished debug [unoptimized + debuginfo] target(s) in 0.38 secs
     Running target/debug/deps/unused-a116605afa74f946
@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Nov 4, 2017

Because I am interested in beginning to learn more about the compiler internals, I tried tracing through the relevant logic in the source code. Here's what I think happens:

  • usages of a trait that are only through method resolution cannot be checked by rustc_resolve, so it defers them to be checked by rustc_typeck.
  • Ideally, the lint would be silenced here in typeck, at pretty much the last moment before it is linted. This checks a table called "used_trait_imports".
  • when method resolution identifies a trait method, it places a single import into the used_trait_imports table.
    • I tried enabling debug logs and got this:
      DEBUG:rustc_typeck::check::method: used_trait_import: DefId { krate: CrateNum(0), index: DefIndex(0:11) => lib[317d]::c[0]::c[0]::{{?}}[0] }
      
      I assume {{?}} refers to the glob import.

As a result, the glob import is marked as used, but not the trait import.

Does it sound like this "amateur analysis" is on the right track? I wonder which component ought to be changed to fix this... (could the glob import be followed by method resolution to insert more entries into the table?)

@pmarcelll

This comment has been minimized.

Copy link
Contributor

@pmarcelll pmarcelll commented Dec 4, 2017

This seems to be similar to #43970.

@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Jun 17, 2018

Status update: this bug is still present. I get hit by it increasingly often as I like to use miniature inline modules (with use super::*) for the purposes of ad-hoc namespacing, privacy barriers, and organization.

I still don't have the chops to fix it myself (at least, not without a fair investment of time learning compiler internals).

@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Jun 19, 2018

^--- I took a trip around the tracker and rounded up these four juicy, delicious duplicate bug reports, for anybody with an appetite

@Aeradriel

This comment has been minimized.

Copy link

@Aeradriel Aeradriel commented Jul 18, 2018

I just ran into this kind of issue myself. It looks like if you import a trait and use it only in submodules you will face this false warning. Here is another playground that shows the bug (importing an existing trait from another crate) => Playground

@dbrgn

This comment has been minimized.

Copy link
Contributor

@dbrgn dbrgn commented Sep 3, 2018

Would be fantastic if this could get fixed.

@sanxiyn in #50537, you wrote that you know how to fix this. Maybe if you give some guidance someone could pick up this issue? (I don't feel qualified myself.)

@sanxiyn

This comment has been minimized.

Copy link
Member

@sanxiyn sanxiyn commented Jan 13, 2019

Sorry I dropped the ball, didn't I? @ExpHP's analysis above is completely correct, and "follow glob" suggestion is also what I thought. Source: I wrote the most of relevant code.

I probably don't have the time to do this myself in near future...

@sanxiyn sanxiyn changed the title Incorrect "unused import" warning with `pub(crate) use Trait` and another module Spurious unused import warning with trait used transitively via glob Jan 13, 2019
@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Mar 1, 2019

Is this a thing that can be fixed just by modifying one crate? Or is it much more complicated (due to e.g. incrementalization, query system, rainbow unicorn dust...)

If it is something a beginner could do, do you have any hints on the proper way to do it? Any method names you can easily recall? I guess basically what I need is to be able to get the DefId for a particular trait in the module referenced by the glob import (assuming that the module is crate-local), but the internal API surface of the compiler is huge 😛 ...

@jespersm

This comment has been minimized.

Copy link
Contributor

@jespersm jespersm commented Apr 29, 2019

So, I've made a fix for this -- the meat of it is in Resolver::get_traits_in_module_containing_item: If a binding comes from an import, just follow the import's binding and collect all the import_id's, instead of stopping after the first (repeat until you reach the actual Def).

I've verified that it actually fixes the problem, and I'm running the full test suite now.

Only thing is that then I'd have to change

pub struct TraitCandidate {
    pub def_id: DefId,
    pub import_id: Option<NodeId>,
}

into

pub struct TraitCandidate {
    pub def_id: DefId,
    pub import_ids: Vec<NodeId>,
}

(and similar change to Pick). I'm just concerned this will lead to increased malloc usage. Is there a "SmallVec" or similar that could be used, because I guess it's going to be very rare that there should be more than one or levels of trait imports.

Please advice!

@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented Apr 29, 2019

That's marvelous!

Once the test suite passes, I think you should go ahead and submit a PR. This will give it higher visibility to the people who are best equipped to answer your concern, and it will also enable them to queue up a rustc-perf comparison if deemed necessary.

@jespersm

This comment has been minimized.

Copy link
Contributor

@jespersm jespersm commented Apr 30, 2019

I'm looking into the test failures.

jespersm added a commit to jespersm/rust that referenced this issue May 2, 2019
jespersm added a commit to jespersm/rust that referenced this issue May 4, 2019
jespersm added a commit to jespersm/rust that referenced this issue May 4, 2019
jespersm added a commit to jespersm/rust that referenced this issue May 4, 2019
bors added a commit that referenced this issue May 4, 2019
Fix #45268 by saving all NodeId's for resolved traits.

This fixes #45268 by saving all NodeId's for resolved traits.

I've verifies this against master (but only on MacOS). However, with some recent changes in master, it appears that there are some failures on my machine. They are unrelated to my changes, anyway.
@bors bors closed this in #60415 May 5, 2019
@jespersm

This comment has been minimized.

Copy link
Contributor

@jespersm jespersm commented May 24, 2019

When does a bug like this get merged to stable?

@sanxiyn

This comment has been minimized.

Copy link
Member

@sanxiyn sanxiyn commented May 24, 2019

It takes 3 months unless explicit backport is done.

@ExpHP

This comment has been minimized.

Copy link
Contributor

@ExpHP ExpHP commented May 24, 2019

Every three months, the current master becomes the new beta, and the current beta becomes the new stable.

Therefore, any change takes 3-6 months to get into stable, depending on its timing. Since a new version of rust was just released, there are 3 months remaining for this change.

@sanxiyn

This comment has been minimized.

Copy link
Member

@sanxiyn sanxiyn commented May 25, 2019

Eh no. master->beta and beta->stable happens every 1.5 months. So 3 months is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.