From 49a230bcc8c7cc604cf4720ed0f3dd752425a291 Mon Sep 17 00:00:00 2001 From: relrelb <relrelbachar@gmail.com> Date: Tue, 16 Aug 2022 23:26:03 +0300 Subject: [PATCH] Suggest `Entry::or_default` for `Entry::or_insert(Default::default())` Unlike past similar work done in #6228, expand the existing `or_fun_call` lint to detect `or_insert` calls with a `T::new()` or `T::default()` argument, much like currently done for `unwrap_or` calls. In that case, suggest the use of `or_default`, which is more idiomatic. Note that even with this change, `or_insert_with(T::default)` calls aren't detected as candidates for `or_default()`, in the same manner that currently `unwrap_or_else(T::default)` calls aren't detected as candidates for `unwrap_or_default()`. Also, as a nearby cleanup, change `KNOW_TYPES` from `static` to `const`, since as far as I understand it's preferred (should Clippy have a lint for that?). The new lint detected 1 instance in the dogfood tests, so fix it along the way. Fixes #3812. --- clippy_lints/src/methods/mod.rs | 4 ++-- clippy_lints/src/methods/or_fun_call.rs | 13 +++++++---- clippy_lints/src/only_used_in_recursion.rs | 2 +- tests/ui/or_fun_call.fixed | 8 +++---- tests/ui/or_fun_call.stderr | 26 +++++++++++++++++++++- 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b68a2651e1bd..aafd9ed307f1 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -778,8 +778,8 @@ declare_clippy_lint! { 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 - /// `unwrap_or_default` instead. + /// `or_insert(foo(...))` etc., and suggests to use `or_else`, + /// `unwrap_or_else`, etc., `unwrap_or_default` or `or_default` instead. /// /// ### Why is this bad? /// The function will always be called and potentially diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index 6af134019a47..c97f714680ef 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -22,7 +22,8 @@ pub(super) fn check<'tcx>( name: &str, args: &'tcx [hir::Expr<'_>], ) { - /// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`. + /// Checks for `unwrap_or(T::new())`, `unwrap_or(T::default())`, + /// `or_insert(T::new())` or `or_insert(T::default())`. #[allow(clippy::too_many_arguments)] fn check_unwrap_or_default( cx: &LateContext<'_>, @@ -42,7 +43,11 @@ pub(super) fn check<'tcx>( if_chain! { if !or_has_args; - if name == "unwrap_or"; + if let Some(sugg) = match name { + "unwrap_or" => Some("unwrap_or_default"), + "or_insert" => Some("or_default"), + _ => None, + }; if let hir::ExprKind::Path(ref qpath) = fun.kind; if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default); let path = last_path_segment(qpath).ident.name; @@ -58,7 +63,7 @@ pub(super) fn check<'tcx>( method_span.with_hi(span.hi()), &format!("use of `{}` followed by a call to `{}`", name, path), "try this", - "unwrap_or_default()".to_string(), + format!("{}()", sugg), Applicability::MachineApplicable, ); @@ -82,7 +87,7 @@ pub(super) fn check<'tcx>( fun_span: Option<Span>, ) { // (path, fn_has_argument, methods, suffix) - static KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [ + const KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [ (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"), (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"), (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"), diff --git a/clippy_lints/src/only_used_in_recursion.rs b/clippy_lints/src/only_used_in_recursion.rs index 413a740be25a..f20eaf396d75 100644 --- a/clippy_lints/src/only_used_in_recursion.rs +++ b/clippy_lints/src/only_used_in_recursion.rs @@ -337,7 +337,7 @@ impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> { if let Some(label) = dest.label { self.break_vars .entry(label.ident) - .or_insert(Vec::new()) + .or_default() .append(&mut self.ret_vars); } self.contains_side_effect = true; diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index 18ea4e550292..5991188ab637 100644 --- a/tests/ui/or_fun_call.fixed +++ b/tests/ui/or_fun_call.fixed @@ -79,16 +79,16 @@ fn or_fun_call() { without_default.unwrap_or_else(Foo::new); let mut map = HashMap::<u64, String>::new(); - map.entry(42).or_insert(String::new()); + map.entry(42).or_default(); let mut map_vec = HashMap::<u64, Vec<i32>>::new(); - map_vec.entry(42).or_insert(vec![]); + map_vec.entry(42).or_default(); let mut btree = BTreeMap::<u64, String>::new(); - btree.entry(42).or_insert(String::new()); + btree.entry(42).or_default(); let mut btree_vec = BTreeMap::<u64, Vec<i32>>::new(); - btree_vec.entry(42).or_insert(vec![]); + btree_vec.entry(42).or_default(); let stringy = Some(String::new()); let _ = stringy.unwrap_or_default(); diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 887f23ac9761..e3dab4cb1477 100644 --- a/tests/ui/or_fun_call.stderr +++ b/tests/ui/or_fun_call.stderr @@ -66,6 +66,30 @@ error: use of `unwrap_or` followed by a function call LL | without_default.unwrap_or(Foo::new()); | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)` +error: use of `or_insert` followed by a call to `new` + --> $DIR/or_fun_call.rs:82:19 + | +LL | map.entry(42).or_insert(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_default()` + +error: use of `or_insert` followed by a call to `new` + --> $DIR/or_fun_call.rs:85:23 + | +LL | map_vec.entry(42).or_insert(vec![]); + | ^^^^^^^^^^^^^^^^^ help: try this: `or_default()` + +error: use of `or_insert` followed by a call to `new` + --> $DIR/or_fun_call.rs:88:21 + | +LL | btree.entry(42).or_insert(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_default()` + +error: use of `or_insert` followed by a call to `new` + --> $DIR/or_fun_call.rs:91:25 + | +LL | btree_vec.entry(42).or_insert(vec![]); + | ^^^^^^^^^^^^^^^^^ help: try this: `or_default()` + error: use of `unwrap_or` followed by a call to `new` --> $DIR/or_fun_call.rs:94:21 | @@ -132,5 +156,5 @@ error: use of `unwrap_or` followed by a call to `new` LL | .unwrap_or(String::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_default()` -error: aborting due to 22 previous errors +error: aborting due to 26 previous errors