Cargo silently ignores patch crates with a missing feature.

[bjorn3]

Steps

1. Create a package my_package with a feature my_funky_feature:

   # my_package/Cargo.toml
   [package]
   name = "my_package
   [features]
   my_funky_feature = []

2. Create a copy of my_package without the feature my_funky_feature:

   # my_package_copy/Cargo.toml
   [package]
   name = "my_package
   [features]

3. Create yet another package my_bin, which depends on my_package, activates the feature my_funky_feature of it.

   # my_bin/Cargo.toml
   [package]
   name = "my_bin"
   [dependencies]
   my_package = { git = "github.com/aiofwiewof/vioe", features = ["my_funky_feature"] }

4. Set the copy of my_package which doesn't have my_funky_feature as patch for my_package.

   # my_bin/Cargo.toml
   # [...]
   [patch."github.com/aiofwiewof/vioe"]
   my_package = { path = "../my_package_copy" }

5. Cargo silently uses the original my_package without warning.

Possible Solution(s)
Give a warning when a patch is ignored, because there is a missing feature.

Notes

Output of cargo version:

cargo 1.33.0-nightly (2cf1f5dda 2018-12-11)

[alexcrichton]

Oh dear definitely sounds like a bug! I think we can probably turn this into a hard error

[ehuss]

I was curious about this, but there is a part of the [patch] implementation that I don't understand. The entry gets stuck on the "unused" list. What does "unused" mean, and why is it there? Why would it allow unused patches?

[alexcrichton]

The unused list is serialized into Cargo.lock to ensure that if a [patch] isn't actually used then every time you type cargo build we don't try to re-update the registry (by ensuring we lock it to something). It does make a little sense that this'd go on the unused list, and we could probably warn about that even though it's a possibly valid situation

[ehuss]

This is actually a bit more complicated than I first expected. It depends on whether or not a Cargo.lock already exists, and whether or not there are alternate candidates. If a lock exists before you add the patch, then you get a nice error message:

error: failed to select a version for `bitflags`.
 ... required by package `my_bin v0.1.0 (/Users/eric/Proj/rust/cargo/scratch/my_bin)`
versions that meet the requirements `= 1.0.4` are: 1.0.4

the package `my_bin` depends on `bitflags`, with features: `example_generated` but `bitflags` does not have these features.


failed to select a version for `bitflags` which could resolve this conflict

However, if you don't have a lock file, it actually resolves to a previous version! Here's a sample project:
my_bin.zip

This has a fake bitflags package that is missing a feature. cargo build will display the error above. However, rm Cargo.lock ; cargo build will actually build version 1.0.3 from crates.io, not 1.0.4! If you change the requirement to "^1.0.4", you get the above message.

I'm not familiar with the resolver. From what I can tell, it doesn't really know about [patch], so I don't think it can specifically detect this scenario. All it sees is that there's a candidate with missing features, so it skips over it.

I'd like to propose just adding a warning whenever there are unused entries. I think if the warning message is crafted well enough, users should be able to figure out what's wrong. Sound reasonable?

[bjorn3]

All it sees is that there's a candidate with missing features, so it skips over it.

So if a new version of a crate on crates.io has a missing feature, it is silently ignored, right?

[ehuss]

So if a new version of a crate on crates.io has a missing feature, it is silently ignored, right?

I believe so. The resolver is a bit of a mystery to me.

[alexcrichton]

Oh thanks for digging in! I think this may also be conflating with #4678 which I've never really fully understood as an issue, but I know it's "lock file plus new patch sections equals weirdness".

I actually think it's relatively correct behavior for the resolver to skip this dependency, it definitely has logic to simply filter out candidates that are missing features. I think the no-lockfile scenario is exhibiting this behavior, where it's actually somewhat correct that we're simply skipping the candidate provided by [patch]. For the with-a-lockfile scenario that should have the same behavior as the no-lockfile scenario, but I suspect #4678 is thwarting it.

I think I agree with a warning here though! @ehuss the [patch] section is actually entirely hidden from the resolver (intentionally!) and it only shows up in the PackageRegistry type, which is sort of like semi-resolver but only part of it. I think in any case your conclusion about the difficulty of otherwise providing a targeted warning is correct that it will be difficult to do so.

In terms of issuing warnings, I would personally prefer to only issue a warning when [unused] is first emitted into the lock file. That way you won't get an un-suppressable warning on every single build, but rather just the first one that adds the unused section to the lock file.

[Eh2406]

So if a new version of a crate on crates.io has a missing feature, it is silently ignored, right?

Yes, removing a feature is a breaking change. Before #5000 this would lead to Cargos resolver erroring loudly. But, it was mostly getting triggered by the resolver doing things out of order and trying really old versions of dependencies. So we switched to just ignoring the versions that don't have the required feature. Like how we just ignore versions that don't match the version requirement. But it is not quite "silently", like all filters the resolver uses if we can not find any valid resolution we report an error message explaining what was skipt and why. As @ehuss described you get a good error message if you force the resolver to use the [patch] version with the missing feature but if the resolver can fix it for you (by using an older one on crates.io) then it just fixes it.

@alexcrichton What is the legitimate use of an unused [patch] section? Why is that use important enough to not just have to live with a warning?

[alexcrichton]

AFAIK there's no legitimate use of an unused [patch] other than "I changed something and forgot to delete it", so it's true yeah an unconditional warning is probably ok