Rollup merge of #60373 - rasendubi:lang-features-sort-since, r=Centril

Tidy: ensure lang features are sorted by since

This is the tidy side of https://github.com/rust-lang/rust/issues/60361.

What is left is actually splitting features into groups and sorting by since.

This PR also likely to produce a small (a couple of lines) merge conflict with https://github.com/rust-lang/rust/pull/60362.

r? @Centril

4 files changed, 192 insertions, 30 deletions

diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml
index f7b491823f8..f5db2487618 100644
--- a/src/tools/tidy/Cargo.toml
+++ b/src/tools/tidy/Cargo.toml
@@ -4,6 +4,7 @@ version = "0.1.0"
 authors = ["Alex Crichton <alex@alexcrichton.com>"]
 
 [dependencies]
+regex = "1"
 serde = "1.0.8"
 serde_derive = "1.0.8"
 serde_json = "1.0.2"
diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs
index 8239fd9dce0..3144df6dd4c 100644
--- a/src/tools/tidy/src/features.rs
+++ b/src/tools/tidy/src/features.rs
@@ -7,6 +7,7 @@
 //! * Library features have at most one stability level.
 //! * Library features have at most one `since` value.
 //! * All unstable lang features have tests to ensure they are actually unstable.
+//! * Language features in a group are sorted by `since` value.
 
 use std::collections::HashMap;
 use std::fmt;
@@ -14,6 +15,14 @@ use std::fs::{self, File};
 use std::io::prelude::*;
 use std::path::Path;
 
+use regex::{Regex, escape};
+
+mod version;
+use self::version::Version;
+
+const FEATURE_GROUP_START_PREFIX: &str = "// feature-group-start";
+const FEATURE_GROUP_END_PREFIX: &str = "// feature-group-end";
+
 #[derive(Debug, PartialEq, Clone)]
 pub enum Status {
     Stable,
@@ -35,7 +44,7 @@ impl fmt::Display for Status {
 #[derive(Debug, Clone)]
 pub struct Feature {
     pub level: Status,
-    pub since: String,
+    pub since: Option<Version>,
     pub has_gate_test: bool,
     pub tracking_issue: Option<u32>,
 }
@@ -129,20 +138,8 @@ pub fn check(path: &Path, bad: &mut bool, quiet: bool) {
     }
 
     let mut lines = Vec::new();
-    for (name, feature) in features.iter() {
-        lines.push(format!("{:<32} {:<8} {:<12} {:<8}",
-                           name,
-                           "lang",
-                           feature.level,
-                           feature.since));
-    }
-    for (name, feature) in lib_features {
-        lines.push(format!("{:<32} {:<8} {:<12} {:<8}",
-                           name,
-                           "lib",
-                           feature.level,
-                           feature.since));
-    }
+    lines.extend(format_features(&features, "lang"));
+    lines.extend(format_features(&lib_features, "lib"));
 
     lines.sort();
     for line in lines {
@@ -150,11 +147,31 @@ pub fn check(path: &Path, bad: &mut bool, quiet: bool) {
     }
 }
 
+fn format_features<'a>(features: &'a Features, family: &'a str) -> impl Iterator<Item = String> + 'a {
+    features.iter().map(move |(name, feature)| {
+        format!("{:<32} {:<8} {:<12} {:<8}",
+                name,
+                family,
+                feature.level,
+                feature.since.map_or("None".to_owned(),
+                                     |since| since.to_string()))
+    })
+}
+
 fn find_attr_val<'a>(line: &'a str, attr: &str) -> Option<&'a str> {
-    line.find(attr)
-        .and_then(|i| line[i..].find('"').map(|j| i + j + 1))
-        .and_then(|i| line[i..].find('"').map(|j| (i, i + j)))
-        .map(|(i, j)| &line[i..j])
+    let r = Regex::new(&format!(r#"{}\s*=\s*"([^"]*)""#, escape(attr)))
+        .expect("malformed regex for find_attr_val");
+    r.captures(line)
+        .and_then(|c| c.get(1))
+        .map(|m| m.as_str())
+}
+
+#[test]
+fn test_find_attr_val() {
+    let s = r#"#[unstable(feature = "checked_duration_since", issue = "58402")]"#;
+    assert_eq!(find_attr_val(s, "feature"), Some("checked_duration_since"));
+    assert_eq!(find_attr_val(s, "issue"), Some("58402"));
+    assert_eq!(find_attr_val(s, "since"), None);
 }
 
 fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool {
@@ -177,6 +194,9 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features {
     // without one inside `// no tracking issue START` and `// no tracking issue END`.
     let mut next_feature_omits_tracking_issue = false;
 
+    let mut in_feature_group = false;
+    let mut prev_since = None;
+
     contents.lines().zip(1..)
         .filter_map(|(line, line_number)| {
             let line = line.trim();
@@ -194,6 +214,25 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features {
                 _ => {}
             }
 
+            if line.starts_with(FEATURE_GROUP_START_PREFIX) {
+                if in_feature_group {
+                    tidy_error!(
+                        bad,
+                        // ignore-tidy-linelength
+                        "libsyntax/feature_gate.rs:{}: new feature group is started without ending the previous one",
+                        line_number,
+                    );
+                }
+
+                in_feature_group = true;
+                prev_since = None;
+                return None;
+            } else if line.starts_with(FEATURE_GROUP_END_PREFIX) {
+                in_feature_group = false;
+                prev_since = None;
+                return None;
+            }
+
             let mut parts = line.split(',');
             let level = match parts.next().map(|l| l.trim().trim_start_matches('(')) {
                 Some("active") => Status::Unstable,
@@ -202,7 +241,33 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features {
                 _ => return None,
             };
             let name = parts.next().unwrap().trim();
-            let since = parts.next().unwrap().trim().trim_matches('"');
+
+            let since_str = parts.next().unwrap().trim().trim_matches('"');
+            let since = match since_str.parse() {
+                Ok(since) => Some(since),
+                Err(err) => {
+                    tidy_error!(
+                        bad,
+                        "libsyntax/feature_gate.rs:{}: failed to parse since: {} ({:?})",
+                        line_number,
+                        since_str,
+                        err,
+                    );
+                    None
+                }
+            };
+            if in_feature_group {
+                if prev_since > since {
+                    tidy_error!(
+                        bad,
+                        "libsyntax/feature_gate.rs:{}: feature {} is not sorted by since",
+                        line_number,
+                        name,
+                    );
+                }
+                prev_since = since;
+            }
+
             let issue_str = parts.next().unwrap().trim();
             let tracking_issue = if issue_str.starts_with("None") {
                 if level == Status::Unstable && !next_feature_omits_tracking_issue {
@@ -222,7 +287,7 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features {
             Some((name.to_owned(),
                 Feature {
                     level,
-                    since: since.to_owned(),
+                    since,
                     has_gate_test: false,
                     tracking_issue,
                 }))
@@ -239,7 +304,7 @@ pub fn collect_lib_features(base_src_path: &Path) -> Features {
     // add it to the set of known library features so we can still generate docs.
     lib_features.insert("compiler_builtins_lib".to_owned(), Feature {
         level: Status::Unstable,
-        since: String::new(),
+        since: None,
         has_gate_test: false,
         tracking_issue: None,
     });
@@ -336,11 +401,11 @@ fn map_lib_features(base_src_path: &Path,
                 // `const fn` features are handled specially.
                 let feature_name = match find_attr_val(line, "feature") {
                     Some(name) => name,
-                    None => err!("malformed stability attribute"),
+                    None => err!("malformed stability attribute: missing `feature` key"),
                 };
                 let feature = Feature {
                     level: Status::Unstable,
-                    since: "None".to_owned(),
+                    since: None,
                     has_gate_test: false,
                     // FIXME(#57563): #57563 is now used as a common tracking issue,
                     // although we would like to have specific tracking issues for each
@@ -359,20 +424,23 @@ fn map_lib_features(base_src_path: &Path,
             };
             let feature_name = match find_attr_val(line, "feature") {
                 Some(name) => name,
-                None => err!("malformed stability attribute"),
+                None => err!("malformed stability attribute: missing `feature` key"),
             };
-            let since = match find_attr_val(line, "since") {
-                Some(name) => name,
+            let since = match find_attr_val(line, "since").map(|x| x.parse()) {
+                Some(Ok(since)) => Some(since),
+                Some(Err(_err)) => {
+                    err!("malformed stability attribute: can't parse `since` key");
+                },
                 None if level == Status::Stable => {
-                    err!("malformed stability attribute");
+                    err!("malformed stability attribute: missing the `since` key");
                 }
-                None => "None",
+                None => None,
             };
             let tracking_issue = find_attr_val(line, "issue").map(|s| s.parse().unwrap());
 
             let feature = Feature {
                 level,
-                since: since.to_owned(),
+                since,
                 has_gate_test: false,
                 tracking_issue,
             };
diff --git a/src/tools/tidy/src/features/version.rs b/src/tools/tidy/src/features/version.rs
new file mode 100644
index 00000000000..6027e7d35e2
--- /dev/null
+++ b/src/tools/tidy/src/features/version.rs
@@ -0,0 +1,92 @@
+use std::str::FromStr;
+use std::num::ParseIntError;
+use std::fmt;
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub struct Version {
+    parts: [u32; 3],
+}
+
+impl fmt::Display for Version {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.pad(&format!("{}.{}.{}", self.parts[0], self.parts[1], self.parts[2]))
+    }
+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub enum ParseVersionError {
+    ParseIntError(ParseIntError),
+    WrongNumberOfParts,
+}
+
+impl From<ParseIntError> for ParseVersionError {
+    fn from(err: ParseIntError) -> Self {
+        ParseVersionError::ParseIntError(err)
+    }
+}
+
+impl FromStr for Version {
+    type Err = ParseVersionError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut iter = s.split('.').map(|part| Ok(part.parse()?));
+
+        let parts = {
+            let mut part = || {
+                iter.next()
+                    .unwrap_or(Err(ParseVersionError::WrongNumberOfParts))
+            };
+
+            [part()?, part()?, part()?]
+        };
+
+        if let Some(_) = iter.next() {
+            // Ensure we don't have more than 3 parts.
+            return Err(ParseVersionError::WrongNumberOfParts);
+        }
+
+        Ok(Self { parts })
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::Version;
+
+    #[test]
+    fn test_try_from_invalid_version() {
+        assert!("".parse::<Version>().is_err());
+        assert!("hello".parse::<Version>().is_err());
+        assert!("1.32.hi".parse::<Version>().is_err());
+        assert!("1.32..1".parse::<Version>().is_err());
+        assert!("1.32".parse::<Version>().is_err());
+        assert!("1.32.0.1".parse::<Version>().is_err());
+    }
+
+    #[test]
+    fn test_try_from_single() {
+        assert_eq!("1.32.0".parse(), Ok(Version { parts: [1, 32, 0] }));
+        assert_eq!("1.0.0".parse(), Ok(Version { parts: [1, 0, 0] }));
+    }
+
+    #[test]
+    fn test_compare() {
+        let v_1_0_0 = "1.0.0".parse::<Version>().unwrap();
+        let v_1_32_0 = "1.32.0".parse::<Version>().unwrap();
+        let v_1_32_1 = "1.32.1".parse::<Version>().unwrap();
+        assert!(v_1_0_0 < v_1_32_1);
+        assert!(v_1_0_0 < v_1_32_0);
+        assert!(v_1_32_0 < v_1_32_1);
+    }
+
+    #[test]
+    fn test_to_string() {
+        let v_1_0_0 = "1.0.0".parse::<Version>().unwrap();
+        let v_1_32_1 = "1.32.1".parse::<Version>().unwrap();
+
+        assert_eq!(v_1_0_0.to_string(), "1.0.0");
+        assert_eq!(v_1_32_1.to_string(), "1.32.1");
+        assert_eq!(format!("{:<8}", v_1_32_1), "1.32.1  ");
+        assert_eq!(format!("{:>8}", v_1_32_1), "  1.32.1");
+    }
+}
diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs
index c4a1246ffdf..30080452edc 100644
--- a/src/tools/tidy/src/lib.rs
+++ b/src/tools/tidy/src/lib.rs
@@ -5,6 +5,7 @@
 
 #![deny(rust_2018_idioms)]
 
+extern crate regex;
 extern crate serde_json;
 #[macro_use]
 extern crate serde_derive;