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

unused_imports redundant import check false positive with dev-dependency #121684

Closed
apoelstra opened this issue Feb 27, 2024 · 3 comments
Closed
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@apoelstra
Copy link
Contributor

apoelstra commented Feb 27, 2024

I tried this code:

Cargo.toml

[package]
name = "unused_import_bug-2024-02"
version = "0.1.0"
edition = "2021"

[features]
default = ["std"]
std = ["bitcoin/std", "bitcoin/secp-recovery" ]

[dependencies]
bitcoin = { version = "0.31.0", default-features = false }

[dev-dependencies]
secp256k1 = {version = "0.28.0", features = ["rand-std"]}

src/main.rs

use bitcoin::secp256k1;

pub struct Thing(pub secp256k1::SecretKey);

fn main() {
    println!("Hello, world!");
}

If you compile this project with a recent nightly you will see the warning

   Compiling unused_import_bug-2024-02 v0.1.0 (/store/home/apoelstra/code/tmp/unused_import_bug-2024-02)
warning: the item `secp256k1` is imported redundantly
 --> src/main.rs:2:5
  |
2 | use bitcoin::secp256k1;
  |     ^^^^^^^^^^^^^^^^^^ the item `secp256k1` is already defined here
  |
  = note: `#[warn(unused_imports)]` on by default

This warning should not show up, and if it must show up, it should say where the compiler thinks the "redundant import" actually is. Because there is only a single import.

If you comment out the [dev-dependencies] part of Cargo.toml the error goes away.

@apoelstra apoelstra added the C-bug Category: This is a bug. label Feb 27, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 27, 2024
@apoelstra
Copy link
Contributor Author

Ok, what I think is going on is that when you run cargo test, all the dev-dependencies are implicitly imported, meaning that your necessary-for-cargo build imports become redundant.

If this is expected behavior then the error message needs to be much clearer.

@apoelstra
Copy link
Contributor Author

Ok, I can mostly patch this by sticking #[cfg(not(test))] onto all of my imports, but for examples this doesn't work. It seems like examples pull in dev-dependencies but do not have cfg(test) set.

I can remove the import entirely, but then I am left with "example" code which does not actually compile outside of an example/ directory.

apoelstra added a commit to apoelstra/rust-miniscript that referenced this issue Feb 27, 2024
apoelstra added a commit to apoelstra/rust-miniscript that referenced this issue Feb 27, 2024
apoelstra added a commit to apoelstra/rust-miniscript that referenced this issue Feb 27, 2024
@jieyouxu jieyouxu added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 28, 2024
apoelstra added a commit to rust-bitcoin/rust-miniscript that referenced this issue Feb 28, 2024
50db03c cargo fmt (Andrew Poelstra)
220f101 ci: screw up imports for rust-lang/rust#121684 (Andrew Poelstra)
df069af ci: remove unnecessary imports of `bitcoin` (Andrew Poelstra)
3725549 ci: tighten import of `Vec` (Andrew Poelstra)
d441ccd ci: remove imports that are redundant with super::* (Andrew Poelstra)
b87b4fb remove sketchy `LikelyFalse` error (Andrew Poelstra)

Pull request description:

  This error would trigger on `l:0` on the basis that this is equivalent to `u:0` but always less efficient. There are no other "linting" errors like this in this library, and the C++ implementation doesn't have it, so we should drop it.

  Fixes #633

ACKs for top commit:
  tcharding:
    ACK 50db03c
  sanket1729:
    ACK 50db03c

Tree-SHA512: 30681e6abe7957b9fbe059e808d8057fd174ea156d1853960370d2fd63b032a500f85965a5c7424764df76ed804c62d9a781ae38cdc2b123e9b4b53308dd89f5
@fmease fmease added D-confusing Diagnostics: Confusing error or lint that should be reworked. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 29, 2024
@petrochenkov
Copy link
Contributor

All unused lints work after cfgs are expanded, this one is no different, so I'm going to close the issue.

The recent extension of unused_imports had some large effect on users in general, that is discussed in #121708 which is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants