Auto merge of rust-lang#7621 - azdavis:master, r=camsteffen

Allow giving reasons for `disallowed_methods`

Fixes rust-lang#7609.

This permits writing the config for `disallowed-methods` as either a list of strings (like before) or a list of tables, where each table gives the path to the disallowed method and an optional reason for why the method is disallowed.

changelog: Allow giving reasons for [`disallowed_methods`]

Author: bors

clippy_lints/src/disallowed_method.rs

@@ -1,33 +1,44 @@
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::fn_def_id;
 
-use rustc_data_structures::fx::FxHashSet;
-use rustc_hir::{def::Res, def_id::DefId, Crate, Expr};
+use rustc_hir::{def::Res, def_id::DefIdMap, Crate, Expr};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::Symbol;
+
+use crate::utils::conf;
 
 declare_clippy_lint! {
     /// ### What it does
     /// Denies the configured methods and functions in clippy.toml
     ///
     /// ### Why is this bad?
-    /// Some methods are undesirable in certain contexts,
-    /// and it's beneficial to lint for them as needed.
+    /// Some methods are undesirable in certain contexts, and it's beneficial to
+    /// lint for them as needed.
     ///
     /// ### Example
     /// An example clippy.toml configuration:
     /// ```toml
     /// # clippy.toml
-    /// disallowed-methods = ["std::vec::Vec::leak", "std::time::Instant::now"]
+    /// disallowed-methods = [
+    ///     # Can use a string as the path of the disallowed method.
+    ///     "std::boxed::Box::new",
+    ///     # Can also use an inline table with a `path` key.
+    ///     { path = "std::time::Instant::now" },
+    ///     # When using an inline table, can add a `reason` for why the method
+    ///     # is disallowed.
+    ///     { path = "std::vec::Vec::leak", reason = "no leaking memory" },
+    /// ]
     /// ```
     ///
     /// ```rust,ignore
     /// // Example code where clippy issues a warning
     /// let xs = vec![1, 2, 3, 4];
     /// xs.leak(); // Vec::leak is disallowed in the config.
+    /// // The diagnostic contains the message "no leaking memory".
     ///
     /// let _now = Instant::now(); // Instant::now is disallowed in the config.
+    ///
+    /// let _box = Box::new(3); // Box::new is disallowed in the config.
     /// ```
     ///
     /// Use instead:
@@ -43,18 +54,15 @@ declare_clippy_lint! {
 
 #[derive(Clone, Debug)]
 pub struct DisallowedMethod {
-    disallowed: FxHashSet<Vec<Symbol>>,
-    def_ids: FxHashSet<(DefId, Vec<Symbol>)>,
+    conf_disallowed: Vec<conf::DisallowedMethod>,
+    disallowed: DefIdMap<Option<String>>,
 }
 
 impl DisallowedMethod {
-    pub fn new(disallowed: &FxHashSet<String>) -> Self {
+    pub fn new(conf_disallowed: Vec<conf::DisallowedMethod>) -> Self {
         Self {
-            disallowed: disallowed
-                .iter()
-                .map(|s| s.split("::").map(|seg| Symbol::intern(seg)).collect::<Vec<_>>())
-                .collect(),
-            def_ids: FxHashSet::default(),
+            conf_disallowed,
+            disallowed: DefIdMap::default(),
         }
     }
 }
@@ -63,32 +71,36 @@ impl_lint_pass!(DisallowedMethod => [DISALLOWED_METHOD]);
 
 impl<'tcx> LateLintPass<'tcx> for DisallowedMethod {
     fn check_crate(&mut self, cx: &LateContext<'_>, _: &Crate<'_>) {
-        for path in &self.disallowed {
-            let segs = path.iter().map(ToString::to_string).collect::<Vec<_>>();
-            if let Res::Def(_, id) = clippy_utils::path_to_res(cx, &segs.iter().map(String::as_str).collect::<Vec<_>>())
-            {
-                self.def_ids.insert((id, path.clone()));
+        for conf in &self.conf_disallowed {
+            let (path, reason) = match conf {
+                conf::DisallowedMethod::Simple(path) => (path, None),
+                conf::DisallowedMethod::WithReason { path, reason } => (
+                    path,
+                    reason.as_ref().map(|reason| format!("{} (from clippy.toml)", reason)),
+                ),
+            };
+            let segs: Vec<_> = path.split("::").collect();
+            if let Res::Def(_, id) = clippy_utils::path_to_res(cx, &segs) {
+                self.disallowed.insert(id, reason);
             }
         }
     }
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if let Some(def_id) = fn_def_id(cx, expr) {
-            if self.def_ids.iter().any(|(id, _)| def_id == *id) {
-                let func_path = cx.get_def_path(def_id);
-                let func_path_string = func_path
-                    .into_iter()
-                    .map(Symbol::to_ident_string)
-                    .collect::<Vec<_>>()
-                    .join("::");
-
-                span_lint(
-                    cx,
-                    DISALLOWED_METHOD,
-                    expr.span,
-                    &format!("use of a disallowed method `{}`", func_path_string),
-                );
+        let def_id = match fn_def_id(cx, expr) {
+            Some(def_id) => def_id,
+            None => return,
+        };
+        let reason = match self.disallowed.get(&def_id) {
+            Some(reason) => reason,
+            None => return,
+        };
+        let func_path = cx.tcx.def_path_str(def_id);
+        let msg = format!("use of a disallowed method `{}`", func_path);
+        span_lint_and_then(cx, DISALLOWED_METHOD, expr.span, &msg, |diag| {
+            if let Some(reason) = reason {
+                diag.note(reason);
             }
-        }
+        });
     }
 }

clippy_lints/src/lib.rs

@@ -2105,8 +2105,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(float_equality_without_abs::FloatEqualityWithoutAbs));
     store.register_late_pass(|| Box::new(semicolon_if_nothing_returned::SemicolonIfNothingReturned));
     store.register_late_pass(|| Box::new(async_yields_async::AsyncYieldsAsync));
-    let disallowed_methods = conf.disallowed_methods.iter().cloned().collect::<FxHashSet<_>>();
-    store.register_late_pass(move || Box::new(disallowed_method::DisallowedMethod::new(&disallowed_methods)));
+    let disallowed_methods = conf.disallowed_methods.clone();
+    store.register_late_pass(move || Box::new(disallowed_method::DisallowedMethod::new(disallowed_methods.clone())));
     store.register_early_pass(|| Box::new(asm_syntax::InlineAsmX86AttSyntax));
     store.register_early_pass(|| Box::new(asm_syntax::InlineAsmX86IntelSyntax));
     store.register_late_pass(|| Box::new(undropped_manually_drops::UndroppedManuallyDrops));

clippy_lints/src/utils/conf.rs

@@ -15,6 +15,14 @@ pub struct Rename {
     pub rename: String,
 }
 
+/// A single disallowed method, used by the `DISALLOWED_METHOD` lint.
+#[derive(Clone, Debug, Deserialize)]
+#[serde(untagged)]
+pub enum DisallowedMethod {
+    Simple(String),
+    WithReason { path: String, reason: Option<String> },
+}
+
 /// Conf with parse errors
 #[derive(Default)]
 pub struct TryConf {
@@ -243,7 +251,7 @@ define_Conf! {
     /// Lint: DISALLOWED_METHOD.
     ///
     /// The list of disallowed methods, written as fully qualified paths.
-    (disallowed_methods: Vec<String> = Vec::new()),
+    (disallowed_methods: Vec<crate::utils::conf::DisallowedMethod> = Vec::new()),
     /// Lint: DISALLOWED_TYPE.
     ///
     /// The list of disallowed types, written as fully qualified paths.

tests/ui-toml/toml_disallowed_method/clippy.toml

@@ -1,5 +1,8 @@
 disallowed-methods = [
+    # just a string is shorthand for path only
     "std::iter::Iterator::sum",
-    "regex::Regex::is_match",
-    "regex::Regex::new"
+    # can give path and reason with an inline table
+    { path = "regex::Regex::is_match", reason = "no matching allowed" },
+    # can use an inline table but omit reason
+    { path = "regex::Regex::new" },
 ]

tests/ui-toml/toml_disallowed_method/conf_disallowed_method.stderr

@@ -1,18 +1,20 @@
-error: use of a disallowed method `regex::re_unicode::Regex::new`
+error: use of a disallowed method `regex::Regex::new`
   --> $DIR/conf_disallowed_method.rs:7:14
    |
 LL |     let re = Regex::new(r"ab.*c").unwrap();
    |              ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::disallowed-method` implied by `-D warnings`
 
-error: use of a disallowed method `regex::re_unicode::Regex::is_match`
+error: use of a disallowed method `regex::Regex::is_match`
   --> $DIR/conf_disallowed_method.rs:8:5
    |
 LL |     re.is_match("abc");
    |     ^^^^^^^^^^^^^^^^^^
+   |
+   = note: no matching allowed (from clippy.toml)
 
-error: use of a disallowed method `core::iter::traits::iterator::Iterator::sum`
+error: use of a disallowed method `std::iter::Iterator::sum`
   --> $DIR/conf_disallowed_method.rs:11:5
    |
 LL |     a.iter().sum::<i32>();