Use the same feature and dep name validation rules from Cargo #7379

Rustin170506 · 2023-10-29T07:08:52Z

This PR allowed . in the feature name. I added some unit tests and integration tests for it.
This PR also updated the feature, crate, and dep name validation rules from Cargo.

The summary:

category	Cargo	crates.io before change	crates.io after change
Crate name	1. cannot start with a number 2. can only start with most letters or `_` 3. can only contain numbers, `-`, `_`, or most letters.	1. must start with alphabetic 2. can only contain numbers, letters, `-`, `_`	1. must start with alphabetic 2. can only contain numbers, letters, `-`, `_` (Nothing changes)
Dep name	1. cannot start with a number 2. can only start with most letters or `_` 3. can only contain numbers, `-`, `_`, or most letters.	1. first character must be alphabetic or `_` 2. can only contain numbers, letters, `-`, `_`	1. cannot start with a number 2. can only start with ASCII letters or `_` 3. can only contain numbers, `-`, `_`, or ASCII letters. (Nothing changes)
Normal feature name	1. can only start with most letters, `_`, or `0` to `9` 2. can only contain numbers, `+`, `-`, `_`, `.`, or most letters	can only contain numbers, `+`, `-`, `_`, or most letters	1. can only start with most letters, `_`, or `0` to `9` 2. can only contain numbers, `+`, `-`, `_`, `.`, or most letters
Optional dep name	same as dep name	can only contain numbers, `+`, `-`, `_`, or most letters	same as dep name
Optional dep feature name	dep_name/feat_name 1. so the first part is the same as the dep name 2. the second part is the same as the normal feature name.	can only contain numbers, `+`, `-`, `_`, or most letters	dep_name/feat_name 1. so the first part is the same as the dep name 2. the second part is the same as the normal feature name.

Rustin170506 · 2023-10-29T07:16:00Z

I also tested it locally. It worked well:

[package]
name = "hello-bisect"
version = "0.2.0"
edition = "2021"
license = "MIT"
description = "A simple hello world program for testing bisect"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

[features]
"foo.bar" = []

❯ cargo publish  --index http://localhost:8888/git/index --token xxxx 
    Updating `http://localhost:8888/git/index` index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging hello-bisect v0.2.0 (/Volumes/t7/code/hello-bisect)
   Verifying hello-bisect v0.2.0 (/Volumes/t7/code/hello-bisect)
   Compiling hello-bisect v0.2.0 (/Volumes/t7/code/hello-bisect/target/package/hello-bisect-0.2.0)
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s
    Packaged 5 files, 1.6KiB (1.0KiB compressed)
   Uploading hello-bisect v0.2.0 (/Volumes/t7/code/hello-bisect)
    Uploaded hello-bisect v0.2.0 to registry `http://localhost:8888/git/index`
note: Waiting for `hello-bisect v0.2.0` to be available at registry `http://localhost:8888/git/index`.
You may press ctrl-c to skip waiting; the crate should be available shortly.
warning: timed out waiting for `hello-bisect v0.2.0` to be available in registry `http://localhost:8888/git/index`
note: The registry may have a backlog that is delaying making the crate available. The crate should be available soon.

Turbo87 · 2023-10-29T07:43:44Z

Allowing . in general seems useful to me. 👍

I'm wondering though whether we should migrate to the ruleset that cargo is using (see https://doc.rust-lang.org/cargo/reference/features.html#the-features-section):

Feature names may include characters from the Unicode XID standard (which includes most letters), and additionally allows starting with _ or digits 0 through 9, and after the first character may also contain -, +, or ..

The original issue is complaining about the server having different rules than cargo, and while allowing . characters would solve this specific case, it might be worth addressing the underlying issue of cargo and crates.io using different validation schemes.

Rustin170506 · 2023-10-29T15:09:16Z

I'm wondering though whether we should migrate to the ruleset that cargo is using (see doc.rust-lang.org/cargo/reference/features.html#the-features-section):

Sounds make sense. I will fix it tomorrow.

Turbo87 · 2023-10-29T16:53:16Z

I will fix it tomorrow.

Might be worth getting opinions from the rest of the team first. I'm not sure if there is a reason why crates.io is currently more restrictive than cargo, but some of the other team members might know.

jtgeibel · 2023-10-29T18:45:11Z

I'm not aware of any intentional restrictions put in place here. Looking through the history outlined here, it looks like this originally shared an implementation with the crate name validation logic and that we've just incrementally allowed more characters as they were requested.

As I understand it, we're only easing the constraints on the feature_name portion of a feature string, and not the crate_name portion in the following examples:

dep:crate_name
crate_name/feature_name
crate_name?/feature_name
feature_name

In that case I don't see any issues with lifting these constraints on feature names to align with cargo.

I am a bit confused by some of our existing logic though:

On publish, the crate name must be a valid_feature_prefix and the first character must be alphabetic (here).
When checking the dependencies during a publish, we enforce that each dependency must be a valid_feature_prefix and the first character must be alphabetic or _. (here). It seems like we allow the _ prefix to handle dependency renaming, though it isn't clear to me that we only enforce it in that scenario. (I also have no idea what constraints cargo puts on renamed dependencies.)
Then when validating the feature prefix (what I call crate_name in the examples above) we seem to only validate that all characters |c| c.is_ascii_alphanumeric() || c == '_' || c == '-' (here).

Not that it would need to be done in this PR, but it seems like maybe we should be more strict when validating the crate_name portion, as currently the following are allowed when they seem invalid to me:

        assert!(Crate::valid_feature("0"));
        assert!(Crate::valid_feature("-"));
        assert!(Crate::valid_feature("0/bar"));
        assert!(Crate::valid_feature("-/bar"));

It's also not clear to me if we should allow the following always, or possibly verify that a dependency has been reanamed to _:

        assert!(Crate::valid_feature("_"));
        assert!(Crate::valid_feature("_/bar"));

Rustin170506 · 2023-10-30T02:03:26Z

From the Cargo side:
For crate name: https://github.com/rust-lang/cargo/blob/b1a3ad24c4088d125fd478670a2e171c3951cfa6/src/cargo/util/restricted_names.rs#L45

Package name can not start with a number.
Package name can only start with most letters or _.
Package name can only contain numbers, -, _, or most letters.

For future name: https://github.com/rust-lang/cargo/blob/b1a3ad24c4088d125fd478670a2e171c3951cfa6/src/cargo/core/summary.rs#L433

Feature name can only start with most letters or _ or 0 to 9.
Feature name can only contain numbers, +, -, _, ., or most letters.

Rustin170506 · 2023-11-03T15:48:35Z

We discussed it in tonight's meeting:

We can try to use the same rules from Cargo.
We will support UnicodeXID for feature names.
We won't support UnicodeXID for crate names.(considering the security issues, we want to keep it only ASCII letters now)

Signed-off-by: hi-rustin <[email protected]>

AppResult. Signed-off-by: hi-rustin <[email protected]>

Signed-off-by: hi-rustin <[email protected]>

Turbo87 · 2023-11-09T16:44:33Z

src/models/krate.rs

+    pub fn valid_name(name: &str) -> AppResult<()> {
+        if name.chars().count() > MAX_NAME_LENGTH {
+            return Err(cargo_err(&format!(
+                "the name `{}` is too long (max {} characters)",
+                name, MAX_NAME_LENGTH
+            )));
+        }
+        Crate::valid_ident(name, "crate name")
    }

-    fn valid_ident(name: &str) -> bool {
-        Self::valid_feature_prefix(name)
-            && name
-                .chars()
-                .next()
-                .map(char::is_alphabetic)
-                .unwrap_or(false)
+    pub fn valid_dependency_name(name: &str) -> AppResult<()> {
+        if name.chars().count() > MAX_NAME_LENGTH {
+            return Err(cargo_err(&format!(
+                "the name `{}` is too long (max {} characters)",
+                name, MAX_NAME_LENGTH
+            )));
+        }
+        Crate::valid_ident(name, "dependency name")
    }

-    pub fn valid_dependency_name(name: &str) -> bool {
-        let under_max_length = name.chars().take(MAX_NAME_LENGTH + 1).count() <= MAX_NAME_LENGTH;
-        Crate::valid_dependency_ident(name) && under_max_length
-    }
+    // Checks that the name is a valid identifier.
+    // 1. The name must be non-empty.
+    // 2. The first character must be an ASCII character or `_`.
+    // 3. The remaining characters must be ASCII alphanumerics or `-` or `_`.


I'm not sure I understand why you changed all of these implementations too? I thought the goal of this PR was to adjust the validation rules for feature names, but not for crate names 🤔

I remember the last meeting, we decided not to allow unicode for crate names, but we can follow the same rules for the validations.

But if you think we should only change the feature name first. I can revert my changes for crate names.

If we don't change the create name validation as well, _a?/feature_name still is invalid in crates.io but it works in Cargo.

I think changing the rules on crate names should be out of scope for this PR. That would need a bit more investigation since these are relevant for URLs, S3 paths, and other things where allowing additional characters could have unintended consequences. Let's focus this PR on just the feature names.

Make sense. I will revert it. Thanks!

I reverted it in f47425f

I retained the modification of the message as it provides a clearer error message to users.

I retained the modification of the message as it provides a clearer error message to users.

the problem is that this makes the diff a lot harder to review because the PR is doing multiple things at once. (see https://mtlynch.io/code-review-love/#5-narrowly-scope-changes 😉)

the problem is that this makes the diff a lot harder to review because the PR is doing multiple things at once.

Sorry for the big PR. I tried to make sure one commit only had one main change. Could you please review it by commit? The reason it has a really big change is the old validations for create name and dep name also use some feature validation rules(functions). We mixed them.

If you think it is still really difficult to review. I can split it into three different PRs. Sorry again for the confusing.

The first one: #7500

Signed-off-by: hi-rustin <[email protected]>

Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Oct 29, 2023

This comment was marked as outdated.

Sign in to view

This was referenced Nov 6, 2023

Expand manifest validation for all settings rust-lang/cargo#4377

Open

Do not allow empty feature name rust-lang/cargo#12928

Merged

Rustin170506 added 3 commits November 7, 2023 20:31

Allow . in feature name

ddeedb9

Signed-off-by: hi-rustin <[email protected]>

Add an integration test for feature name with dot

3eb8107

Signed-off-by: hi-rustin <[email protected]>

Add unicode-xid crate to dependencies

72356e4

Signed-off-by: hi-rustin <[email protected]>

Rustin170506 force-pushed the rustin-patch-features branch 3 times, most recently from 9cda4d0 to 318e165 Compare November 7, 2023 12:48

Rustin170506 marked this pull request as draft November 7, 2023 13:33

This comment was marked as outdated.

Sign in to view

Rustin170506 force-pushed the rustin-patch-features branch 3 times, most recently from 7d91969 to 8753f8a Compare November 8, 2023 12:44

Rustin170506 added 4 commits November 8, 2023 20:51

Use the same feature name validation rule from Cargo

22a9589

Signed-off-by: hi-rustin <[email protected]>

Use the same crate name validation rule from Cargo

0b5f262

Signed-off-by: hi-rustin <[email protected]>

Refactor valid_feature function to return

028a235

AppResult. Signed-off-by: hi-rustin <[email protected]>

Fix the validation for optional dep

b23fe07

Signed-off-by: hi-rustin <[email protected]>

Rustin170506 force-pushed the rustin-patch-features branch from 8753f8a to b23fe07 Compare November 8, 2023 12:52

Rustin170506 marked this pull request as ready for review November 8, 2023 12:53

Rustin170506 changed the title ~~Allow . in the feature name~~ Use the same feature/crate/dep name validation rules from Cargo Nov 8, 2023

Turbo87 reviewed Nov 9, 2023

View reviewed changes

Do not allow create name starts with _

f47425f

Signed-off-by: hi-rustin <[email protected]>

Rustin170506 force-pushed the rustin-patch-features branch from c495ff4 to f47425f Compare November 10, 2023 13:14

Rustin170506 changed the title ~~Use the same feature/crate/dep name validation rules from Cargo~~ Use the same feature and dep name validation rules from Cargo Nov 10, 2023

Rustin170506 mentioned this pull request Nov 10, 2023

Use the same feature name validation rule from Cargo #7500

Merged

Rustin170506 closed this Nov 14, 2023

Rustin170506 mentioned this pull request Nov 24, 2023

Add better error message for invalid crate names #7603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same feature and dep name validation rules from Cargo #7379

Use the same feature and dep name validation rules from Cargo #7379

Rustin170506 commented Oct 29, 2023 •

edited

Loading

Rustin170506 commented Oct 29, 2023

Turbo87 commented Oct 29, 2023

Rustin170506 commented Oct 29, 2023

Turbo87 commented Oct 29, 2023

jtgeibel commented Oct 29, 2023

Rustin170506 commented Oct 30, 2023

This comment was marked as outdated.

Rustin170506 commented Nov 3, 2023

This comment was marked as outdated.

Turbo87 Nov 9, 2023

Rustin170506 Nov 10, 2023 •

edited

Loading

Rustin170506 Nov 10, 2023 •

edited

Loading

Turbo87 Nov 10, 2023

Rustin170506 Nov 10, 2023

Rustin170506 Nov 10, 2023 •

edited

Loading

Turbo87 Nov 10, 2023

Rustin170506 Nov 10, 2023

Rustin170506 Nov 10, 2023

Rustin170506 Nov 10, 2023

Use the same feature and dep name validation rules from Cargo #7379

Use the same feature and dep name validation rules from Cargo #7379

Conversation

Rustin170506 commented Oct 29, 2023 • edited Loading

Rustin170506 commented Oct 29, 2023

Turbo87 commented Oct 29, 2023

Rustin170506 commented Oct 29, 2023

Turbo87 commented Oct 29, 2023

jtgeibel commented Oct 29, 2023

Rustin170506 commented Oct 30, 2023

This comment was marked as outdated.

Rustin170506 commented Nov 3, 2023

This comment was marked as outdated.

Turbo87 Nov 9, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Turbo87 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Turbo87 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 10, 2023

Choose a reason for hiding this comment

Rustin170506 commented Oct 29, 2023 •

edited

Loading

Rustin170506 Nov 10, 2023 •

edited

Loading

Rustin170506 Nov 10, 2023 •

edited

Loading

Rustin170506 Nov 10, 2023 •

edited

Loading