v2 resolver: different handling for inactive, optional dependencies based on how they're specified

[sunshowers]

Problem
Looks like the v2 resolver appears to have some small differences to a dependency that's optional, target-specific, and currently inactive, based on how it's specified.

Specifically, if inactive is a target-specific, optional dependency that is inactive on the platform under consideration:

[features]
foo = ["inactive/extra"]

causes the inactive feature to not be activated, while

[features]
foo = ["inactive", "inactive/extra"]

does cause the feature to be activated.

Steps

Test workspace.

git clone https://github.com/sunshowers/cargo-guppy && cd cargo-guppy
git checkout bce9ea7e6150049a8e660b401b7dabf79f8abdef
cd fixtures/workspace/inside-outside/workspace
cargo clean && cargo +nightly build -Zfeatures=all -p internal --all-features --verbose 2>&1 | grep -F -- "--crate-name internal "

causes:

     Running `rustc --crate-name internal --edition=2018 internal/src/lib.rs
--error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -Cembed-bitcode=no -C debuginfo=2 --cfg 'feature="build-feature"' --cfg 'feature="default"' --cfg 'feature="dev-feature"' --cfg 'feature="extra"' [...]

Now if you change to:

[features]
extra = ["x86-active', "x86-active/extra"]

You'll get:

     Running `rustc --crate-name internal --edition=2018 internal/src/lib.rs
--error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -Cembed-bitcode=no -C debuginfo=2 --cfg 'feature="build-feature"' --cfg 'feature="default"' --cfg 'feature="dev-feature"' --cfg 'feature="extra"' --cfg 'feature="x86-active"' [...]

There is no difference between the two for active features, as you can find out by setting --target i686-unknown-linux-gnu.

Possible Solution(s)
A uniform solution for both, I'd prefer the x86-active feature to be present for both I think.

Notes

Output of cargo version:

cargo 1.45.0-nightly (9fcb8c1d2 2020-05-25)

cc @ehuss

[ehuss]

Unfortunately the commit bce9ea7e6150049a8e660b401b7dabf79f8abdef is no longer available on any branch in your repository. Would it be possible to create a branch with this reproduction? I tried manually updating the x86-active branch, but I couldn't reproduce the problem.

[sunshowers]

@ehuss could you try with the x86-active branch on https://github.com/sunshowers/cargo-guppy again? I just tested with cargo 1.46.0-nightly (c26576f9a 2020-06-23) and I can reproduce with the given instructions.

[ehuss]

I guess maybe I'm a little confused. If I:

1. Edit inside-outside/workspace/internal/Cargo.toml and change the extra feature to extra = ["x86-active", "x86-active/extra"].
2. Run cargo build -p internal --all-features --verbose 2>&1 | grep "crate-name internal"
3. Run cargo build -p internal --all-features --verbose -Zfeatures=all 2>&1 | grep "crate-name internal"

I get the same output for the --cfg flags (both include --cfg 'feature="x86-active"'). That is, it doesn't seem that the behavior differs based on the v2 resolver.

I can see how the difference between having a feature x86-active and a feature x86-active/extra being different is a little strange. I'm curious why you say you would prefer for it to be present, though. If the dependency is not included (because it is for another platform), I would assume most people would want it to be absent, since it isn't actually enabled.

[sunshowers]

Thanks! The main difference between the v1 and v2 resolvers is that inactive targets are always enabled for the v1 resolver so the issue is moot there. So in other words, on x86_64, here's the table for if the x86-active feature is enabled:

	["x86-active/extra"]	["x86-active", "x86-active/extra"]
v1	enabled	enabled
v2	disabled	enabled

Thinking about it more, you're probably right -- for the v2 resolver both columns should probably read "disabled".

[sunshowers]

@ehuss do you think this would be fixable before the v2 resolver is stabilized? or would fixing this after stabilization not be considered a breaking change.

[ehuss]

Yep, I think this should definitely be fixed before stabilization. I'm not sure how likely it is to break anything, but better to be safe. If you want to try to fix it, feel free to dive in! Otherwise, it might be a while since I need to work on #8549 first, and it is going to be challenging.

I have put together an update to the existing testsuite to cover this situation and exhibits the undesired behavior:

diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs
index 3790ab4be..54a0ffd71 100644
--- a/tests/testsuite/features2.rs
+++ b/tests/testsuite/features2.rs
@@ -96,6 +96,7 @@ fn inactive_target_optional() {
 
             [features]
             foo1 = ["dep1/f2"]
+            foo2 = ["dep2"]
             "#,
         )
         .file(
@@ -103,6 +104,7 @@ fn inactive_target_optional() {
             r#"
             fn main() {
                 if cfg!(feature="foo1") { println!("foo1"); }
+                if cfg!(feature="foo2") { println!("foo2"); }
                 if cfg!(feature="dep1") { println!("dep1"); }
                 if cfg!(feature="dep2") { println!("dep2"); }
                 if cfg!(feature="common") { println!("common"); }
@@ -149,7 +151,7 @@ fn inactive_target_optional() {
         .build();
 
     p.cargo("run --all-features")
-        .with_stdout("foo1\ndep1\ndep2\ncommon\nf1\nf2\nf3\nf4\n")
+        .with_stdout("foo1\nfoo2\ndep1\ndep2\ncommon\nf1\nf2\nf3\nf4\n")
         .run();
     p.cargo("run --features dep1")
         .with_stdout("dep1\nf1\n")
@@ -164,9 +166,10 @@ fn inactive_target_optional() {
         .with_stdout("common\nf4\n")
         .run();
 
+    // ERROR: This fails with "dep2" feature enabled.
     p.cargo("run -Zfeatures=itarget --all-features")
         .masquerade_as_nightly_cargo()
-        .with_stdout("foo1\n")
+        .with_stdout("foo1\nfoo2\n")
         .run();
     p.cargo("run -Zfeatures=itarget --features dep1")
         .masquerade_as_nightly_cargo()

[ehuss]

@sunshowers If you have something like this:

[target.'cfg(not_my_platform)'.dependencies]
log = { version = "0.4", optional = true }

and you ran cargo build -Zfeatures=itarget --features log, would you expect the log feature to be enabled?

[sunshowers]

@ehuss (sorry for the late response, I had to be away for a while and then I missed your q...)

Hmm, code that uses it would probably be like:

#[cfg(feature = "log")]
use log::{...};

If the log feature exists, I think most code would expect the log dependency to exist. So I'd lean towards no, I'd probably not expect the log feature to be enabled if the dependency is inactive.

[sunshowers]

BTW, following up here, I did some more tests against cargo 1.51.0-nightly (a73e5b7d5 2021-01-12) and here's what I saw:

	["x86-active/extra"]	["x86-active", "x86-active/extra"]
v1	enabled	enabled
v2	enabled	enabled

This is a bit different from what we discussed above (both lines for v2 read disabled). Based on the argument above I also suspect that disabled really is the correct behavior... I'm not sure how to reopen this issue @ehuss, lmk.

[ehuss]

We (the team) had some long discussions about this, and decided to go with that behavior, particularly when considering namespaced features. By decoupling features with dependencies, that means that a feature gets enabled, even if one of its dependencies is not. So for example, an optional dependency creates an implicit feature like this:

[features]
somedep = ["dep:somedep"]

Here, if "dep:somedep" is deactivated, the feature is still set. Making the feature disappear if its dep is deactivated would introduce an inconsistency with how other features behave. It could also cause some complications with more complex features like thing = ["dep:dep1", "dep:dep2"]. Should thing disappear if both dep1 and dep2 are disabled?

I realize it may cause some confusion in some cases if someone does #[cfg(feature="somedep")] and the dependency is missing. The intended way to deal with that is to instead use the same cfg expression for the target dependency. So, for example, if it was cfg(windows), then use #[cfg(windows)]. Another option is to use cfg(accessible) whenever that is stabilized.

I thought I had included this in the new documentation, but I see I missed it.

[sunshowers]

Ahh, that makes sense in the context of namespaced features. Thanks.

[sunshowers]

(reopening in case you want to close it after documenting)