Merge #1331

1331: Allow feature names to begin with numbers r=carols10cents

Diesel 1.2 had planned on renaming our `large-tables` and `huge-tables`
features to `32-column-tables` and `64-column-tables` respectively,
while also introducing the `128-column-tables` feature. This change was
made several months ago in Diesel. Cargo will happily accept those
as feature names, and resolve them properly from other crates.

However while publishing Diesel 1.2, I ran into a snag mid-release when
I realized that Cargo is incorrectly assuming that a feature name must
be the same as a crate name. I suspect this is an artifact of the fact
that feature names often are crate names (and perhaps in the past that
was the only form of feature?).

However, now they are an entirely separate thing, and we should allow
the same set of feature names that Cargo does. (As an aside, do we want
to apply the same 64 character limit that we apply to crate names to
feature names?)

Fixes #1329.

src/models/krate.rs

@@ -250,24 +250,29 @@ impl Crate {
     }
 
     fn valid_ident(name: &str) -> bool {
-        if name.is_empty() {
-            return false;
-        }
-        name.chars().next().unwrap().is_alphabetic()
+        Self::valid_feature_name(name)
+            && name.chars()
+                .nth(0)
+                .map(char::is_alphabetic)
+                .unwrap_or(false)
+    }
+
+    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())
     }
 
-    pub fn valid_feature_name(name: &str) -> bool {
+    pub fn valid_feature(name: &str) -> bool {
         let mut parts = name.split('/');
         match parts.next() {
-            Some(part) if !Crate::valid_ident(part) => return false,
+            Some(part) if !Crate::valid_feature_name(part) => return false,
             None => return false,
             _ => {}
         }
         match parts.next() {
-            Some(part) if !Crate::valid_ident(part) => return false,
+            Some(part) if !Crate::valid_feature_name(part) => return false,
             _ => {}
         }
         parts.next().is_none()

src/tests/krate.rs

@@ -753,11 +753,12 @@ fn new_krate_bad_name() {
 
 #[test]
 fn valid_feature_names() {
-    assert!(Crate::valid_feature_name("foo"));
-    assert!(!Crate::valid_feature_name(""));
-    assert!(!Crate::valid_feature_name("/"));
-    assert!(!Crate::valid_feature_name("%/%"));
-    assert!(Crate::valid_feature_name("a/a"));
+    assert!(Crate::valid_feature("foo"));
+    assert!(!Crate::valid_feature(""));
+    assert!(!Crate::valid_feature("/"));
+    assert!(!Crate::valid_feature("%/%"));
+    assert!(Crate::valid_feature("a/a"));
+    assert!(Crate::valid_feature("32-column-tables"));
 }
 
 #[test]

src/views/krate_publish.rs

@@ -18,7 +18,7 @@ pub struct NewCrate {
     pub name: CrateName,
     pub vers: CrateVersion,
     pub deps: Vec<CrateDependency>,
-    pub features: HashMap<CrateName, Vec<Feature>>,
+    pub features: HashMap<FeatureName, Vec<Feature>>,
     pub authors: Vec<String>,
     pub description: Option<String>,
     pub homepage: Option<String>,
@@ -51,6 +51,8 @@ pub struct CategoryList(pub Vec<Category>);
 pub struct Category(pub String);
 #[derive(Serialize, Debug, Deref)]
 pub struct Feature(pub String);
+#[derive(PartialEq, Eq, Hash, Serialize, Debug, Deref)]
+pub struct FeatureName(pub String);
 
 #[derive(Serialize, Deserialize, Debug)]
 pub struct CrateDependency {
@@ -102,6 +104,20 @@ impl<'de> Deserialize<'de> for Keyword {
     }
 }
 
+impl<'de> Deserialize<'de> for FeatureName {
+    fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
+        let s = String::deserialize(d)?;
+        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";
+            Err(de::Error::invalid_value(value, &expected))
+        } else {
+            Ok(FeatureName(s))
+        }
+    }
+}
+
 impl<'de> Deserialize<'de> for Feature {
     fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Feature, D::Error> {
         let s = String::deserialize(d)?;