Provide higher-level API around a Package's features?

[regexident]

Before opening a PR for this I wanted to first check if such a change would be aligned with your vision of this crate, @oli-obk.

Motivation

With the transitive required features of a package encoding semantic information (weak vs. strong, dep vs. feature, transitive vs. non-transitive etc.) these days it feels like cargo_metadata should provide a fail-safe and higher-level API for that, rather than expecting users to manually perform pattern matching on the bare string representation, potentially fragmenting the ecosystem in the long-term due to subtle bugs downstream.

Draft implementation

Edit: See update here: #279 (comment)

Outdated implementation

Change the following member of Package:

pub struct Package {
    // ...

    // before:
    pub features: BTreeMap<String, Vec<String>>,
    // after:
    pub features: BTreeMap<String, Vec<FeatureDep>>,

    // ...
}

Introduce a new-type around the map's value type, that could look something like this:

/// Dependency of a crate's feature.
#[derive(Clone, Serialize, Deserialize, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)]
#[serde(transparent)]
pub struct FeatureDep {
    /// The underlying string representation.
    pub repr: String,
}

impl FeatureDep {
    /// Returns true if it is a dependency feature
    /// (i.e. `"dep:<dependency>"`).
    pub fn is_dependency(&self) -> bool {
        self.repr.starts_with("dep:")
    }

    /// Returns true if it is a transitive feature of a dependency
    /// (i.e. `"<dependency>/<feature>"` or `"<dependency>?/<feature>"`).
    pub fn is_transitive(&self) -> bool {
        self.repr.contains("/")
    }

    /// Returns true if it is a weak transitive feature of an optional dependency
    /// (i.e. `"<dependency>?/<feature>"`).
    pub fn is_weak(&self) -> bool {
        self.repr.contains("?/")
    }

    /// Returns the name of the package dependency if it is a dependency feature
    /// (i.e. `"dep:<package>"`, `"<package>/<feature>"` or `"<package>?/<feature>"`).
    ///
    /// Note: For dependency features without explicit `"dep:"` prefix this will return `None`.
    pub fn package(&self) -> Option<String> {
        if let Some(package) = self.repr.strip_prefix("dep:") {
            return Some(package.to_owned());
        }

        if let Some((package, _)) = self.repr.split_once("/") {
            if let Some(package) = package.strip_suffix("?") {
                return Some(package.to_owned());
            }

            return Some(package.to_owned());
        }

        None
    }

    /// Returns the name of the feature if it is not a dependency feature
    /// (i.e. `"dep:<package>"`, `"<package>/<feature>"` or `"<package>?/<feature>"`).
    ///
    /// Note: For dependency features without explicit `"dep:"` prefix this will return
    /// the optional dependency's implicit feature of the same name as the dependency.
    pub fn feature(&self) -> Option<String> {
        if self.repr.starts_with("dep:") {
            return None;
        }

        if let Some((_, feature)) = self.repr.split_once("/") {
            return Some(feature.to_owned());
        }

        Some(self.repr.clone())
    }
}

impl fmt::Display for FeatureDep {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(&self.repr, f)
    }
}

Correctness

Afaict the implementation provided above should be correct.

That said for implicit dependencies the result might be unexpected (yet technically correct, I think), since a FeatureDep instance cannot locally reason about whether or not it is an implicit dependency feature, without access to the set of package's features (and dependencies). (I shortly looked into wrapping the whole BTreeMap<…> in a PackageFeatures new-type, but the changes required for this felt disproportionate to the benefit of addressing this one edge-case that's already being phased-out.)

But that's arguably the fault of the (since deprecated) implicit features themselves, and not an issue with this particular implementation. And given that are already effectively deprecated and scheduled for removal in the next edition (afaict?) this feels like an acceptable edge-case?

What do you think about the general direction?

[oli-obk]

Yea I like this. There are various other high level APIs I want to figure out. In ui_test I'm ad-hoc implementing various ways to search and filter deps

[ehuss]

Just curious, why not use an enum like cargo does?

[regexident]

Just curious, why not use an enum like cargo does?

We probably should do that, indeed!

I didn't want to spend too much time looking into (/coding up) the details before checking if such an API would actually be welcome.

[obi1kenobi]

As another interested user (for cargo-semver-checks), I support this PR as well — thank you for working on it!

The upcoming release of cargo-semver-checks will also lint features and manifest information, so knowing which features are enabled when is very useful. Right now I use a hodgepodge of cargo_metadata, my own code, and some other crates, and I'd love to be able to use just cargo_metadata instead.

In principle, it would even be useful to know "what is the full set of transitively-enabled features from enabling feature X." This requires a traversal of the feature graph, which isn't that hard to implement but it would be nice if we didn't all have to maintain our own copies. One canonical well-tested implementation in cargo_metadata would be wonderful to have, so that not everyone has to learn all the edge cases of cargo features.

[regexident]

@obi1kenobi while this is wasn't the motivation behind my interest in a richer features API, now that you mention it I would actually need such a functionality later on as well! 👍🏻

[regexident]

After having done some further exploration today I've come to the conclusion that a visitor API would probably be the most flexible, for which I've just opened a draft PR at #281