diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 3e60915c224cc..150a9594350ed 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/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, + 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, + 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) }); } diff --git a/src/tools/tidy/src/alphabetical/tests.rs b/src/tools/tidy/src/alphabetical/tests.rs new file mode 100644 index 0000000000000..560e0284b89a1 --- /dev/null +++ b/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`"); +} diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index fc69c1432227e..eb0a2fda290d9 100644 --- a/src/tools/tidy/src/lib.rs +++ b/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)))?;