diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a0d190a58af..b7d2d980db8 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -825,8 +825,9 @@ 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(|| foo(..))`, + /// `.unwrap_or_else(|| foo(..))`, `.unwrap_or_default()` or `.or_default()` + /// etc. 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 6af134019a4..c97f714680e 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 @@ fn check_unwrap_or_default( 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 @@ fn check_unwrap_or_default( 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 @@ fn check_general_case<'tcx>( fun_span: Option, ) { // (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/src/docs/or_fun_call.txt b/src/docs/or_fun_call.txt index 5605e96c98e..6ce77cc268c 100644 --- a/src/docs/or_fun_call.txt +++ b/src/docs/or_fun_call.txt @@ -1,7 +1,8 @@ ### 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(|| foo(..))`, +`.unwrap_or_else(|| foo(..))`, `.unwrap_or_default()` or `.or_default()` +etc. instead. ### Why is this bad? The function will always be called and potentially diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index 18ea4e55029..5991188ab63 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::::new(); - map.entry(42).or_insert(String::new()); + map.entry(42).or_default(); let mut map_vec = HashMap::>::new(); - map_vec.entry(42).or_insert(vec![]); + map_vec.entry(42).or_default(); let mut btree = BTreeMap::::new(); - btree.entry(42).or_insert(String::new()); + btree.entry(42).or_default(); let mut btree_vec = BTreeMap::>::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 887f23ac976..e3dab4cb147 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