Don't walk the hir unnecessarily let the visitor do it

DevinR528 · DevinR528 · commit 237aef42ffd1 · 2021-06-08T16:38:04.000-04:00
diff --git a/clippy_lints/src/disallowed_type.rs b/clippy_lints/src/disallowed_type.rs
@@ -1,17 +1,13 @@
 use clippy_utils::diagnostics::span_lint;
 
 use rustc_data_structures::fx::FxHashSet;
-use rustc_hir::{
-    def::Res, FnRetTy, GenericArg, GenericArgs, GenericBound, GenericParamKind, Item, ItemKind, Path, PolyTraitRef, Ty,
-    TyKind, TypeBindingKind, UseKind,
-};
+use rustc_hir::{def::Res, Item, ItemKind, PolyTraitRef, TraitBoundModifier, Ty, TyKind, UseKind};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::Symbol;
+use rustc_span::{Span, Symbol};
 
 declare_clippy_lint! {
-    /// **What it does:** Denies the configured types in clippy.toml
+    /// **What it does:** Denies the configured types in clippy.toml.
     ///
     /// **Why is this bad?** Some types are undesirable in certain contexts.
     ///
@@ -24,8 +20,16 @@ declare_clippy_lint! {
     /// `disallowed-methods = ["alloc::collections::btree::map::BTreeMap"]` and not
     /// `disallowed-methods = ["std::collections::BTreeMap"]` as you might expect.
     ///
+    /// N.B. There is no way to ban primitive types.
+    ///
     /// **Example:**
     ///
+    /// An example clippy.toml configuration:
+    /// ```toml
+    /// # clippy.toml
+    /// disallowed-methods = ["alloc::collections::btree::map::BTreeMap"]
+    /// ```
+    ///
     /// ```rust,ignore
     /// use std::collections::BTreeMap;
     /// // or its use
@@ -58,198 +62,61 @@ impl DisallowedType {
 
 impl_lint_pass!(DisallowedType => [DISALLOWED_TYPE]);
 
-impl LateLintPass<'_> for DisallowedType {
-    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+impl<'tcx> LateLintPass<'tcx> for DisallowedType {
+    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);
             then {
-                let name = name.iter()
-                    .map(|s| s.to_ident_string())
-                    .collect::<Vec<_>>()
-                    .join("::");
-                span_lint(
-                    cx,
-                    DISALLOWED_TYPE,
-                    item.span,
-                    &format!("`{}` is not allowed according to config", name),
-                );
+                emit(cx, name, item.span,);
             }
         }
     }
 
-    fn check_ty(&mut self, cx: &LateContext<'_>, ty: &Ty<'_>) {
-        walk_to_path(cx, ty, &self.disallowed);
-    }
-}
-
-fn walk_to_path(cx: &LateContext<'_>, ty: &Ty<'_>, disallowed: &FxHashSet<Vec<Symbol>>) {
-    match &ty.kind {
-        TyKind::Slice(t) => walk_to_path(cx, t, disallowed),
-        TyKind::Array(t, c) => {
-            walk_to_path(cx, t, disallowed);
-            // TODO: a bit hacky
-            walk_to_path(
-                cx,
-                &Ty {
-                    hir_id: ty.hir_id,
-                    kind: TyKind::Typeof(*c),
-                    span: ty.span,
-                },
-                disallowed,
-            );
-        },
-        TyKind::Ptr(mutable) | TyKind::Rptr(_, mutable) => walk_to_path(cx, mutable.ty, disallowed),
-        TyKind::BareFn(barefn) => {
-            for input in barefn.decl.inputs {
-                walk_to_path(cx, input, disallowed);
-            }
-            if let FnRetTy::Return(t) = barefn.decl.output {
-                walk_to_path(cx, t, disallowed);
-            }
-        },
-        TyKind::Tup(tup) => {
-            for t in *tup {
-                walk_to_path(cx, t, disallowed);
-            }
-        },
-        TyKind::Path(path) => {
-            if_chain! {
-                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) = disallowed.iter().find(|path| **path == use_path);
-                then {
-                    let name = name.iter()
-                        .map(|s| s.to_ident_string())
-                        .collect::<Vec<_>>()
-                        .join("::");
-                    span_lint(
-                        cx,
-                        DISALLOWED_TYPE,
-                        path.span(),
-                        &format!("`{}` is not allowed according to config", name),
-                    );
-                }
-            }
-        },
-        TyKind::OpaqueDef(_, gen) => {
-            for arg in *gen {
-                if let GenericArg::Type(t) = arg {
-                    walk_to_path(cx, t, disallowed);
-                }
-            }
-        },
-        TyKind::TraitObject(polyref, _, _) => walk_trait_obj(cx, &polyref.iter().collect::<Vec<_>>(), disallowed),
-        TyKind::Typeof(anonconst) => {
-            if_chain! {
-                if let Some(tyck) = cx.maybe_typeck_results();
-                if let Some(var_ty) = tyck.node_type_opt(anonconst.hir_id);
-                if let ty::Adt(ty::AdtDef { did, .. }, _) = var_ty.kind();
-                let use_path = cx.get_def_path(*did);
-                if let Some(name) = disallowed.iter().find(|path| **path == use_path);
-                then {
-                    let name = name.iter()
-                        .map(|s| s.to_ident_string())
-                        .collect::<Vec<_>>()
-                        .join("::");
-                    span_lint(
-                        cx,
-                        DISALLOWED_TYPE,
-                        ty.span,
-                        &format!("`{}` is not allowed according to config", name),
-                    );
-                }
+    fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) {
+        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);
+            then {
+                emit(cx, name, path.span());
             }
-        },
-        TyKind::Infer | TyKind::Never | TyKind::Err => {},
-    }
-}
-
-fn walk_path_segments(cx: &LateContext<'_>, path: &Path<'_>, disallowed: &FxHashSet<Vec<Symbol>>) {
-    for arg in path.segments.iter().filter_map(|s| s.args) {
-        walk_gen_args(cx, arg, disallowed);
+        }
     }
-}
 
-fn walk_trait_obj(cx: &LateContext<'_>, poly: &[&PolyTraitRef<'_>], disallowed: &FxHashSet<Vec<Symbol>>) {
-    for poly in poly {
+    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) = disallowed.iter().find(|path| **path == use_path);
+            if let Some(name) = self.disallowed.iter().find(|path| **path == use_path);
             then {
-                let name = name.iter()
-                    .map(|s| s.to_ident_string())
-                    .collect::<Vec<_>>()
-                    .join("::");
-                span_lint(
-                    cx,
-                    DISALLOWED_TYPE,
-                    poly.trait_ref.path.span,
-                    &format!("`{}` is not allowed according to config", name),
-                );
-            }
-        }
-        walk_path_segments(cx, poly.trait_ref.path, disallowed);
-
-        for param in poly.bound_generic_params {
-            walk_gen_bounds(cx, param.bounds, disallowed);
-            match param.kind {
-                GenericParamKind::Type { default: Some(ty), .. } => walk_to_path(cx, ty, disallowed),
-                GenericParamKind::Const {
-                    ty,
-                    default: Some(anon),
-                } => {
-                    walk_to_path(cx, ty, disallowed);
-                    walk_to_path(
-                        cx,
-                        &Ty {
-                            hir_id: ty.hir_id,
-                            kind: TyKind::Typeof(anon),
-                            span: ty.span,
-                        },
-                        disallowed,
-                    );
-                },
-                _ => {},
+                emit(cx, name, poly.trait_ref.path.span);
             }
         }
     }
-}
 
-fn walk_gen_args(cx: &LateContext<'_>, gen_args: &GenericArgs<'_>, disallowed: &FxHashSet<Vec<Symbol>>) {
-    for arg in gen_args.args {
-        match arg {
-            GenericArg::Type(t) => walk_to_path(cx, t, disallowed),
-            GenericArg::Const(c) => walk_to_path(
-                cx,
-                &Ty {
-                    hir_id: c.value.hir_id,
-                    kind: TyKind::Typeof(c.value),
-                    span: c.span,
-                },
-                disallowed,
-            ),
-            GenericArg::Lifetime(_) => {},
-        }
-    }
-    for bind in gen_args.bindings {
-        match bind.kind {
-            TypeBindingKind::Equality { ty } => walk_to_path(cx, ty, disallowed),
-            TypeBindingKind::Constraint { bounds } => walk_gen_bounds(cx, bounds, disallowed),
-        }
-        walk_gen_args(cx, bind.gen_args, disallowed);
-    }
+    // TODO: if non primitive const generics are a thing
+    // fn check_generic_arg(&mut self, cx: &LateContext<'tcx>, arg: &'tcx GenericArg<'tcx>) {
+    //     match arg {
+    //         GenericArg::Const(c) => {},
+    //     }
+    // }
+    // fn check_generic_param(&mut self, cx: &LateContext<'tcx>, param: &'tcx GenericParam<'tcx>) {
+    //     match param.kind {
+    //         GenericParamKind::Const { .. } => {},
+    //     }
+    // }
 }
 
-fn walk_gen_bounds(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], disallowed: &FxHashSet<Vec<Symbol>>) {
-    for b in bounds {
-        match b {
-            GenericBound::Trait(poly, _) => walk_trait_obj(cx, &[poly], disallowed),
-            GenericBound::LangItemTrait(_, _, _, gen) => walk_gen_args(cx, gen, disallowed),
-            GenericBound::Outlives(_) => {},
-        }
-    }
+fn emit(cx: &LateContext<'_>, name: &[Symbol], span: Span) {
+    let name = name.iter().map(|s| s.to_ident_string()).collect::<Vec<_>>().join("::");
+    span_lint(
+        cx,
+        DISALLOWED_TYPE,
+        span,
+        &format!("`{}` is not allowed according to config", name),
+    );
 }
diff --git a/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.rs b/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.rs
@@ -21,14 +21,15 @@ fn trait_obj(_: &dyn std::io::Read) {
     todo!()
 }
 
+static BAD: foo::atomic::AtomicPtr<()> = foo::atomic::AtomicPtr::new(std::ptr::null_mut());
+
 #[allow(clippy::diverging_sub_expression)]
 fn main() {
     let _: std::collections::HashMap<(), ()> = std::collections::HashMap::new();
     let _ = Sneaky::now();
     let _ = foo::atomic::AtomicU32::new(0);
     static FOO: std::sync::atomic::AtomicU32 = foo::atomic::AtomicU32::new(1);
     let _: std::collections::BTreeMap<(), syn::TypePath> = Default::default();
-    // diverging_sub_expression ?
     let _ = syn::Ident::new("", todo!());
     let _ = HashMap;
 }
diff --git a/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.stderr b/tests/ui-toml/toml_disallowed_type/conf_disallowed_type.stderr
@@ -37,49 +37,49 @@ LL | fn trait_obj(_: &dyn std::io::Read) {
    |                      ^^^^^^^^^^^^^
 
 error: `std::collections::hash::map::HashMap` is not allowed according to config
-  --> $DIR/conf_disallowed_type.rs:26:48
+  --> $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
-  --> $DIR/conf_disallowed_type.rs:26:12
+  --> $DIR/conf_disallowed_type.rs:28:12
    |
 LL |     let _: std::collections::HashMap<(), ()> = std::collections::HashMap::new();
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: `std::time::Instant` is not allowed according to config
-  --> $DIR/conf_disallowed_type.rs:27:13
+  --> $DIR/conf_disallowed_type.rs:29:13
    |
 LL |     let _ = Sneaky::now();
    |             ^^^^^^
 
 error: `core::sync::atomic::AtomicU32` is not allowed according to config
-  --> $DIR/conf_disallowed_type.rs:28:13
+  --> $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
-  --> $DIR/conf_disallowed_type.rs:29:17
+  --> $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
-  --> $DIR/conf_disallowed_type.rs:29:48
+  --> $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
-  --> $DIR/conf_disallowed_type.rs:30:43
+  --> $DIR/conf_disallowed_type.rs:32:43
    |
 LL |     let _: std::collections::BTreeMap<(), syn::TypePath> = Default::default();
    |                                           ^^^^^^^^^^^^^
 
 error: `proc_macro2::Ident` is not allowed according to config
-  --> $DIR/conf_disallowed_type.rs:32:13
+  --> $DIR/conf_disallowed_type.rs:33:13
    |
 LL |     let _ = syn::Ident::new("", todo!());
    |             ^^^^^^^^^^
