Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fix): Address problems with implicit -> explicit feature migration #14018

Merged
merged 6 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> {
fixes += rename_dep_fields_2024(workspace, "dependencies");
}

fixes += add_feature_for_unused_deps(pkg, root);
fixes += add_feature_for_unused_deps(pkg, root, ws.gctx());
fixes += rename_table(root, "project", "package");
if let Some(target) = root.get_mut("lib").and_then(|t| t.as_table_like_mut()) {
fixes += rename_target_fields_2024(target);
Expand Down Expand Up @@ -435,7 +435,11 @@ fn rename_table(parent: &mut dyn toml_edit::TableLike, old: &str, new: &str) ->
1
}

fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableLike) -> usize {
fn add_feature_for_unused_deps(
pkg: &Package,
parent: &mut dyn toml_edit::TableLike,
gctx: &GlobalContext,
) -> usize {
let manifest = pkg.manifest();

let activated_opt_deps = manifest
Expand All @@ -456,18 +460,48 @@ fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableL
for dep in manifest.dependencies() {
let dep_name_in_toml = dep.name_in_toml();
if dep.is_optional() && !activated_opt_deps.contains(dep_name_in_toml.as_str()) {
fixes += 1;
if let Some(features) = parent
.entry("features")
.or_insert(toml_edit::table())
.as_table_like_mut()
{
features.insert(
dep_name_in_toml.as_str(),
toml_edit::Item::Value(toml_edit::Value::Array(toml_edit::Array::from_iter(
&[format!("dep:{}", dep_name_in_toml)],
))),
);
let activate_dep = format!("dep:{dep_name_in_toml}");
let strong_dep_feature_prefix = format!("{dep_name_in_toml}/");
features
.entry(dep_name_in_toml.as_str())
.or_insert_with(|| {
fixes += 1;
toml_edit::Item::Value(toml_edit::Value::Array(
toml_edit::Array::from_iter([&activate_dep]),
))
});
// Ensure `dep:dep_name` is present for `dep_name/feature_name` since `dep:` is the
// only way to guarantee an optional dependency is available for use.
//
// The way we avoid implicitly creating features in Edition2024 is we remove the
// dependency from `resolved_toml` if there is no `dep:` syntax as that is the only
// syntax that suppresses the creation of the implicit feature.
for (feature_name, activations) in features.iter_mut() {
let Some(activations) = activations.as_array_mut() else {
let _ = gctx.shell().warn(format_args!("skipping fix of feature `{feature_name}` in package `{}`: unsupported feature schema", pkg.name()));
continue;
Comment on lines +485 to +487
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was written with rust-lang/rfcs#3416 in mind

That syntax is likely to work on any Edition and if we forget to touch this code, I'd prefer to make that known so people can report it to us and we can fix it rather than silently ignoring it.

The question that remains is to warn or error. I figured we shouldn't block the user from getting other fixes for something they can't workaround in a reasonable way.

};
if activations
.iter()
.any(|a| a.as_str().map(|a| a == activate_dep).unwrap_or(false))
{
continue;
}
let Some(activate_dep_pos) = activations.iter().position(|a| {
a.as_str()
.map(|a| a.starts_with(&strong_dep_feature_prefix))
.unwrap_or(false)
}) else {
continue;
};
fixes += 1;
activations.insert(activate_dep_pos, &activate_dep);
}
}
}
}
Expand Down
Loading