Auto merge of rust-lang#12256 - y21:single_call_fn_rm_visitor, r=llogiq

Merge `single_call_fn` post-crate visitor into lint pass

The `single_call_fn` lint worked by first collecting a list of function definitions in the lint pass, then populating the list of uses for each function in a second visitor after the crate is checked.
Doing another pass through the crate shouldn't be needed, and we should be able to do it in the same lint pass, by looking for path references to functions only and then processing them post-crate.

Other changes:
- `FxHashMap` -> `FxIndexMap` so that we emit warnings in a consistent order, as we see them (making the diff a bit confusing to look at, because warnings were moved around)
- no longer storing a `Vec<Span>` per function: an enum representing "seen once" or "seen more than once" should be enough (only the first element is used later)
- "used here" help is now a note

I also noticed that it lints on trait methods with a default implementation, but not on regular trait methods without a body (because that's what `check_fn` does). I'm not sure if that's useful though, maybe we shouldn't lint trait methods at all? It's not like you can avoid it sometimes (but then again it's a restriction lint). Either way, I left the behavior where it was before so that there are no functional changes made in this PR and it's purely a refactor. I can change it though

changelog: none

Author: bors

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 || {

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);
-    }
-}

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() };
+}

tests/ui/single_call_fn.stderr

@@ -1,3 +1,29 @@
+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
    |
@@ -8,49 +34,47 @@ 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
    |
 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