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_crate_dependencies false-positives in crates with muliple (lib/bin) targets #95513

Closed
MarijnS95 opened this issue Mar 31, 2022 · 5 comments
Labels
A-crates Area: Crates and their interactions (like crate loading) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@MarijnS95
Copy link
Contributor

MarijnS95 commented Mar 31, 2022

I tried this code:

Consider a crate setup with the clap dependency, where clap is used only by the lib target:

Cargo.toml:

[package]
name = "multiple_targets"

[dependencies]
clap = { version = "3", features = ["derive"] }

src/lib.rs:

use clap::Parser;

#[derive(Parser)]
pub struct Args {}

This is fine when checking with this lint enabled:

$ RUSTFLAGS=-Wunused-crate-dependencies cargo c --all --all-targets

Now, consider adding multiple targets to this crate. For the sake of demonstration, we use a test and bin target that both utilize the library crate defined above:

src/main.rs:

fn main() {
    let _x = multiple_targets::Args {};
}

tests/test.rs:

#[test]
fn test() {
    let _x = multiple_targets::Args {};
}

We run the same check command again:

$ RUSTFLAGS=-Wunused-crate-dependencies cargo c --all --all-targets

I expected to see this happen:

No external crate `clap` unused warnings show up.

Instead, this happened:

Every target seems to be checked individually against the set of defined crate dependencies, instead of identifying dependencies that are not used by all targets combined (or at the very least lib+bins, as test-only dependencies should reside under dev-dependencies). As such, both the test and binary targets above are accused of not using clap:

warning: external crate `clap` unused in `multiple_targets`: remove the dependency or add `use clap as _;`
  |
  = note: requested on the command line with `-W unused-crate-dependencies`
  = help: remove unnecessary dependency `clap`

warning: external crate `clap` unused in `test`: remove the dependency or add `use clap as _;`
  |
  = note: requested on the command line with `-W unused-crate-dependencies`
  = help: remove unnecessary dependency `clap`

warning: `multiple_targets` (bin "multiple_targets") generated 1 warning
warning: `multiple_targets` (bin "multiple_targets" test) generated 1 warning (1 duplicate)
warning: `multiple_targets` (test "test") generated 1 warning

For the most part, especially larger library crates, this makes it impossible to write clean test binaries without having the top half filled with use {dep_a as _, dep_b as _, ...};.

Note that our project currently comprises 92 crates in a single git repository, and we use a single .cargo/config.toml in the workspace root to configure lints all at once. It is completely infeasible for us to duplicate these to every existing and up and coming crate, nor keep them in sync after the fact. As such, copy-pasting #[warn(unused-crate-dependencies)] across just the lib.rs/main.rs bits of our code base (where only a single target is used) is pretty much impossible.


EDIT: I can understand this warning to show up for the binary (test) targets if they do not reference the library target, but it is particularly confusing when these binary targets at least transitively use the dependency through the library target.

Meta

rustc --version --verbose:

rustc 1.61.0-nightly (c5cf08d37 2022-03-30)
binary: rustc
commit-hash: c5cf08d37b85f953b132951e868df5b924250fdc
commit-date: 2022-03-30
host: x86_64-unknown-linux-gnu
release: 1.61.0-nightly
LLVM version: 14.0.0
@ehuss
Copy link
Contributor

ehuss commented Mar 31, 2022

Thanks for the report! This is a known issue, as rustc does not know about cargo projects or their structure. #57274 contains more discussion of the issue and its problems. As an alternative, you may want to consider using cargo udeps.

@MarijnS95
Copy link
Contributor Author

@ehuss Thanks for linking that! I wouldn't have thought to look at that issue as it asks for implementing this feature in the first place. It seems non-trivial anyway given how the user can dictate what targets to build, and this issue only seems to be solvable if the usage results from all targets can be aggregated and combined.

For future lurkers, to save you some scrolling and searching, the exact comment that describes this is #57274 (comment)

@ehuss Is it worth keeping this issue open as a more explicit tracking place for this particular problem or do you think the mention in #57274 is enough?

I'll check out cargo udeps but prefer to have everything "mainline" as mentioned by other commenters in that issue. All our developers are pretty used to just having to run cargo clippy --all --all-targets, matching (approx...) what our CI is doing.

For now though, as a workaround, I opted to run this lint exclusively on --libs in crates as we currently don't have any bin+lib crates where the bin uses deps that are not used by the libs.

@ehuss
Copy link
Contributor

ehuss commented Apr 1, 2022

I think it would be fine to close this. I think the same solution for #57274 would cover this, and I don't see a way to address this particular situation separately.

@ChrisDenton ChrisDenton added the needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. label Jul 16, 2023
mrcnski added a commit to paritytech/polkadot-sdk that referenced this issue Dec 4, 2023
This lint doesn't work with multiple targets (in the case of prepare-worker, the
bench-only dependencies were messing it up). See:

- rust-lang/rust#95513
- rust-lang/rust#57274 (comment)
@fmease
Copy link
Member

fmease commented Jan 26, 2024

Closing in favor of #57274 and rust-lang/cargo#1982. If you disagree with my assessment I can of course reopen this issue.

@fmease fmease closed this as not planned Won't fix, can't repro, duplicate, stale Jan 26, 2024
@fmease fmease 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. A-crates Area: Crates and their interactions (like crate loading) and removed needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. labels Jan 26, 2024
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jan 26, 2024

If you disagree with my assessment

@fmease neither of those issues seem to describe this problem.

#57274 describes implementing this lint in the first place (which already happened now in the form of unused_crate_dependencies?).
EDIT: From the comment above it seems discussion about this particular issue is hidden somewhere in a few comments of that issue? I don't think that gives this the visibility it deserves though, unless the title and OP is updated.

rust-lang/cargo#1982 describes a way to configure per-target dependencies. While such a feature likely impacts how this lint is implemented, and could implicitly solve this issue, it is of far wider scope than this issue. To repeat, this issue describes false positives from the unused_crate_dependencies lint when multiple targets are used, and does not request for being able to set per-target dependencies.

EDIT: You could however close this as a "WONTFIX", anticipating rust-lang/cargo#1982 to be a likable alternative instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-crates Area: Crates and their interactions (like crate loading) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. 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

4 participants