Auto merge of #14026 - linyihai:weak-optional-inactive, r=weihanglo

fix: improve message for inactive weak optional feature with edition2024 through unused dep collection

### What does this PR try to resolve?

Collect the unused dependencies to check whether a weak optional dependency had set. Then we can improve the message when weak optional dependency inactive.

Fixes https://github.com/rust-lang/cargo/issues/14015

### How should we test and review this PR?

One commit test added, one commit fixed and updated

### Additional information
Part of https://github.com/rust-lang/cargo/issues/14039
- migrate `tests/testsuite/lints/unused_optional_dependencies.rs` to snapshot

And rename `MissingField` to `MissingFieldError`
This commit is contained in:
bors 2024-07-03 14:24:48 +00:00
commit 838d81d0e0
6 changed files with 372 additions and 83 deletions

View file

@ -30,6 +30,35 @@ struct Inner {
rust_version: Option<RustVersion>,
}
/// Indicates the dependency inferred from the `dep` syntax that should exist,
/// but missing on the resolved dependencies tables.
#[derive(Debug)]
pub struct MissingDependencyError {
pub dep_name: InternedString,
pub feature: InternedString,
pub feature_value: FeatureValue,
/// Indicates the dependency inferred from the `dep?` syntax that is weak optional
pub weak_optional: bool,
}
impl std::error::Error for MissingDependencyError {}
impl fmt::Display for MissingDependencyError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self {
dep_name,
feature,
feature_value: fv,
..
} = self;
write!(
f,
"feature `{feature}` includes `{fv}`, but `{dep_name}` is not a dependency",
)
}
}
impl Summary {
#[tracing::instrument(skip_all)]
pub fn new(
@ -274,7 +303,12 @@ fn build_feature_map(
// Validation of the feature name will be performed in the resolver.
if !is_any_dep {
bail!("feature `{feature}` includes `{fv}`, but `{dep_name}` is not a dependency");
bail!(MissingDependencyError {
feature: *feature,
feature_value: (*fv).clone(),
dep_name: *dep_name,
weak_optional: *weak,
})
}
if *weak && !is_optional_dep {
bail!(

View file

@ -2031,7 +2031,7 @@ impl ConfigError {
}
fn is_missing_field(&self) -> bool {
self.error.downcast_ref::<MissingField>().is_some()
self.error.downcast_ref::<MissingFieldError>().is_some()
}
fn missing(key: &ConfigKey) -> ConfigError {
@ -2067,15 +2067,15 @@ impl fmt::Display for ConfigError {
}
#[derive(Debug)]
struct MissingField(String);
struct MissingFieldError(String);
impl fmt::Display for MissingField {
impl fmt::Display for MissingFieldError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "missing field `{}`", self.0)
}
}
impl std::error::Error for MissingField {}
impl std::error::Error for MissingFieldError {}
impl serde::de::Error for ConfigError {
fn custom<T: fmt::Display>(msg: T) -> Self {
@ -2087,7 +2087,7 @@ impl serde::de::Error for ConfigError {
fn missing_field(field: &'static str) -> Self {
ConfigError {
error: anyhow::Error::new(MissingField(field.to_string())),
error: anyhow::Error::new(MissingFieldError(field.to_string())),
definition: None,
}
}

View file

@ -199,7 +199,11 @@ fn verify_feature_enabled(
Ok(())
}
fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Option<Range<usize>> {
pub fn get_span(
document: &ImDocument<String>,
path: &[&str],
get_value: bool,
) -> Option<Range<usize>> {
let mut table = document.as_item().as_table_like()?;
let mut iter = path.into_iter().peekable();
while let Some(key) = iter.next() {
@ -240,7 +244,7 @@ fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Op
/// Gets the relative path to a manifest from the current working directory, or
/// the absolute path of the manifest if a relative path cannot be constructed
fn rel_cwd_manifest_path(path: &Path, gctx: &GlobalContext) -> String {
pub fn rel_cwd_manifest_path(path: &Path, gctx: &GlobalContext) -> String {
diff_paths(path, gctx.cwd())
.unwrap_or_else(|| path.to_path_buf())
.display()

View file

@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
use std::rc::Rc;
use std::str::{self, FromStr};
use crate::core::summary::MissingDependencyError;
use crate::AlreadyPrintedError;
use anyhow::{anyhow, bail, Context as _};
use cargo_platform::Platform;
@ -14,6 +15,7 @@ use cargo_util_schemas::manifest::{RustVersion, StringOrBool};
use itertools::Itertools;
use lazycell::LazyCell;
use pathdiff::diff_paths;
use toml_edit::ImDocument;
use url::Url;
use crate::core::compiler::{CompileKind, CompileTarget};
@ -28,6 +30,7 @@ use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, Worksp
use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::util::errors::{CargoResult, ManifestError};
use crate::util::interning::InternedString;
use crate::util::lints::{get_span, rel_cwd_manifest_path};
use crate::util::{self, context::ConfigRelativePath, GlobalContext, IntoUrl, OptVersionReq};
mod embedded;
@ -1435,24 +1438,42 @@ fn to_real_manifest(
.unwrap_or_else(|| semver::Version::new(0, 0, 0)),
source_id,
);
let summary = Summary::new(
pkgid,
deps,
&resolved_toml
.features
.as_ref()
.unwrap_or(&Default::default())
.iter()
.map(|(k, v)| {
(
InternedString::new(k),
v.iter().map(InternedString::from).collect(),
)
})
.collect(),
resolved_package.links.as_deref(),
rust_version.clone(),
)?;
let summary = {
let summary = Summary::new(
pkgid,
deps,
&resolved_toml
.features
.as_ref()
.unwrap_or(&Default::default())
.iter()
.map(|(k, v)| {
(
InternedString::new(k),
v.iter().map(InternedString::from).collect(),
)
})
.collect(),
resolved_package.links.as_deref(),
rust_version.clone(),
);
// editon2024 stops exposing implicit features, which will strip weak optional dependencies from `dependencies`,
// need to check whether `dep_name` is stripped as unused dependency
if let Err(ref err) = summary {
if let Some(missing_dep) = err.downcast_ref::<MissingDependencyError>() {
missing_dep_diagnostic(
missing_dep,
&original_toml,
&document,
&contents,
manifest_file,
gctx,
)?;
}
}
summary?
};
if summary.features().contains_key("default-features") {
warnings.push(
"`default-features = [\"..\"]` was found in [features]. \
@ -1558,6 +1579,85 @@ fn to_real_manifest(
Ok(manifest)
}
fn missing_dep_diagnostic(
missing_dep: &MissingDependencyError,
orig_toml: &TomlManifest,
document: &ImDocument<String>,
contents: &str,
manifest_file: &Path,
gctx: &GlobalContext,
) -> CargoResult<()> {
let dep_name = missing_dep.dep_name;
let manifest_path = rel_cwd_manifest_path(manifest_file, gctx);
let feature_value_span =
get_span(&document, &["features", missing_dep.feature.as_str()], true).unwrap();
let title = format!(
"feature `{}` includes `{}`, but `{}` is not a dependency",
missing_dep.feature, missing_dep.feature_value, &dep_name
);
let help = format!("enable the dependency with `dep:{dep_name}`");
let info_label = format!(
"`{}` is an unused optional dependency since no feature enables it",
&dep_name
);
let message = Level::Error.title(&title);
let snippet = Snippet::source(&contents)
.origin(&manifest_path)
.fold(true)
.annotation(Level::Error.span(feature_value_span.start..feature_value_span.end));
let message = if missing_dep.weak_optional {
let mut orig_deps = vec![
(
orig_toml.dependencies.as_ref(),
vec![DepKind::Normal.kind_table()],
),
(
orig_toml.build_dependencies.as_ref(),
vec![DepKind::Build.kind_table()],
),
];
for (name, platform) in orig_toml.target.iter().flatten() {
orig_deps.push((
platform.dependencies.as_ref(),
vec!["target", name, DepKind::Normal.kind_table()],
));
orig_deps.push((
platform.build_dependencies.as_ref(),
vec!["target", name, DepKind::Normal.kind_table()],
));
}
if let Some((_, toml_path)) = orig_deps.iter().find(|(deps, _)| {
if let Some(deps) = deps {
deps.keys().any(|p| *p.as_str() == *dep_name)
} else {
false
}
}) {
let toml_path = toml_path
.iter()
.map(|s| *s)
.chain(std::iter::once(dep_name.as_str()))
.collect::<Vec<_>>();
let dep_span = get_span(&document, &toml_path, false).unwrap();
message
.snippet(snippet.annotation(Level::Warning.span(dep_span).label(&info_label)))
.footer(Level::Help.title(&help))
} else {
message.snippet(snippet)
}
} else {
message.snippet(snippet)
};
if let Err(err) = gctx.shell().print_message(message) {
return Err(err.into());
}
Err(AlreadyPrintedError::new(anyhow!("").into()).into())
}
fn to_virtual_manifest(
contents: String,
document: toml_edit::ImDocument<String>,

View file

@ -257,11 +257,14 @@ fn invalid6() {
p.cargo("check --features foo")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] feature `foo` includes `bar/baz`, but `bar` is not a dependency
--> Cargo.toml:9:23
|
9 | foo = ["bar/baz"]
| ^^^^^^^^^^^
|
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`
Caused by:
feature `foo` includes `bar/baz`, but `bar` is not a dependency
"#]])
.run();
}
@ -289,11 +292,14 @@ fn invalid7() {
p.cargo("check --features foo")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] feature `foo` includes `bar/baz`, but `bar` is not a dependency
--> Cargo.toml:9:23
|
9 | foo = ["bar/baz"]
| ^^^^^^^^^^^
|
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`
Caused by:
feature `foo` includes `bar/baz`, but `bar` is not a dependency
"#]])
.run();
}

View file

@ -1,7 +1,6 @@
#![allow(deprecated)]
use cargo_test_support::project;
use cargo_test_support::registry::Package;
use cargo_test_support::str;
#[cargo_test(nightly, reason = "edition2024 is not stable")]
fn default() {
@ -33,34 +32,33 @@ target-dep = { version = "0.1.0", optional = true }
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_stderr(
"\
warning: unused optional dependency
.with_stderr_data(str![[r#"
[WARNING] unused optional dependency
--> Cargo.toml:9:1
|
9 | bar = { version = \"0.1.0\", optional = true }
9 | bar = { version = "0.1.0", optional = true }
| ---
|
= note: `cargo::unused_optional_dependency` is set to `warn` by default
= help: remove the dependency or activate it in a feature with `dep:bar`
warning: unused optional dependency
= [NOTE] `cargo::unused_optional_dependency` is set to `warn` by default
= [HELP] remove the dependency or activate it in a feature with `dep:bar`
[WARNING] unused optional dependency
--> Cargo.toml:12:1
|
12 | baz = { version = \"0.1.0\", optional = true }
12 | baz = { version = "0.1.0", optional = true }
| ---
|
= help: remove the dependency or activate it in a feature with `dep:baz`
warning: unused optional dependency
= [HELP] remove the dependency or activate it in a feature with `dep:baz`
[WARNING] unused optional dependency
--> Cargo.toml:15:1
|
15 | target-dep = { version = \"0.1.0\", optional = true }
15 | target-dep = { version = "0.1.0", optional = true }
| ----------
|
= help: remove the dependency or activate it in a feature with `dep:target-dep`
[CHECKING] foo v0.1.0 ([CWD])
[FINISHED] [..]
",
)
= [HELP] remove the dependency or activate it in a feature with `dep:target-dep`
[CHECKING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]])
.run();
}
@ -88,14 +86,13 @@ implicit_features = "allow"
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints"])
.with_stderr(
"\
[UPDATING] [..]
.with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 2 packages to latest compatible versions
[CHECKING] foo v0.1.0 ([CWD])
[FINISHED] [..]
",
)
[CHECKING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]])
.run();
}
@ -129,34 +126,33 @@ target-dep = { version = "0.1.0", optional = true }
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_stderr(
"\
warning: unused optional dependency
.with_stderr_data(str![[r#"
[WARNING] unused optional dependency
--> Cargo.toml:9:1
|
9 | bar = { version = \"0.1.0\", optional = true }
9 | bar = { version = "0.1.0", optional = true }
| ---
|
= note: `cargo::unused_optional_dependency` is set to `warn` by default
= help: remove the dependency or activate it in a feature with `dep:bar`
warning: unused optional dependency
= [NOTE] `cargo::unused_optional_dependency` is set to `warn` by default
= [HELP] remove the dependency or activate it in a feature with `dep:bar`
[WARNING] unused optional dependency
--> Cargo.toml:12:1
|
12 | baz = { version = \"0.2.0\", package = \"bar\", optional = true }
12 | baz = { version = "0.2.0", package = "bar", optional = true }
| ---
|
= help: remove the dependency or activate it in a feature with `dep:baz`
warning: unused optional dependency
= [HELP] remove the dependency or activate it in a feature with `dep:baz`
[WARNING] unused optional dependency
--> Cargo.toml:15:1
|
15 | target-dep = { version = \"0.1.0\", optional = true }
15 | target-dep = { version = "0.1.0", optional = true }
| ----------
|
= help: remove the dependency or activate it in a feature with `dep:target-dep`
[CHECKING] foo v0.1.0 ([CWD])
[FINISHED] [..]
",
)
= [HELP] remove the dependency or activate it in a feature with `dep:target-dep`
[CHECKING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]])
.run();
}
@ -185,19 +181,168 @@ optional-dep = []
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_stderr(
"\
warning: unused optional dependency
.with_stderr_data(str![[r#"
[WARNING] unused optional dependency
--> Cargo.toml:9:1
|
9 | optional-dep = { version = \"0.1.0\", optional = true }
9 | optional-dep = { version = "0.1.0", optional = true }
| ------------
|
= note: `cargo::unused_optional_dependency` is set to `warn` by default
= help: remove the dependency or activate it in a feature with `dep:optional-dep`
[CHECKING] foo v0.1.0 ([CWD])
[FINISHED] [..]
",
)
= [NOTE] `cargo::unused_optional_dependency` is set to `warn` by default
= [HELP] remove the dependency or activate it in a feature with `dep:optional-dep`
[CHECKING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]])
.run();
}
#[cargo_test(nightly, reason = "edition2024 is not stable")]
fn inactive_weak_optional_dep() {
Package::new("dep_name", "0.1.0")
.feature("dep_feature", &[])
.publish();
// `dep_name`` is included as a weak optional dependency throught speficying the `dep_name?/dep_feature` in feature table.
// In edition2024, `dep_name` need to be add `dep:dep_name` to feature table to activate it.
// This test explain the conclusion mentioned above
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2024"
[dependencies]
dep_name = { version = "0.1.0", optional = true }
[features]
foo_feature = ["dep:dep_name", "dep_name?/dep_feature"]
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.run();
// This test proves no regression when dep_name isn't included
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2024"
[dependencies]
[features]
foo_feature = ["dep_name?/dep_feature"]
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] feature `foo_feature` includes `dep_name?/dep_feature`, but `dep_name` is not a dependency
--> Cargo.toml:11:27
|
11 | foo_feature = ["dep_name?/dep_feature"]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`
"#]])
.run();
// Ensure that a weak dependency feature requires the existence of a `dep:` feature in edition 2024.
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2024"
[dependencies]
dep_name = { version = "0.1.0", optional = true }
[features]
foo_feature = ["dep_name?/dep_feature"]
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] feature `foo_feature` includes `dep_name?/dep_feature`, but `dep_name` is not a dependency
--> Cargo.toml:12:31
|
9 | dep_name = { version = "0.1.0", optional = true }
| -------- `dep_name` is an unused optional dependency since no feature enables it
10 |
11 | [features]
12 | foo_feature = ["dep_name?/dep_feature"]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= [HELP] enable the dependency with `dep:dep_name`
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`
"#]])
.run();
// Check target.'cfg(unix)'.dependencies can work
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2024"
[target.'cfg(unix)'.dependencies]
dep_name = { version = "0.1.0", optional = true }
[features]
foo_feature = ["dep_name?/dep_feature"]
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] feature `foo_feature` includes `dep_name?/dep_feature`, but `dep_name` is not a dependency
--> Cargo.toml:12:27
|
9 | dep_name = { version = "0.1.0", optional = true }
| -------- `dep_name` is an unused optional dependency since no feature enables it
10 |
11 | [features]
12 | foo_feature = ["dep_name?/dep_feature"]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= [HELP] enable the dependency with `dep:dep_name`
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`
"#]])
.run();
}