From b7bea6e46fa1f00d96a779cc75f5a652cf5cdc5d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 20 Oct 2023 14:19:28 +1100 Subject: [PATCH 1/4] Fix a bug in tidy's alphabetical checking. Currently, if a `tidy-alphabetical-end` marker appears on the last line of a file, tidy will erroneously issue a "reach end of file expecting `tidy-alphabetical-end`" error. This is because it uses `take_while` within `check_section`, which consumes the line with the end marker, and then after `check_section` returns `check` peeks for at least one more line, which won't be there is the marker was on the last line. This commit fixes the problem, by removing the use of `take_while`, and doing the "reached end of file" test within `check_section` without using `peek`. It also renames `{START,END}_COMMENT` as `{START,END}_MARKER`, which is a more appropriate name. --- src/tools/tidy/src/alphabetical.rs | 31 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 3e60915c224cc..dde5bef5a5f7f 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() {} @@ -30,27 +30,25 @@ fn is_close_bracket(c: char) -> bool { } // 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 = concat!("tidy-alphabetical", "-start"); +const END_MARKER: &str = "tidy-alphabetical-end"; fn check_section<'a>( file: impl Display, lines: impl Iterator, 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!( - bad, - "{file}:{} found `{START_COMMENT}` expecting `{END_COMMENT}`", - line_idx - ) + for (line_idx, line) in lines { + if line.contains(START_MARKER) { + tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", line_idx) + } + + if line.contains(END_MARKER) { + return; } let indent = first_indent.unwrap_or_else(|| { @@ -92,19 +90,18 @@ fn check_section<'a>( prev_line = line; } + + tidy_error!(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(); - let mut lines = contents.lines().enumerate().peekable(); + let mut lines = contents.lines().enumerate(); while let Some((_, line)) = lines.next() { - if line.contains(START_COMMENT) { + if line.contains(START_MARKER) { check_section(file, &mut lines, bad); - if lines.peek().is_none() { - tidy_error!(bad, "{file}: reached end of file expecting `{END_COMMENT}`") - } } } }); From 40797eef5ea3f8d50c98fbb81b0c005f7a358539 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 23 Oct 2023 08:53:16 +1100 Subject: [PATCH 2/4] tidy: skip lines starting with `#` in alphabetical check. These are comment lines in `Cargo.toml` files. But exclude lines starting with `#!` from the skipping, because we want to check them. (Rust `#![feature(...)]` lines.) Also allow empty lines, which are occasionally useful. --- src/tools/tidy/src/alphabetical.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index dde5bef5a5f7f..98a8f4a998019 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -10,9 +10,10 @@ //! ``` //! //! 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 //! //! If a line ends with an opening bracket, the line is ignored and the next line will have //! its extra indentation ignored. @@ -43,6 +44,10 @@ fn check_section<'a>( let mut in_split_line = None; for (line_idx, line) in lines { + if line.is_empty() { + continue; + } + if line.contains(START_MARKER) { tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", line_idx) } @@ -71,7 +76,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; From a79a2c729df5086a64c2aaf6c77c0ce582cb1499 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Oct 2023 10:45:56 +1100 Subject: [PATCH 3/4] tidy: some minor improvements. - Tweak some comments. - No need to do the `concat!` trick on `START_MARKER` because it's immediately followed by `END_MARKER`. - Fix an off-by-one error in the line number for an error message. - When a second start marker is found without an intervening end marker, after giving an error, treat it as though it ends the section. It's hard to know exactly what to do in this case, but it makes unit testing this case a little simpler (in the next commit). - If an end marker occurs without a preceding start marker, issue an error. --- src/tools/tidy/src/alphabetical.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 98a8f4a998019..32fb16b0a4214 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -14,11 +14,13 @@ //! - Lines that are indented with more or less spaces than 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}; @@ -30,8 +32,7 @@ fn is_close_bracket(c: char) -> bool { matches!(c, ')' | ']' | '}') } -// Don't let tidy check this here :D -const START_MARKER: &str = concat!("tidy-alphabetical", "-start"); +const START_MARKER: &str = "tidy-alphabetical-start"; const END_MARKER: &str = "tidy-alphabetical-end"; fn check_section<'a>( @@ -43,13 +44,14 @@ fn check_section<'a>( let mut first_indent = None; let mut in_split_line = None; - for (line_idx, line) in lines { + for (idx, line) in lines { if line.is_empty() { continue; } if line.contains(START_MARKER) { - tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", line_idx) + tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", idx + 1); + return; } if line.contains(END_MARKER) { @@ -63,6 +65,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 { @@ -90,7 +93,7 @@ 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!(bad, "{file}:{}: line not in alphabetical order", idx + 1); } prev_line = line; @@ -104,7 +107,15 @@ pub fn check(path: &Path, bad: &mut bool) { let file = &entry.path().display(); let mut lines = contents.lines().enumerate(); - while let Some((_, line)) = lines.next() { + while let Some((idx, line)) = lines.next() { + if line.contains(END_MARKER) { + tidy_error!( + bad, + "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`", + idx + 1 + ) + } + if line.contains(START_MARKER) { check_section(file, &mut lines, bad); } From a0b0ace061e797e26dc764b5a3e41f7440864a8c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Oct 2023 10:51:29 +1100 Subject: [PATCH 4/4] tidy: add unit tests for alphabetical markers. This requires introducing `tidy_error_ext!` as an alternative to `tidy_error!`. It is passed a closure that is called for an error. This lets tests capture tidy error messages in a buffer instead of printing them to stderr. It also requires pulling part of `check` out into a new function `check_lines`, so that tests can pass in an iterator over some strings instead of a file. --- src/tools/tidy/src/alphabetical.rs | 59 ++++--- src/tools/tidy/src/alphabetical/tests.rs | 188 +++++++++++++++++++++++ src/tools/tidy/src/lib.rs | 16 +- 3 files changed, 238 insertions(+), 25 deletions(-) create mode 100644 src/tools/tidy/src/alphabetical/tests.rs diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 32fb16b0a4214..150a9594350ed 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -24,6 +24,9 @@ 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) } @@ -38,6 +41,7 @@ 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 mut prev_line = String::new(); @@ -50,7 +54,12 @@ fn check_section<'a>( } if line.contains(START_MARKER) { - tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", idx + 1); + tidy_error_ext!( + err, + bad, + "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", + idx + 1 + ); return; } @@ -93,32 +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", idx + 1); + tidy_error_ext!(err, bad, "{file}:{}: line not in alphabetical order", idx + 1); } prev_line = line; } - tidy_error!(bad, "{file}: reached end of file expecting `{END_MARKER}`") + 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(); - while let Some((idx, line)) = lines.next() { - if line.contains(END_MARKER) { - tidy_error!( - bad, - "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`", - idx + 1 - ) - } - - if line.contains(START_MARKER) { - check_section(file, &mut lines, bad); - } + 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)))?;