Skip to content

Commit 32aa9b5

Browse files
authored
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.
2 parents c5efee4 + 944a540 commit 32aa9b5

2 files changed

Lines changed: 86 additions & 40 deletions

File tree

src/cargo/core/compiler/unused_deps.rs

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,13 @@ impl UnusedDepState {
9898
} else {
9999
continue;
100100
};
101-
state.externs.insert(dep.extern_crate_name, manifest_deps);
101+
state.externs.insert(
102+
dep.extern_crate_name,
103+
ExternState {
104+
unit: dep.unit.clone(),
105+
manifest_deps,
106+
},
107+
);
102108
}
103109
}
104110

@@ -213,7 +219,7 @@ impl UnusedDepState {
213219
continue;
214220
}
215221

216-
for (ext, dependency) in &state.externs {
222+
for (ext, extern_state) in &state.externs {
217223
if state
218224
.unused_externs
219225
.values()
@@ -227,9 +233,17 @@ impl UnusedDepState {
227233
);
228234
continue;
229235
}
236+
if is_transitive_dep(&extern_state.unit, &state.unused_externs, build_runner) {
237+
debug!(
238+
"pkg {} v{} ({dep_kind:?}): ignoring unused extern `{ext}`, may be activating features",
239+
pkg_id.name(),
240+
pkg_id.version(),
241+
);
242+
continue;
243+
}
230244

231245
// Implicitly added dependencies (in the same crate) aren't interesting
232-
let dependency = if let Some(dependency) = dependency {
246+
let dependency = if let Some(dependency) = &extern_state.manifest_deps {
233247
dependency
234248
} else {
235249
continue;
@@ -318,7 +332,7 @@ impl UnusedDepState {
318332
#[derive(Default)]
319333
struct DependenciesState {
320334
/// All declared dependencies
321-
externs: IndexMap<InternedString, Option<Vec<Dependency>>>,
335+
externs: IndexMap<InternedString, ExternState>,
322336
/// Expected [`Self::unused_externs`] entries to know we've received them all
323337
///
324338
/// To avoid warning in cases where we didn't,
@@ -328,6 +342,12 @@ struct DependenciesState {
328342
unused_externs: IndexMap<Unit, Vec<InternedString>>,
329343
}
330344

345+
#[derive(Clone)]
346+
struct ExternState {
347+
unit: Unit,
348+
manifest_deps: Option<Vec<Dependency>>,
349+
}
350+
331351
fn dep_kind_of(unit: &Unit) -> DepKind {
332352
match unit.target.kind() {
333353
TargetKind::Lib(_) => match unit.mode {
@@ -352,3 +372,34 @@ fn unit_desc(unit: &Unit) -> String {
352372
unit.mode,
353373
)
354374
}
375+
376+
#[instrument(skip_all)]
377+
fn is_transitive_dep(
378+
direct_dep_unit: &Unit,
379+
unused_externs: &IndexMap<Unit, Vec<InternedString>>,
380+
build_runner: &mut BuildRunner<'_, '_>,
381+
) -> bool {
382+
let mut queue = std::collections::VecDeque::new();
383+
for root_unit in unused_externs.keys() {
384+
for unit_dep in build_runner.unit_deps(root_unit) {
385+
if root_unit.pkg.package_id() == unit_dep.unit.pkg.package_id() {
386+
continue;
387+
}
388+
if unit_dep.unit == *direct_dep_unit {
389+
continue;
390+
}
391+
queue.push_back(&unit_dep.unit);
392+
}
393+
}
394+
395+
while let Some(dep_unit) = queue.pop_front() {
396+
for unit_dep in build_runner.unit_deps(dep_unit) {
397+
if unit_dep.unit == *direct_dep_unit {
398+
return true;
399+
}
400+
queue.push_back(&unit_dep.unit);
401+
}
402+
}
403+
404+
false
405+
}

tests/testsuite/lints/unused_dependencies.rs

Lines changed: 31 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -968,7 +968,9 @@ fn package_selection() {
968968
Package::new("used_bar", "0.1.0").publish();
969969
Package::new("used_foo", "0.1.0").publish();
970970
Package::new("used_external", "0.1.0").publish();
971-
Package::new("unused", "0.1.0").publish();
971+
Package::new("unused_bar", "0.1.0").publish();
972+
Package::new("unused_foo", "0.1.0").publish();
973+
Package::new("unused_external", "0.1.0").publish();
972974
let p = project()
973975
.file(
974976
"Cargo.toml",
@@ -988,7 +990,7 @@ fn package_selection() {
988990
edition = "2018"
989991
990992
[dependencies]
991-
unused = "0.1.0"
993+
unused_foo = "0.1.0"
992994
used_foo = "0.1.0"
993995
bar.path = "../bar"
994996
external.path = "../external"
@@ -1013,7 +1015,7 @@ fn package_selection() {
10131015
edition = "2018"
10141016
10151017
[dependencies]
1016-
unused = "0.1.0"
1018+
unused_bar = "0.1.0"
10171019
used_bar = "0.1.0"
10181020
10191021
[lints.cargo]
@@ -1036,7 +1038,7 @@ fn package_selection() {
10361038
edition = "2018"
10371039
10381040
[dependencies]
1039-
unused = "0.1.0"
1041+
unused_external = "0.1.0"
10401042
used_external = "0.1.0"
10411043
10421044
[lints.cargo]
@@ -1056,30 +1058,33 @@ fn package_selection() {
10561058
.with_stderr_data(
10571059
str![[r#"
10581060
[UPDATING] `dummy-registry` index
1061+
[LOCKING] 7 packages to latest compatible versions
10591062
[DOWNLOADING] crates ...
1060-
[CHECKING] bar v0.1.0 ([ROOT]/foo/bar)
1061-
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
1062-
[LOCKING] 5 packages to latest compatible versions
1063-
[DOWNLOADED] used_foo v0.1.0 (registry `dummy-registry`)
1064-
[DOWNLOADED] used_external v0.1.0 (registry `dummy-registry`)
1063+
[DOWNLOADED] unused_bar v0.1.0 (registry `dummy-registry`)
1064+
[DOWNLOADED] unused_external v0.1.0 (registry `dummy-registry`)
1065+
[DOWNLOADED] unused_foo v0.1.0 (registry `dummy-registry`)
10651066
[DOWNLOADED] used_bar v0.1.0 (registry `dummy-registry`)
1066-
[DOWNLOADED] unused v0.1.0 (registry `dummy-registry`)
1067-
[CHECKING] unused v0.1.0
1067+
[DOWNLOADED] used_external v0.1.0 (registry `dummy-registry`)
1068+
[DOWNLOADED] used_foo v0.1.0 (registry `dummy-registry`)
10681069
[CHECKING] used_bar v0.1.0
1070+
[CHECKING] unused_external v0.1.0
10691071
[CHECKING] used_external v0.1.0
1072+
[CHECKING] unused_bar v0.1.0
10701073
[CHECKING] used_foo v0.1.0
1074+
[CHECKING] unused_foo v0.1.0
1075+
[CHECKING] bar v0.1.0 ([ROOT]/foo/bar)
10711076
[CHECKING] external v0.1.0 ([ROOT]/foo/external)
10721077
[CHECKING] foo v0.1.0 ([ROOT]/foo/foo)
10731078
[WARNING] unused dependency
10741079
--> bar/Cargo.toml:9:13
10751080
|
1076-
9 | unused = "0.1.0"
1077-
| ^^^^^^^^^^^^^^^^
1081+
9 | unused_bar = "0.1.0"
1082+
| ^^^^^^^^^^^^^^^^^^^^
10781083
|
10791084
= [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]`
10801085
[HELP] remove the dependency
10811086
|
1082-
9 - unused = "0.1.0"
1087+
9 - unused_bar = "0.1.0"
10831088
|
10841089
[WARNING] unused dependency
10851090
--> foo/Cargo.toml:11:13
@@ -1107,13 +1112,14 @@ fn package_selection() {
11071112
[WARNING] unused dependency
11081113
--> foo/Cargo.toml:9:13
11091114
|
1110-
9 | unused = "0.1.0"
1111-
| ^^^^^^^^^^^^^^^^
1115+
9 | unused_foo = "0.1.0"
1116+
| ^^^^^^^^^^^^^^^^^^^^
11121117
|
11131118
[HELP] remove the dependency
11141119
|
1115-
9 - unused = "0.1.0"
1120+
9 - unused_foo = "0.1.0"
11161121
|
1122+
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11171123
11181124
"#]]
11191125
.unordered(),
@@ -1124,7 +1130,6 @@ fn package_selection() {
11241130
.masquerade_as_nightly_cargo(&["cargo-lints"])
11251131
.with_stderr_data(
11261132
str![[r#"
1127-
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11281133
[WARNING] unused dependency
11291134
--> foo/Cargo.toml:11:13
11301135
|
@@ -1151,13 +1156,14 @@ fn package_selection() {
11511156
[WARNING] unused dependency
11521157
--> foo/Cargo.toml:9:13
11531158
|
1154-
9 | unused = "0.1.0"
1155-
| ^^^^^^^^^^^^^^^^
1159+
9 | unused_foo = "0.1.0"
1160+
| ^^^^^^^^^^^^^^^^^^^^
11561161
|
11571162
[HELP] remove the dependency
11581163
|
1159-
9 - unused = "0.1.0"
1164+
9 - unused_foo = "0.1.0"
11601165
|
1166+
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11611167
11621168
"#]]
11631169
.unordered(),
@@ -1168,18 +1174,18 @@ fn package_selection() {
11681174
.masquerade_as_nightly_cargo(&["cargo-lints"])
11691175
.with_stderr_data(
11701176
str![[r#"
1171-
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11721177
[WARNING] unused dependency
11731178
--> bar/Cargo.toml:9:13
11741179
|
1175-
9 | unused = "0.1.0"
1176-
| ^^^^^^^^^^^^^^^^
1180+
9 | unused_bar = "0.1.0"
1181+
| ^^^^^^^^^^^^^^^^^^^^
11771182
|
11781183
= [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]`
11791184
[HELP] remove the dependency
11801185
|
1181-
9 - unused = "0.1.0"
1186+
9 - unused_bar = "0.1.0"
11821187
|
1188+
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11831189
11841190
"#]]
11851191
.unordered(),
@@ -1314,17 +1320,6 @@ pub fn fun() -> &'static str {
13141320
[CHECKING] transitive v0.1.1
13151321
[CHECKING] intermediate v0.1.0
13161322
[CHECKING] foo v0.1.0 ([ROOT]/foo)
1317-
[WARNING] unused dependency
1318-
--> Cargo.toml:10:13
1319-
|
1320-
10 | transitive = { version = "0.1.1", features = ["a"] }
1321-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1322-
|
1323-
= [NOTE] `cargo::unused_dependencies` is set to `warn` in `[lints]`
1324-
[HELP] remove the dependency
1325-
|
1326-
10 - transitive = { version = "0.1.1", features = ["a"] }
1327-
|
13281323
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
13291324
13301325
"#]]

0 commit comments

Comments
 (0)