Skip to content

Commit

Permalink
Auto merge of #14026 - linyihai:weak-optional-inactive, r=weihanglo
Browse files Browse the repository at this point in the history
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 #14015

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

One commit test added, one commit fixed and updated

### Additional information
Part of #14039
- migrate `tests/testsuite/lints/unused_optional_dependencies.rs` to snapshot

And rename `MissingField` to `MissingFieldError`
  • Loading branch information
bors committed Jul 3, 2024
2 parents a0b2803 + b28eef9 commit 838d81d
Show file tree
Hide file tree
Showing 6 changed files with 371 additions and 82 deletions.
36 changes: 35 additions & 1 deletion src/cargo/core/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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!(
Expand Down
10 changes: 5 additions & 5 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/cargo/util/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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()
Expand Down
136 changes: 118 additions & 18 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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]. \
Expand Down Expand Up @@ -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>,
Expand Down
18 changes: 12 additions & 6 deletions tests/testsuite/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
Expand Down
Loading

0 comments on commit 838d81d

Please sign in to comment.