refactor: Pull name validation into `util_schemas` #13166

epage · 2023-12-13T20:16:24Z

What does this PR try to resolve?

In preparation for #12801, this moves the last dependency on the rest of the cargo crate into the future util_schemas crate. It does this by refocusing the code from being validation functions to being newtypes. I did not try to thread the newtypes everywhere, that can be an exercise for the future. There are places I didn't put newtypes because it didn't seem worth it (e.g. places needing StringOrVec, ProfileName not being used in CLI because of the check psuedo-profile, etc)

The main risk with this is when validation changes according to a nightly feature, like packages-as-namespaces. My thought is that the validation code would be updated for the nightly behavior and then the caller would check if it isn't nightly and fail in that case.

This has a side effect of improving the error messages for manifest parsing because we will show more context

How should we test and review this PR?

#13164 should be reviewed first

Additional information

This moves feature name validation early enough in the process to get TOML errors.

I was going to have clap use `ProfileName` but the `cargo rustc --profile` (yes that specific) accepts `check` as a profile name and we convert that to `dev` later in the process, making that not work.

Because of workspace inheritance, the errors aren't the greatest

rustbot · 2023-12-13T20:16:29Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

weihanglo

This has a side effect of improving the error messages for manifest parsing because we will show more context

I love this! Definitely a live saver 👍🏾.

src/cargo/core/summary.rs

tests/testsuite/cargo_add/empty_dep_name/stderr.log

tests/testsuite/cargo_new/empty_name/stderr.log

src/cargo/util_schemas/manifest.rs

weihanglo

Thank you. Those error message regressions are pretty minor and shouldn't block this from merge.

weihanglo · 2023-12-15T17:52:10Z

@bors r+

bors · 2023-12-15T17:52:13Z

📌 Commit cfa9421 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-12-15T17:53:29Z

⌛ Testing commit cfa9421 with merge 594e2be...

bors · 2023-12-15T18:32:28Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 594e2be to master...

bors · 2023-12-15T18:32:28Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 594e2be to master...

bors · 2023-12-15T18:33:28Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Update cargo 10 commits in 1aa9df1a5be205cce621f0bc0ea6062a5e22a98c..37bc5f0232a0bb72dedd2c14149614fd8cdae649 2023-12-12 14:52:31 +0000 to 2023-12-15 18:33:31 +0000 - docs(home): prepare the changelog for 0.5.9 (rust-lang/cargo#13177) - refactor: Pull name validation into `util_schemas` (rust-lang/cargo#13166) - chore(deps): bump zerocopy from 0.7.29 to 0.7.31 (rust-lang/cargo#13174) - Replace SHGetFolderPathW with SHGetKnownFolderPath (rust-lang/cargo#13173) - chore(bump-check): dont check `home` against beta/stable branches (rust-lang/cargo#13167) - fix: Fill in more empty name holes (rust-lang/cargo#13164) - Do not allow empty name in package ID spec (rust-lang/cargo#13152) - chore(deps): update rust crate openssl to 0.10.61 (rust-lang/cargo#13159) - `all-static` feature should include `vendored-libgit2` (rust-lang/cargo#13134) - doc/registry-web-api: Adjust success response code documentation (rust-lang/cargo#13160) r? ghost

Update cargo 11 commits in 1aa9df1a5be205cce621f0bc0ea6062a5e22a98c..1a2666ddd14cf0a255d4ddb61c63531c259a7b39 2023-12-12 14:52:31 +0000 to 2023-12-17 17:53:53 +0000 - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13179) - docs(home): prepare the changelog for 0.5.9 (rust-lang/cargo#13177) - refactor: Pull name validation into `util_schemas` (rust-lang/cargo#13166) - chore(deps): bump zerocopy from 0.7.29 to 0.7.31 (rust-lang/cargo#13174) - Replace SHGetFolderPathW with SHGetKnownFolderPath (rust-lang/cargo#13173) - chore(bump-check): dont check `home` against beta/stable branches (rust-lang/cargo#13167) - fix: Fill in more empty name holes (rust-lang/cargo#13164) - Do not allow empty name in package ID spec (rust-lang/cargo#13152) - chore(deps): update rust crate openssl to 0.10.61 (rust-lang/cargo#13159) - `all-static` feature should include `vendored-libgit2` (rust-lang/cargo#13134) - doc/registry-web-api: Adjust success response code documentation (rust-lang/cargo#13160) r? ghost

epage added 15 commits December 13, 2023 10:53

refactor: Consolidate feature name validation logic

b0ce24a

refactor: Consolidate name validation logicc

f0bc0d2

refactor: Use inline formatting

b2afdcc

refactor: Allow more/less feature name locations

a8a376c

fix: Improve feature name errors

c1d9b76

This moves feature name validation early enough in the process to get TOML errors.

refactor: Remove unused feature location parameter

4e1fac8

refactor: Do all feature name validation through FeatureName

586b010

fix: Improve profile name errors

a3976cd

refactor(cli): Validate via ProfileName

847804d

I was going to have clap use `ProfileName` but the `cargo rustc --profile` (yes that specific) accepts `check` as a profile name and we convert that to `dev` later in the process, making that not work.

refactor(schema): Reduce str newtype duplication

00bde96

fix: Improve package name errors

6b6eb06

fix: Improve registry name errors?

db2e314

Because of workspace inheritance, the errors aren't the greatest

refactor: Validate via RegistryName/PackageName

1da3053

refactor: Sanitize via PackageName

8e68a0a

refactor: Move validation into util_schemas

d9a6043

rustbot assigned ehuss Dec 13, 2023

rustbot added A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues Command-add Command-new S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2023

epage mentioned this pull request Dec 13, 2023

Official API for parsing Cargo.tomls schema #12801

Closed

20 tasks

epage force-pushed the validate branch from a18c724 to 3f011a7 Compare December 13, 2023 21:12

epage changed the title ~~refactor: Pull validation into util_schemas~~ refactor: Pull name validation into util_schemas Dec 14, 2023

weihanglo reviewed Dec 15, 2023

View reviewed changes

epage added 3 commits December 15, 2023 11:27

test(schema): Unit test for more feature name cases

b9524d4

refactor(schema): Remove redundant path

acef3c0

docs(schema): Document newtypes

cfa9421

epage force-pushed the validate branch 3 times, most recently from f93e64a to cfa9421 Compare December 15, 2023 17:47

weihanglo approved these changes Dec 15, 2023

View reviewed changes

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 Dec 15, 2023

bors merged commit 594e2be into rust-lang:master Dec 15, 2023
20 checks passed

bors mentioned this pull request Dec 15, 2023

feat: Add rustc style errors for manifest parsing #13172

Merged

epage deleted the validate branch December 15, 2023 18:46

weihanglo mentioned this pull request Dec 16, 2023

Update cargo rust-lang/rust#119007

Merged

rustbot added this to the 1.76.0 milestone Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Pull name validation into `util_schemas` #13166

refactor: Pull name validation into `util_schemas` #13166

epage commented Dec 13, 2023

rustbot commented Dec 13, 2023

weihanglo left a comment

weihanglo left a comment

weihanglo commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

refactor: Pull name validation into util_schemas #13166

refactor: Pull name validation into util_schemas #13166

Conversation

epage commented Dec 13, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 13, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

refactor: Pull name validation into `util_schemas` #13166

refactor: Pull name validation into `util_schemas` #13166