Skip to content

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

Closed
wants to merge 1 commit into from
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 @@ -1694,6 +1694,7 @@ Released 2018-09-13
[`empty_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_enum
[`empty_line_after_outer_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_line_after_outer_attr
[`empty_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_loop
[`entry_or_insert_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#entry_or_insert_with_default
[`enum_clike_unportable_variant`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_clike_unportable_variant
[`enum_glob_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_glob_use
[`enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::CLONE_DOUBLE_REF,
&methods::CLONE_ON_COPY,
&methods::CLONE_ON_REF_PTR,
&methods::ENTRY_OR_INSERT_WITH_DEFAULT,
&methods::EXPECT_FUN_CALL,
&methods::EXPECT_USED,
&methods::FILETYPE_IS_FILE,
Expand Down Expand Up @@ -1409,6 +1410,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::CHARS_NEXT_CMP),
LintId::of(&methods::CLONE_DOUBLE_REF),
LintId::of(&methods::CLONE_ON_COPY),
LintId::of(&methods::ENTRY_OR_INSERT_WITH_DEFAULT),
LintId::of(&methods::EXPECT_FUN_CALL),
LintId::of(&methods::FILTER_NEXT),
LintId::of(&methods::FLAT_MAP_IDENTITY),
Expand Down Expand Up @@ -1609,6 +1611,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT),
LintId::of(&methods::CHARS_LAST_CMP),
LintId::of(&methods::CHARS_NEXT_CMP),
LintId::of(&methods::ENTRY_OR_INSERT_WITH_DEFAULT),
LintId::of(&methods::INTO_ITER_ON_REF),
LintId::of(&methods::ITER_CLONED_COLLECT),
LintId::of(&methods::ITER_NEXT_SLICE),
Expand Down
66 changes: 66 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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])
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

},
_ => {},
}

Expand Down Expand Up @@ -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,
);
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this function should be merged with check_unwrap_or_default a few lines below

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 unwrap_or part a parameter

/// Checks for the `OR_FUN_CALL` lint.
#[allow(clippy::too_many_lines)]
fn lint_or_fun_call<'tcx>(
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,13 @@ vec![
deprecation: None,
module: "loops",
},
Lint {
name: "entry_or_insert_with_default",
group: "style",
desc: "calling `or_insert_with` on an `Entry` with `Default::default`",
deprecation: None,
module: "methods",
},
Lint {
name: "enum_clike_unportable_variant",
group: "correctness",
Expand Down
43 changes: 43 additions & 0 deletions tests/ui/entry_or_insert_with_default.rs
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);
}
28 changes: 28 additions & 0 deletions tests/ui/entry_or_insert_with_default.stderr
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