Add macro_braces lint to check for irregular brace use in certain macros

CHANGELOG.md

@@ -2575,6 +2575,7 @@ Released 2018-09-13
 [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
 [`nonminimal_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
 [`nonsensical_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonsensical_open_options
+[`nonstandard_macro_braces`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonstandard_macro_braces
 [`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
 [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref

clippy_lints/src/lib.rs

@@ -293,6 +293,7 @@ mod no_effect;
 mod non_copy_const;
 mod non_expressive_names;
 mod non_octal_unix_permissions;
+mod nonstandard_macro_braces;
 mod open_options;
 mod option_env_unwrap;
 mod option_if_let_else;
@@ -844,6 +845,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         non_expressive_names::MANY_SINGLE_CHAR_NAMES,
         non_expressive_names::SIMILAR_NAMES,
         non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS,
+        nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         option_env_unwrap::OPTION_ENV_UNWRAP,
         option_if_let_else::OPTION_IF_LET_ELSE,
@@ -1360,6 +1362,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
         LintId::of(non_expressive_names::MANY_SINGLE_CHAR_NAMES),
         LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS),
+        LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES),
         LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS),
         LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP),
         LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
@@ -1536,6 +1539,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
         LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
         LintId::of(non_expressive_names::MANY_SINGLE_CHAR_NAMES),
+        LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES),
         LintId::of(ptr::CMP_NULL),
         LintId::of(ptr::PTR_ARG),
         LintId::of(ptr_eq::PTR_EQ),
@@ -2040,6 +2044,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(move || box non_expressive_names::NonExpressiveNames {
         single_char_binding_names_threshold,
     });
+    let macro_matcher = conf.standard_macro_braces.iter().cloned().collect::<FxHashSet<_>>();
+    store.register_early_pass(move || box nonstandard_macro_braces::MacroBraces::new(&macro_matcher));
     store.register_late_pass(|| box macro_use::MacroUseImports::default());
     store.register_late_pass(|| box pattern_type_mismatch::PatternTypeMismatch);
     store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive);

clippy_lints/src/nonstandard_macro_braces.rs

@@ -0,0 +1,276 @@
+use std::{
+    fmt,
+    hash::{Hash, Hasher},
+};
+
+use clippy_utils::{diagnostics::span_lint_and_help, in_macro, is_direct_expn_of, source::snippet_opt};
+use if_chain::if_chain;
+use rustc_ast::ast;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::Span;
+use serde::{de, Deserialize};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks that common macros are used with consistent bracing.
+    ///
+    /// **Why is this bad?** This is mostly a consistency lint although using () or []
+    /// doesn't give you a semicolon in item position, which can be unexpected.
+    ///
+    /// **Known problems:**
+    /// None
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// vec!{1, 2, 3};
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// vec![1, 2, 3];
+    /// ```
+    pub NONSTANDARD_MACRO_BRACES,
+    style,
+    "check consistent use of braces in macro"
+}
+
+const BRACES: &[(&str, &str)] = &[("(", ")"), ("{", "}"), ("[", "]")];
+
+/// The (name, (open brace, close brace), source snippet)
+type MacroInfo<'a> = (&'a str, &'a (String, String), String);
+
+#[derive(Clone, Debug, Default)]
+pub struct MacroBraces {
+    macro_braces: FxHashMap<String, (String, String)>,
+    done: FxHashSet<Span>,
+}
+
+impl MacroBraces {
+    pub fn new(conf: &FxHashSet<MacroMatcher>) -> Self {
+        let macro_braces = macro_braces(conf.clone());
+        Self {
+            macro_braces,
+            done: FxHashSet::default(),
+        }
+    }
+}
+
+impl_lint_pass!(MacroBraces => [NONSTANDARD_MACRO_BRACES]);
+
+impl EarlyLintPass for MacroBraces {
+    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
+        if let Some((name, braces, snip)) = is_offending_macro(cx, item.span, self) {
+            let span = item.span.ctxt().outer_expn_data().call_site;
+            emit_help(cx, snip, braces, name, span);
+            self.done.insert(span);
+        }
+    }
+
+    fn check_stmt(&mut self, cx: &EarlyContext<'_>, stmt: &ast::Stmt) {
+        if let Some((name, braces, snip)) = is_offending_macro(cx, stmt.span, self) {
+            let span = stmt.span.ctxt().outer_expn_data().call_site;
+            emit_help(cx, snip, braces, name, span);
+            self.done.insert(span);
+        }
+    }
+
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
+        if let Some((name, braces, snip)) = is_offending_macro(cx, expr.span, self) {
+            let span = expr.span.ctxt().outer_expn_data().call_site;
+            emit_help(cx, snip, braces, name, span);
+            self.done.insert(span);
+        }
+    }
+
+    fn check_ty(&mut self, cx: &EarlyContext<'_>, ty: &ast::Ty) {
+        if let Some((name, braces, snip)) = is_offending_macro(cx, ty.span, self) {
+            let span = ty.span.ctxt().outer_expn_data().call_site;
+            emit_help(cx, snip, braces, name, span);
+            self.done.insert(span);
+        }
+    }
+}
+
+fn is_offending_macro<'a>(cx: &EarlyContext<'_>, span: Span, this: &'a MacroBraces) -> Option<MacroInfo<'a>> {
+    if_chain! {
+        if in_macro(span);
+        if let Some((name, braces)) = find_matching_macro(span, &this.macro_braces);
+        if let Some(snip) = snippet_opt(cx, span.ctxt().outer_expn_data().call_site);
+        let c = snip.replace(" ", ""); // make formatting consistent
+        if !c.starts_with(&format!("{}!{}", name, braces.0));
+        if !this.done.contains(&span.ctxt().outer_expn_data().call_site);
+        then {
+            Some((name, braces, snip))
+        } else {
+            None
+        }
+    }
+}
+
+fn emit_help(cx: &EarlyContext<'_>, snip: String, braces: &(String, String), name: &str, span: Span) {
+    let with_space = &format!("! {}", braces.0);
+    let without_space = &format!("!{}", braces.0);
+    let mut help = snip;
+    for b in BRACES.iter().filter(|b| b.0 != braces.0) {
+        help = help.replace(b.0, &braces.0).replace(b.1, &braces.1);
+        // Only `{` traditionally has space before the brace
+        if braces.0 != "{" && help.contains(with_space) {
+            help = help.replace(with_space, without_space);
+        } else if braces.0 == "{" && help.contains(without_space) {
+            help = help.replace(without_space, with_space);
+        }
+    }
+    span_lint_and_help(
+        cx,
+        NONSTANDARD_MACRO_BRACES,
+        span,
+        &format!("use of irregular braces for `{}!` macro", name),
+        Some(span),
+        &format!("consider writing `{}`", help),
+    );
+}
+
+fn find_matching_macro(
+    span: Span,
+    braces: &FxHashMap<String, (String, String)>,
+) -> Option<(&String, &(String, String))> {
+    braces
+        .iter()
+        .find(|(macro_name, _)| is_direct_expn_of(span, macro_name).is_some())
+}
+
+fn macro_braces(conf: FxHashSet<MacroMatcher>) -> FxHashMap<String, (String, String)> {
+    let mut braces = vec![
+        macro_matcher!(
+            name: "print",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "println",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "eprint",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "eprintln",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "write",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "writeln",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "format",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "format_args",
+            braces: ("(", ")"),
+        ),
+        macro_matcher!(
+            name: "vec",
+            braces: ("[", "]"),
+        ),
+    ]
+    .into_iter()
+    .collect::<FxHashMap<_, _>>();
+    // We want users items to override any existing items
+    for it in conf {
+        braces.insert(it.name, it.braces);
+    }
+    braces
+}
+
+macro_rules! macro_matcher {
+    (name: $name:expr, braces: ($open:expr, $close:expr) $(,)?) => {
+        ($name.to_owned(), ($open.to_owned(), $close.to_owned()))
+    };
+}
+pub(crate) use macro_matcher;
+
+#[derive(Clone, Debug)]
+pub struct MacroMatcher {
+    name: String,
+    braces: (String, String),
+}
+
+impl Hash for MacroMatcher {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.name.hash(state);
+    }
+}
+
+impl PartialEq for MacroMatcher {
+    fn eq(&self, other: &Self) -> bool {
+        self.name == other.name
+    }
+}
+impl Eq for MacroMatcher {}
+
+impl<'de> Deserialize<'de> for MacroMatcher {
+    fn deserialize<D>(deser: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        #[serde(field_identifier, rename_all = "lowercase")]
+        enum Field {
+            Name,
+            Brace,
+        }
+        struct MacVisitor;
+        impl<'de> de::Visitor<'de> for MacVisitor {
+            type Value = MacroMatcher;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+                formatter.write_str("struct MacroMatcher")
+            }
+
+            fn visit_map<V>(self, mut map: V) -> Result<Self::Value, V::Error>
+            where
+                V: de::MapAccess<'de>,
+            {
+                let mut name = None;
+                let mut brace: Option<&str> = None;
+                while let Some(key) = map.next_key()? {
+                    match key {
+                        Field::Name => {
+                            if name.is_some() {
+                                return Err(de::Error::duplicate_field("name"));
+                            }
+                            name = Some(map.next_value()?);
+                        },
+                        Field::Brace => {
+                            if brace.is_some() {
+                                return Err(de::Error::duplicate_field("brace"));
+                            }
+                            brace = Some(map.next_value()?);
+                        },
+                    }
+                }
+                let name = name.ok_or_else(|| de::Error::missing_field("name"))?;
+                let brace = brace.ok_or_else(|| de::Error::missing_field("brace"))?;
+                Ok(MacroMatcher {
+                    name,
+                    braces: BRACES
+                        .iter()
+                        .find(|b| b.0 == brace)
+                        .map(|(o, c)| ((*o).to_owned(), (*c).to_owned()))
+                        .ok_or_else(|| {
+                            de::Error::custom(&format!("expected one of `(`, `{{`, `[` found `{}`", brace))
+                        })?,
+                })
+            }
+        }
+
+        const FIELDS: &[&str] = &["name", "brace"];
+        deser.deserialize_struct("MacroMatcher", FIELDS, MacVisitor)
+    }
+}

clippy_lints/src/utils/conf.rs

@@ -26,13 +26,13 @@ impl TryConf {
 
 macro_rules! define_Conf {
     ($(
-        #[doc = $doc:literal]
+        $(#[doc = $doc:literal])*
         $(#[conf_deprecated($dep:literal)])?
         ($name:ident: $ty:ty = $default:expr),
     )*) => {
         /// Clippy lint configuration
         pub struct Conf {
-            $(#[doc = $doc] pub $name: $ty,)*
+            $($(#[doc = $doc])* pub $name: $ty,)*
         }
 
         mod defaults {
@@ -109,7 +109,7 @@ macro_rules! define_Conf {
                                 stringify!($name),
                                 stringify!($ty),
                                 format!("{:?}", super::defaults::$name()),
-                                $doc,
+                                concat!($($doc,)*),
                                 deprecation_reason,
                             )
                         },
@@ -182,9 +182,9 @@ define_Conf! {
     (vec_box_size_threshold: u64 = 4096),
     /// Lint: TYPE_REPETITION_IN_BOUNDS. The maximum number of bounds a trait can have to be linted
     (max_trait_bounds: u64 = 3),
-    /// Lint: STRUCT_EXCESSIVE_BOOLS. The maximum number of bools a struct can have
+    /// Lint: STRUCT_EXCESSIVE_BOOLS. The maximum number of bool fields a struct can have
     (max_struct_bools: u64 = 3),
-    /// Lint: FN_PARAMS_EXCESSIVE_BOOLS. The maximum number of bools function parameters can have
+    /// Lint: FN_PARAMS_EXCESSIVE_BOOLS. The maximum number of bool parameters a function can have
     (max_fn_params_bools: u64 = 3),
     /// Lint: WILDCARD_IMPORTS. Whether to allow certain wildcard imports (prelude, super in tests).
     (warn_on_all_wildcard_imports: bool = false),
@@ -198,6 +198,12 @@ define_Conf! {
     (upper_case_acronyms_aggressive: bool = false),
     /// Lint: _CARGO_COMMON_METADATA. For internal testing only, ignores the current `publish` settings in the Cargo manifest.
     (cargo_ignore_publish: bool = false),
+    /// Lint: NONSTANDARD_MACRO_BRACES. Enforce the named macros always use the braces specified.
+    ///
+    /// A `MacroMatcher` can be added like so `{ name = "macro_name", brace = "(" }`.
+    /// If the macro is could be used with a full path two `MacroMatcher`s have to be added one
+    /// with the full path `crate_name::macro_name` and one with just the macro name.
+    (standard_macro_braces: Vec<crate::nonstandard_macro_braces::MacroMatcher> = Vec::new()),
 }
 
 /// Search for the configuration file.

tests/ui-toml/nonstandard_macro_braces/clippy.toml

@@ -0,0 +1,6 @@
+standard-macro-braces = [
+    { name = "quote", brace = "{" },
+    { name = "quote::quote", brace = "{" },
+    { name = "eprint", brace = "[" },
+    { name = "type_pos", brace = "[" },
+]