Skip to content

Commit

Permalink
Auto merge of #7573 - Jarcho:option_if_let_else, r=giraffate
Browse files Browse the repository at this point in the history
Fix `option_if_let_else`

fixes: #5822
fixes: #6737
fixes: #7567

The inference from #6137 still exists so I'm not sure if this should be moved from the nursery. Before doing that though I'd almost want to see this split into two lints. One suggesting `map_or` and the other suggesting `map_or_else`.

`map_or_else` tends to have longer expressions for both branches so it doesn't end up much shorter than a match expression in practice. It also seems most people find it harder to read. `map_or` at least has the terseness benefit of being on one line most of the time, especially when the `None` branch is just a literal or path expression.

changelog: `break` and `continue` statments local to the would-be closure are allowed in `option_if_let_else`
changelog: don't lint in const contexts  in `option_if_let_else`
changelog: don't lint when yield expressions are used  in `option_if_let_else`
changelog: don't lint when the captures made by the would-be closure conflict with the other branch  in `option_if_let_else`
changelog: don't lint when a field of a local is used when the type could be pontentially moved from  in `option_if_let_else`
changelog: in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure  in `option_if_let_else`
  • Loading branch information
bors committed Aug 30, 2021
2 parents 5e1e9b0 + 3e5dcba commit fd30241
Show file tree
Hide file tree
Showing 5 changed files with 237 additions and 23 deletions.
53 changes: 42 additions & 11 deletions clippy_lints/src/option_if_let_else.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::usage::contains_return_break_continue_macro;
use clippy_utils::{eager_or_lazy, in_macro, is_else_clause, is_lang_ctor};
use clippy_utils::{
can_move_expr_to_closure, eager_or_lazy, in_constant, in_macro, is_else_clause, is_lang_ctor, peel_hir_expr_while,
CaptureKind,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::LangItem::OptionSome;
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp};
use rustc_hir::{
def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, Path, QPath, UnOp,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;
Expand Down Expand Up @@ -127,21 +131,30 @@ fn detect_option_if_let_else<'tcx>(
) -> Option<OptionIfLetElseOccurence> {
if_chain! {
if !in_macro(expr.span); // Don't lint macros, because it behaves weirdly
if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
if !in_constant(cx, expr.hir_id);
if let ExprKind::Match(cond_expr, [some_arm, none_arm], MatchSource::IfLetDesugar{contains_else_clause: true})
= &expr.kind;
if !is_else_clause(cx.tcx, expr);
if arms.len() == 2;
if !is_result_ok(cx, cond_expr); // Don't lint on Result::ok because a different lint does it already
if let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &arms[0].pat.kind;
if let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &some_arm.pat.kind;
if is_lang_ctor(cx, struct_qpath, OptionSome);
if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind;
if !contains_return_break_continue_macro(arms[0].body);
if !contains_return_break_continue_macro(arms[1].body);
if let Some(some_captures) = can_move_expr_to_closure(cx, some_arm.body);
if let Some(none_captures) = can_move_expr_to_closure(cx, none_arm.body);
if some_captures
.iter()
.filter_map(|(id, &c)| none_captures.get(id).map(|&c2| (c, c2)))
.all(|(x, y)| x.is_imm_ref() && y.is_imm_ref());

then {
let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" };
let some_body = extract_body_from_arm(&arms[0])?;
let none_body = extract_body_from_arm(&arms[1])?;
let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { "map_or" } else { "map_or_else" };
let some_body = extract_body_from_arm(some_arm)?;
let none_body = extract_body_from_arm(none_arm)?;
let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) {
"map_or"
} else {
"map_or_else"
};
let capture_name = id.name.to_ident_string();
let (as_ref, as_mut) = match &cond_expr.kind {
ExprKind::AddrOf(_, Mutability::Not, _) => (true, false),
Expand All @@ -153,6 +166,24 @@ fn detect_option_if_let_else<'tcx>(
ExprKind::Unary(UnOp::Deref, expr) | ExprKind::AddrOf(_, _, expr) => expr,
_ => cond_expr,
};
// Check if captures the closure will need conflict with borrows made in the scrutinee.
// TODO: check all the references made in the scrutinee expression. This will require interacting
// with the borrow checker. Currently only `<local>[.<field>]*` is checked for.
if as_ref || as_mut {
let e = peel_hir_expr_while(cond_expr, |e| match e.kind {
ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e),
_ => None,
});
if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(local_id), .. })) = e.kind {
match some_captures.get(local_id)
.or_else(|| (method_sugg == "map_or_else").then(|| ()).and_then(|_| none_captures.get(local_id)))
{
Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return None,
Some(CaptureKind::Ref(Mutability::Not)) if as_mut => return None,
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
}
}
}
Some(OptionIfLetElseOccurence {
option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut),
method_sugg: method_sugg.to_string(),
Expand Down
5 changes: 5 additions & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,11 @@ pub enum CaptureKind {
Value,
Ref(Mutability),
}
impl CaptureKind {
pub fn is_imm_ref(self) -> bool {
self == Self::Ref(Mutability::Not)
}
}
impl std::ops::BitOr for CaptureKind {
type Output = Self;
fn bitor(self, rhs: Self) -> Self::Output {
Expand Down
62 changes: 62 additions & 0 deletions tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// edition:2018
// run-rustfix
#![warn(clippy::option_if_let_else)]
#![allow(clippy::redundant_closure)]
Expand Down Expand Up @@ -86,4 +87,65 @@ fn main() {
test_map_or_else(None);
let _ = negative_tests(None);
let _ = impure_else(None);

let _ = Some(0).map_or(0, |x| loop {
if x == 0 {
break x;
}
});

// #7576
const fn _f(x: Option<u32>) -> u32 {
// Don't lint, `map_or` isn't const
if let Some(x) = x { x } else { 10 }
}

// #5822
let s = String::new();
// Don't lint, `Some` branch consumes `s`, but else branch uses `s`
let _ = if let Some(x) = Some(0) {
let s = s;
s.len() + x
} else {
s.len()
};

let s = String::new();
// Lint, both branches immutably borrow `s`.
let _ = Some(0).map_or_else(|| s.len(), |x| s.len() + x);

let s = String::new();
// Lint, `Some` branch consumes `s`, but else branch doesn't use `s`.
let _ = Some(0).map_or(1, |x| {
let s = s;
s.len() + x
});

let s = Some(String::new());
// Don't lint, `Some` branch borrows `s`, but else branch consumes `s`
let _ = if let Some(x) = &s {
x.len()
} else {
let _s = s;
10
};

let mut s = Some(String::new());
// Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s`
let _ = if let Some(x) = &mut s {
x.push_str("test");
x.len()
} else {
let _s = &s;
10
};

async fn _f1(x: u32) -> u32 {
x
}

async fn _f2() {
// Don't lint. `await` can't be moved into a closure.
let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 };
}
}
68 changes: 68 additions & 0 deletions tests/ui/option_if_let_else.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// edition:2018
// run-rustfix
#![warn(clippy::option_if_let_else)]
#![allow(clippy::redundant_closure)]
Expand Down Expand Up @@ -105,4 +106,71 @@ fn main() {
test_map_or_else(None);
let _ = negative_tests(None);
let _ = impure_else(None);

let _ = if let Some(x) = Some(0) {
loop {
if x == 0 {
break x;
}
}
} else {
0
};

// #7576
const fn _f(x: Option<u32>) -> u32 {
// Don't lint, `map_or` isn't const
if let Some(x) = x { x } else { 10 }
}

// #5822
let s = String::new();
// Don't lint, `Some` branch consumes `s`, but else branch uses `s`
let _ = if let Some(x) = Some(0) {
let s = s;
s.len() + x
} else {
s.len()
};

let s = String::new();
// Lint, both branches immutably borrow `s`.
let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };

let s = String::new();
// Lint, `Some` branch consumes `s`, but else branch doesn't use `s`.
let _ = if let Some(x) = Some(0) {
let s = s;
s.len() + x
} else {
1
};

let s = Some(String::new());
// Don't lint, `Some` branch borrows `s`, but else branch consumes `s`
let _ = if let Some(x) = &s {
x.len()
} else {
let _s = s;
10
};

let mut s = Some(String::new());
// Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s`
let _ = if let Some(x) = &mut s {
x.push_str("test");
x.len()
} else {
let _s = &s;
10
};

async fn _f1(x: u32) -> u32 {
x
}

async fn _f2() {
// Don't lint. `await` can't be moved into a closure.
let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 };
}
}
72 changes: 60 additions & 12 deletions tests/ui/option_if_let_else.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:7:5
--> $DIR/option_if_let_else.rs:8:5
|
LL | / if let Some(x) = string {
LL | | (true, x)
Expand All @@ -11,19 +11,19 @@ LL | | }
= note: `-D clippy::option-if-let-else` implied by `-D warnings`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:25:13
--> $DIR/option_if_let_else.rs:26:13
|
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:26:13
--> $DIR/option_if_let_else.rs:27:13
|
LL | let _ = if let Some(s) = &num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:27:13
--> $DIR/option_if_let_else.rs:28:13
|
LL | let _ = if let Some(s) = &mut num {
| _____________^
Expand All @@ -43,13 +43,13 @@ LL ~ });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:33:13
--> $DIR/option_if_let_else.rs:34:13
|
LL | let _ = if let Some(ref s) = num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:34:13
--> $DIR/option_if_let_else.rs:35:13
|
LL | let _ = if let Some(mut s) = num {
| _____________^
Expand All @@ -69,7 +69,7 @@ LL ~ });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:40:13
--> $DIR/option_if_let_else.rs:41:13
|
LL | let _ = if let Some(ref mut s) = num {
| _____________^
Expand All @@ -89,7 +89,7 @@ LL ~ });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:49:5
--> $DIR/option_if_let_else.rs:50:5
|
LL | / if let Some(x) = arg {
LL | | let y = x * x;
Expand All @@ -108,7 +108,7 @@ LL + })
|

error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:62:13
--> $DIR/option_if_let_else.rs:63:13
|
LL | let _ = if let Some(x) = arg {
| _____________^
Expand All @@ -120,7 +120,7 @@ LL | | };
| |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`

error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:71:13
--> $DIR/option_if_let_else.rs:72:13
|
LL | let _ = if let Some(x) = arg {
| _____________^
Expand All @@ -143,10 +143,58 @@ LL ~ }, |x| x * x * x * x);
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:100:13
--> $DIR/option_if_let_else.rs:101:13
|
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`

error: aborting due to 11 previous errors
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:110:13
|
LL | let _ = if let Some(x) = Some(0) {
| _____________^
LL | | loop {
LL | | if x == 0 {
LL | | break x;
... |
LL | | 0
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = Some(0).map_or(0, |x| loop {
LL + if x == 0 {
LL + break x;
LL + }
LL ~ });
|

error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:138:13
|
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:142:13
|
LL | let _ = if let Some(x) = Some(0) {
| _____________^
LL | | let s = s;
LL | | s.len() + x
LL | | } else {
LL | | 1
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = Some(0).map_or(1, |x| {
LL + let s = s;
LL + s.len() + x
LL ~ });
|

error: aborting due to 14 previous errors

0 comments on commit fd30241

Please sign in to comment.