diff --git a/.gitignore b/.gitignore index 8d889ea9bd19..f0ff2599d09b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,3 @@ target Cargo.lock *~ -style -!style/ diff --git a/ci/style.sh b/ci/style.sh index bd1432e50bd9..da16bf4fe9ba 100755 --- a/ci/style.sh +++ b/ci/style.sh @@ -9,7 +9,7 @@ if [ -n "${CI:-}" ]; then check="--check" fi -cargo test --manifest-path libc-test/Cargo.toml --test style --test style_tests -- --nocapture +cargo test --manifest-path libc-test/Cargo.toml --test style -- --nocapture command -v rustfmt rustfmt -V diff --git a/libc-test/test/style/mod.rs b/libc-test/test/style/mod.rs index 66b85ea3a23a..cc953d32c3ae 100644 --- a/libc-test/test/style/mod.rs +++ b/libc-test/test/style/mod.rs @@ -37,6 +37,8 @@ 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; @@ -47,6 +49,8 @@ pub struct StyleChecker { /// 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. @@ -59,6 +63,7 @@ pub struct StyleChecker { in_impl: bool, } +/// The part of the module layout we are currently checking. #[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] enum State { #[default] @@ -75,16 +80,34 @@ enum State { /// Similar to [syn::ExprIf] except with [syn::Attribute] /// as the condition instead of [syn::Expr]. struct ExprCfgIf { - _attr: syn::Attribute, + _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() @@ -149,6 +172,9 @@ impl StyleChecker { 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, @@ -158,10 +184,7 @@ impl StyleChecker { self.state.desc() ), ( - Some( - self.state_span - .expect("state_span should be set since we are on a second state"), - ), + Some(help_span), format!( "move the {} to before this {}", new_state.desc(), @@ -200,7 +223,68 @@ impl StyleChecker { self.state = initial_state; } - fn push_error(&mut self, title: String, span: Span, msg: String, help: Option) { + /// 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, @@ -210,29 +294,14 @@ impl StyleChecker { }); } - fn error(&mut self, title: String, span: Span, msg: String, help: 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 { - 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); - } - + /// 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(_)) { @@ -272,58 +341,15 @@ impl<'ast> Visit<'ast> for StyleChecker { 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() { - 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); - } - } + self.handle_s_macro_no_attrs(item_macro); } else { - for attr in &item_macro.attrs { - if let Ok(meta_list) = attr.meta.require_list() { - 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()), - ); - } - } - } - } + self.handle_s_macro_with_attrs(item_macro); } } @@ -339,37 +365,33 @@ impl<'ast> Visit<'ast> for StyleChecker { self.visit_expr_cfg_if(&expr_cfg_if); } else { - let new_state = if mac.path.is_ident("s") { - // multiple macros are allowed if they have proper #[cfg(...)] - // attributes, see Self::visit_item_macro - State::Structs - } else if mac.path.is_ident("s_no_extra_traits") { - // multiple macros of this type are allowed - State::Structs - } else if mac.path.is_ident("s_paren") { - // 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); + 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 - }; + State::FunctionDefinitions + } else { + self.state + }; self.set_state(new_state, span); } @@ -389,12 +411,29 @@ impl<'ast> Visit<'ast> for StyleChecker { 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 attr = input + let cond = input .call(syn::Attribute::parse_outer)? .into_iter() .next() @@ -402,17 +441,14 @@ impl Parse for ExprCfgIf { let content; syn::braced!(content in input); - let then_branch: Vec = { - let mut items = Vec::new(); - while !content.is_empty() { - let mut value = content.parse()?; - if let syn::Item::Macro(item_macro) = &mut value { - item_macro.attrs.push(attr.clone()); - } - items.push(value); + 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()); } - items - }; + then_branch.push(value); + } let mut else_branch = None; if input.peek(Token![else]) { @@ -431,7 +467,7 @@ impl Parse for ExprCfgIf { } } Ok(Self { - _attr: attr, + _cond: cond, then_branch, else_branch, }) @@ -452,14 +488,3 @@ impl State { } } } - -#[derive(Debug)] -struct FileError { - path: PathBuf, - span: Span, - title: String, - msg: String, - help: Option, -} - -type Help = (Option, String);