fix(compile): Ignore unused deps if also transitive (#16935) ### What does this PR try to resolve? This changes `unused_dependencies` lint to reduce the chance of false positives while we work out the story of how we want people to response to false positives (rust-lang/rfcs#3920). ### How to test and review this PR? My assumption is that I will revert the `ignore` lint control added in #16600 after this. Looking at the known cases of false positives, my short term plan is: - transitive version constraint: don't lint - transitive feature activation: don't lint - dynamically used dep (e.g. `build.rs` in `curl`): users must `allow` Longer term, I'd like to replace this change with either - `lib = false` from artifact deps - `cfg(resolver = "versions")`, [#t-cargo > Additional dependency tables @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Additional.20dependency.20tables/near/590208010) We can evolve this because the exact nature of warning-by-default lints is not stable.
diff --git a/src/cargo/core/compiler/unused_deps.rs b/src/cargo/core/compiler/unused_deps.rs index 39b27f7..519bd6c 100644 --- a/src/cargo/core/compiler/unused_deps.rs +++ b/src/cargo/core/compiler/unused_deps.rs
@@ -98,7 +98,13 @@ } else { continue; }; - state.externs.insert(dep.extern_crate_name, manifest_deps); + state.externs.insert( + dep.extern_crate_name, + ExternState { + unit: dep.unit.clone(), + manifest_deps, + }, + ); } } @@ -213,7 +219,7 @@ continue; } - for (ext, dependency) in &state.externs { + for (ext, extern_state) in &state.externs { if state .unused_externs .values() @@ -227,9 +233,17 @@ ); continue; } + if is_transitive_dep(&extern_state.unit, &state.unused_externs, build_runner) { + debug!( + "pkg {} v{} ({dep_kind:?}): ignoring unused extern `{ext}`, may be activating features", + pkg_id.name(), + pkg_id.version(), + ); + continue; + } // Implicitly added dependencies (in the same crate) aren't interesting - let dependency = if let Some(dependency) = dependency { + let dependency = if let Some(dependency) = &extern_state.manifest_deps { dependency } else { continue; @@ -318,7 +332,7 @@ #[derive(Default)] struct DependenciesState { /// All declared dependencies - externs: IndexMap<InternedString, Option<Vec<Dependency>>>, + externs: IndexMap<InternedString, ExternState>, /// Expected [`Self::unused_externs`] entries to know we've received them all /// /// To avoid warning in cases where we didn't, @@ -328,6 +342,12 @@ unused_externs: IndexMap<Unit, Vec<InternedString>>, } +#[derive(Clone)] +struct ExternState { + unit: Unit, + manifest_deps: Option<Vec<Dependency>>, +} + fn dep_kind_of(unit: &Unit) -> DepKind { match unit.target.kind() { TargetKind::Lib(_) => match unit.mode { @@ -352,3 +372,34 @@ unit.mode, ) } + +#[instrument(skip_all)] +fn is_transitive_dep( + direct_dep_unit: &Unit, + unused_externs: &IndexMap<Unit, Vec<InternedString>>, + build_runner: &mut BuildRunner<'_, '_>, +) -> bool { + let mut queue = std::collections::VecDeque::new(); + for root_unit in unused_externs.keys() { + for unit_dep in build_runner.unit_deps(root_unit) { + if root_unit.pkg.package_id() == unit_dep.unit.pkg.package_id() { + continue; + } + if unit_dep.unit == *direct_dep_unit { + continue; + } + queue.push_back(&unit_dep.unit); + } + } + + while let Some(dep_unit) = queue.pop_front() { + for unit_dep in build_runner.unit_deps(dep_unit) { + if unit_dep.unit == *direct_dep_unit { + return true; + } + queue.push_back(&unit_dep.unit); + } + } + + false +}
diff --git a/tests/testsuite/lints/unused_dependencies.rs b/tests/testsuite/lints/unused_dependencies.rs index 8c7b915..664950f 100644 --- a/tests/testsuite/lints/unused_dependencies.rs +++ b/tests/testsuite/lints/unused_dependencies.rs
@@ -968,7 +968,9 @@ Package::new("used_bar", "0.1.0").publish(); Package::new("used_foo", "0.1.0").publish(); Package::new("used_external", "0.1.0").publish(); - Package::new("unused", "0.1.0").publish(); + Package::new("unused_bar", "0.1.0").publish(); + Package::new("unused_foo", "0.1.0").publish(); + Package::new("unused_external", "0.1.0").publish(); let p = project() .file( "Cargo.toml", @@ -988,7 +990,7 @@ edition = "2018" [dependencies] - unused = "0.1.0" + unused_foo = "0.1.0" used_foo = "0.1.0" bar.path = "../bar" external.path = "../external" @@ -1013,7 +1015,7 @@ edition = "2018" [dependencies] - unused = "0.1.0" + unused_bar = "0.1.0" used_bar = "0.1.0" [lints.cargo] @@ -1036,7 +1038,7 @@ edition = "2018" [dependencies] - unused = "0.1.0" + unused_external = "0.1.0" used_external = "0.1.0" [lints.cargo] @@ -1056,30 +1058,33 @@ .with_stderr_data( str![[r#" [UPDATING] `dummy-registry` index +[LOCKING] 7 packages to latest compatible versions [DOWNLOADING] crates ... -[CHECKING] bar v0.1.0 ([ROOT]/foo/bar) -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s -[LOCKING] 5 packages to latest compatible versions -[DOWNLOADED] used_foo v0.1.0 (registry `dummy-registry`) -[DOWNLOADED] used_external v0.1.0 (registry `dummy-registry`) +[DOWNLOADED] unused_bar v0.1.0 (registry `dummy-registry`) +[DOWNLOADED] unused_external v0.1.0 (registry `dummy-registry`) +[DOWNLOADED] unused_foo v0.1.0 (registry `dummy-registry`) [DOWNLOADED] used_bar v0.1.0 (registry `dummy-registry`) -[DOWNLOADED] unused v0.1.0 (registry `dummy-registry`) -[CHECKING] unused v0.1.0 +[DOWNLOADED] used_external v0.1.0 (registry `dummy-registry`) +[DOWNLOADED] used_foo v0.1.0 (registry `dummy-registry`) [CHECKING] used_bar v0.1.0 +[CHECKING] unused_external v0.1.0 [CHECKING] used_external v0.1.0 +[CHECKING] unused_bar v0.1.0 [CHECKING] used_foo v0.1.0 +[CHECKING] unused_foo v0.1.0 +[CHECKING] bar v0.1.0 ([ROOT]/foo/bar) [CHECKING] external v0.1.0 ([ROOT]/foo/external) [CHECKING] foo v0.1.0 ([ROOT]/foo/foo) [WARNING] unused dependency --> bar/Cargo.toml:9:13 | -9 | unused = "0.1.0" - | ^^^^^^^^^^^^^^^^ +9 | unused_bar = "0.1.0" + | ^^^^^^^^^^^^^^^^^^^^ | = [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]` [HELP] remove the dependency | -9 - unused = "0.1.0" +9 - unused_bar = "0.1.0" | [WARNING] unused dependency --> foo/Cargo.toml:11:13 @@ -1107,13 +1112,14 @@ [WARNING] unused dependency --> foo/Cargo.toml:9:13 | -9 | unused = "0.1.0" - | ^^^^^^^^^^^^^^^^ +9 | unused_foo = "0.1.0" + | ^^^^^^^^^^^^^^^^^^^^ | [HELP] remove the dependency | -9 - unused = "0.1.0" +9 - unused_foo = "0.1.0" | +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]] .unordered(), @@ -1124,7 +1130,6 @@ .masquerade_as_nightly_cargo(&["cargo-lints"]) .with_stderr_data( str![[r#" -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s [WARNING] unused dependency --> foo/Cargo.toml:11:13 | @@ -1151,13 +1156,14 @@ [WARNING] unused dependency --> foo/Cargo.toml:9:13 | -9 | unused = "0.1.0" - | ^^^^^^^^^^^^^^^^ +9 | unused_foo = "0.1.0" + | ^^^^^^^^^^^^^^^^^^^^ | [HELP] remove the dependency | -9 - unused = "0.1.0" +9 - unused_foo = "0.1.0" | +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]] .unordered(), @@ -1168,18 +1174,18 @@ .masquerade_as_nightly_cargo(&["cargo-lints"]) .with_stderr_data( str![[r#" -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s [WARNING] unused dependency --> bar/Cargo.toml:9:13 | -9 | unused = "0.1.0" - | ^^^^^^^^^^^^^^^^ +9 | unused_bar = "0.1.0" + | ^^^^^^^^^^^^^^^^^^^^ | = [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]` [HELP] remove the dependency | -9 - unused = "0.1.0" +9 - unused_bar = "0.1.0" | +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]] .unordered(), @@ -1314,17 +1320,6 @@ [CHECKING] transitive v0.1.1 [CHECKING] intermediate v0.1.0 [CHECKING] foo v0.1.0 ([ROOT]/foo) -[WARNING] unused dependency - --> Cargo.toml:10:13 - | -10 | transitive = { version = "0.1.1", features = ["a"] } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]` -[HELP] remove the dependency - | -10 - transitive = { version = "0.1.1", features = ["a"] } - | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]