diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 77b6775eb776..76e759683145 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 038774207744..223cbb3fae10 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)>, + pub def_id_to_usage: FxIndexMap, +} + +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)>, -} - -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 c20214feccce..55e7508a9573 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(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 f48b78c94e9a..14529beac616 100644 --- a/tests/ui/single_call_fn.stderr +++ b/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,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