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

False Positive: unused import #11050

Closed
dc0d opened this issue Sep 4, 2022 · 7 comments
Closed

False Positive: unused import #11050

dc0d opened this issue Sep 4, 2022 · 7 comments
Labels
C-bug Category: bug

Comments

@dc0d
Copy link

dc0d commented Sep 4, 2022

Problem

1 - $ cargo test (and rust analyzer in vscode) say this line is an unused import: use std::collections::HashMap;. While HashMap is used in this line: let mut map: HashMap<String, u8> = HashMap::new();.

What am I missing?

On Mac:

$ rustc --version
rustc 1.63.0 (4b91a6ea7 2022-08-08)

$ cargo --version
cargo 1.63.0 (fd9c4297c 2022-07-01)

Output of cargo test:

$ cargo test
warning: unused import: `std::collections::HashMap`
 --> src/lib.rs:2:5
  |
2 | use std::collections::HashMap;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `nuid` (lib) generated 1 warning
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src/lib.rs (target/debug/deps/nuid-61219c4efa23785a)

running 5 tests
test tests::test_basic_uniqueness ... ignored
test tests::test_digits ... ok
test tests::test_nuid_rollover ... ok
test tests::test_guid_len ... ok
test tests::test_proper_prefix ... ok

test result: ok. 4 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.29s

   Doc-tests nuid

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

2 - $ cargo fix removes the import, then the build fails:

cargo fix
    Checking nuid v0.0.1 (/Users/.../nuid)
       Fixed src/lib.rs (1 fix)
error[E0412]: cannot find type `HashMap` in this scope
   --> src/lib.rs:130:22
    |
130 |         let mut map: HashMap<String, u8> = HashMap::new();
    |                      ^^^^^^^ not found in this scope
    |
help: consider importing this struct
    |
77  |     use std::collections::HashMap;
    |

error[E0433]: failed to resolve: use of undeclared type `HashMap`
   --> src/lib.rs:130:44
    |
130 |         let mut map: HashMap<String, u8> = HashMap::new();
    |                                            ^^^^^^^ not found in this scope
    |
help: consider importing this struct
    |
77  |     use std::collections::HashMap;
    |

Some errors have detailed explanations: E0412, E0433.
For more information about an error, try `rustc --explain E0412`.
error: could not compile `nuid` due to 2 previous errors

Steps

No response

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.63.0 (fd9c4297c 2022-07-01)
release: 1.63.0
commit-hash: fd9c4297ccbee36d39e9a79067edab0b614edb5a
commit-date: 2022-07-01
host: x86_64-apple-darwin
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.64.1 (sys:0.4.55+curl-7.83.1 system ssl:(SecureTransport) LibreSSL/2.8.3)
os: Mac OS 11.6.8 [64-bit]
@dc0d dc0d added the C-bug Category: bug label Sep 4, 2022
@ChayimFriedman2
Copy link

Smaller reproduction (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4425eac0ba9d8e3ba7edd3ca594ac000):

use std::collections::HashMap;

#[cfg(test)]
mod foo {
    use super::*;
    fn bar() {
        _ = HashMap::<u32, u32>::new();
    }
}

@jyn514
Copy link
Member

jyn514 commented Sep 4, 2022

Cargo fix is just forwarding the diagnostic from the compiler, not emitting it itself. This should be moved to rust-lang/rust.

I understand why it looks wrong but also why the compiler emits the diagnostic; there are infinite possible cfgs and having a use behind a cfg shouldn't automatically make it count as used IMO.

That said maybe we can improve the diagnostics, or improve the error for cfg(test) / --check-cfg values specifically

@ChayimFriedman2
Copy link

But the cfg is active, no?

@jyn514
Copy link
Member

jyn514 commented Sep 4, 2022

Look at the output from the playground link:

warning: unused import: `std::collections::HashMap`
 --> src/lib.rs:1:5
  |
1 | use std::collections::HashMap;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `playground` (lib) generated 1 warning
warning: function `bar` is never used
 --> src/lib.rs:6:8
  |
6 |     fn bar() {
  |        ^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `playground` (lib test) generated 1 warning
    Finished test [unoptimized + debuginfo] target(s) in 3.32s
     Running unittests src/lib.rs (target/debug/deps/playground-e26000b484666636)
   Doc-tests playground

Notice how the warning about the unused function only appears once, as does the warning about the import, but they appear in different sections: one says lib and one says lib test. Cargo is first building your crate as a library for doctests, then building it with the test cfg so it can run unit tests. So at the time the warning is emitted, the cfg is not active.

It's possible cargo could be smarter and not build a library if there are no doctests, but that seems quite tricky, and still doesn't help if there are doctests or integration tests.

@weihanglo
Copy link
Member

@jyn514 is really correct. From the perspective of a library target, it knows nothing inside cfg(test) so HashMap seems to be unused. I suggest moving that use statement into cfg(test) module. Or, you can run cargo test --lib to run unit tests exactly inside library target.

It's possible cargo could be smarter and not build a library if there are no doctests, but that seems quite tricky, and still doesn't help if there are doctests or integration tests.

That's true. And I don't think it is feasible without touching rustdoc itself, right? IIRC only after compiling with rustdoc can cargo know if there are doctests.

@jyn514
Copy link
Member

jyn514 commented Sep 4, 2022

@weihanglo correct, at a minimum it requires a rust parser to know if there are any doc-comments at all and to be reliable it needs a full markdown parser. I don't think it's a good investment of time.

@dc0d
Copy link
Author

dc0d commented Sep 4, 2022

It seems to change this behavior the tooling needs to change significantly. So, I close this because it does not seem to be a big issue.

@dc0d dc0d closed this as completed Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants