Auto merge of #117149 - nnethercote:tidy-alphabetical-unit-tests, r=N…

…ilstrieb

tidy: add unit tests for alphabetical checks

I discovered there aren't any tests while working on #117068.

r? `@Nilstrieb`

src/tools/tidy/src/alphabetical.rs

@@ -1,6 +1,6 @@
 //! Checks that a list of items is in alphabetical order
 //!
-//! To use, use the following annotation in the code:
+//! Use the following marker in the code:
 //! ```rust
 //! // tidy-alphabetical-start
 //! fn aaa() {}
@@ -10,17 +10,23 @@
 //! ```
 //!
 //! The following lines are ignored:
+//! - Empty lines
 //! - Lines that are indented with more or less spaces than the first line
-//! - Lines starting with `//`, `#[`, `)`, `]`, `}` if the comment has the same indentation as
-//!   the first line
+//! - Lines starting with `//`, `#` (except those starting with `#!`), `)`, `]`, `}` if the comment
+//!   has the same indentation as the first line
+//! - Lines starting with a closing delimiter (`)`, `[`, `}`) are ignored.
 //!
-//! If a line ends with an opening bracket, the line is ignored and the next line will have
-//! its extra indentation ignored.
+//! If a line ends with an opening delimiter, we effectively join the following line to it before
+//! checking it. E.g. `foo(\nbar)` is treated like `foo(bar)`.
 
-use std::{fmt::Display, path::Path};
+use std::fmt::Display;
+use std::path::Path;
 
 use crate::walk::{filter_dirs, walk};
 
+#[cfg(test)]
+mod tests;
+
 fn indentation(line: &str) -> usize {
     line.find(|c| c != ' ').unwrap_or(0)
 }
@@ -29,28 +35,36 @@ fn is_close_bracket(c: char) -> bool {
     matches!(c, ')' | ']' | '}')
 }
 
-// Don't let tidy check this here :D
-const START_COMMENT: &str = concat!("tidy-alphabetical", "-start");
-const END_COMMENT: &str = "tidy-alphabetical-end";
+const START_MARKER: &str = "tidy-alphabetical-start";
+const END_MARKER: &str = "tidy-alphabetical-end";
 
 fn check_section<'a>(
     file: impl Display,
     lines: impl Iterator<Item = (usize, &'a str)>,
+    err: &mut dyn FnMut(&str) -> std::io::Result<()>,
     bad: &mut bool,
 ) {
-    let content_lines = lines.take_while(|(_, line)| !line.contains(END_COMMENT));
-
     let mut prev_line = String::new();
     let mut first_indent = None;
     let mut in_split_line = None;
 
-    for (line_idx, line) in content_lines {
-        if line.contains(START_COMMENT) {
-            tidy_error!(
+    for (idx, line) in lines {
+        if line.is_empty() {
+            continue;
+        }
+
+        if line.contains(START_MARKER) {
+            tidy_error_ext!(
+                err,
                 bad,
-                "{file}:{} found `{START_COMMENT}` expecting `{END_COMMENT}`",
-                line_idx
-            )
+                "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`",
+                idx + 1
+            );
+            return;
+        }
+
+        if line.contains(END_MARKER) {
+            return;
         }
 
         let indent = first_indent.unwrap_or_else(|| {
@@ -60,6 +74,7 @@ fn check_section<'a>(
         });
 
         let line = if let Some(prev_split_line) = in_split_line {
+            // Join the split lines.
             in_split_line = None;
             format!("{prev_split_line}{}", line.trim_start())
         } else {
@@ -73,7 +88,7 @@ fn check_section<'a>(
         let trimmed_line = line.trim_start_matches(' ');
 
         if trimmed_line.starts_with("//")
-            || trimmed_line.starts_with("#[")
+            || (trimmed_line.starts_with("#") && !trimmed_line.starts_with("#!"))
             || trimmed_line.starts_with(is_close_bracket)
         {
             continue;
@@ -87,25 +102,44 @@ fn check_section<'a>(
         let prev_line_trimmed_lowercase = prev_line.trim_start_matches(' ').to_lowercase();
 
         if trimmed_line.to_lowercase() < prev_line_trimmed_lowercase {
-            tidy_error!(bad, "{file}:{}: line not in alphabetical order", line_idx + 1,);
+            tidy_error_ext!(err, bad, "{file}:{}: line not in alphabetical order", idx + 1);
         }
 
         prev_line = line;
     }
+
+    tidy_error_ext!(err, bad, "{file}: reached end of file expecting `{END_MARKER}`")
 }
 
-pub fn check(path: &Path, bad: &mut bool) {
-    walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| {
-        let file = &entry.path().display();
+fn check_lines<'a>(
+    file: &impl Display,
+    mut lines: impl Iterator<Item = (usize, &'a str)>,
+    err: &mut dyn FnMut(&str) -> std::io::Result<()>,
+    bad: &mut bool,
+) {
+    while let Some((idx, line)) = lines.next() {
+        if line.contains(END_MARKER) {
+            tidy_error_ext!(
+                err,
+                bad,
+                "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`",
+                idx + 1
+            )
+        }
 
-        let mut lines = contents.lines().enumerate().peekable();
-        while let Some((_, line)) = lines.next() {
-            if line.contains(START_COMMENT) {
-                check_section(file, &mut lines, bad);
-                if lines.peek().is_none() {
-                    tidy_error!(bad, "{file}: reached end of file expecting `{END_COMMENT}`")
-                }
-            }
+        if line.contains(START_MARKER) {
+            check_section(file, &mut lines, err, bad);
         }
+    }
+}
+
+pub fn check(path: &Path, bad: &mut bool) {
+    let skip =
+        |path: &_, _is_dir| filter_dirs(path) || path.ends_with("tidy/src/alphabetical/tests.rs");
+
+    walk(path, skip, &mut |entry, contents| {
+        let file = &entry.path().display();
+        let lines = contents.lines().enumerate();
+        check_lines(file, lines, &mut crate::tidy_error, bad)
     });
 }

src/tools/tidy/src/alphabetical/tests.rs

@@ -0,0 +1,188 @@
+use super::*;
+use std::io::Write;
+use std::str::from_utf8;
+
+fn test(lines: &str, name: &str, expected_msg: &str, expected_bad: bool) {
+    let mut actual_msg = Vec::new();
+    let mut actual_bad = false;
+    let mut err = |args: &_| {
+        write!(&mut actual_msg, "{args}")?;
+        Ok(())
+    };
+    check_lines(&name, lines.lines().enumerate(), &mut err, &mut actual_bad);
+    assert_eq!(expected_msg, from_utf8(&actual_msg).unwrap());
+    assert_eq!(expected_bad, actual_bad);
+}
+
+fn good(lines: &str) {
+    test(lines, "good", "", false);
+}
+
+fn bad(lines: &str, expected_msg: &str) {
+    test(lines, "bad", expected_msg, true);
+}
+
+#[test]
+fn test_no_markers() {
+    let lines = "\
+        def
+        abc
+        xyz
+    ";
+    good(lines);
+}
+
+#[test]
+fn test_rust_good() {
+    let lines = "\
+        // tidy-alphabetical-start
+        abc
+        def
+        xyz
+        // tidy-alphabetical-end"; // important: end marker on last line
+    good(lines);
+}
+
+#[test]
+fn test_complex_good() {
+    let lines = "\
+        zzz
+
+        // tidy-alphabetical-start
+        abc
+        // Rust comments are ok
+        def
+        # TOML comments are ok
+        xyz
+        // tidy-alphabetical-end
+
+        # tidy-alphabetical-start
+        foo(abc);
+        // blank lines are ok
+
+        // split line gets joined
+        foo(
+            def
+        );
+
+        foo(xyz);
+        # tidy-alphabetical-end
+
+        % tidy-alphabetical-start
+        abc
+            ignored_due_to_different_indent
+        def
+        % tidy-alphabetical-end
+
+        aaa
+    ";
+    good(lines);
+}
+
+#[test]
+fn test_rust_bad() {
+    let lines = "\
+        // tidy-alphabetical-start
+        abc
+        xyz
+        def
+        // tidy-alphabetical-end
+    ";
+    bad(lines, "bad:4: line not in alphabetical order");
+}
+
+#[test]
+fn test_toml_bad() {
+    let lines = "\
+        # tidy-alphabetical-start
+        abc
+        xyz
+        def
+        # tidy-alphabetical-end
+    ";
+    bad(lines, "bad:4: line not in alphabetical order");
+}
+
+#[test]
+fn test_features_bad() {
+    // Even though lines starting with `#` are treated as comments, lines
+    // starting with `#!` are an exception.
+    let lines = "\
+        tidy-alphabetical-start
+        #![feature(abc)]
+        #![feature(xyz)]
+        #![feature(def)]
+        tidy-alphabetical-end
+    ";
+    bad(lines, "bad:4: line not in alphabetical order");
+}
+
+#[test]
+fn test_indent_bad() {
+    // All lines are indented the same amount, and so are checked.
+    let lines = "\
+        $ tidy-alphabetical-start
+            abc
+            xyz
+            def
+        $ tidy-alphabetical-end
+    ";
+    bad(lines, "bad:4: line not in alphabetical order");
+}
+
+#[test]
+fn test_split_bad() {
+    let lines = "\
+        || tidy-alphabetical-start
+        foo(abc)
+        foo(
+            xyz
+        )
+        foo(
+            def
+        )
+        && tidy-alphabetical-end
+    ";
+    bad(lines, "bad:7: line not in alphabetical order");
+}
+
+#[test]
+fn test_double_start() {
+    let lines = "\
+        tidy-alphabetical-start
+        abc
+        tidy-alphabetical-start
+    ";
+    bad(lines, "bad:3 found `tidy-alphabetical-start` expecting `tidy-alphabetical-end`");
+}
+
+#[test]
+fn test_missing_start() {
+    let lines = "\
+        abc
+        tidy-alphabetical-end
+        abc
+    ";
+    bad(lines, "bad:2 found `tidy-alphabetical-end` expecting `tidy-alphabetical-start`");
+}
+
+#[test]
+fn test_missing_end() {
+    let lines = "\
+        tidy-alphabetical-start
+        abc
+    ";
+    bad(lines, "bad: reached end of file expecting `tidy-alphabetical-end`");
+}
+
+#[test]
+fn test_double_end() {
+    let lines = "\
+        tidy-alphabetical-start
+        abc
+        tidy-alphabetical-end
+        def
+        tidy-alphabetical-end
+    ";
+    bad(lines, "bad:5 found `tidy-alphabetical-end` expecting `tidy-alphabetical-start`");
+}

src/tools/tidy/src/lib.rs

@@ -3,8 +3,6 @@
 //! This library contains the tidy lints and exposes it
 //! to be used by tools.
 
-use std::fmt::Display;
-
 use termcolor::WriteColor;
 
 /// A helper macro to `unwrap` a result except also print out details like:
@@ -31,16 +29,22 @@ macro_rules! t {
 
 macro_rules! tidy_error {
     ($bad:expr, $($fmt:tt)*) => ({
-        $crate::tidy_error($bad, format_args!($($fmt)*)).expect("failed to output error");
+        $crate::tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error");
+        *$bad = true;
     });
 }
 
-fn tidy_error(bad: &mut bool, args: impl Display) -> std::io::Result<()> {
+macro_rules! tidy_error_ext {
+    ($tidy_error:path, $bad:expr, $($fmt:tt)*) => ({
+        $tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error");
+        *$bad = true;
+    });
+}
+
+fn tidy_error(args: &str) -> std::io::Result<()> {
     use std::io::Write;
     use termcolor::{Color, ColorChoice, ColorSpec, StandardStream};
 
-    *bad = true;
-
     let mut stderr = StandardStream::stdout(ColorChoice::Auto);
     stderr.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?;