Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the [unnecessary_box_returns] lint #9102

Merged
merged 4 commits into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Added the new lint with some docs and tests
  • Loading branch information
botahamec committed Mar 26, 2023
commit 022f76d432f3e3289af68845d9169c1866c14d70
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4974,6 +4974,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 clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,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
2 changes: 2 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,7 @@ 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));
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
88 changes: 88 additions & 0 deletions clippy_lints/src/unnecessary_box_returns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
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_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;

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"
}
declare_lint_pass!(UnnecessaryBoxReturns => [UNNECESSARY_BOX_RETURNS]);

impl LateLintPass<'_> for UnnecessaryBoxReturns {
fn check_fn(
botahamec marked this conversation as resolved.
Show resolved Hide resolved
&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;
}

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() {
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
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");
},
);
}
}
}
31 changes: 31 additions & 0 deletions tests/ui/unnecessary_box_returns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#![warn(clippy::unnecessary_box_returns)]

struct Foo {}

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

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

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

// 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) };
}
19 changes: 19 additions & 0 deletions tests/ui/unnecessary_box_returns.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error: boxed return of the sized type `usize`
--> $DIR/unnecessary_box_returns.rs:6:21
|
LL | fn boxed_usize() -> 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 `Foo`
--> $DIR/unnecessary_box_returns.rs:11:19
|
LL | fn boxed_foo() -> Box<Foo> {
| ^^^^^^^^ help: try: `Foo`
|
= help: changing this also requires a change to the return expressions in this function

error: aborting due to 2 previous errors