feat(manifest)!: implement feature-metadata RFC3416 #15056

AudaciousAxiom · 2025-01-12T17:43:16Z

What does this PR try to resolve?

This PR implements RFC3416: feature-metadata.
It introduces an alternate syntax to define crate features in manifests: instead of simply being an array of features the feature enables, it can now be a table with, for now, one required array key, enables, which is equivalent to the existing array. This lays the groundwork for later supporting additional keys in that table, to provide feature metadata.

Related to #14157

How should we test and review this PR?

Integrations tests are included in this PR, including for the normalized manifest used for publishing on a crate registry.

Additional information

The JSON schema of the manifest was updated using:

cargo test -p cargo-util-schemas --features unstable-schema -- 'dump_manifest_schema'

Questions

Should there be a Cargo unstable feature for this change?
This PR of course introduces a breaking change in the cargo-util-schemas crate, is there anything to do regarding this in this PR?
Should this PR also attempt to update core::Summary or should this be left to future implementations of RFCs providing other keys (e.g., doc)?

rustbot · 2025-01-12T17:43:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

AudaciousAxiom · 2025-01-12T17:44:35Z

crates/cargo-util-schemas/src/manifest/mod.rs

+
+impl FeatureDefinition {
+    /// Returns the features that this feature enables.
+    pub fn enables(&self) -> &[String] {


This method could also return an impl Iterator<Item = String> if preferred.

src/cargo/util/toml/mod.rs

tests/testsuite/features.rs

AudaciousAxiom · 2025-01-12T17:47:39Z

src/cargo/util/toml/mod.rs

+            let feature_array = feature_deps
+                .enables()
+                .iter()
+                .filter(|feature_dep| {
+                    let feature_value = FeatureValue::new(InternedString::new(feature_dep));
+                    match feature_value {
+                        FeatureValue::Dep { dep_name }
+                        | FeatureValue::DepFeature { dep_name, .. } => {
+                            let k = &manifest::PackageName::new(dep_name.to_string()).unwrap();
+                            dep_name_set.contains(k)
+                        }
+                        _ => true,
                    }
-                    _ => true,
-                }
-            });
+                })
+                .cloned()
+                .collect();
+            *feature_deps = FeatureDefinition::Array(feature_array);


For compatibility, this uses the array syntax for generating the normalized manifest, even when the table syntax is used by authors (see the corresponding integration test).

weihanglo · 2025-01-12T18:42:45Z

src/cargo/util/toml/mod.rs

+    for (feature, feature_definition) in features {
+        match feature_definition {
+            FeatureDefinition::Array(..) => {}
+            FeatureDefinition::Metadata(FeatureMetadata { _unused_keys, .. }) => {


haven't got time into full review, though I think the meta field should be behind a nightly feature flag.

Should there be a Cargo unstable feature for this change?

Yes. Probably behind a cargo-feature "feature-metadata".

This PR of course introduces a breaking change in the cargo-util-schemas crate, is there anything to do regarding this in this PR?

Could add a doc comment on relevant field/variant indicating it is unstable/nightly only.

cargo/crates/cargo-util-schemas/src/manifest/mod.rs

Lines 936 to 937 in f15df8f

/// Unstable feature `-Ztrim-paths`.

pub trim_paths: Option<TomlTrimPaths>,

There is a CI job checking if a member crate needs a version bump. It didn't warn you so I assume it has already been bumped in this release cycle. You do not need to do anything.

Should this PR also attempt to update core::Summary or should this be left to future implementations of RFCs providing other keys (e.g., doc)?

Summary is more like a thing for dependency resolution. I think we revisit it in the future. Regardless, see epage's comment #14157 (comment) that the feature itself is not particularly useful until other RFC gets merged. Anyway, thanks for the contribution!

epage · 2025-01-13T12:57:46Z

What is your motivation for moving this forward?

In #14157 (comment) I was wondering if this was worth it without a feature depending on it.

AudaciousAxiom · 2025-01-13T21:50:20Z

What is your motivation for moving this forward?

I have recently published featurecomb, a crate that allows to define relations between Cargo features in manifests (e.g., mutual exclusion, or a feature requiring another one to be enabled), and to enforce these relations through a proc-macro that reads the manifest and generates #[cfg]-gated compile_error! statements.

I wanted to see how much of this could be introduced into Cargo (I am aware of other existing effort in that direction), and the first step for that was to tackle feature-metadata.
(The most challenging open question I think would be that enforcement cannot be done through the manifest only, even if fully implemented in Cargo, at the very least because of course Cargo is not mandatory for compiling Rust code and it would therefore not be possible to rely on these relations being actually enforced without a dedicated attribute/macro in the source code itself, as is the case with featurecomb.)

I have introduced a feature-metadata unstable feature which can be dynamically enabled through cargo-features. However, I realize now that the same cannot be done for the cargo-util-schemas crate (we can only document the FeatureDefinition::Metadata variant as unstable). So, unless there is a way to dynamically feature-gate the change in the schema, and to not cause unnecessary churn to the dependents of cargo-util-schemas, I agree it may be best not to move forward on this PR for now, until other keys are introduced. Future implementations of these could hopefully use this PR as a base.

epage · 2025-01-13T21:55:28Z

I wanted to see how much of this could be introduced into Cargo (I am aware of other existing effort in that direction), and the first step for that was to tackle feature-metadata.

Interesting effort. Some cautions though

Trying to add mutually exclusive support for the existing feature system is likely a dead end because features must be additive
This would created "unused manifest fields" which are not subject to our compatibility guarantees. I'd recommend finding a different way to declare the relevant information

I have introduced a feature-metadata unstable feature which can be dynamically enabled through cargo-features. However, I realize now that the same cannot be done for the cargo-util-schemas crate (we can only document the FeatureDefinition::Metadata variant as unstable). So, unless there is a way to dynamically feature-gate the change in the schema, and to not cause unnecessary churn to the dependents of cargo-util-schemas, I agree it may be best not to move forward on this PR for now, until other keys are introduced. Future implementations of these could hopefully use this PR as a base.

cargo-util-schemas does not get feature gated. We do that in cargo/utils/toml/mod.rs or later.

AudaciousAxiom added 2 commits January 12, 2025 16:20

test(feature-metadata): test valid and invalid feature metadata

b414a91

feat(manifest)!: implement feature-metadata RFC3416

af99add

rustbot assigned epage Jan 12, 2025

rustbot added A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues Command-publish S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2025

AudaciousAxiom commented Jan 12, 2025

View reviewed changes

fixup! feat(manifest)!: implement feature-metadata RFC3416

338281e

AudaciousAxiom force-pushed the feat/rfc-impl-feature-metadata branch from 790a0e8 to 338281e Compare January 12, 2025 17:49

AudaciousAxiom marked this pull request as ready for review January 12, 2025 18:04

weihanglo reviewed Jan 12, 2025

View reviewed changes

fixup! test(feature-metadata): test valid and invalid feature metadata

1cdcd4a

AudaciousAxiom added 3 commits January 13, 2025 17:53

fixup! test(feature-metadata): test valid and invalid feature metadata

d67aee5

feat(feature-metadata): feature-gate the unstable feature

567a104

fixup! feat(manifest)!: implement feature-metadata RFC3416

d61ffb5

rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-unstable Area: nightly unstable support labels Jan 13, 2025

fixup! feat(manifest)!: implement feature-metadata RFC3416

2faf5e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifest)!: implement feature-metadata RFC3416 #15056

feat(manifest)!: implement feature-metadata RFC3416 #15056

AudaciousAxiom commented Jan 12, 2025

rustbot commented Jan 12, 2025

AudaciousAxiom Jan 12, 2025

AudaciousAxiom Jan 12, 2025 •

edited

Loading

weihanglo Jan 12, 2025

weihanglo Jan 13, 2025

epage commented Jan 13, 2025

AudaciousAxiom commented Jan 13, 2025

epage commented Jan 13, 2025

	/// Unstable feature `-Ztrim-paths`.
	pub trim_paths: Option<TomlTrimPaths>,

feat(manifest)!: implement feature-metadata RFC3416 #15056

Are you sure you want to change the base?

feat(manifest)!: implement feature-metadata RFC3416 #15056

Conversation

AudaciousAxiom commented Jan 12, 2025

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Questions

rustbot commented Jan 12, 2025

AudaciousAxiom Jan 12, 2025

Choose a reason for hiding this comment

AudaciousAxiom Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

weihanglo Jan 12, 2025

Choose a reason for hiding this comment

weihanglo Jan 13, 2025

Choose a reason for hiding this comment

epage commented Jan 13, 2025

AudaciousAxiom commented Jan 13, 2025

epage commented Jan 13, 2025

AudaciousAxiom Jan 12, 2025 •

edited

Loading