Auto merge of #2510 - dtolnay:plus, r=smarnach

Permit '+' in feature name, as in "c++20"

Up front: I am familiar with the several discussions linked by rust-lang/crates-io-cargo-teams#41 (comment), and the conversation beginning at #1331 (comment). This PR is not motivated by attempting to match whatever behavior Cargo currently has. Instead, it's a small thing I think we can decide now whether to allow. But it's necessary to say no corresponding Cargo change is required to accommodate this crates.io change.

This PR updates feature validation during publish to accept e.g. "c++20" and "dependency/c++20". We continue to not accept "c++20/feature" as the prefix before the slash would normally refer to a crate name of a dependency and a '+' would not be allowed in those.

I am interested in using such feature names in https://github.com/dtolnay/cxx.

In a Cargo.toml plusses appear as:

```toml
[features]
default = ["c++20"]
"c++20" = ["my-dependency/c++20"]
```

To give some slight further justification for why this particular ascii character above other possible characters we might allow: `+` is pretty common in OS package names in a way that no other currently disallowed character is. Some examples pulled arbitrarily from `apt-cache pkgnames | rg '\+'`:

- https://packages.debian.org/buster/dvd+rw-tools
- https://packages.debian.org/buster/elpa-ghub+
- https://packages.debian.org/buster/libarpack++2-dev
- https://packages.debian.org/buster/libdwarf++0
- https://packages.debian.org/buster/libelf++0
- https://packages.debian.org/buster/magics++
- https://packages.debian.org/buster/memtest86+
- https://packages.debian.org/buster/minisat+
- https://packages.debian.org/buster/paw++
- https://packages.debian.org/buster/swish++
- https://packages.debian.org/buster/vera++
- https://packages.debian.org/buster/voro++

The actual names of the projects contain `+`; various ones in the descriptions in the above links are styled as ARPACK++, Memtest86+, Magics++, Paw++, MinSat+, SWISH++, Vera++, Voro++. When binding to something like this behind a feature, using `+` in the feature name is the most intuitive.

Author: bors

src/models/krate.rs

@@ -269,26 +269,31 @@ impl Crate {
                 .unwrap_or(false)
     }
 
+    /// Validates the THIS parts of `features = ["THIS", "and/THIS"]`.
     pub fn valid_feature_name(name: &str) -> bool {
         !name.is_empty()
             && name
                 .chars()
-                .all(|c| c.is_alphanumeric() || c == '_' || c == '-')
-            && name.chars().all(|c| c.is_ascii())
+                .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '+')
     }
 
+    /// Validates the prefix in front of the slash: `features = ["THIS/feature"]`.
+    /// Normally this corresponds to the crate name of a dependency.
+    fn valid_feature_prefix(name: &str) -> bool {
+        !name.is_empty()
+            && name
+                .chars()
+                .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
+    }
+
+    /// Validates a whole feature string, `features = ["THIS", "ALL/THIS"]`.
     pub fn valid_feature(name: &str) -> bool {
         let mut parts = name.split('/');
-        match parts.next() {
-            Some(part) if !Crate::valid_feature_name(part) => return false,
-            None => return false,
-            _ => {}
-        }
-        match parts.next() {
-            Some(part) if !Crate::valid_feature_name(part) => return false,
-            _ => {}
-        }
+        let name_part = parts.next_back(); // required
+        let prefix_part = parts.next_back(); // optional
         parts.next().is_none()
+            && name_part.map_or(false, Crate::valid_feature_name)
+            && prefix_part.map_or(true, Crate::valid_feature_prefix)
     }
 
     pub fn minimal_encodable(

src/tests/krate.rs

@@ -983,6 +983,9 @@ fn valid_feature_names() {
     assert!(!Crate::valid_feature("%/%"));
     assert!(Crate::valid_feature("a/a"));
     assert!(Crate::valid_feature("32-column-tables"));
+    assert!(Crate::valid_feature("c++20"));
+    assert!(Crate::valid_feature("krate/c++20"));
+    assert!(!Crate::valid_feature("c++20/wow"));
 }
 
 #[test]

src/views/krate_publish.rs

@@ -113,7 +113,7 @@ impl<'de> Deserialize<'de> for EncodableFeatureName {
         if !Crate::valid_feature_name(&s) {
             let value = de::Unexpected::Str(&s);
             let expected = "a valid feature name containing only letters, \
-                            numbers, hyphens, or underscores";
+                            numbers, '-', '+', or '_'";
             Err(de::Error::invalid_value(value, &expected))
         } else {
             Ok(EncodableFeatureName(s))