From 1b55c81db565d8e94146b9e2a7ed54f242e349ec Mon Sep 17 00:00:00 2001 From: Micha White Date: Sun, 26 Feb 2023 12:11:47 -0500 Subject: [PATCH] Lint on trait declarations, not implementations --- clippy_lints/src/unnecessary_box_returns.rs | 102 +++++++++++--------- tests/ui/unnecessary_box_returns.rs | 19 ++++ tests/ui/unnecessary_box_returns.stderr | 24 ++++- 3 files changed, 94 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/unnecessary_box_returns.rs b/clippy_lints/src/unnecessary_box_returns.rs index 533ee026500..77c1506e542 100644 --- a/clippy_lints/src/unnecessary_box_returns.rs +++ b/clippy_lints/src/unnecessary_box_returns.rs @@ -1,9 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_then; use rustc_errors::Applicability; -use rustc_hir::{def_id::LocalDefId, intravisit::FnKind, Body, FnDecl, FnRetTy}; +use rustc_hir::{def_id::LocalDefId, FnDecl, FnRetTy, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -35,54 +34,63 @@ declare_clippy_lint! { } declare_lint_pass!(UnnecessaryBoxReturns => [UNNECESSARY_BOX_RETURNS]); -impl LateLintPass<'_> for UnnecessaryBoxReturns { - fn check_fn( - &mut self, - cx: &LateContext<'_>, - fn_kind: FnKind<'_>, - decl: &FnDecl<'_>, - _: &Body<'_>, - _: Span, - def_id: LocalDefId, - ) { - // it's unclear what part of a closure you would span, so for now it's ignored - // if this is changed, please also make sure not to call `hir_ty_to_ty` below - if matches!(fn_kind, FnKind::Closure) { - return; - } +fn check_fn_decl(cx: &LateContext<'_>, decl: &FnDecl<'_>, def_id: LocalDefId) { + let FnRetTy::Return(return_ty_hir) = &decl.output else { return }; - let FnRetTy::Return(return_ty_hir) = &decl.output else { return }; + let return_ty = cx + .tcx + .erase_late_bound_regions(cx.tcx.fn_sig(def_id).skip_binder()) + .output(); - let return_ty = cx - .tcx - .erase_late_bound_regions(cx.tcx.fn_sig(def_id).skip_binder()) - .output(); + if !return_ty.is_box() { + return; + } - if !return_ty.is_box() { - return; - } + let boxed_ty = return_ty.boxed_ty(); - let boxed_ty = return_ty.boxed_ty(); - - // it's sometimes useful to return Box if T is unsized, so don't lint those - if boxed_ty.is_sized(cx.tcx, cx.param_env) { - span_lint_and_then( - cx, - UNNECESSARY_BOX_RETURNS, - return_ty_hir.span, - format!("boxed return of the sized type `{boxed_ty}`").as_str(), - |diagnostic| { - diagnostic.span_suggestion( - return_ty_hir.span, - "try", - boxed_ty.to_string(), - // the return value and function callers also needs to - // be changed, so this can't be MachineApplicable - Applicability::Unspecified, - ); - diagnostic.help("changing this also requires a change to the return expressions in this function"); - }, - ); - } + // it's sometimes useful to return Box if T is unsized, so don't lint those + if boxed_ty.is_sized(cx.tcx, cx.param_env) { + span_lint_and_then( + cx, + UNNECESSARY_BOX_RETURNS, + return_ty_hir.span, + format!("boxed return of the sized type `{boxed_ty}`").as_str(), + |diagnostic| { + diagnostic.span_suggestion( + return_ty_hir.span, + "try", + boxed_ty.to_string(), + // the return value and function callers also needs to + // be changed, so this can't be MachineApplicable + Applicability::Unspecified, + ); + diagnostic.help("changing this also requires a change to the return expressions in this function"); + }, + ); + } +} + +impl LateLintPass<'_> for UnnecessaryBoxReturns { + fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { + let TraitItemKind::Fn(signature, _) = &item.kind else { return }; + check_fn_decl(cx, signature.decl, item.owner_id.def_id); + } + + fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &rustc_hir::ImplItem<'_>) { + // Ignore implementations of traits, because the lint should be on the + // trait, not on the implmentation of it. + let Node::Item(parent) = cx.tcx.hir().get_parent(item.hir_id()) else { return }; + let ItemKind::Impl(parent) = parent.kind else { return }; + if parent.of_trait.is_some() { + return; + } + + let ImplItemKind::Fn(signature, ..) = &item.kind else { return }; + check_fn_decl(cx, signature.decl, item.owner_id.def_id); + } + + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + let ItemKind::Fn(signature, ..) = &item.kind else { return }; + check_fn_decl(cx, signature.decl, item.owner_id.def_id); } } diff --git a/tests/ui/unnecessary_box_returns.rs b/tests/ui/unnecessary_box_returns.rs index 49e24878c82..399266f90b0 100644 --- a/tests/ui/unnecessary_box_returns.rs +++ b/tests/ui/unnecessary_box_returns.rs @@ -1,7 +1,26 @@ #![warn(clippy::unnecessary_box_returns)] +trait Bar { + // lint + fn baz(&self) -> Box; +} + struct Foo {} +impl Bar for Foo { + // don't lint: this is a problem with the trait, not the implementation + fn baz(&self) -> Box { + Box::new(42) + } +} + +impl Foo { + fn baz(&self) -> Box { + // lint + Box::new(13) + } +} + // lint fn boxed_usize() -> Box { Box::new(5) diff --git a/tests/ui/unnecessary_box_returns.stderr b/tests/ui/unnecessary_box_returns.stderr index 8acaf33de04..a431e37a201 100644 --- a/tests/ui/unnecessary_box_returns.stderr +++ b/tests/ui/unnecessary_box_returns.stderr @@ -1,19 +1,35 @@ error: boxed return of the sized type `usize` - --> $DIR/unnecessary_box_returns.rs:6:21 + --> $DIR/unnecessary_box_returns.rs:5:22 + | +LL | fn baz(&self) -> Box; + | ^^^^^^^^^^ help: try: `usize` + | + = help: changing this also requires a change to the return expressions in this function + = note: `-D clippy::unnecessary-box-returns` implied by `-D warnings` + +error: boxed return of the sized type `usize` + --> $DIR/unnecessary_box_returns.rs:18:22 + | +LL | fn baz(&self) -> Box { + | ^^^^^^^^^^ help: try: `usize` + | + = help: changing this also requires a change to the return expressions in this function + +error: boxed return of the sized type `usize` + --> $DIR/unnecessary_box_returns.rs:25:21 | LL | fn boxed_usize() -> Box { | ^^^^^^^^^^ help: try: `usize` | = help: changing this also requires a change to the return expressions in this function - = note: `-D clippy::unnecessary-box-returns` implied by `-D warnings` error: boxed return of the sized type `Foo` - --> $DIR/unnecessary_box_returns.rs:11:19 + --> $DIR/unnecessary_box_returns.rs:30:19 | LL | fn boxed_foo() -> Box { | ^^^^^^^^ help: try: `Foo` | = help: changing this also requires a change to the return expressions in this function -error: aborting due to 2 previous errors +error: aborting due to 4 previous errors