-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add entry_or_insert_with_default
lint
#6228
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -563,6 +563,46 @@ declare_clippy_lint! { | |
"using `.chars().next()` to check if a string starts with a char" | ||
} | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for calls to `.or_insert_with` on `Entry` | ||
/// (for `BTreeMap` or `HashMap`) with `Default::default` or equivalent | ||
/// as the argument, and suggests `.or_default`. | ||
/// | ||
/// **Why is this bad?** There is a specific method for this, and using it can make | ||
/// the code cleaner. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// use std::collections::{BTreeMap, HashMap}; | ||
/// | ||
/// fn btree(map: &mut BTreeMap<i32, i32>) { | ||
/// map.entry(42).or_insert_with(Default::default); | ||
/// } | ||
/// | ||
/// fn hash(map: &mut HashMap<i32, i32>) { | ||
/// map.entry(42).or_insert_with(i32::default); | ||
/// } | ||
/// ``` | ||
/// both can be instead written as | ||
/// ```rust | ||
/// use std::collections::{BTreeMap, HashMap}; | ||
/// | ||
/// fn btree(map: &mut BTreeMap<i32, i32>) { | ||
/// map.entry(42).or_default(); | ||
/// } | ||
/// | ||
/// fn hash(map: &mut HashMap<i32, i32>) { | ||
/// map.entry(42).or_default(); | ||
/// } | ||
/// ``` | ||
pub ENTRY_OR_INSERT_WITH_DEFAULT, | ||
style, | ||
"calling `or_insert_with` on an `Entry` with `Default::default`" | ||
} | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`, | ||
/// etc., and suggests to use `or_else`, `unwrap_or_else`, etc., or | ||
|
@@ -1394,6 +1434,7 @@ declare_lint_pass!(Methods => [ | |
RESULT_MAP_OR_INTO_OPTION, | ||
OPTION_MAP_OR_NONE, | ||
BIND_INSTEAD_OF_MAP, | ||
ENTRY_OR_INSERT_WITH_DEFAULT, | ||
OR_FUN_CALL, | ||
EXPECT_FUN_CALL, | ||
CHARS_NEXT_CMP, | ||
|
@@ -1515,6 +1556,9 @@ impl<'tcx> LateLintPass<'tcx> for Methods { | |
["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or"), | ||
["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "get_or_insert"), | ||
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"), | ||
["or_insert_with", ..] => { | ||
lint_entry_or_insert_with_default(cx, method_spans[0].with_hi(expr.span.hi()), arg_lists[0]) | ||
}, | ||
_ => {}, | ||
} | ||
|
||
|
@@ -1709,6 +1753,28 @@ impl<'tcx> LateLintPass<'tcx> for Methods { | |
} | ||
} | ||
|
||
/// Checks for the `ENTRY_OR_INSERT_WITH_DEFAULT` lint. | ||
fn lint_entry_or_insert_with_default(cx: &LateContext<'tcx>, span: Span, args: &'tcx [hir::Expr<'_>]) { | ||
if_chain! { | ||
let ty = cx.typeck_results().expr_ty(&args[0]); | ||
if match_type(cx, ty, &paths::BTREEMAP_ENTRY) || match_type(cx, ty, &paths::HASHMAP_ENTRY); | ||
if let hir::ExprKind::Path(arg_path) = &args[1].kind; | ||
if let hir::def::Res::Def(_, arg_def_id) = cx.qpath_res(&arg_path, args[1].hir_id); | ||
if match_def_path(cx, arg_def_id, &paths::DEFAULT_TRAIT_METHOD); | ||
then { | ||
span_lint_and_sugg( | ||
cx, | ||
ENTRY_OR_INSERT_WITH_DEFAULT, | ||
span, | ||
"use of `or_insert_with` with `Default::default`", | ||
"replace with", | ||
"or_default()".to_owned(), | ||
Applicability::MachineApplicable, | ||
); | ||
} | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this function should be merged with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, but I would need some help figuring out how, to be honest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this should be merged. You'll want to hoist that function out and make the |
||
/// Checks for the `OR_FUN_CALL` lint. | ||
#[allow(clippy::too_many_lines)] | ||
fn lint_or_fun_call<'tcx>( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
#![warn(clippy::entry_or_insert_with_default)] | ||
|
||
use std::collections::{BTreeMap, HashMap}; | ||
|
||
fn btree(map: &mut BTreeMap<i32, i32>) { | ||
map.entry(42).or_insert_with(Default::default); | ||
map.entry(42).or_insert_with(i32::default); | ||
map.entry(42).or_default(); | ||
} | ||
|
||
fn hash(map: &mut HashMap<i32, i32>) { | ||
map.entry(42).or_insert_with(Default::default); | ||
map.entry(42).or_insert_with(i32::default); | ||
map.entry(42).or_default(); | ||
} | ||
|
||
struct InformalDefault; | ||
|
||
impl InformalDefault { | ||
fn default() -> Self { | ||
InformalDefault | ||
} | ||
} | ||
|
||
fn not_default_btree(map: &mut BTreeMap<i32, InformalDefault>) { | ||
map.entry(42).or_insert_with(InformalDefault::default); | ||
} | ||
|
||
fn not_default_hash(map: &mut HashMap<i32, InformalDefault>) { | ||
map.entry(42).or_insert_with(InformalDefault::default); | ||
} | ||
|
||
#[derive(Default)] | ||
struct NotAnEntry; | ||
|
||
impl NotAnEntry { | ||
fn or_insert_with(self, f: impl Fn() -> Self) {} | ||
} | ||
|
||
fn main() { | ||
NotAnEntry.or_insert_with(Default::default); | ||
NotAnEntry.or_insert_with(NotAnEntry::default); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
error: use of `or_insert_with` with `Default::default` | ||
--> $DIR/entry_or_insert_with_default.rs:6:19 | ||
| | ||
LL | map.entry(42).or_insert_with(Default::default); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `or_default()` | ||
| | ||
= note: `-D clippy::entry-or-insert-with-default` implied by `-D warnings` | ||
|
||
error: use of `or_insert_with` with `Default::default` | ||
--> $DIR/entry_or_insert_with_default.rs:7:19 | ||
| | ||
LL | map.entry(42).or_insert_with(i32::default); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `or_default()` | ||
|
||
error: use of `or_insert_with` with `Default::default` | ||
--> $DIR/entry_or_insert_with_default.rs:12:19 | ||
| | ||
LL | map.entry(42).or_insert_with(Default::default); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `or_default()` | ||
|
||
error: use of `or_insert_with` with `Default::default` | ||
--> $DIR/entry_or_insert_with_default.rs:13:19 | ||
| | ||
LL | map.entry(42).or_insert_with(i32::default); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `or_default()` | ||
|
||
error: aborting due to 4 previous errors | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should need to do span splicing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? I confess I didn't quite understand this part and just tried to adapt what was being done for the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, never mind, some of the others do span splicing too. I just don't think the splicing should be here, but if you're merging this with the
check_unwrap_or_default
thing then it's going to go away from here anyway.