mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-23 05:03:21 +00:00
Auto merge of #11896 - samueltardieu:issue-11893, r=Alexendoo
`option_if_let_else`: do not trigger on expressions returning `()` Fix #11893 Trigerring on expressions returning `()` uses the arguments of the `map_or_else()` rewrite only for their side effects. This does lead to code which is harder to read than the original. changelog: [`option_if_let_else`]: do not trigger on unit expressions
This commit is contained in:
bors
commit
646b28f5f6
4 changed files with 85 additions and 44 deletions
|
|
@ -239,21 +239,24 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) ->
|
|||
if_then,
|
||||
if_else: Some(if_else),
|
||||
}) = higher::IfLet::hir(cx, expr)
|
||||
&& !cx.typeck_results().expr_ty(expr).is_unit()
|
||||
&& !is_else_clause(cx.tcx, expr)
|
||||
{
|
||||
if !is_else_clause(cx.tcx, expr) {
|
||||
return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else);
|
||||
}
|
||||
try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn detect_option_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<OptionOccurrence> {
|
||||
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
|
||||
if let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms) {
|
||||
return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else);
|
||||
}
|
||||
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind
|
||||
&& !cx.typeck_results().expr_ty(expr).is_unit()
|
||||
&& let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms)
|
||||
{
|
||||
try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn try_convert_match<'tcx>(
|
||||
|
|
|
|||
|
|
@ -92,11 +92,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
|
|||
}
|
||||
|
||||
// #10335
|
||||
fn test_result_impure_else(variable: Result<u32, &str>) {
|
||||
fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
|
||||
variable.map_or_else(|_| {
|
||||
println!("Err");
|
||||
false
|
||||
}, |binding| {
|
||||
println!("Ok {binding}");
|
||||
true
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -213,15 +215,19 @@ mod issue10729 {
|
|||
|
||||
pub fn reproduce(initial: &Option<String>) {
|
||||
// 👇 needs `.as_ref()` because initial is an `&Option<_>`
|
||||
initial.as_ref().map_or({}, |value| do_something(value))
|
||||
let _ = initial.as_ref().map_or(42, |value| do_something(value));
|
||||
}
|
||||
|
||||
pub fn reproduce2(initial: &mut Option<String>) {
|
||||
initial.as_mut().map_or({}, |value| do_something2(value))
|
||||
let _ = initial.as_mut().map_or(42, |value| do_something2(value));
|
||||
}
|
||||
|
||||
fn do_something(_value: &str) {}
|
||||
fn do_something2(_value: &mut str) {}
|
||||
fn do_something(_value: &str) -> u32 {
|
||||
todo!()
|
||||
}
|
||||
fn do_something2(_value: &mut str) -> u32 {
|
||||
todo!()
|
||||
}
|
||||
}
|
||||
|
||||
fn issue11429() {
|
||||
|
|
@ -237,3 +243,13 @@ fn issue11429() {
|
|||
|
||||
let mut _hm = opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone());
|
||||
}
|
||||
|
||||
fn issue11893() {
|
||||
use std::io::Write;
|
||||
let mut output = std::io::stdout().lock();
|
||||
if let Some(name) = Some("stuff") {
|
||||
writeln!(output, "{name:?}").unwrap();
|
||||
} else {
|
||||
panic!("Haven't thought about this condition.");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -115,11 +115,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
|
|||
}
|
||||
|
||||
// #10335
|
||||
fn test_result_impure_else(variable: Result<u32, &str>) {
|
||||
fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
|
||||
if let Ok(binding) = variable {
|
||||
println!("Ok {binding}");
|
||||
true
|
||||
} else {
|
||||
println!("Err");
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -254,21 +256,25 @@ mod issue10729 {
|
|||
|
||||
pub fn reproduce(initial: &Option<String>) {
|
||||
// 👇 needs `.as_ref()` because initial is an `&Option<_>`
|
||||
match initial {
|
||||
let _ = match initial {
|
||||
Some(value) => do_something(value),
|
||||
None => {},
|
||||
}
|
||||
None => 42,
|
||||
};
|
||||
}
|
||||
|
||||
pub fn reproduce2(initial: &mut Option<String>) {
|
||||
match initial {
|
||||
let _ = match initial {
|
||||
Some(value) => do_something2(value),
|
||||
None => {},
|
||||
}
|
||||
None => 42,
|
||||
};
|
||||
}
|
||||
|
||||
fn do_something(_value: &str) {}
|
||||
fn do_something2(_value: &mut str) {}
|
||||
fn do_something(_value: &str) -> u32 {
|
||||
todo!()
|
||||
}
|
||||
fn do_something2(_value: &mut str) -> u32 {
|
||||
todo!()
|
||||
}
|
||||
}
|
||||
|
||||
fn issue11429() {
|
||||
|
|
@ -288,3 +294,13 @@ fn issue11429() {
|
|||
|
||||
let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
|
||||
}
|
||||
|
||||
fn issue11893() {
|
||||
use std::io::Write;
|
||||
let mut output = std::io::stdout().lock();
|
||||
if let Some(name) = Some("stuff") {
|
||||
writeln!(output, "{name:?}").unwrap();
|
||||
} else {
|
||||
panic!("Haven't thought about this condition.");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -158,8 +158,10 @@ error: use Option::map_or_else instead of an if let/else
|
|||
|
|
||||
LL | / if let Ok(binding) = variable {
|
||||
LL | | println!("Ok {binding}");
|
||||
LL | | true
|
||||
LL | | } else {
|
||||
LL | | println!("Err");
|
||||
LL | | false
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
|
|
@ -167,19 +169,21 @@ help: try
|
|||
|
|
||||
LL ~ variable.map_or_else(|_| {
|
||||
LL + println!("Err");
|
||||
LL + false
|
||||
LL + }, |binding| {
|
||||
LL + println!("Ok {binding}");
|
||||
LL + true
|
||||
LL + })
|
||||
|
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:141:13
|
||||
--> $DIR/option_if_let_else.rs:143:13
|
||||
|
|
||||
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:151:13
|
||||
--> $DIR/option_if_let_else.rs:153:13
|
||||
|
|
||||
LL | let _ = if let Some(x) = Some(0) {
|
||||
| _____________^
|
||||
|
|
@ -201,13 +205,13 @@ LL ~ });
|
|||
|
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:179:13
|
||||
--> $DIR/option_if_let_else.rs:181:13
|
||||
|
|
||||
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:183:13
|
||||
--> $DIR/option_if_let_else.rs:185:13
|
||||
|
|
||||
LL | let _ = if let Some(x) = Some(0) {
|
||||
| _____________^
|
||||
|
|
@ -227,7 +231,7 @@ LL ~ });
|
|||
|
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:222:13
|
||||
--> $DIR/option_if_let_else.rs:224:13
|
||||
|
|
||||
LL | let _ = match s {
|
||||
| _____________^
|
||||
|
|
@ -237,7 +241,7 @@ LL | | };
|
|||
| |_____^ help: try: `s.map_or(1, |string| string.len())`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:226:13
|
||||
--> $DIR/option_if_let_else.rs:228:13
|
||||
|
|
||||
LL | let _ = match Some(10) {
|
||||
| _____________^
|
||||
|
|
@ -247,7 +251,7 @@ LL | | };
|
|||
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:232:13
|
||||
--> $DIR/option_if_let_else.rs:234:13
|
||||
|
|
||||
LL | let _ = match res {
|
||||
| _____________^
|
||||
|
|
@ -257,7 +261,7 @@ LL | | };
|
|||
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:236:13
|
||||
--> $DIR/option_if_let_else.rs:238:13
|
||||
|
|
||||
LL | let _ = match res {
|
||||
| _____________^
|
||||
|
|
@ -267,31 +271,33 @@ LL | | };
|
|||
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:240:13
|
||||
--> $DIR/option_if_let_else.rs:242:13
|
||||
|
|
||||
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:257:9
|
||||
--> $DIR/option_if_let_else.rs:259:17
|
||||
|
|
||||
LL | / match initial {
|
||||
LL | let _ = match initial {
|
||||
| _________________^
|
||||
LL | | Some(value) => do_something(value),
|
||||
LL | | None => {},
|
||||
LL | | }
|
||||
| |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
|
||||
LL | | None => 42,
|
||||
LL | | };
|
||||
| |_________^ help: try: `initial.as_ref().map_or(42, |value| do_something(value))`
|
||||
|
||||
error: use Option::map_or instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:264:9
|
||||
--> $DIR/option_if_let_else.rs:266:17
|
||||
|
|
||||
LL | / match initial {
|
||||
LL | let _ = match initial {
|
||||
| _________________^
|
||||
LL | | Some(value) => do_something2(value),
|
||||
LL | | None => {},
|
||||
LL | | }
|
||||
| |_________^ help: try: `initial.as_mut().map_or({}, |value| do_something2(value))`
|
||||
LL | | None => 42,
|
||||
LL | | };
|
||||
| |_________^ help: try: `initial.as_mut().map_or(42, |value| do_something2(value))`
|
||||
|
||||
error: use Option::map_or_else instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:283:24
|
||||
--> $DIR/option_if_let_else.rs:289:24
|
||||
|
|
||||
LL | let mut _hashmap = if let Some(hm) = &opt {
|
||||
| ________________________^
|
||||
|
|
@ -302,7 +308,7 @@ LL | | };
|
|||
| |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())`
|
||||
|
||||
error: use Option::map_or_else instead of an if let/else
|
||||
--> $DIR/option_if_let_else.rs:289:19
|
||||
--> $DIR/option_if_let_else.rs:295:19
|
||||
|
|
||||
LL | let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())`
|
||||
|
|
|
|||
Loading…
Reference in a new issue