diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 9ed6627b741..dbd1a404150 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -130,6 +130,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat * [option_option](https://rust-lang.github.io/rust-clippy/master/index.html#option_option) * [linkedlist](https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist) * [rc_mutex](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex) +* [unnecessary_box_returns](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns) ### msrv diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c172ee263c6..117732c6efe 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -941,7 +941,11 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(allow_attributes::AllowAttribute)); store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(msrv()))); store.register_late_pass(|_| Box::new(unnecessary_struct_initialization::UnnecessaryStruct)); - store.register_late_pass(|_| Box::new(unnecessary_box_returns::UnnecessaryBoxReturns)); + store.register_late_pass(move |_| { + Box::new(unnecessary_box_returns::UnnecessaryBoxReturns::new( + avoid_breaking_exported_api, + )) + }); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unnecessary_box_returns.rs b/clippy_lints/src/unnecessary_box_returns.rs index 77c1506e542..10aee89606d 100644 --- a/clippy_lints/src/unnecessary_box_returns.rs +++ b/clippy_lints/src/unnecessary_box_returns.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use rustc_errors::Applicability; 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_session::{declare_tool_lint, impl_lint_pass}; declare_clippy_lint! { /// ### What it does @@ -32,48 +32,66 @@ declare_clippy_lint! { pedantic, "Needlessly returning a Box" } -declare_lint_pass!(UnnecessaryBoxReturns => [UNNECESSARY_BOX_RETURNS]); -fn check_fn_decl(cx: &LateContext<'_>, decl: &FnDecl<'_>, def_id: LocalDefId) { - let FnRetTy::Return(return_ty_hir) = &decl.output else { return }; +pub struct UnnecessaryBoxReturns { + avoid_breaking_exported_api: bool, +} - let return_ty = cx - .tcx - .erase_late_bound_regions(cx.tcx.fn_sig(def_id).skip_binder()) - .output(); +impl_lint_pass!(UnnecessaryBoxReturns => [UNNECESSARY_BOX_RETURNS]); - if !return_ty.is_box() { - return; +impl UnnecessaryBoxReturns { + pub fn new(avoid_breaking_exported_api: bool) -> Self { + Self { + avoid_breaking_exported_api, + } } - let boxed_ty = return_ty.boxed_ty(); + fn check_fn_decl(&mut self, cx: &LateContext<'_>, decl: &FnDecl<'_>, def_id: LocalDefId) { + // we don't want to tell someone to break an exported function if they ask us not to + if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id) { + return; + } - // 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"); - }, - ); + 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(); + + if !return_ty.is_box() { + return; + } + + 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"); + }, + ); + } } } 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); + self.check_fn_decl(cx, signature.decl, item.owner_id.def_id); } fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &rustc_hir::ImplItem<'_>) { @@ -86,11 +104,11 @@ impl LateLintPass<'_> for UnnecessaryBoxReturns { } let ImplItemKind::Fn(signature, ..) = &item.kind else { return }; - check_fn_decl(cx, signature.decl, item.owner_id.def_id); + self.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); + self.check_fn_decl(cx, signature.decl, item.owner_id.def_id); } } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 8ba252425a3..5384ae01f92 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -249,7 +249,7 @@ define_Conf! { /// arithmetic-side-effects-allowed-unary = ["SomeType", "AnotherType"] /// ``` (arithmetic_side_effects_allowed_unary: rustc_data_structures::fx::FxHashSet = <_>::default()), - /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX. + /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX, UNNECESSARY_BOX_RETURNS. /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), diff --git a/tests/ui/unnecessary_box_returns.rs b/tests/ui/unnecessary_box_returns.rs index 399266f90b0..f2f6aae380e 100644 --- a/tests/ui/unnecessary_box_returns.rs +++ b/tests/ui/unnecessary_box_returns.rs @@ -5,7 +5,7 @@ trait Bar { fn baz(&self) -> Box; } -struct Foo {} +pub struct Foo {} impl Bar for Foo { // don't lint: this is a problem with the trait, not the implementation @@ -27,7 +27,12 @@ fn boxed_usize() -> Box { } // lint -fn boxed_foo() -> Box { +fn _boxed_foo() -> Box { + Box::new(Foo {}) +} + +// don't lint: this is exported +pub fn boxed_foo() -> Box { Box::new(Foo {}) } diff --git a/tests/ui/unnecessary_box_returns.stderr b/tests/ui/unnecessary_box_returns.stderr index a431e37a201..0870af5f28f 100644 --- a/tests/ui/unnecessary_box_returns.stderr +++ b/tests/ui/unnecessary_box_returns.stderr @@ -24,10 +24,10 @@ LL | fn boxed_usize() -> Box { = help: changing this also requires a change to the return expressions in this function error: boxed return of the sized type `Foo` - --> $DIR/unnecessary_box_returns.rs:30:19 + --> $DIR/unnecessary_box_returns.rs:30:20 | -LL | fn boxed_foo() -> Box { - | ^^^^^^^^ help: try: `Foo` +LL | fn _boxed_foo() -> Box { + | ^^^^^^^^ help: try: `Foo` | = help: changing this also requires a change to the return expressions in this function