Auto merge of #7345 - DevinR528:disallowed-fix, r=Manishearth

Remove requirement of fully qualified path for disallowed_method/type changelog: Remove the need for fully qualified paths in disallowed_method and disallowed_type After fixing this issue in [import_rename](#7300 (comment)) I figured I'd fix it for these two. If this does in fact fix the **Known problems:** section I was planning to remove them from both lints after confirmation.
rust-lang · Jun 14, 2021 · 2d1e9ab · 2d1e9ab
2 parents a36a7c8 + d4eff81
commit 2d1e9ab
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 41 deletions.
diff --git a/clippy_lints/src/disallowed_method.rs b/clippy_lints/src/disallowed_method.rs
@@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint;
 use clippy_utils::fn_def_id;
 
 use rustc_data_structures::fx::FxHashSet;
-use rustc_hir::Expr;
+use rustc_hir::{def::Res, def_id::DefId, Crate, Expr};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::Symbol;
@@ -13,21 +13,14 @@ declare_clippy_lint! {
     /// **Why is this bad?** Some methods are undesirable in certain contexts,
     /// and it's beneficial to lint for them as needed.
     ///
-    /// **Known problems:** Currently, you must write each function as a
-    /// fully-qualified path. This lint doesn't support aliases or reexported
-    /// names; be aware that many types in `std` are actually reexports.
-    ///
-    /// For example, if you want to disallow `Duration::as_secs`, your clippy.toml
-    /// configuration would look like
-    /// `disallowed-methods = ["core::time::Duration::as_secs"]` and not
-    /// `disallowed-methods = ["std::time::Duration::as_secs"]` as you might expect.
+    /// **Known problems:** None.
     ///
     /// **Example:**
     ///
     /// An example clippy.toml configuration:
     /// ```toml
     /// # clippy.toml
-    /// disallowed-methods = ["alloc::vec::Vec::leak", "std::time::Instant::now"]
+    /// disallowed-methods = ["std::vec::Vec::leak", "std::time::Instant::now"]
     /// ```
     ///
     /// ```rust,ignore
@@ -52,6 +45,7 @@ declare_clippy_lint! {
 #[derive(Clone, Debug)]
 pub struct DisallowedMethod {
     disallowed: FxHashSet<Vec<Symbol>>,
+    def_ids: FxHashSet<(DefId, Vec<Symbol>)>,
 }
 
 impl DisallowedMethod {
@@ -61,17 +55,28 @@ impl DisallowedMethod {
                 .iter()
                 .map(|s| s.split("::").map(|seg| Symbol::intern(seg)).collect::<Vec<_>>())
                 .collect(),
+            def_ids: FxHashSet::default(),
         }
     }
 }
 
 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()));
+            }
+        }
+    }
+
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if let Some(def_id) = fn_def_id(cx, expr) {
-            let func_path = cx.get_def_path(def_id);
-            if self.disallowed.contains(&func_path) {
+            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)

diff --git a/clippy_lints/src/disallowed_type.rs b/clippy_lints/src/disallowed_type.rs
@@ -1,7 +1,9 @@
 use clippy_utils::diagnostics::span_lint;
 
 use rustc_data_structures::fx::FxHashSet;
-use rustc_hir::{def::Res, Item, ItemKind, PolyTraitRef, TraitBoundModifier, Ty, TyKind, UseKind};
+use rustc_hir::{
+    def::Res, def_id::DefId, Crate, Item, ItemKind, PolyTraitRef, TraitBoundModifier, Ty, TyKind, UseKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{Span, Symbol};
@@ -11,14 +13,7 @@ declare_clippy_lint! {
     ///
     /// **Why is this bad?** Some types are undesirable in certain contexts.
     ///
-    /// **Known problems:** The fully qualified path must be used. This lint
-    /// doesn't support aliases or reexported names; be aware that many types
-    /// in `std` are actually reexports.
-    ///
-    /// For example, if you want to disallow `BTreeMap`, your clippy.toml
-    /// configuration would look like
-    /// `disallowed-methods = ["alloc::collections::btree::map::BTreeMap"]` and not
-    /// `disallowed-methods = ["std::collections::BTreeMap"]` as you might expect.
+    /// **Known problems:** None.
     ///
     /// N.B. There is no way to ban primitive types.
     ///
@@ -27,7 +22,7 @@ declare_clippy_lint! {
     /// An example clippy.toml configuration:
     /// ```toml
     /// # clippy.toml
-    /// disallowed-methods = ["alloc::collections::btree::map::BTreeMap"]
+    /// disallowed-methods = ["std::collections::BTreeMap"]
     /// ```
     ///
     /// ```rust,ignore
@@ -47,6 +42,7 @@ declare_clippy_lint! {
 #[derive(Clone, Debug)]
 pub struct DisallowedType {
     disallowed: FxHashSet<Vec<Symbol>>,
+    def_ids: FxHashSet<(DefId, Vec<Symbol>)>,
 }
 
 impl DisallowedType {
@@ -56,19 +52,29 @@ impl DisallowedType {
                 .iter()
                 .map(|s| s.split("::").map(|seg| Symbol::intern(seg)).collect::<Vec<_>>())
                 .collect(),
+            def_ids: FxHashSet::default(),
         }
     }
 }
 
 impl_lint_pass!(DisallowedType => [DISALLOWED_TYPE]);
 
 impl<'tcx> LateLintPass<'tcx> for DisallowedType {
+    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()));
+            }
+        }
+    }
+
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
         if_chain! {
             if let ItemKind::Use(path, UseKind::Single) = &item.kind;
-            if let Res::Def(_, id) = path.res;
-            let use_path = cx.get_def_path(id);
-            if let Some(name) = self.disallowed.iter().find(|path| **path == use_path);
+            if let Res::Def(_, did) = path.res;
+            if let Some((_, name)) = self.def_ids.iter().find(|(id, _)| *id == did);
             then {
                 emit(cx, name, item.span,);
             }
@@ -79,8 +85,7 @@ impl<'tcx> LateLintPass<'tcx> for DisallowedType {
         if_chain! {
             if let TyKind::Path(path) = &ty.kind;
             if let Some(did) = cx.qpath_res(path, ty.hir_id).opt_def_id();
-            let use_path = cx.get_def_path(did);
-            if let Some(name) = self.disallowed.iter().find(|path| **path == use_path);
+            if let Some((_, name)) = self.def_ids.iter().find(|(id, _)| *id == did);
             then {
                 emit(cx, name, path.span());
             }
@@ -90,8 +95,7 @@ impl<'tcx> LateLintPass<'tcx> for DisallowedType {
     fn check_poly_trait_ref(&mut self, cx: &LateContext<'tcx>, poly: &'tcx PolyTraitRef<'tcx>, _: TraitBoundModifier) {
         if_chain! {
             if let Res::Def(_, did) = poly.trait_ref.path.res;
-            let use_path = cx.get_def_path(did);
-            if let Some(name) = self.disallowed.iter().find(|path| **path == use_path);
+            if let Some((_, name)) = self.def_ids.iter().find(|(id, _)| *id == did);
             then {
                 emit(cx, name, poly.trait_ref.path.span);
             }

diff --git a/tests/ui-toml/toml_disallowed_method/clippy.toml b/tests/ui-toml/toml_disallowed_method/clippy.toml
@@ -1 +1,5 @@
-disallowed-methods = ["core::iter::traits::iterator::Iterator::sum", "regex::re_unicode::Regex::is_match", "regex::re_unicode::Regex::new"]
+disallowed-methods = [
+    "std::iter::Iterator::sum",
+    "regex::Regex::is_match",
+    "regex::Regex::new"
+]
diff --git a/tests/ui-toml/toml_disallowed_type/clippy.toml b/tests/ui-toml/toml_disallowed_type/clippy.toml
@@ -1,7 +1,7 @@
 disallowed-types = [
-    "std::collections::hash::map::HashMap",
-    "core::sync::atomic::AtomicU32",
-    "syn::ty::TypePath",
+    "std::collections::HashMap",
+    "std::sync::atomic::AtomicU32",
+    "syn::TypePath",
     "proc_macro2::Ident",
     "std::thread::Thread",
     "std::time::Instant",

diff --git a/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.stderr b/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.stderr
@@ -1,4 +1,4 @@
-error: `core::sync::atomic::AtomicU32` is not allowed according to config
+error: `std::sync::atomic::AtomicU32` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:7:1
    |
 LL | use std::sync::atomic::AtomicU32;
@@ -24,7 +24,7 @@ error: `std::time::Instant` is not allowed according to config
 LL | fn bad_arg_type(_: impl Fn(Sneaky) -> foo::atomic::AtomicU32) {
    |                            ^^^^^^
 
-error: `core::sync::atomic::AtomicU32` is not allowed according to config
+error: `std::sync::atomic::AtomicU32` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:16:39
    |
 LL | fn bad_arg_type(_: impl Fn(Sneaky) -> foo::atomic::AtomicU32) {
@@ -36,13 +36,13 @@ error: `std::io::Read` is not allowed according to config
 LL | fn trait_obj(_: &dyn std::io::Read) {
    |                      ^^^^^^^^^^^^^
 
-error: `std::collections::hash::map::HashMap` is not allowed according to config
+error: `std::collections::HashMap` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:28:48
    |
 LL |     let _: std::collections::HashMap<(), ()> = std::collections::HashMap::new();
    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: `std::collections::hash::map::HashMap` is not allowed according to config
+error: `std::collections::HashMap` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:28:12
    |
 LL |     let _: std::collections::HashMap<(), ()> = std::collections::HashMap::new();
@@ -54,25 +54,25 @@ error: `std::time::Instant` is not allowed according to config
 LL |     let _ = Sneaky::now();
    |             ^^^^^^
 
-error: `core::sync::atomic::AtomicU32` is not allowed according to config
+error: `std::sync::atomic::AtomicU32` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:30:13
    |
 LL |     let _ = foo::atomic::AtomicU32::new(0);
    |             ^^^^^^^^^^^^^^^^^^^^^^
 
-error: `core::sync::atomic::AtomicU32` is not allowed according to config
+error: `std::sync::atomic::AtomicU32` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:31:17
    |
 LL |     static FOO: std::sync::atomic::AtomicU32 = foo::atomic::AtomicU32::new(1);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: `core::sync::atomic::AtomicU32` is not allowed according to config
+error: `std::sync::atomic::AtomicU32` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:31:48
    |
 LL |     static FOO: std::sync::atomic::AtomicU32 = foo::atomic::AtomicU32::new(1);
    |                                                ^^^^^^^^^^^^^^^^^^^^^^
 
-error: `syn::ty::TypePath` is not allowed according to config
+error: `syn::TypePath` is not allowed according to config
   --> $DIR/conf_disallowed_type.rs:32:43
    |
 LL |     let _: std::collections::BTreeMap<(), syn::TypePath> = Default::default();