From 286cb69bb25e202f3da63c768c61f44ec04b25d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cau=C3=AA=20Baasch=20de=20Souza?= Date: Mon, 26 Oct 2020 11:15:31 -0300 Subject: [PATCH] Add `entry_or_insert_with_default` lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/methods/mod.rs | 66 ++++++++++++++++++++ src/lintlist/mod.rs | 7 +++ tests/ui/entry_or_insert_with_default.rs | 43 +++++++++++++ tests/ui/entry_or_insert_with_default.stderr | 28 +++++++++ 6 files changed, 148 insertions(+) create mode 100644 tests/ui/entry_or_insert_with_default.rs create mode 100644 tests/ui/entry_or_insert_with_default.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 25f3b5da198a..087580341d41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3be8bc0e36d6..655a11d4fe80 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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, @@ -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), @@ -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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c0824bacbc73..89c3c8938fdf 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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) { + /// map.entry(42).or_insert_with(Default::default); + /// } + /// + /// fn hash(map: &mut HashMap) { + /// 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) { + /// map.entry(42).or_default(); + /// } + /// + /// fn hash(map: &mut HashMap) { + /// 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, + ); + } + } +} + /// Checks for the `OR_FUN_CALL` lint. #[allow(clippy::too_many_lines)] fn lint_or_fun_call<'tcx>( diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6272ce45efbc..c031eec3d192 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -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", diff --git a/tests/ui/entry_or_insert_with_default.rs b/tests/ui/entry_or_insert_with_default.rs new file mode 100644 index 000000000000..ed140d3bb2b6 --- /dev/null +++ b/tests/ui/entry_or_insert_with_default.rs @@ -0,0 +1,43 @@ +#![warn(clippy::entry_or_insert_with_default)] + +use std::collections::{BTreeMap, HashMap}; + +fn btree(map: &mut BTreeMap) { + 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) { + 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) { + map.entry(42).or_insert_with(InformalDefault::default); +} + +fn not_default_hash(map: &mut HashMap) { + 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); +} diff --git a/tests/ui/entry_or_insert_with_default.stderr b/tests/ui/entry_or_insert_with_default.stderr new file mode 100644 index 000000000000..4468b506d8bc --- /dev/null +++ b/tests/ui/entry_or_insert_with_default.stderr @@ -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 +