From 56e82108afa1e54a4bb1996251d3d79016a092e0 Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Fri, 10 Jan 2025 19:01:24 -0800 Subject: [PATCH] port style.rs to syn and add tests for the style checker --- .gitignore | 1 - libc-test/Cargo.toml | 16 ++ libc-test/test/check_style.rs | 50 ++++ libc-test/test/style/mod.rs | 490 ++++++++++++++++++++++++++++++++++ libc-test/test/style_tests.rs | 260 ++++++++++++++++++ 5 files changed, 816 insertions(+), 1 deletion(-) create mode 100644 libc-test/test/check_style.rs create mode 100644 libc-test/test/style/mod.rs create mode 100644 libc-test/test/style_tests.rs diff --git a/.gitignore b/.gitignore index bbbad4bc51532..f0ff2599d09b5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,3 @@ target Cargo.lock *~ -style diff --git a/libc-test/Cargo.toml b/libc-test/Cargo.toml index 721ccc90932dc..857886711dfa3 100644 --- a/libc-test/Cargo.toml +++ b/libc-test/Cargo.toml @@ -15,6 +15,12 @@ A test crate for the libc crate. [dependencies] libc = { path = "..", version = "1.0.0-alpha.1", default-features = false } +[dev-dependencies] +syn = { version = "2.0.91", features = ["full", "visit"] } +proc-macro2 = { version = "1.0.92", features = ["span-locations"] } +glob = "0.3.2" +annotate-snippets = { version = "0.11.5", features = ["testing-colors"] } + [build-dependencies] cc = "1.0.83" # FIXME: Use fork ctest until the maintainer gets back. @@ -90,3 +96,13 @@ harness = false name = "primitive_types" path = "test/primitive_types.rs" harness = true + +[[test]] +name = "style" +path = "test/check_style.rs" +harness = true + +[[test]] +name = "style_tests" +path = "test/style_tests.rs" +harness = true diff --git a/libc-test/test/check_style.rs b/libc-test/test/check_style.rs new file mode 100644 index 0000000000000..ee5e134891104 --- /dev/null +++ b/libc-test/test/check_style.rs @@ -0,0 +1,50 @@ +//! Simple script to verify the coding style of this library. +//! +//! ## How to run +//! +//! The first argument to this script is the directory to run on, so running +//! this script should be as simple as: +//! +//! ```notrust +//! cargo test --test style +//! ``` + +pub mod style; + +use std::env; +use std::path::Path; + +use style::{Result, StyleChecker}; + +#[test] +fn check_style() { + let root_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../src"); + walk(&root_dir).unwrap(); + eprintln!("good style!"); +} + +fn walk(root_dir: &Path) -> Result<()> { + let mut style_checker = StyleChecker::new(); + + for entry in glob::glob(&format!( + "{}/**/*.rs", + root_dir.to_str().expect("dir should be valid UTF-8") + ))? { + let entry = entry?; + + let name = entry + .file_name() + .expect("file name should not end in ..") + .to_str() + .expect("file name should be valid UTF-8"); + if let "lib.rs" | "macros.rs" = &name[..] { + continue; + } + + let path = entry.as_path(); + style_checker.check_file(path)?; + style_checker.reset_state(); + } + + style_checker.finalize() +} diff --git a/libc-test/test/style/mod.rs b/libc-test/test/style/mod.rs new file mode 100644 index 0000000000000..cc953d32c3aed --- /dev/null +++ b/libc-test/test/style/mod.rs @@ -0,0 +1,490 @@ +//! Provides the [StyleChecker] visitor to verify the coding style of +//! this library. +//! +//! This is split out so that the implementation itself can be tested +//! separately, see test/check_style.rs for how it's used and +//! test/style_tests.rs for the implementation tests. +//! +//! ## Guidelines +//! +//! The current style is: +//! +//! * Specific module layout: +//! 1. use directives +//! 2. typedefs +//! 3. structs +//! 4. constants +//! 5. f! { ... } functions +//! 6. extern functions +//! 7. modules + pub use +//! * No manual deriving Copy/Clone +//! * Only one f! per module +//! * Multiple s! macros are allowed as long as there isn't a duplicate cfg, +//! whether as a standalone attribute (#[cfg]) or in a cfg_if! +//! * s! macros should not just have a positive cfg since they should +//! just go into the relevant file but combined cfgs with all(...) and +//! any(...) are allowed + +use std::collections::HashMap; +use std::fs; +use std::ops::Deref; +use std::path::{Path, PathBuf}; + +use annotate_snippets::{Level, Renderer, Snippet}; +use proc_macro2::Span; +use syn::parse::{Parse, ParseStream}; +use syn::spanned::Spanned; +use syn::visit::{self, Visit}; +use syn::Token; + +const ALLOWED_REPEATED_MACROS: &[&str] = &["s", "s_no_extra_traits", "s_paren"]; + +pub type Error = Box; +pub type Result = std::result::Result; + +#[derive(Default)] +pub struct StyleChecker { + /// The state the style checker is in, used to enforce the module layout. + state: State, + /// Span of the first item encountered in this state to use in help + /// diagnostic text. + state_span: Option, + /// The s! macro cfgs we have seen, whether through #[cfg] attributes + /// or within the branches of cfg_if! blocks so that we can check for duplicates. + seen_s_macro_cfgs: HashMap, + /// Span of the first f! macro seen, used to enforce only one f! macro + /// per module. + first_f_macro: Option, + /// The errors that the style checker has seen. + errors: Vec, + /// Path of the currently active file. + path: PathBuf, + /// Whether the style checker is currently in an `impl` block. + in_impl: bool, +} + +/// The part of the module layout we are currently checking. +#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum State { + #[default] + Start, + Imports, + Typedefs, + Structs, + Constants, + FunctionDefinitions, + Functions, + Modules, +} + +/// Similar to [syn::ExprIf] except with [syn::Attribute] +/// as the condition instead of [syn::Expr]. +struct ExprCfgIf { + _cond: syn::Attribute, + /// A `cfg_if!` branch can only contain items. + then_branch: Vec, + else_branch: Option>, +} + +enum ExprCfgElse { + /// Final block with no condition `else { /* ... */ }`. + Block(Vec), + /// `else if { /* ... */ }` block. + If(ExprCfgIf), +} + +/// Describes an that occurred error when checking the file +/// at the given `path`. Besides the error message, it contains +/// additional span information so that we can print nice error messages. +#[derive(Debug)] +struct FileError { + path: PathBuf, + span: Span, + title: String, + msg: String, + help: Option, +} + +/// Help message with an optional span where the help should point to. +type HelpMsg = (Option, String); + +impl StyleChecker { + pub fn new() -> Self { + Self::default() + } + + /// Reads and parses the file at the given path and checks + /// for any style violations. + pub fn check_file(&mut self, path: &Path) -> Result<()> { + let contents = fs::read_to_string(path)?; + + self.path = PathBuf::from(path); + self.check_string(contents) + } + + pub fn check_string(&mut self, contents: String) -> Result<()> { + let file = syn::parse_file(&contents)?; + self.visit_file(&file); + Ok(()) + } + + /// Resets the state of the [StyleChecker]. + pub fn reset_state(&mut self) { + *self = Self { + errors: std::mem::take(&mut self.errors), + ..Self::default() + }; + } + + /// Collect all errors into a single error, reporting them if any. + pub fn finalize(self) -> Result<()> { + if self.errors.is_empty() { + return Ok(()); + } + + let renderer = Renderer::styled(); + for error in self.errors { + let source = fs::read_to_string(&error.path)?; + + let mut snippet = Snippet::source(&source) + .origin(error.path.to_str().expect("path to be UTF-8")) + .fold(true) + .annotation(Level::Error.span(error.span.byte_range()).label(&error.msg)); + if let Some((help_span, help_msg)) = &error.help { + if let Some(help_span) = help_span { + snippet = snippet + .annotation(Level::Help.span(help_span.byte_range()).label(help_msg)); + } + } + + let mut msg = Level::Error.title(&error.title).snippet(snippet); + if let Some((help_span, help_msg)) = &error.help { + if help_span.is_none() { + msg = msg.footer(Level::Help.title(help_msg)) + } + } + + eprintln!("{}", renderer.render(msg)); + } + + Err("some tests failed".into()) + } + + fn set_state(&mut self, new_state: State, span: Span) { + if self.state > new_state && !self.in_impl { + let help_span = self + .state_span + .expect("state_span should be set since we are on a second state"); + self.error( + "incorrect module layout".to_string(), + span, + format!( + "{} found after {} when it belongs before", + new_state.desc(), + self.state.desc() + ), + ( + Some(help_span), + format!( + "move the {} to before this {}", + new_state.desc(), + self.state.desc() + ), + ), + ); + } + + if self.state != new_state { + self.state = new_state; + self.state_span = Some(span); + } + } + + /// Visit the items inside the [ExprCfgIf], restoring the state after + /// each branch. + fn visit_expr_cfg_if(&mut self, expr_cfg_if: &ExprCfgIf) { + let initial_state = self.state; + + for item in &expr_cfg_if.then_branch { + self.visit_item(item); + } + self.state = initial_state; + + if let Some(else_branch) = &expr_cfg_if.else_branch { + match else_branch.deref() { + ExprCfgElse::Block(items) => { + for item in items { + self.visit_item(item); + } + } + ExprCfgElse::If(expr_cfg_if) => self.visit_expr_cfg_if(&expr_cfg_if), + } + } + self.state = initial_state; + } + + /// If we see a normal s! macro without any attributes we just need + /// to check if there are any duplicates. + fn handle_s_macro_no_attrs(&mut self, item_macro: &syn::ItemMacro) { + let span = item_macro.span(); + match self.seen_s_macro_cfgs.get("") { + Some(seen_span) => { + self.error( + "duplicate s! macro".to_string(), + span, + format!("other s! macro"), + (Some(*seen_span), "combine the two".to_string()), + ); + } + None => { + self.seen_s_macro_cfgs.insert(String::new(), span); + } + } + } + + /// If an s! macro has attributes we check for any duplicates as well + /// as if they are standalone positive cfgs that would be better + /// in a separate file. + fn handle_s_macro_with_attrs(&mut self, item_macro: &syn::ItemMacro) { + for attr in &item_macro.attrs { + let Ok(meta_list) = attr.meta.require_list() else { + continue; + }; + + if meta_list.path.is_ident("cfg") { + let span = meta_list.span(); + let meta_str = meta_list.tokens.to_string(); + + match self.seen_s_macro_cfgs.get(&meta_str) { + Some(seen_span) => { + self.error( + "duplicate #[cfg] for s! macro".to_string(), + span, + "duplicated #[cfg]".to_string(), + (Some(*seen_span), "combine the two".to_string()), + ); + } + None => { + self.seen_s_macro_cfgs.insert(meta_str.clone(), span); + } + } + + if !meta_str.starts_with("not") + && !meta_str.starts_with("any") + && !meta_str.starts_with("all") + { + self.error( + "positive #[cfg] for s! macro".to_string(), + span, + String::new(), + (None, "move it to the relevant file".to_string()), + ); + } + } + } + } + + fn push_error(&mut self, title: String, span: Span, msg: String, help: Option) { + self.errors.push(FileError { + path: self.path.clone(), + title, + span, + msg, + help, + }); + } + + fn error(&mut self, title: String, span: Span, msg: String, help: HelpMsg) { + self.push_error(title, span, msg, Some(help)); + } +} + +impl<'ast> Visit<'ast> for StyleChecker { + /// Visit all items; most just update our current state but some also + /// perform additional checks like for the s! macro. + fn visit_item_use(&mut self, item_use: &'ast syn::ItemUse) { + let span = item_use.span(); + let new_state = if matches!(item_use.vis, syn::Visibility::Public(_)) { + State::Modules + } else { + State::Imports + }; + self.set_state(new_state, span); + + visit::visit_item_use(self, item_use); + } + + fn visit_item_const(&mut self, item_const: &'ast syn::ItemConst) { + let span = item_const.span(); + self.set_state(State::Constants, span); + + visit::visit_item_const(self, item_const); + } + + fn visit_item_impl(&mut self, item_impl: &'ast syn::ItemImpl) { + self.in_impl = true; + visit::visit_item_impl(self, item_impl); + self.in_impl = false; + } + + fn visit_item_struct(&mut self, item_struct: &'ast syn::ItemStruct) { + let span = item_struct.span(); + self.set_state(State::Structs, span); + + visit::visit_item_struct(self, item_struct); + } + + fn visit_item_type(&mut self, item_type: &'ast syn::ItemType) { + let span = item_type.span(); + self.set_state(State::Typedefs, span); + + visit::visit_item_type(self, item_type); + } + + /// Checks s! macros for any duplicate cfgs and whether they are + /// just positive #[cfg(...)] attributes. We need [syn::ItemMacro] + /// instead of [syn::Macro] because it contains the attributes. + fn visit_item_macro(&mut self, item_macro: &'ast syn::ItemMacro) { + if item_macro.mac.path.is_ident("s") { + if item_macro.attrs.is_empty() { + self.handle_s_macro_no_attrs(item_macro); + } else { + self.handle_s_macro_with_attrs(item_macro); + } + } + + visit::visit_item_macro(self, item_macro); + } + + fn visit_macro(&mut self, mac: &'ast syn::Macro) { + let span = mac.span(); + if mac.path.is_ident("cfg_if") { + let expr_cfg_if: ExprCfgIf = mac + .parse_body() + .expect("cfg_if! should be parsed since it compiled"); + + self.visit_expr_cfg_if(&expr_cfg_if); + } else { + let new_state = + if mac.path.get_ident().is_some_and(|ident| { + ALLOWED_REPEATED_MACROS.contains(&ident.to_string().as_str()) + }) { + // multiple macros of this type are allowed + State::Structs + } else if mac.path.is_ident("f") { + match self.first_f_macro { + Some(f_macro_span) => { + self.error( + "multiple f! macros in one module".to_string(), + span, + "other f! macro".to_string(), + ( + Some(f_macro_span), + "combine it with this f! macro".to_string(), + ), + ); + } + None => { + self.first_f_macro = Some(span); + } + } + State::FunctionDefinitions + } else { + self.state + }; + self.set_state(new_state, span); + } + + visit::visit_macro(self, mac); + } + + fn visit_item_foreign_mod(&mut self, item_foreign_mod: &'ast syn::ItemForeignMod) { + let span = item_foreign_mod.span(); + self.set_state(State::Functions, span); + + visit::visit_item_foreign_mod(self, item_foreign_mod); + } + + fn visit_item_mod(&mut self, item_mod: &'ast syn::ItemMod) { + let span = item_mod.span(); + self.set_state(State::Modules, span); + + visit::visit_item_mod(self, item_mod); + } + + fn visit_meta_list(&mut self, meta_list: &'ast syn::MetaList) { + let span = meta_list.span(); + let meta_str = meta_list.tokens.to_string(); + if meta_list.path.is_ident("derive") + && (meta_str.contains("Copy") || meta_str.contains("Clone")) + { + self.error( + "impl Copy and Clone manually".to_string(), + span, + "found manual implementation of Copy and/or Clone".to_string(), + (None, "use one of the s! macros instead".to_string()), + ); + } + + visit::visit_meta_list(self, meta_list); + } +} + +impl Parse for ExprCfgIf { + fn parse(input: ParseStream) -> syn::Result { + input.parse::()?; + let cond = input + .call(syn::Attribute::parse_outer)? + .into_iter() + .next() + .expect("an attribute should be present since it compiled"); + + let content; + syn::braced!(content in input); + let mut then_branch = Vec::new(); + while !content.is_empty() { + let mut value = content.parse()?; + if let syn::Item::Macro(item_macro) = &mut value { + item_macro.attrs.push(cond.clone()); + } + then_branch.push(value); + } + + let mut else_branch = None; + if input.peek(Token![else]) { + input.parse::()?; + + if input.peek(Token![if]) { + else_branch = Some(Box::new(ExprCfgElse::If(input.parse()?))); + } else { + let content; + syn::braced!(content in input); + let mut items = Vec::new(); + while !content.is_empty() { + items.push(content.parse()?); + } + else_branch = Some(Box::new(ExprCfgElse::Block(items))); + } + } + Ok(Self { + _cond: cond, + then_branch, + else_branch, + }) + } +} + +impl State { + fn desc(&self) -> &str { + match *self { + State::Start => "start", + State::Imports => "import", + State::Typedefs => "typedef", + State::Structs => "struct", + State::Constants => "constant", + State::FunctionDefinitions => "function definition", + State::Functions => "extern function", + State::Modules => "module", + } + } +} diff --git a/libc-test/test/style_tests.rs b/libc-test/test/style_tests.rs new file mode 100644 index 0000000000000..be8fddbccf644 --- /dev/null +++ b/libc-test/test/style_tests.rs @@ -0,0 +1,260 @@ +//! Verifies the implementation of the style checker in [style]. + +use style::StyleChecker; + +pub mod style; + +#[test] +fn check_style_accept_correct_module_layout() { + let contents = r#" +use core::mem::size_of; +pub type foo_t = u32; +struct Foo {} +pub const FOO: u32 = 0x20000; +f! {} +extern "C" {} +mod foolib; +pub use self::foolib::*; +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_incorrect_module_layout() { + let contents = r#" +use core::mem::size_of; +pub type foo_t = u32; +struct Foo {} +pub const FOO: u32 = 0x20000; +extern "C" {} +f! {} +mod foolib; +pub use self::foolib::*; +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_incorrect_cfg_if_layout() { + let contents = r#" +cfg_if! { + if #[cfg(foo)] { + pub type foo_t = u32; + use core::mem::size_of; + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_cfg_if_branch_resets_state() { + let contents = r#" +cfg_if! { + if #[cfg(foo)] { + use core::mem::size_of; + pub type foo_t = u32; + } else { + use core::mem::align_of; + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_multiple_f_macros() { + let contents = r#" +f! {} +f! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_cfg_ignore_target_endian_nested() { + let contents = r#" +pub struct Foo { + #[cfg(target_endian = "little")] + pub id: __u16, +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_manual_copy() { + let contents = r#" +#[derive(Copy)] +pub struct Foo {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_manual_clone() { + let contents = r#" +#[derive(Clone)] +pub struct Foo {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_multiple_s_macros_with_disjoint_cfg() { + let contents = r#" +// Main `s!` +s! {} + +// These are not supported on a single arch. It doesn't make sense to +// duplicate `foo` into every single file except one, so allow this here. +#[cfg(not(target_arch = "foo"))] +s! { pub struct foo { /* ... */ } } + +// Similar to the above, no problems here +#[cfg(not(target_os = "illumos"))] +s! { pub struct bar { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_duplicated_s_macro() { + let contents = r#" +s! {} +s! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_duplicated_s_macro_cfg() { + let contents = r#" +#[cfg(not(target_arch = "foo"))] +s! {} + +#[cfg(not(target_arch = "foo"))] +s! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_single_positive_s_macro_cfg() { + let contents = r#" +// A positive (no `not`) config: reject because this should go into +// the relevant file. +#[cfg(target_arch = "foo")] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_single_positive_s_macro_cfg_target_os() { + let contents = r#" +// A positive (no `not`) config: reject because this should go into +// the relevant file. +#[cfg(target_os = "foo")] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_positive_s_macro_any() { + let contents = r#" +// It's nicer to accept this so that we don't have to duplicate the same struct 3 times. +#[cfg(any(target_arch = "foo", target_arch = "bar", target_arch = "baz"))] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_accept_positive_s_macro_all() { + let contents = r#" +#[cfg(all(target_arch = "foo", target_arch = "bar", target_arch = "baz"))] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_duplicated_cfg_and_cfg_if() { + let contents = r#" +#[cfg(not(target_arch = "foo"))] +s! { pub struct foo { /* ... */ } } + +cfg_if! { + if #[cfg(not(target_arch = "foo"))] { + s!{ pub struct bar {} } + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +}