Check manifest for requiring nonexistent features

[aleksator]

Fixes #4854: Examples requiring a nonexistent feature should be an error

Thanks @lukaslueg with his #4874 for the inspiration!

[rust-highfive]

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

[ehuss]

This will probably need to be a warning instead of an error, since this will affect a number of existing packages. We can then maybe turn it to an error in the distant future.

I think it might be better to take a different approach here. Optional dependencies create implicitly named features which are not defined in the [features] table. Also, it's probably a good idea to handle the foo/bar feature syntax. And features can have namespaced syntax like crate:name.

I think to handle this properly, it would be best to put the validation in Workspace::validate. The goal is to get access to FeatureMap which has all the parsed features (including optional dependencies). You can iterate over each member, and get its summary. From the Summary, you can get the FeatureMap. Then while iterating over each workspace member, you can get the member's targets. Then for each target, check if it has required-features, and if so, compare them against the FeatureMap. I think maybe calling FeatureValue::new will be required to parse / syntax stuff of the required-features.

[aleksator]

I think to handle this properly, it would be best to put the validation in Workspace::validate.

This function quits in the beginning on this condition:

        if self.root_manifest.is_none() {
            return Ok(());
        }

Reading the docs for root_manifest (when it's None) makes me believe that there won't be any validation done if no workspace was declared in Cargo.toml:

    // If this workspace includes more than one crate, this points to the root
    // of the workspace. This is `None` in the case that `[workspace]` is
    // missing, `package.workspace` is missing, and no `Cargo.toml` above
    // `current_manifest` was found on the filesystem with `[workspace]`.
    root_manifest: Option<PathBuf>,

@ehuss Wouldn't it lead to the issue persisting for projects with no workspaces if we do the check in this function?

[ehuss]

I think it would be fine to split up the validation function so it was a little clearer (and not nearly 200 lines long). Maybe something like:

    fn validate(&mut self) -> CargoResult<()> {
        if self.root_manifest.is_some() {
            // These checks require a VirtualManifest or multiple members.
            self.validate_unique_names()?;
            self.validate_ws_roots()?;
            self.validate_members()?;
            self.validate_unused()?;
        }
        self.validate_required_features()?;
        Ok(())
    }

[aleksator]

@ehuss
What is the behavior we want in regards to features with '/' in them? Check if what comes after slash is present in the features section, and ignore what comes before '/'?

What about features with '::' instead of slash?

[ehuss]

Hm, sorry, I think I may have led you astray with how validation should be done. For some reason I was thinking the Summary feature map included dependencies, but it doesn't.

We want to accurately parse required-features and handle them the same way as Cargo normally interprets them.

I'm thinking the right route here is to change how required-features are checked. Currently in resolve_all_features it collects all enabled feature names, and all enabled features of every direct dependency into a HashSet. Instead, I'm thinking it should iterate over each required-feature, parse it into a FeatureValue, and run a query to check (A) does that feature exist (warn if it doesn't) and (B) is that feature enabled.

There isn't anything that will do that directly, so it will require some new code. Checking will depend on the FeatureValue kind:

• FeatureValue::Feature: Check if the given string is in the package's featuremap.
• FeatureValue::Crate: Check if there is an optional dependency matching the given name (dependencies can be obtained from the Summary).
• FeatureValue::CrateFeature: Fetch the summary of the given crate name from Resolve, and check for an optional dependency with that name, or a feature in the package's featuremap.

To check if the feature is enabled, it should be a little simpler, just calling resolved_features.activated_features_unverified(…) as needed.

One drawback to this approach is that it will only validate when you try to build the target, but I think that is fine.

Hopefully that's not too confusing. Let me know if you have any questions.

[aleksator]

Checking will depend on the FeatureValue kind:

FeatureValue::Feature: Check if the given string is in the package's featuremap.
FeatureValue::Crate: Check if there is an optional dependency matching the given name (dependencies can be obtained from the Summary).
FeatureValue::CrateFeature: Fetch the summary of the given crate name from Resolve, and check for an optional dependency with that name, or a feature in the package's featuremap.

When trying to solve this as per your first suggestion, I noticed that if you have features listed like this:

            [features]
            existing = []
            [[example]]
            name = "ololo"
            required-features = ["not_present_1", "existing",
                                 "not_present_2", "doesnt_matter_has/slash",
                                 "with::colons"]

...and you call FeatureValue::new() on them they are parsed like:

[src/cargo/core/workspace.rs:606] required_features = [
    "not_present",
    "existing",
    "doesnt_matter_has/slash",
    "with::colons",
]
[src/cargo/core/workspace.rs:609] &fv = Crate(
    "not_present",
)
[src/cargo/core/workspace.rs:609] &fv = Feature(
    "existing",
)
[src/cargo/core/workspace.rs:609] &fv = CrateFeature(
    "doesnt_matter_has",
    "slash",
)
[src/cargo/core/workspace.rs:609] &fv = Crate(
    "with::colons",
)

Meaning that non-existing features (+ the ones with ::) are parsed as Crate, and if they have '/' as CrateFeature.
That means that I can probably check for "Crate" meaning "actually a feature, but doesn't exist".

Would it be correct?

[ehuss]

Yes, assuming there isn't an optional dependency by that name.

[aleksator]

I just pushed a draft to get some feedback: is the logic already introduced is correct at least.

Still need to wrap my head around:

1. Accounting for dependencies being optional.
2. "crate:feature" syntax.
3. Checking if the feature is enabled - is it required everywhere or just in some cases.
4. The proper way to warn the user - I just eprintln'ed some messages as a temporary solution.

[ehuss]

I'm not sure there will be anything special needed for the crate: syntax, since FeatureValue takes care of that.

Warnings are done through Shell.warn. A shell instance can be obtained through config.shell().

[bors]

☔ The latest upstream changes (presumably #8068) made this pull request unmergeable. Please resolve the merge conflicts.

[aleksator]

Hi @ehuss! I have applied your suggestions to the code.

There is one more thing I want to clarify about the behavior for optional dependencies. If you run nonexistent_required_features() test, you'll notice that there is a warning saying

warning: dependency optional_dependency specified in required-features as optional_dependency/optional does not exist.

This optional dependency is specified in required-features and in [features] sections though, so it should be found. It is not, however, present in the resolve inside of warn_on_missing_features() function.

Is there a bug somewhere (in the function, in my manifest, in the resolver) or am I misunderstanding how it is supposed to work and the resulting warning is correct?

[ehuss]

Hm, so the issue here is the the Resolver has already filtered out any optional dependencies that are not enabled, so optional_dependency is not found in the dependency list.

I think the solution is to use workspace_resolve instead. When resolution is done, there are two resolves. The "workspace resolve" is the one in Cargo.lock, and it has all features enabled. The "targeted resolve" is the one with the specific user-enabled features which narrows the set of dependencies.

I'd suggest passing workspace_resolve into warn_on_missing_features. Since workspace_resolve is an Option, I'd just skip the checks if it is None. (It can be None in some cases like cargo install or cargo package where the entire workspace is not available.)

I think that should work. Sorry this is ending up to be a bit of a tricky problem, but the PR is looking pretty good to me!

[aleksator]

@ehuss Thank you, that worked! I had to add workspace_resolve as a parameter to generate_targets() though, which already had like 10 others.

Sorry this is ending up to be a bit of a tricky problem, but the PR is looking pretty good to me!

You have a patience of a saint. It was probably 10 times easier to do all of this yourself than to explain the proper solution to me :)

As a side note, I had to fix one of the benchmark tests since it generated a new warning and noticed that all of them only run on a nightly compiler. Is there a reason for it or should they be enabled on stable as well?

[ehuss]

Thanks!

As for the benchmark tests, benchmarks are not stable so they only work on the nightly compiler.

@bors r+

[bors]

📌 Commit 19a9579 has been approved by ehuss

[bors]

⌛ Testing commit 19a9579 with merge cdbd9b7...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing cdbd9b7 to master...