[`single_call_fn`]: merge post-crate visitor into lint pass

4 files changed, 141 insertions, 85 deletions

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 77b6775eb77..76e75968314 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -1068,7 +1068,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| {
         Box::new(single_call_fn::SingleCallFn {
             avoid_breaking_exported_api,
-            def_id_to_usage: rustc_data_structures::fx::FxHashMap::default(),
+            def_id_to_usage: rustc_data_structures::fx::FxIndexMap::default(),
         })
     });
     store.register_early_pass(move || {
diff --git a/clippy_lints/src/single_call_fn.rs b/clippy_lints/src/single_call_fn.rs
index 03877420774..223cbb3fae1 100644
--- a/clippy_lints/src/single_call_fn.rs
+++ b/clippy_lints/src/single_call_fn.rs
@@ -1,11 +1,10 @@
 use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::{is_from_proc_macro, is_in_test_function};
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxIndexMap, IndexEntry};
+use rustc_hir::def::DefKind;
 use rustc_hir::def_id::LocalDefId;
-use rustc_hir::intravisit::{walk_expr, FnKind, Visitor};
-use rustc_hir::{Body, Expr, ExprKind, FnDecl};
+use rustc_hir::{Expr, ExprKind, HirId, Node};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
-use rustc_middle::hir::nested_filter::OnlyBodies;
 use rustc_middle::lint::in_external_macro;
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
@@ -54,82 +53,93 @@ declare_clippy_lint! {
 }
 impl_lint_pass!(SingleCallFn => [SINGLE_CALL_FN]);
 
+#[derive(Debug, Clone)]
+pub enum CallState {
+    Once { call_site: Span },
+    Multiple,
+}
+
 #[derive(Clone)]
 pub struct SingleCallFn {
     pub avoid_breaking_exported_api: bool,
-    pub def_id_to_usage: FxHashMap<LocalDefId, (Span, Vec<Span>)>,
+    pub def_id_to_usage: FxIndexMap<LocalDefId, CallState>,
+}
+
+impl SingleCallFn {
+    fn is_function_allowed(
+        &self,
+        cx: &LateContext<'_>,
+        fn_def_id: LocalDefId,
+        fn_hir_id: HirId,
+        fn_span: Span,
+    ) -> bool {
+        (self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id))
+            || in_external_macro(cx.sess(), fn_span)
+            || cx
+                .tcx
+                .hir()
+                .maybe_body_owned_by(fn_def_id)
+                .map(|body| cx.tcx.hir().body(body))
+                .map_or(true, |body| is_in_test_function(cx.tcx, body.value.hir_id))
+            || match cx.tcx.hir_node(fn_hir_id) {
+                Node::Item(item) => is_from_proc_macro(cx, item),
+                Node::ImplItem(item) => is_from_proc_macro(cx, item),
+                Node::TraitItem(item) => is_from_proc_macro(cx, item),
+                _ => true,
+            }
+    }
+}
+
+/// Whether a called function is a kind of item that the lint cares about.
+/// For example, calling an `extern "C" { fn fun(); }` only once is totally fine and does not
+/// to be considered.
+fn is_valid_item_kind(cx: &LateContext<'_>, def_id: LocalDefId) -> bool {
+    matches!(
+        cx.tcx.hir_node(cx.tcx.local_def_id_to_hir_id(def_id)),
+        Node::Item(_) | Node::ImplItem(_) | Node::TraitItem(_)
+    )
 }
 
 impl<'tcx> LateLintPass<'tcx> for SingleCallFn {
-    fn check_fn(
-        &mut self,
-        cx: &LateContext<'tcx>,
-        kind: FnKind<'tcx>,
-        _: &'tcx FnDecl<'_>,
-        body: &'tcx Body<'_>,
-        span: Span,
-        def_id: LocalDefId,
-    ) {
-        if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id)
-            || in_external_macro(cx.sess(), span)
-            || is_in_test_function(cx.tcx, body.value.hir_id)
-            || is_from_proc_macro(cx, &(&kind, body, cx.tcx.local_def_id_to_hir_id(def_id), span))
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Path(qpath) = expr.kind
+            && let res = cx.qpath_res(&qpath, expr.hir_id)
+            && let Some(call_def_id) = res.opt_def_id()
+            && let Some(def_id) = call_def_id.as_local()
+            && let DefKind::Fn | DefKind::AssocFn = cx.tcx.def_kind(def_id)
+            && is_valid_item_kind(cx, def_id)
         {
-            return;
+            match self.def_id_to_usage.entry(def_id) {
+                IndexEntry::Occupied(mut entry) => {
+                    if let CallState::Once { .. } = entry.get() {
+                        entry.insert(CallState::Multiple);
+                    }
+                },
+                IndexEntry::Vacant(entry) => {
+                    entry.insert(CallState::Once { call_site: expr.span });
+                },
+            }
         }
-
-        self.def_id_to_usage.insert(def_id, (span, vec![]));
     }
 
     fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
-        let mut v = FnUsageVisitor {
-            cx,
-            def_id_to_usage: &mut self.def_id_to_usage,
-        };
-        cx.tcx.hir().visit_all_item_likes_in_crate(&mut v);
-
         for (&def_id, usage) in &self.def_id_to_usage {
-            let single_call_fn_span = usage.0;
-            if let [caller_span] = *usage.1 {
+            if let CallState::Once { call_site } = *usage
+                && let fn_hir_id = cx.tcx.local_def_id_to_hir_id(def_id)
+                && let fn_span = cx.tcx.hir().span_with_body(fn_hir_id)
+                && !self.is_function_allowed(cx, def_id, fn_hir_id, fn_span)
+            {
                 span_lint_hir_and_then(
                     cx,
                     SINGLE_CALL_FN,
-                    cx.tcx.local_def_id_to_hir_id(def_id),
-                    single_call_fn_span,
+                    fn_hir_id,
+                    fn_span,
                     "this function is only used once",
                     |diag| {
-                        diag.span_help(caller_span, "used here");
+                        diag.span_note(call_site, "used here");
                     },
                 );
             }
         }
     }
 }
-
-struct FnUsageVisitor<'a, 'tcx> {
-    cx: &'a LateContext<'tcx>,
-    def_id_to_usage: &'a mut FxHashMap<LocalDefId, (Span, Vec<Span>)>,
-}
-
-impl<'a, 'tcx> Visitor<'tcx> for FnUsageVisitor<'a, 'tcx> {
-    type NestedFilter = OnlyBodies;
-
-    fn nested_visit_map(&mut self) -> Self::Map {
-        self.cx.tcx.hir()
-    }
-
-    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
-        let Self { cx, .. } = *self;
-
-        if let ExprKind::Path(qpath) = expr.kind
-            && let res = cx.qpath_res(&qpath, expr.hir_id)
-            && let Some(call_def_id) = res.opt_def_id()
-            && let Some(def_id) = call_def_id.as_local()
-            && let Some(usage) = self.def_id_to_usage.get_mut(&def_id)
-        {
-            usage.1.push(expr.span);
-        }
-
-        walk_expr(self, expr);
-    }
-}
diff --git a/tests/ui/single_call_fn.rs b/tests/ui/single_call_fn.rs
index c20214feccc..55e7508a957 100644
--- a/tests/ui/single_call_fn.rs
+++ b/tests/ui/single_call_fn.rs
@@ -84,3 +84,25 @@ mod issue12182 {
 fn l() {
     k();
 }
+
+trait Trait {
+    fn default() {}
+    fn foo(&self);
+}
+extern "C" {
+    // test some kind of foreign item
+    fn rand() -> std::ffi::c_int;
+}
+fn m<T: Trait>(v: T) {
+    const NOT_A_FUNCTION: i32 = 1;
+    let _ = NOT_A_FUNCTION;
+
+    struct S;
+    impl S {
+        fn foo() {}
+    }
+    T::default();
+    S::foo();
+    v.foo();
+    unsafe { rand() };
+}
diff --git a/tests/ui/single_call_fn.stderr b/tests/ui/single_call_fn.stderr
index f48b78c94e9..14529beac61 100644
--- a/tests/ui/single_call_fn.stderr
+++ b/tests/ui/single_call_fn.stderr
@@ -1,4 +1,30 @@
 error: this function is only used once
+  --> tests/ui/single_call_fn.rs:13:1
+   |
+LL | fn i() {}
+   | ^^^^^^^^^
+   |
+note: used here
+  --> tests/ui/single_call_fn.rs:18:13
+   |
+LL |     let a = i;
+   |             ^
+   = note: `-D clippy::single-call-fn` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::single_call_fn)]`
+
+error: this function is only used once
+  --> tests/ui/single_call_fn.rs:14:1
+   |
+LL | fn j() {}
+   | ^^^^^^^^^
+   |
+note: used here
+  --> tests/ui/single_call_fn.rs:25:9
+   |
+LL |         j();
+   |         ^
+
+error: this function is only used once
   --> tests/ui/single_call_fn.rs:34:1
    |
 LL | / fn c() {
@@ -8,25 +34,11 @@ LL | |     println!("function...");
 LL | | }
    | |_^
    |
-help: used here
+note: used here
   --> tests/ui/single_call_fn.rs:41:5
    |
 LL |     c();
    |     ^
-   = note: `-D clippy::single-call-fn` implied by `-D warnings`
-   = help: to override `-D warnings` add `#[allow(clippy::single_call_fn)]`
-
-error: this function is only used once
-  --> tests/ui/single_call_fn.rs:13:1
-   |
-LL | fn i() {}
-   | ^^^^^^^^^
-   |
-help: used here
-  --> tests/ui/single_call_fn.rs:18:13
-   |
-LL |     let a = i;
-   |             ^
 
 error: this function is only used once
   --> tests/ui/single_call_fn.rs:44:1
@@ -34,23 +46,35 @@ error: this function is only used once
 LL | fn a() {}
    | ^^^^^^^^^
    |
-help: used here
+note: used here
   --> tests/ui/single_call_fn.rs:47:5
    |
 LL |     a();
    |     ^
 
 error: this function is only used once
-  --> tests/ui/single_call_fn.rs:14:1
+  --> tests/ui/single_call_fn.rs:89:5
    |
-LL | fn j() {}
-   | ^^^^^^^^^
+LL |     fn default() {}
+   |     ^^^^^^^^^^^^^^^
    |
-help: used here
-  --> tests/ui/single_call_fn.rs:25:9
+note: used here
+  --> tests/ui/single_call_fn.rs:104:5
    |
-LL |         j();
-   |         ^
+LL |     T::default();
+   |     ^^^^^^^^^^
+
+error: this function is only used once
+  --> tests/ui/single_call_fn.rs:102:9
+   |
+LL |         fn foo() {}
+   |         ^^^^^^^^^^^
+   |
+note: used here
+  --> tests/ui/single_call_fn.rs:105:5
+   |
+LL |     S::foo();
+   |     ^^^^^^
 
-error: aborting due to 4 previous errors
+error: aborting due to 6 previous errors