Refactor feature handling, and improve error messages. #9290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @Eh2406 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
ehuss
commented
Mar 19, 2021
Comment on lines
+165
to
+173
| // This returns `false` for CliFeatures just for simplicity. It | ||
| // would take a bit of work to compare since they are not in the | ||
| // same format as DepFeatures (and that may be expensive | ||
| // performance-wise). Also, it should only occur once for a root | ||
| // package. The only drawback is that it may re-activate a root | ||
| // package again, which should only affect performance, but that | ||
| // should be rare. Cycles should still be detected since those | ||
| // will have `DepFeatures` edges. | ||
| RequestedFeatures::CliFeatures(_) => return Ok(false), |
There was a problem hiding this comment.
I don't know if this is actually correct. I didn't fully understand why the original code returned false when all_features was set.
This function was originally added in 06f37a0 to deal with cycles. It treated Method::Everything as "false". The check for all_features as added in 7de30dd without an explanation.
There was a problem hiding this comment.
I'm unfortunately unable to piece together more context than what you probably already know. I don't really know what this check was doing prior other than exactly what you're doing here which is to generate a little bit more work for the resolver to avoid trying to handle more at this call-site.
I think that's fine, however, and I doubt this will have any noticeable impact on perf or anything like that.
|
I like the additional type safety, and checking for invariants earlier on. I have always wondered why feature parsing code was part of the resolver. Thank you for doing something about that! |
|
@bors: r+ |
|
📌 Commit 85854b1 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 23, 2021
Update cargo 8 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..58a961314437258065e23cb6316dfc121d96fb71 2021-03-16 21:36:55 +0000 to 2021-03-22 22:59:56 +0000 - Emit note when `--future-incompat-report` had nothing to report (rust-lang/cargo#9263) - RFC 3052: Stop including authors field in manifests made by cargo new (rust-lang/cargo#9282) - Refactor feature handling, and improve error messages. (rust-lang/cargo#9290) - Split out cargo-util package for cargo-test-support. (rust-lang/cargo#9292) - Fix redundant_semicolons warning in resolver-tests. (rust-lang/cargo#9293) - Use serde's error message option to avoid implementing `Deserialize`. (rust-lang/cargo#9237) - Allow `cargo update` to operate with the --offline flag (rust-lang/cargo#9279) - Fix typo in faq.md (rust-lang/cargo#9285)
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 23, 2021
Update cargo 8 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..58a961314437258065e23cb6316dfc121d96fb71 2021-03-16 21:36:55 +0000 to 2021-03-22 22:59:56 +0000 - Emit note when `--future-incompat-report` had nothing to report (rust-lang/cargo#9263) - RFC 3052: Stop including authors field in manifests made by cargo new (rust-lang/cargo#9282) - Refactor feature handling, and improve error messages. (rust-lang/cargo#9290) - Split out cargo-util package for cargo-test-support. (rust-lang/cargo#9292) - Fix redundant_semicolons warning in resolver-tests. (rust-lang/cargo#9293) - Use serde's error message option to avoid implementing `Deserialize`. (rust-lang/cargo#9237) - Allow `cargo update` to operate with the --offline flag (rust-lang/cargo#9279) - Fix typo in faq.md (rust-lang/cargo#9285)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 26, 2021
Update cargo 12 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..1e8703890f285befb5e32627ad4e0a0454dde1fb 2021-03-16 21:36:55 +0000 to 2021-03-26 16:59:39 +0000 - tests: Tolerate "exit status" in error messages (rust-lang/cargo#9307) - Default macOS targets to `unpacked` debuginfo (rust-lang/cargo#9298) - Fix publication of packages with metadata and resolver (rust-lang/cargo#9300) - Fix config includes not working. (rust-lang/cargo#9299) - Emit note when `--future-incompat-report` had nothing to report (rust-lang/cargo#9263) - RFC 3052: Stop including authors field in manifests made by cargo new (rust-lang/cargo#9282) - Refactor feature handling, and improve error messages. (rust-lang/cargo#9290) - Split out cargo-util package for cargo-test-support. (rust-lang/cargo#9292) - Fix redundant_semicolons warning in resolver-tests. (rust-lang/cargo#9293) - Use serde's error message option to avoid implementing `Deserialize`. (rust-lang/cargo#9237) - Allow `cargo update` to operate with the --offline flag (rust-lang/cargo#9279) - Fix typo in faq.md (rust-lang/cargo#9285)
bors
added a commit
to rust-lang/rust-semverver
that referenced
this pull request
Jun 29, 2021
Update `cargo` dependency to 0.54 The breaking change was introduced in rust-lang/cargo#9290.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the way feature strings are handled with an eye towards fixing some improper handling and to improve error messages. The key change is to stop treating all features as free-form strings and instead try to handle them as typed values. This helps avoid needing to deal with parsing different feature syntax (like
dep:orfoo/bar) or forgetting to handle it properly.Overview of refactoring changes:
RequestedFeatureschanged to an enum to differentiate between features coming from the command-line, and those that are from a dependency.CompileOptions), and ensures that they are properly handled asFeatureValueinstead of strings.DetailedTomlDependencynow validates things so you can see the location for the errantCargo.toml(previously some validation was deep in the resolver, which provided poor errors).This is a pretty large PR, but at the core it is just changing
RequestedFeaturesand then dealing with the fallout from that. Hopefully this is an improvement overall.List of user-visible changes:
--features bar?/featand--features dep:barbar/featanddep:barin dependency declarations.[features]table can no longer contain slashes.cargo tree -e features --all-features -Z namespaced-featurescargo treeinvolving-Z weak-dep-featuresI did a small amount of benchmarking, and I wasn't able to record much of a difference.