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?