diff --git a/libc-test/test/style/mod.rs b/libc-test/test/style/mod.rs index 77bd7ee151dd8..66b85ea3a23a6 100644 --- a/libc-test/test/style/mod.rs +++ b/libc-test/test/style/mod.rs @@ -17,10 +17,15 @@ //! 5. f! { ... } functions //! 6. extern functions //! 7. modules + pub use -//! * Use cfg_if! over #[cfg(...)] //! * 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}; @@ -42,8 +47,7 @@ pub struct StyleChecker { /// Span of the first item encountered in this state to use in help /// diagnostic text. state_span: Option, - // FIXME: see StyleChecker::set_state - _s_macros: usize, + 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, @@ -167,13 +171,6 @@ impl StyleChecker { ); } - // FIXME(#4109): multiple should be allowed if at least one is `cfg(not) within `cfg_if`. - // For now just disable this and check by hand. - // if self.s_macros == 2 { - // self.s_macros += 1; - // (self.on_err)(line, "multiple s! macros in one module"); - // } - if self.state != new_state { self.state = new_state; self.state_span = Some(span); @@ -182,22 +179,22 @@ impl StyleChecker { /// Visit the items inside the [ExprCfgIf], restoring the state after /// each branch. - fn visit_cfg_expr_if(&mut self, cfg_expr_if: &ExprCfgIf) { + fn visit_expr_cfg_if(&mut self, expr_cfg_if: &ExprCfgIf) { let initial_state = self.state; - for item in &cfg_expr_if.then_branch { + for item in &expr_cfg_if.then_branch { self.visit_item(item); } self.state = initial_state; - if let Some(else_branch) = &cfg_expr_if.else_branch { + 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(cfg_expr_if) => self.visit_cfg_expr_if(&cfg_expr_if), + ExprCfgElse::If(expr_cfg_if) => self.visit_expr_cfg_if(&expr_cfg_if), } } self.state = initial_state; @@ -222,19 +219,7 @@ 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("cfg") - && !(meta_str.contains("target_endian") || meta_str.contains("target_arch")) - { - self.error( - "cfg_if! over #[cfg]".to_string(), - span, - String::new(), - ( - None, - "use cfg_if! and submodules instead of #[cfg]".to_string(), - ), - ); - } else if meta_list.path.is_ident("derive") + if meta_list.path.is_ident("derive") && (meta_str.contains("Copy") || meta_str.contains("Clone")) { self.error( @@ -287,18 +272,76 @@ impl<'ast> Visit<'ast> for StyleChecker { visit::visit_item_type(self, item_type); } + 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); + } + } + } 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()), + ); + } + } + } + } + } + } + + 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 cfg_expr_if: ExprCfgIf = mac + let expr_cfg_if: ExprCfgIf = mac .parse_body() .expect("cfg_if! should be parsed since it compiled"); - self.visit_cfg_expr_if(&cfg_expr_if); + self.visit_expr_cfg_if(&expr_cfg_if); } else { let new_state = if mac.path.is_ident("s") { - // FIXME: see StyleChecker::set_state - // self.s_macros += 1; + // 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 @@ -362,7 +405,11 @@ impl Parse for ExprCfgIf { let then_branch: Vec = { let mut items = Vec::new(); while !content.is_empty() { - items.push(content.parse()?); + let mut value = content.parse()?; + if let syn::Item::Macro(item_macro) = &mut value { + item_macro.attrs.push(attr.clone()); + } + items.push(value); } items }; diff --git a/libc-test/test/style_tests.rs b/libc-test/test/style_tests.rs index 7a19afd0c4861..e42f1d704aa66 100644 --- a/libc-test/test/style_tests.rs +++ b/libc-test/test/style_tests.rs @@ -5,7 +5,7 @@ use style::StyleChecker; pub mod style; #[test] -fn correct_module_layout() { +fn accept_correct_module_layout() { let contents = r#" use core::mem::size_of; pub type foo_t = u32; @@ -24,7 +24,7 @@ pub use self::foolib::*; } #[test] -fn incorrect_module_layout() { +fn reject_incorrect_module_layout() { let contents = r#" use core::mem::size_of; pub type foo_t = u32; @@ -43,7 +43,7 @@ pub use self::foolib::*; } #[test] -fn incorrect_cfg_if_layout() { +fn reject_incorrect_cfg_if_layout() { let contents = r#" cfg_if! { if #[cfg(foo)] { @@ -60,7 +60,7 @@ cfg_if! { } #[test] -fn cfg_if_branch_resets_state() { +fn accept_cfg_if_branch_resets_state() { let contents = r#" cfg_if! { if #[cfg(foo)] { @@ -79,7 +79,7 @@ cfg_if! { } #[test] -fn multiple_f_macros() { +fn reject_multiple_f_macros() { let contents = r#" f! {} f! {} @@ -92,9 +92,24 @@ f! {} } #[test] -fn cfg_if_over_cfg() { +fn cfg_ignore_target_endian_nested() { let contents = r#" -#[cfg(foo)] +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 reject_manual_copy() { + let contents = r#" +#[derive(Copy)] pub struct Foo {} "# .to_string(); @@ -105,23 +120,32 @@ pub struct Foo {} } #[test] -fn cfg_if_ignore_target_arch() { +fn reject_manual_clone() { let contents = r#" -#[cfg(target_arch = "x86")] +#[derive(Clone)] pub struct Foo {} "# .to_string(); let mut checker = StyleChecker::new(); checker.check_string(contents).unwrap(); - checker.finalize().unwrap(); + assert!(checker.finalize().is_err()); } #[test] -fn cfg_if_ignore_target_endian_nested() { +fn accept_multiple_s_macros_with_disjoint_cfg() { let contents = r#" -#[cfg(all(target_endian = "little"))] -pub struct Foo {} +// 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(); @@ -131,10 +155,10 @@ pub struct Foo {} } #[test] -fn manual_copy() { +fn reject_duplicated_s_macro() { let contents = r#" -#[derive(Copy)] -pub struct Foo {} +s! {} +s! {} "# .to_string(); @@ -144,10 +168,89 @@ pub struct Foo {} } #[test] -fn manual_clone() { +fn reject_duplicated_s_macro_cfg() { let contents = r#" -#[derive(Clone)] -pub struct Foo {} +#[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 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 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 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 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 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();