Improve errors for TOML fields that support workspace inheritance #11113

rdimartino · 2022-09-20T03:59:07Z

This also addresses the issue with MaybeWorkspace<VecStringOrBool> mentioned in #10942:

Caused by:
  invalid type: string "foo", expected a boolean or vector of strings for key `package.publish`

I removed the maybe_workspace_vec_string deserializer in 7a50c0c because the error message from the inner Vec<String> is now surfaced, but I can revert that if it's preferable to keep those changes.

I tried to base the deserialize implementation off of the derived impl for an untagged enum from cargo expand. This approach ultimately led me to adding the serde-value dependency.

rust-highfive · 2022-09-20T03:59:10Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information.

weihanglo

Thank you for this pull request.

It seems that the test case cargo_metadata_with_invalid_version_field is also valid even without this patch. Does that mean the change is not relevant anymore? If in this case, I suggest leave the test here in PR to prevent regression and remove other parts.

rdimartino · 2022-09-20T13:37:22Z

@weihanglo Ah, you're right! Sorry about that. I overlooked that version has deserialize_with = "version_trim_whitespace" so that test that I added didn't actually test my patch. I've added a test for the field publish: Option<MaybeWorkspace<VecStringOrBool>> which shows the improvement.

Before:

$ cargo metadata --verbose --format-version 1 --all-features --manifest-path /tmp/test/Cargo.toml
error: failed to parse manifest at `/private/tmp/test/Cargo.toml`

Caused by:
  data did not match any variant of untagged enum MaybeWorkspace for key `package.publish`

After:

$ ./target/debug/cargo metadata --verbose --format-version 1 --all-features --manifest-path /tmp/test/Cargo.toml
error: failed to parse manifest at `/tmp/test/Cargo.toml`

Caused by:
  invalid type: string "foo", expected a boolean or vector of strings for key `package.publish`

Cargo.toml

epage · 2022-09-22T20:10:27Z

@bors r+

Looks like @weihanglo 's test concern was resolved so this seems good to go. Thanks for doing this!

bors · 2022-09-22T20:10:29Z

📌 Commit 7b16e59 has been approved by epage

It is now in the queue for this repository.

bors · 2022-09-22T20:10:35Z

⌛ Testing commit 7b16e59 with merge 9230e48...

bors · 2022-09-22T20:51:57Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 9230e48 to master...

22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000 - build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150) - Indicate how Cargo locates the manifest (rust-lang/cargo#10770) - Reduce references to `[project]` within cargo (rust-lang/cargo#11135) - Iteratively construct target cfg (rust-lang/cargo#11114) - update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146) - Call out that not all config values can be set via env vars (rust-lang/cargo#11139) - Bump to 0.67.0, update changelog (rust-lang/cargo#11137) - ci: update toolchain for building api doc (rust-lang/cargo#11134) - Http publish not noop (rust-lang/cargo#11111) - Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113) - switch to `std::task::ready!()` where possible (rust-lang/cargo#11130) - Report cmd aliasing failure with more contexts (rust-lang/cargo#11087) - minor: remove unused mut (rust-lang/cargo#11127) - fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118) - This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938) - Add support for relative git submodule paths (rust-lang/cargo#11106) - make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098) - Unlink old final artifacts before compilation (rust-lang/cargo#11122) - refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116) - fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119) - Add a minor clarification (rust-lang/cargo#11093) - doc(changelog): mention CVE fixes (rust-lang/cargo#11104)

Update cargo 22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000 - build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150) - Indicate how Cargo locates the manifest (rust-lang/cargo#10770) - Reduce references to `[project]` within cargo (rust-lang/cargo#11135) - Iteratively construct target cfg (rust-lang/cargo#11114) - update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146) - Call out that not all config values can be set via env vars (rust-lang/cargo#11139) - Bump to 0.67.0, update changelog (rust-lang/cargo#11137) - ci: update toolchain for building api doc (rust-lang/cargo#11134) - Http publish not noop (rust-lang/cargo#11111) - Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113) - switch to `std::task::ready!()` where possible (rust-lang/cargo#11130) - Report cmd aliasing failure with more contexts (rust-lang/cargo#11087) - minor: remove unused mut (rust-lang/cargo#11127) - fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118) - This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938) - Add support for relative git submodule paths (rust-lang/cargo#11106) - make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098) - Unlink old final artifacts before compilation (rust-lang/cargo#11122) - refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116) - fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119) - Add a minor clarification (rust-lang/cargo#11093) - doc(changelog): mention CVE fixes (rust-lang/cargo#11104)

rdimartino added 2 commits September 19, 2022 23:41

Improve errors for TOML fields that support workspace inheritance

06c31de

Remove maybe_workspace_vec_string specific deserializer

7a50c0c

rust-highfive assigned epage Sep 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2022

weihanglo reviewed Sep 20, 2022

View reviewed changes

Add metadata test for invalid publish field

7b16e59

Muscraft reviewed Sep 20, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2022

bors merged commit 9230e48 into rust-lang:master Sep 22, 2022

This was referenced Sep 22, 2022

package.edition gives confusing error when integer passed instead of string #11132

Closed

Update cargo rust-lang/rust#102365

Merged

ehuss added this to the 1.66.0 milestone Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve errors for TOML fields that support workspace inheritance #11113

Improve errors for TOML fields that support workspace inheritance #11113

rdimartino commented Sep 20, 2022

rust-highfive commented Sep 20, 2022

weihanglo left a comment

rdimartino commented Sep 20, 2022

epage commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 22, 2022

Improve errors for TOML fields that support workspace inheritance #11113

Improve errors for TOML fields that support workspace inheritance #11113

Conversation

rdimartino commented Sep 20, 2022

rust-highfive commented Sep 20, 2022

weihanglo left a comment

Choose a reason for hiding this comment

rdimartino commented Sep 20, 2022

epage commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 22, 2022