add updated lint rules for s! macros

Author: rmehri01

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<Span>,
-    // FIXME: see StyleChecker::set_state
-    _s_macros: usize,
+    seen_s_macro_cfgs: HashMap<String, Span>,
     /// Span of the first f! macro seen, used to enforce only one f! macro
     /// per module.
     first_f_macro: Option<Span>,
@@ -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<syn::Item> = {
             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
         };