Skip to content

Commit

Permalink
Suggest Entry::or_default for Entry::or_insert(Default::default())
Browse files Browse the repository at this point in the history
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?).

Fixes #3812.
  • Loading branch information
relrelb committed Aug 19, 2022
1 parent 3a54117 commit e8c8a93
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 11 deletions.
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -797,8 +797,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
Expand Down
13 changes: 9 additions & 4 deletions clippy_lints/src/methods/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>,
Expand All @@ -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;
Expand All @@ -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,
);

Expand All @@ -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"),
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/or_fun_call.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
26 changes: 25 additions & 1 deletion tests/ui/or_fun_call.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down Expand Up @@ -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

0 comments on commit e8c8a93

Please sign in to comment.