Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix option_if_let_else #7573

Merged
merged 2 commits into from
Aug 30, 2021
+237 −23
Merged

Fix option_if_let_else #7573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b3ca9a
Fix `option_if_let_else`
Jarcho Aug 16, 2021
3e5dcba
Small cleanup of `option_if_let_else` and additional tests.
Jarcho Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions clippy_lints/src/option_if_let_else.rs
View file
Open in desktop
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,
Jarcho marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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