Skip to content

Commit

Permalink
Auto merge of rust-lang#9102 - botahamec:unused-box, r=xFrednet
Browse files Browse the repository at this point in the history
Added the `[unnecessary_box_returns]` lint

fixes #5

I'm not confident in the name of this lint. Let me know if you can think of something better

---

changelog: New lint: ``[`unnecessary_box_returns`]``
[rust-lang#9102](rust-lang/rust-clippy#9102)
<!-- changelog_checked -->
  • Loading branch information
bors committed Mar 30, 2023
2 parents c5011e9 + 76d13bb commit ef3867f
Show file tree
Hide file tree
Showing 8 changed files with 225 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4975,6 +4975,7 @@ Released 2018-09-13
[`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash
[`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord
[`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints
[`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns
[`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
[`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
[`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::unit_types::UNIT_CMP_INFO,
crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO,
crate::unnamed_address::VTABLE_ADDRESS_COMPARISONS_INFO,
crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO,
crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO,
crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO,
crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO,
Expand Down
6 changes: 6 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ mod uninit_vec;
mod unit_return_expecting_ord;
mod unit_types;
mod unnamed_address;
mod unnecessary_box_returns;
mod unnecessary_owned_empty_strings;
mod unnecessary_self_imports;
mod unnecessary_struct_initialization;
Expand Down Expand Up @@ -940,6 +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(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`
}

Expand Down
120 changes: 120 additions & 0 deletions clippy_lints/src/unnecessary_box_returns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
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_tool_lint, impl_lint_pass};
use rustc_span::Symbol;

declare_clippy_lint! {
/// ### What it does
///
/// Checks for a return type containing a `Box<T>` where `T` implements `Sized`
///
/// ### Why is this bad?
///
/// It's better to just return `T` in these cases. The caller may not need
/// the value to be boxed, and it's expensive to free the memory once the
/// `Box<T>` been dropped.
///
/// ### Example
/// ```rust
/// fn foo() -> Box<String> {
/// Box::new(String::from("Hello, world!"))
/// }
/// ```
/// Use instead:
/// ```rust
/// fn foo() -> String {
/// String::from("Hello, world!")
/// }
/// ```
#[clippy::version = "1.70.0"]
pub UNNECESSARY_BOX_RETURNS,
pedantic,
"Needlessly returning a Box"
}

pub struct UnnecessaryBoxReturns {
avoid_breaking_exported_api: bool,
}

impl_lint_pass!(UnnecessaryBoxReturns => [UNNECESSARY_BOX_RETURNS]);

impl UnnecessaryBoxReturns {
pub fn new(avoid_breaking_exported_api: bool) -> Self {
Self {
avoid_breaking_exported_api,
}
}

fn check_fn_item(&mut self, cx: &LateContext<'_>, decl: &FnDecl<'_>, def_id: LocalDefId, name: Symbol) {
// 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;
}

// functions which contain the word "box" are exempt from this lint
if name.as_str().contains("box") {
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();

if !return_ty.is_box() {
return;
}

let boxed_ty = return_ty.boxed_ty();

// it's sometimes useful to return Box<T> 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 };
self.check_fn_item(cx, signature.decl, item.owner_id.def_id, item.ident.name);
}

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 };
self.check_fn_item(cx, signature.decl, item.owner_id.def_id, item.ident.name);
}

fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
let ItemKind::Fn(signature, ..) = &item.kind else { return };
self.check_fn_item(cx, signature.decl, item.owner_id.def_id, item.ident.name);
}
}
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ define_Conf! {
/// arithmetic-side-effects-allowed-unary = ["SomeType", "AnotherType"]
/// ```
(arithmetic_side_effects_allowed_unary: rustc_data_structures::fx::FxHashSet<String> = <_>::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),
Expand Down
60 changes: 60 additions & 0 deletions tests/ui/unnecessary_box_returns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#![warn(clippy::unnecessary_box_returns)]

trait Bar {
// lint
fn baz(&self) -> Box<usize>;
}

pub struct Foo {}

impl Bar for Foo {
// don't lint: this is a problem with the trait, not the implementation
fn baz(&self) -> Box<usize> {
Box::new(42)
}
}

impl Foo {
fn baz(&self) -> Box<usize> {
// lint
Box::new(13)
}
}

// lint
fn bxed_usize() -> Box<usize> {
Box::new(5)
}

// lint
fn _bxed_foo() -> Box<Foo> {
Box::new(Foo {})
}

// don't lint: this is exported
pub fn bxed_foo() -> Box<Foo> {
Box::new(Foo {})
}

// don't lint: str is unsized
fn bxed_str() -> Box<str> {
"Hello, world!".to_string().into_boxed_str()
}

// don't lint: function contains the word, "box"
fn boxed_usize() -> Box<usize> {
Box::new(7)
}

// don't lint: this has an unspecified return type
fn default() {}

// don't lint: this doesn't return a Box
fn string() -> String {
String::from("Hello, world")
}

fn main() {
// don't lint: this is a closure
let a = || -> Box<usize> { Box::new(5) };
}
35 changes: 35 additions & 0 deletions tests/ui/unnecessary_box_returns.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
error: boxed return of the sized type `usize`
--> $DIR/unnecessary_box_returns.rs:5:22
|
LL | fn baz(&self) -> Box<usize>;
| ^^^^^^^^^^ 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<usize> {
| ^^^^^^^^^^ 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:20
|
LL | fn bxed_usize() -> Box<usize> {
| ^^^^^^^^^^ help: try: `usize`
|
= 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
|
LL | fn _bxed_foo() -> Box<Foo> {
| ^^^^^^^^ help: try: `Foo`
|
= help: changing this also requires a change to the return expressions in this function

error: aborting due to 4 previous errors

0 comments on commit ef3867f

Please sign in to comment.