Warn or error when using mandatory dependencies as a feature

[RalfJung]

Currently, Cargo warns when I ask for a feature of another crate that does not exist. However, it seems that it treats all dependencies also as features. For optional dependencies, this makes sense, and indeed the documentation says "A feature of a package is either an optional dependency, or a set of other features."

So, I can depend on regex = { version = "0.2.2", features = ["memchr"]} and there is no warning or error that this (a) doesn't make any sense, and (b) memchr is not technically a feature according to the definition of the docs (and indeed, it should not be).

[alexcrichton]

Hm this is surprising! Definitely sounds like a bug!

[RalfJung]

So I wrote these testcases demonstrating the issue, both of which currently fail (cargo tries to build the package):
RalfJung@12c4e3b

However, I have a hard time wrapping my head around the features code trying to fix this.^^

[RalfJung]

In particular, something is wrong with the comments here it seems:

        // Next, sanitize all requested features by whitelisting all the
        // requested features that correspond to optional dependencies
        for dep in deps {
            // weed out optional dependencies, but not those required
            if dep.is_optional() && !feature_deps.contains_key(dep.name()) {
                continue
            }
            let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]);
            base.extend(dep.features().iter().cloned());
            for feature in base.iter() {
                if feature.contains("/") {
                    bail!("feature names may not contain slashes: `{}`", feature);
                }
            }
            ret.push((dep.clone(), base));
        }

Above the loop it says something about whitelisting optional features, but then what it actually does it go over all enabled (required and optional) dependencies and remove them from feature_deps.

[RalfJung]

In fact, the existing data structures seem unsuited to even detect this case: If bar is a required dep, bar/feat is a valid feature but bar is not. However, both look the same when build_features is done.

The fact that resolve_features does not have a comment explaining what it does doesn't help, btw... ;) . (And it has an argument called "candidate" that is actually a summary...). It seems this is actually responsible for collecting all dependencies? Its return value includes required dependencies that have nothing to do with any features. However, when I broke things and had it return the empty list, dependencies were still built, so there seems to be another source. confused

[sfackler]

There are valid use cases for using these kinds of features. You may happen to use a crate internally, and also want to offer e.g. trait implementations for its types optionally. Unconditionally implementing the traits locks you into depending on that library, while putting the public trait implementations behind a feature gives you more flexibility in the future.

[RalfJung]

@sfackler Not sure what you are saying. I never doubted that bar/feat is useful even when bar is required. However, bar is a feature only when it is an optional dependency, and this bug is about cargo not implementing that properly.

[sfackler]

I am saying there are times where you do want to use features named after required dependencies.

[RalfJung]

I see. I somehow forgot about the cfg(feature = "...") check adding functionality; for cargo itself, a required dependency feature is a NOP. Furthermore, Cargo's documentation (quoted in the original post opening the issue) is pretty clear that only optional dependencies are features, and same for its source code:

// All features can only point to optional dependencies, in which case
// they should have all been weeded out by the above iteration. Any
// remaining features are bugs in that the package does not actually
// have those features.

So it seems fairly clear that such features were not intended to work.

The trouble with allowing such features (which is what lead me to discover this bug) is that usually, if you enable a feature that's called like a dependency, that actually makes a difference. So I thought by setting that feature, I accomplished something -- but actually, I did not. That's just like enabling a feature that does not exist at all. Unfortunately, there is no way for cargo to know whether some particular required dependency is used as a feature or not.

(TBH, I think features and optional dependencies should be disjoint concepts. Allowing others to enable random combinations of optional dependencies in my crate seems scary, I'd rather carefully control which features flags are actually available. If that was the case, features and dependencies could have the same name, and we wouldn't have any of these problems. But oh well, there's no way changing that now.)

Note that there is no fundamental reason why the feature has to be called like the required dependency. The crate author could pick any name.

One way to fix all of this may be to actually allow features that have the same name as a dependency, and specify them as adding additional things that are enabled (beyond just enablng the dependency, in case it is an optional one) when the feature is set.

[sfackler]

Yeah, making feature names disjoint from dependency names would be great.