Skip to content

Add new useless_allocation lint #12315

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5777,6 +5777,7 @@ Released 2018-09-13
[`use_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_debug
[`use_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_self
[`used_underscore_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#used_underscore_binding
[`useless_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_allocation
[`useless_asref`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_asref
[`useless_attribute`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_attribute
[`useless_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
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 @@ -726,6 +726,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::unwrap_in_result::UNWRAP_IN_RESULT_INFO,
crate::upper_case_acronyms::UPPER_CASE_ACRONYMS_INFO,
crate::use_self::USE_SELF_INFO,
crate::useless_allocation::USELESS_ALLOCATION_INFO,
crate::useless_conversion::USELESS_CONVERSION_INFO,
crate::vec::USELESS_VEC_INFO,
crate::vec_init_then_push::VEC_INIT_THEN_PUSH_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 @@ -359,6 +359,7 @@ mod unwrap;
mod unwrap_in_result;
mod upper_case_acronyms;
mod use_self;
mod useless_allocation;
mod useless_conversion;
mod vec;
mod vec_init_then_push;
Expand Down Expand Up @@ -1111,6 +1112,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
});
store.register_late_pass(move |_| Box::new(incompatible_msrv::IncompatibleMsrv::new(msrv())));
store.register_late_pass(|_| Box::new(to_string_trait_impl::ToStringTraitImpl));
store.register_late_pass(|_| Box::new(useless_allocation::UselessAllocation));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
119 changes: 119 additions & 0 deletions clippy_lints/src/useless_allocation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, LangItem};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, ClauseKind, ImplPolarity, Mutability, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_diag_trait_item;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};

declare_clippy_lint! {
/// ### What it does
/// Checks if an unneeded allocation is performed when trying to get information
/// related to a given key is a `HashMap`-like type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/is/in/ ?

///
/// ### Why is this bad?
/// Using less resources is generally a good idea.
///
/// ### Example
/// ```no_run
/// let mut s = HashSet::from(["a".to_string()]);
/// s.remove(&"b".to_owned());
/// ```
/// Use instead:
/// ```no_run
/// let mut s = HashSet::from(["a".to_string()]);
/// s.remove("b");
/// ```
#[clippy::version = "1.78.0"]
pub USELESS_ALLOCATION,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I think this lint name is quite general for how specific the actual lint is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a bit as if needless_clone and unnecessary_to_owned had a child together.

Copy link
Member

@y21 y21 Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes me wonder if it would make sense to just extend unnecessary_to_owned with these additional cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, unnecessary_to_owned already lints on:

fn f(x: &str) {}
f(&"".to_owned());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a bit as if needless_clone and unnecessary_to_owned had a child together.

Pretty much yes. 😆

@y21 I wondered about that as well but since it's covering than just to_owned call, I felt like it should not be there. Same goes for needless_clone since it's not related to clone.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, unnecessary_to_owned already lints on:

fn f(x: &str) {}
f(&"".to_owned());

But it doesn't work when the argument is generic (yet?) unfortunately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what do you want me to do here? Rename it or merge it with another existing lint?

Copy link
Member

@y21 y21 Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, merging it with unnecessary_to_owned makes the most sense.

I wondered about that as well but since it's covering than just to_owned call

But the lint description for it also says "[...] and other to_owned-like functions", and also covers the same to_owned, to_vec and to_string functions.

But it doesn't work when the argument is generic (yet?) unfortunately.

I mentioned this in my comment, but it does seem to understand some generic cases:

fn f<K: Deref<Target=str>>(x: K) {}
f("".to_owned());

This gets linted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in the case of the maps, it gets a bit more complicated because the argument is a generic Q and Q doesn't have the Borrow trait as predicate (as you can see here). So I can merge both lints, but it might require a bit more work than just adding a check for Borrow in this case...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tested to add the Borrow trait as check and it doesn't detect this code:

fn borrow_str<Q: Borrow<str>>(s: &Q) {}
fn borrow_slice<Q: Borrow<[u8]>>(s: &Q) {}
fn check_borrow() {
    borrow_str(&"a".to_owned());
    borrow_str(&"a".to_string());
    borrow_slice(&[0].to_vec());
}

So I'll merge this code with the lint for now.

suspicious,
"default lint description"
}

declare_lint_pass!(UselessAllocation => [USELESS_ALLOCATION]);

fn is_a_std_map_type(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
is_type_diagnostic_item(cx, ty, sym::HashSet)
|| is_type_diagnostic_item(cx, ty, sym::HashMap)
|| is_type_diagnostic_item(cx, ty, sym::BTreeMap)
|| is_type_diagnostic_item(cx, ty, sym::BTreeSet)
}

fn is_str_and_string(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool {
original_arg_ty.is_str() && is_type_lang_item(cx, arg_ty, LangItem::String)
}

fn is_slice_and_vec(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool {
(original_arg_ty.is_slice() || original_arg_ty.is_array() || original_arg_ty.is_array_slice())
&& is_type_diagnostic_item(cx, arg_ty, sym::Vec)
}

fn check_if_applicable_to_argument(cx: &LateContext<'_>, arg: &Expr<'_>) {
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, expr) = arg.kind
&& let ExprKind::MethodCall(method_path, caller, &[], _) = expr.kind
&& let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
&& let method_name = method_path.ident.name.as_str()
&& match method_name {
"to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
"to_string" => is_diag_trait_item(cx, method_def_id, sym::ToString),
"to_vec" => cx
.tcx
.impl_of_method(method_def_id)
.filter(|&impl_did| {
cx.tcx.type_of(impl_did).instantiate_identity().is_slice()
&& cx.tcx.impl_trait_ref(impl_did).is_none()
})
.is_some(),
_ => false,
}
&& let original_arg_ty = cx.typeck_results().node_type(caller.hir_id)
&& let arg_ty = cx.typeck_results().expr_ty(arg)
&& let ty::Ref(_, arg_ty, Mutability::Not) = arg_ty.kind()
&& let arg_ty = arg_ty.peel_refs()
// For now we limit this lint to `String` and `Vec`.
&& let is_str = is_str_and_string(cx, arg_ty, original_arg_ty.peel_refs())
&& (is_str || is_slice_and_vec(cx, arg_ty, original_arg_ty))
&& let Some(snippet) = snippet_opt(cx, caller.span)
{
span_lint_and_sugg(
cx,
USELESS_ALLOCATION,
arg.span,
"unneeded allocation",
"replace it with",
if is_str {
snippet
} else {
format!("{}.as_slice()", snippet)
},
Applicability::MaybeIncorrect,
);
}
}

impl<'tcx> LateLintPass<'tcx> for UselessAllocation {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if let ExprKind::MethodCall(_, caller, &[arg], _) = expr.kind
&& let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
&& let Some(borrow_id) = cx.tcx.get_diagnostic_item(sym::Borrow)
&& cx.tcx.predicates_of(method_def_id).predicates.iter().any(|(pred, _)| {
if let ClauseKind::Trait(trait_pred) = pred.kind().skip_binder()
&& trait_pred.polarity == ImplPolarity::Positive
&& trait_pred.trait_ref.def_id == borrow_id
{
true
} else {
false
}
})
&& let caller_ty = cx.typeck_results().expr_ty(caller)
&& is_a_std_map_type(cx, caller_ty)
{
check_if_applicable_to_argument(cx, &arg);
}
}
}
18 changes: 18 additions & 0 deletions tests/ui/useless_allocation.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![warn(clippy::useless_allocation)]

use std::collections::HashSet;

fn main() {
let mut s = HashSet::from(["a".to_string()]);
s.remove("b"); //~ ERROR: unneeded allocation
s.remove("b"); //~ ERROR: unneeded allocation
// Should not warn.
s.remove("b");

let mut s = HashSet::from([vec!["a"]]);
s.remove(["b"].as_slice()); //~ ERROR: unneeded allocation

// Should not warn.
s.remove(&["b"].to_vec().clone());
s.remove(["a"].as_slice());
}
18 changes: 18 additions & 0 deletions tests/ui/useless_allocation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![warn(clippy::useless_allocation)]

use std::collections::HashSet;

fn main() {
let mut s = HashSet::from(["a".to_string()]);
s.remove(&"b".to_owned()); //~ ERROR: unneeded allocation
s.remove(&"b".to_string()); //~ ERROR: unneeded allocation
// Should not warn.
s.remove("b");

let mut s = HashSet::from([vec!["a"]]);
s.remove(&["b"].to_vec()); //~ ERROR: unneeded allocation

// Should not warn.
s.remove(&["b"].to_vec().clone());
s.remove(["a"].as_slice());
}
23 changes: 23 additions & 0 deletions tests/ui/useless_allocation.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
error: unneeded allocation
--> tests/ui/useless_allocation.rs:7:14
|
LL | s.remove(&"b".to_owned());
| ^^^^^^^^^^^^^^^ help: replace it with: `"b"`
|
= note: `-D clippy::useless-allocation` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::useless_allocation)]`

error: unneeded allocation
--> tests/ui/useless_allocation.rs:8:14
|
LL | s.remove(&"b".to_string());
| ^^^^^^^^^^^^^^^^ help: replace it with: `"b"`

error: unneeded allocation
--> tests/ui/useless_allocation.rs:13:14
|
LL | s.remove(&["b"].to_vec());
| ^^^^^^^^^^^^^^^ help: replace it with: `["b"].as_slice()`

error: aborting due to 3 previous errors