Skip to content

Commit

Permalink
Auto merge of rust-lang#12498 - y21:issue12489, r=blyxyas
Browse files Browse the repository at this point in the history
[`map_entry`]: call the visitor on the local's `else` block

Fixes rust-lang#12489

The lint already has all the logic it needs for figuring out if it can or can't suggest a closure if it sees control flow expressions like `break` or `continue`, but it was ignoring the local's else block, which meant that it didn't see the `return None;` in a `let..else`.

changelog: [`map_entry`]: suggest `if let` instead of a closure when `return` expressions exist in the else block of a `let..else`
  • Loading branch information
bors committed Mar 18, 2024
2 parents e9a50f2 + 4e72ca3 commit b5dcaae
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 2 deletions.
5 changes: 4 additions & 1 deletion clippy_lints/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ struct InsertSearcher<'cx, 'tcx> {
can_use_entry: bool,
/// Whether this expression is the final expression in this code path. This may be a statement.
in_tail_pos: bool,
// Is this expression a single insert. A slightly better suggestion can be made in this case.
/// Is this expression a single insert. A slightly better suggestion can be made in this case.
is_single_insert: bool,
/// If the visitor has seen the map being used.
is_map_used: bool,
Expand Down Expand Up @@ -431,6 +431,9 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
self.is_single_insert = false;
self.visit_expr(e);
}
if let Some(els) = &l.els {
self.visit_block(els);
}
},
StmtKind::Item(_) => {
self.allow_insert_closure &= !self.in_tail_pos;
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/entry.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,14 @@ pub fn issue_11935() {
}
}

fn issue12489(map: &mut HashMap<u64, u64>) -> Option<()> {
if let std::collections::hash_map::Entry::Vacant(e) = map.entry(1) {
let Some(1) = Some(2) else {
return None;
};
e.insert(42);
}
Some(())
}

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,14 @@ pub fn issue_11935() {
}
}

fn issue12489(map: &mut HashMap<u64, u64>) -> Option<()> {
if !map.contains_key(&1) {
let Some(1) = Some(2) else {
return None;
};
map.insert(1, 42);
}
Some(())
}

fn main() {}
23 changes: 22 additions & 1 deletion tests/ui/entry.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -214,5 +214,26 @@ LL + v
LL + });
|

error: aborting due to 10 previous errors
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> tests/ui/entry.rs:184:5
|
LL | / if !map.contains_key(&1) {
LL | | let Some(1) = Some(2) else {
LL | | return None;
LL | | };
LL | | map.insert(1, 42);
LL | | }
| |_____^
|
help: try
|
LL ~ if let std::collections::hash_map::Entry::Vacant(e) = map.entry(1) {
LL + let Some(1) = Some(2) else {
LL + return None;
LL + };
LL + e.insert(42);
LL + }
|

error: aborting due to 11 previous errors

0 comments on commit b5dcaae

Please sign in to comment.