fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces#13664
Merged
bors merged 12 commits intorust-lang:masterfrom Mar 28, 2024
Merged
fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces#13664bors merged 12 commits intorust-lang:masterfrom
bors merged 12 commits intorust-lang:masterfrom
Conversation
Collaborator
This is the only error we do this for and we have the context for what manifest we are parsing. By removing this, it makes it easier to adjust lifetimes in the short term.
weihanglo
approved these changes
Mar 28, 2024
Member
weihanglo
left a comment
There was a problem hiding this comment.
Feel free to address nits or r=weihanglo as-is.
src/cargo/util/toml/mod.rs
Outdated
| .features | ||
| .as_ref() | ||
| .unwrap_or(&empty_features) | ||
| .unwrap_or(&Default::default()) |
Member
There was a problem hiding this comment.
nit
Suggested change
| .unwrap_or(&Default::default()) | |
| .unwrap_or_default() |
Contributor
Author
There was a problem hiding this comment.
There is no impl Default for &T in this case
| if let manifest::TomlDependency::Detailed(ref mut d) = resolved { | ||
| if d.public.is_some() { | ||
| if matches!(dep.kind(), DepKind::Normal) { | ||
| if matches!(kind, None) { |
Member
There was a problem hiding this comment.
nit: matches! against an Option looks pretty not idiomatic to me
Member
There was a problem hiding this comment.
BTW it seems like non of the usage of resolve_and_validate_dependencies receives a DepKind::Normal. I wonder if we could remove the Option.
Contributor
Author
There was a problem hiding this comment.
This is a high churn area in a follow up PR, can we defer this?
Comment on lines
+461
to
+462
| let mut warnings = Default::default(); | ||
| let mut errors = Default::default(); |
Member
There was a problem hiding this comment.
nit: Default::default is used here and in read_manifest we use vec![]. Any reason for the inconsistency?
Contributor
Author
There was a problem hiding this comment.
Not really but this will be going away in the next PR
Contributor
Author
|
@bors r=weihanglo |
Contributor
Contributor
bors
added a commit
that referenced
this pull request
Mar 28, 2024
fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces ### What does this PR try to resolve? This splits out refactors that build on #13589 in preparation for resolving #13456. As part of those refactors, I noticed an inconsistency on when we warn for unused keys. We have parallel code paths between `to_virtual_manifest` and `to_real_manifest` and only one got updated on a change. This syncs them up. Hopefully the end state this builds to will reduce duplication. ### How should we test and review this PR? ### Additional information
Contributor
|
💔 Test failed - checks-actions |
Contributor
Author
|
@bors retry |
Contributor
Author
|
Resolver prop test Details |
Contributor
Contributor
|
☀️ Test successful - checks-actions |
epage
added a commit
to epage/cargo
that referenced
this pull request
Mar 28, 2024
This resolves feedback from rust-lang#13664
bors
added a commit
that referenced
this pull request
Mar 28, 2024
refactor(package): Simplify getting of published Manifest ### What does this PR try to resolve? This is a parallel effort to #13664 in an effort to #13456 This abstracts away the logic for getting the published version of `Cargo.toml` so we can more easily change the APIs that were previously exposed ### How should we test and review this PR? ### Additional information
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 29, 2024
Update cargo 8 commits in 499a61ce7a0fc6a72040084862a68b2603e770e8..a59aba136aab5510c16b0750a36cbd9916f91796 2024-03-26 04:17:04 +0000 to 2024-03-28 21:21:41 +0000 - refactor(package): Simplify getting of published Manifest (rust-lang/cargo#13666) - fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces (rust-lang/cargo#13664) - docs: clarify `--locked` ensures Cargo uses dependency versions in lockfile (rust-lang/cargo#13665) - RUSTC_WORKSPACE_WRAPPER: clarify docs (rust-lang/cargo#13648) - fix(add): Preserve comments when updating simple deps (rust-lang/cargo#13655) - fix(generate-lockfile): hold lock before querying index (rust-lang/cargo#13657) - test: Add asserts to catch BorrowMutError's (rust-lang/cargo#13651) - Publish test crates (rust-lang/cargo#13418) r? ghost
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Mar 30, 2024
Update cargo 8 commits in 499a61ce7a0fc6a72040084862a68b2603e770e8..a59aba136aab5510c16b0750a36cbd9916f91796 2024-03-26 04:17:04 +0000 to 2024-03-28 21:21:41 +0000 - refactor(package): Simplify getting of published Manifest (rust-lang/cargo#13666) - fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces (rust-lang/cargo#13664) - docs: clarify `--locked` ensures Cargo uses dependency versions in lockfile (rust-lang/cargo#13665) - RUSTC_WORKSPACE_WRAPPER: clarify docs (rust-lang/cargo#13648) - fix(add): Preserve comments when updating simple deps (rust-lang/cargo#13655) - fix(generate-lockfile): hold lock before querying index (rust-lang/cargo#13657) - test: Add asserts to catch BorrowMutError's (rust-lang/cargo#13651) - Publish test crates (rust-lang/cargo#13418) r? ghost
bors
added a commit
that referenced
this pull request
Apr 3, 2024
refactor(toml): Split out an explicit step to resolve `Cargo.toml` ### What does this PR try to resolve? This builds on #13664 and #13666. Currently, once we have deserialized `Cargo.toml`, we pass it to a large machinery (`to_real_manifest`, `to_virtual_manifest`) so that - `Cargo.toml` is resolved - `Summary` is created - `Manifest` is created This splits out the resolving of `Cargo.toml` which is mostly workspace inheritance today. While splitting logic conjoined like this can be a bit messy in the short term, the hope is that overall this makes the logic easier to follow (more condensed, focused sections to view; more explicit inputs/outputs). In particular, I hope that this will make it clearer and easier to shift more logic into the resolving step, specifically the inferring of build targets for #13456. ### How should we test and review this PR? This is broken up into very small steps in the hope that it makes it easier to analyze a step. ### Additional information
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
What does this PR try to resolve?
This splits out refactors that build on #13589 in preparation for resolving #13456.
As part of those refactors, I noticed an inconsistency on when we warn for unused keys. We have parallel code paths between
to_virtual_manifestandto_real_manifestand only one got updated on a change. This syncs them up. Hopefully the end state this builds to will reduce duplication.How should we test and review this PR?
Additional information