Skip to content

Commit

Permalink
Auto merge of #13839 - epage:df_2024, r=Muscraft
Browse files Browse the repository at this point in the history 
fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting

### What does this PR try to resolve?

This is part of rust-lang/rust#123754

This is a follow up to #11409 which tweaked how we do inheritance of default-features, including warning when `default-features = false` is ignored.  This turns those warnings into an error.

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

### Additional information
  • Loading branch information
@bors
bors committed May 1, 2024
2 parents 57d3248 + 627b1d1 commit d34d0a1
Show file tree
Hide file tree
Showing 5 changed files with 401 additions and 66 deletions.
7 changes: 7 additions & 0 deletions crates/cargo-util-schemas/src/manifest/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,13 @@ impl TomlDependency {
}
}

pub fn default_features(&self) -> Option<bool> {
match self {
TomlDependency::Detailed(d) => d.default_features(),
TomlDependency::Simple(..) => None,
}
}

pub fn unused_keys(&self) -> Vec<String> {
match self {
TomlDependency::Simple(_) => vec![],
Expand Down
63 changes: 63 additions & 0 deletions src/cargo/ops/fix.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use std::{env, fs, str};

use anyhow::{bail, Context as _};
use cargo_util::{exit_status_to_string, is_simple_exit_code, paths, ProcessBuilder};
use cargo_util_schemas::manifest::TomlManifest;
use rustfix::diagnostics::Diagnostic;
use rustfix::CodeFix;
use semver::Version;
Expand Down Expand Up @@ -265,6 +266,10 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
format!("{file} from {existing_edition} edition to {prepare_for_edition}"),
)?;

let ws_original_toml = match ws.root_maybe() {
MaybePackage::Package(package) => package.manifest().original_toml(),
MaybePackage::Virtual(manifest) => manifest.original_toml(),
};
if Edition::Edition2024 <= prepare_for_edition {
let mut document = pkg.manifest().document().clone().into_mut();
let mut fixes = 0;
Expand All @@ -290,10 +295,15 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
fixes += rename_array_of_target_fields_2024(root, "test");
fixes += rename_array_of_target_fields_2024(root, "bench");
fixes += rename_dep_fields_2024(root, "dependencies");
fixes += remove_ignored_default_features_2024(root, "dependencies", ws_original_toml);
fixes += rename_table(root, "dev_dependencies", "dev-dependencies");
fixes += rename_dep_fields_2024(root, "dev-dependencies");
fixes +=
remove_ignored_default_features_2024(root, "dev-dependencies", ws_original_toml);
fixes += rename_table(root, "build_dependencies", "build-dependencies");
fixes += rename_dep_fields_2024(root, "build-dependencies");
fixes +=
remove_ignored_default_features_2024(root, "build-dependencies", ws_original_toml);
for target in root
.get_mut("target")
.and_then(|t| t.as_table_like_mut())
Expand All @@ -302,10 +312,22 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
.filter_map(|(_k, t)| t.as_table_like_mut())
{
fixes += rename_dep_fields_2024(target, "dependencies");
fixes +=
remove_ignored_default_features_2024(target, "dependencies", ws_original_toml);
fixes += rename_table(target, "dev_dependencies", "dev-dependencies");
fixes += rename_dep_fields_2024(target, "dev-dependencies");
fixes += remove_ignored_default_features_2024(
target,
"dev-dependencies",
ws_original_toml,
);
fixes += rename_table(target, "build_dependencies", "build-dependencies");
fixes += rename_dep_fields_2024(target, "build-dependencies");
fixes += remove_ignored_default_features_2024(
target,
"build-dependencies",
ws_original_toml,
);
}

if 0 < fixes {
Expand Down Expand Up @@ -337,6 +359,47 @@ fn rename_dep_fields_2024(parent: &mut dyn toml_edit::TableLike, dep_kind: &str)
fixes
}

fn remove_ignored_default_features_2024(
parent: &mut dyn toml_edit::TableLike,
dep_kind: &str,
ws_original_toml: &TomlManifest,
) -> usize {
let mut fixes = 0;
for (name_in_toml, target) in parent
.get_mut(dep_kind)
.and_then(|t| t.as_table_like_mut())
.iter_mut()
.flat_map(|t| t.iter_mut())
.filter_map(|(k, t)| t.as_table_like_mut().map(|t| (k, t)))
{
let name_in_toml: &str = &name_in_toml;
let ws_deps = ws_original_toml
.workspace
.as_ref()
.and_then(|ws| ws.dependencies.as_ref());
if let Some(ws_dep) = ws_deps.and_then(|ws_deps| ws_deps.get(name_in_toml)) {
if ws_dep.default_features() == Some(false) {
continue;
}
}
if target
.get("workspace")
.and_then(|i| i.as_value())
.and_then(|i| i.as_bool())
== Some(true)
&& target
.get("default-features")
.and_then(|i| i.as_value())
.and_then(|i| i.as_bool())
== Some(false)
{
target.remove("default-features");
fixes += 1;
}
}
fixes
}

fn rename_array_of_target_fields_2024(root: &mut dyn toml_edit::TableLike, kind: &str) -> usize {
let mut fixes = 0;
for target in root
Expand Down
150 changes: 84 additions & 66 deletions src/cargo/util/toml/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,8 +692,14 @@ fn resolve_dependencies<'a>(

let mut deps = BTreeMap::new();
for (name_in_toml, v) in dependencies.iter() {
let mut resolved =
dependency_inherit_with(v.clone(), name_in_toml, inherit, package_root, warnings)?;
let mut resolved = dependency_inherit_with(
v.clone(),
name_in_toml,
inherit,
package_root,
edition,
warnings,
)?;
if let manifest::TomlDependency::Detailed(ref mut d) = resolved {
deprecated_underscore(
&d.default_features2,
Expand Down Expand Up @@ -949,12 +955,13 @@ fn dependency_inherit_with<'a>(
name: &str,
inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>,
package_root: &Path,
edition: Edition,
warnings: &mut Vec<String>,
) -> CargoResult<manifest::TomlDependency> {
match dependency {
manifest::InheritableDependency::Value(value) => Ok(value),
manifest::InheritableDependency::Inherit(w) => {
inner_dependency_inherit_with(w, name, inherit, package_root, warnings).with_context(|| {
inner_dependency_inherit_with(w, name, inherit, package_root, edition, warnings).with_context(|| {
format!(
"error inheriting `{name}` from workspace root manifest's `workspace.dependencies.{name}`",
)
Expand All @@ -968,76 +975,87 @@ fn inner_dependency_inherit_with<'a>(
name: &str,
inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>,
package_root: &Path,
edition: Edition,
warnings: &mut Vec<String>,
) -> CargoResult<manifest::TomlDependency> {
fn default_features_msg(label: &str, ws_def_feat: Option<bool>, warnings: &mut Vec<String>) {
let ws_def_feat = match ws_def_feat {
Some(true) => "true",
Some(false) => "false",
None => "not specified",
};
let ws_dep = inherit()?.get_dependency(name, package_root)?;
let mut merged_dep = match ws_dep {
manifest::TomlDependency::Simple(ws_version) => manifest::TomlDetailedDependency {
version: Some(ws_version),
..Default::default()
},
manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(),
};
let manifest::TomlInheritedDependency {
workspace: _,

features,
optional,
default_features,
default_features2,
public,

_unused_keys: _,
} = &pkg_dep;
let default_features = default_features.or(*default_features2);

match (default_features, merged_dep.default_features()) {
// member: default-features = true and
// workspace: default-features = false should turn on
// default-features
(Some(true), Some(false)) => {
merged_dep.default_features = Some(true);
}
// member: default-features = false and
// workspace: default-features = true should ignore member
// default-features
(Some(false), Some(true)) => {
deprecated_ws_default_features(name, Some(true), edition, warnings)?;
}
// member: default-features = false and
// workspace: dep = "1.0" should ignore member default-features
(Some(false), None) => {
deprecated_ws_default_features(name, None, edition, warnings)?;
}
_ => {}
}
merged_dep.features = match (merged_dep.features.clone(), features.clone()) {
(Some(dep_feat), Some(inherit_feat)) => Some(
dep_feat
.into_iter()
.chain(inherit_feat)
.collect::<Vec<String>>(),
),
(Some(dep_fet), None) => Some(dep_fet),
(None, Some(inherit_feat)) => Some(inherit_feat),
(None, None) => None,
};
merged_dep.optional = *optional;
merged_dep.public = *public;
Ok(manifest::TomlDependency::Detailed(merged_dep))
}

fn deprecated_ws_default_features(
label: &str,
ws_def_feat: Option<bool>,
edition: Edition,
warnings: &mut Vec<String>,
) -> CargoResult<()> {
let ws_def_feat = match ws_def_feat {
Some(true) => "true",
Some(false) => "false",
None => "not specified",
};
if Edition::Edition2024 <= edition {
anyhow::bail!("`default-features = false` cannot override workspace's `default-features`");
} else {
warnings.push(format!(
"`default-features` is ignored for {label}, since `default-features` was \
{ws_def_feat} for `workspace.dependencies.{label}`, \
this could become a hard error in the future"
))
));
}
inherit()?.get_dependency(name, package_root).map(|ws_dep| {
let mut merged_dep = match ws_dep {
manifest::TomlDependency::Simple(ws_version) => manifest::TomlDetailedDependency {
version: Some(ws_version),
..Default::default()
},
manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(),
};
let manifest::TomlInheritedDependency {
workspace: _,

features,
optional,
default_features,
default_features2,
public,

_unused_keys: _,
} = &pkg_dep;
let default_features = default_features.or(*default_features2);

match (default_features, merged_dep.default_features()) {
// member: default-features = true and
// workspace: default-features = false should turn on
// default-features
(Some(true), Some(false)) => {
merged_dep.default_features = Some(true);
}
// member: default-features = false and
// workspace: default-features = true should ignore member
// default-features
(Some(false), Some(true)) => {
default_features_msg(name, Some(true), warnings);
}
// member: default-features = false and
// workspace: dep = "1.0" should ignore member default-features
(Some(false), None) => {
default_features_msg(name, None, warnings);
}
_ => {}
}
merged_dep.features = match (merged_dep.features.clone(), features.clone()) {
(Some(dep_feat), Some(inherit_feat)) => Some(
dep_feat
.into_iter()
.chain(inherit_feat)
.collect::<Vec<String>>(),
),
(Some(dep_fet), None) => Some(dep_fet),
(None, Some(inherit_feat)) => Some(inherit_feat),
(None, None) => None,
};
merged_dep.optional = *optional;
merged_dep.public = *public;
manifest::TomlDependency::Detailed(merged_dep)
})
Ok(())
}

#[tracing::instrument(skip_all)]
Expand Down
Loading

0 comments on commit d34d0a1

Please sign in to comment.