option_if_let_else - distinguish pure from impure else expressions

rust-lang · Aug 21, 2020 · 56cea57 · 56cea57
1 parent 4104611
commit 56cea57
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 28 deletions.
diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs
@@ -4,31 +4,26 @@ use crate::utils::{match_type, paths, span_lint_and_sugg};
 use if_chain::if_chain;
 
 use rustc_errors::Applicability;
+use rustc_hir::def::{DefKind, Res};
 use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
-use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp};
+use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, Path, QPath, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
     /// **What it does:**
     /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more
-    /// idiomatically done with `Option::map_or` (if the else bit is a simple
-    /// expression) or `Option::map_or_else` (if the else bit is a longer
-    /// block).
+    /// idiomatically done with `Option::map_or` (if the else bit is a pure
+    /// expression) or `Option::map_or_else` (if the else bit is an impure
+    /// expresion).
     ///
     /// **Why is this bad?**
     /// Using the dedicated functions of the Option type is clearer and
     /// more concise than an if let expression.
     ///
     /// **Known problems:**
-    /// This lint uses whether the block is just an expression or if it has
-    /// more statements to decide whether to use `Option::map_or` or
-    /// `Option::map_or_else`. If you have a single expression which calls
-    /// an expensive function, then it would be more efficient to use
-    /// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
-    ///
-    /// Also, this lint uses a deliberately conservative metric for checking
+    /// This lint uses a deliberately conservative metric for checking
     /// if the inside of either body contains breaks or continues which will
     /// cause it to not suggest a fix if either block contains a loop with
     /// continues or breaks contained within the loop.
@@ -92,13 +87,15 @@ struct OptionIfLetElseOccurence {
 struct ReturnBreakContinueMacroVisitor {
     seen_return_break_continue: bool,
 }
+
 impl ReturnBreakContinueMacroVisitor {
     fn new() -> ReturnBreakContinueMacroVisitor {
         ReturnBreakContinueMacroVisitor {
             seen_return_break_continue: false,
         }
     }
 }
+
 impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor {
     type Map = Map<'tcx>;
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
@@ -157,7 +154,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> {
 }
 
 /// If this is the else body of an if/else expression, then we need to wrap
-/// it in curcly braces. Otherwise, we don't.
+/// it in curly braces. Otherwise, we don't.
 fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
         if let Some(Expr {
@@ -196,6 +193,35 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo
     )
 }
 
+/// Is the expr pure (is it free from side-effects)?
+/// If yes, we can use `map_or`, else we must use `map_or_else` to preserve the behavior.
+/// This implementation can be improved and identify more pure patterns.
+fn is_pure(expr: &Expr<'_>) -> bool {
+    match expr.kind {
+        ExprKind::Lit(..) | ExprKind::Path(..) => true,
+        ExprKind::AddrOf(_, _, addr_of_expr) => is_pure(addr_of_expr),
+        ExprKind::Tup(tup_exprs) => tup_exprs.iter().all(|expr| is_pure(expr)),
+        ExprKind::Struct(_, fields, expr) => {
+            fields.iter().all(|f| is_pure(f.expr)) && expr.map_or(true, |e| is_pure(e))
+        },
+        ExprKind::Call(
+            &Expr {
+                kind:
+                    ExprKind::Path(QPath::Resolved(
+                        _,
+                        Path {
+                            res: Res::Def(DefKind::Ctor(..) | DefKind::Variant, ..),
+                            ..
+                        },
+                    )),
+                ..
+            },
+            args,
+        ) => args.iter().all(|expr| is_pure(expr)),
+        _ => false,
+    }
+}
+
 /// If this expression is the option if let/else construct we're detecting, then
 /// this function returns an `OptionIfLetElseOccurence` struct with details if
 /// this construct is found, or None if this construct is not found.
@@ -214,10 +240,7 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Op
             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 = match &none_body.kind {
-                ExprKind::Block(..) => "map_or_else",
-                _ => "map_or",
-            };
+            let method_sugg = if is_pure(none_body) { "map_or" } else { "map_or_else" };
             let capture_name = id.name.to_ident_string();
             let wrap_braces = should_wrap_in_braces(cx, expr);
             let (as_ref, as_mut) = match &cond_expr.kind {

diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
@@ -1,5 +1,6 @@
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
+#![allow(clippy::redundant_closure)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))
@@ -36,6 +37,14 @@ fn longer_body(arg: Option<u32>) -> u32 {
     })
 }
 
+fn impure_else(arg: Option<i32>) {
+    let side_effect = || {
+        println!("return 1");
+        1
+    };
+    let _ = arg.map_or_else(|| side_effect(), |x| x);
+}
+
 fn test_map_or_else(arg: Option<u32>) {
     let _ = arg.map_or_else(|| {
         let mut y = 1;
@@ -71,4 +80,5 @@ fn main() {
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);
+    let _ = impure_else(None);
 }
diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
@@ -1,5 +1,6 @@
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
+#![allow(clippy::redundant_closure)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     if let Some(x) = string {
@@ -52,6 +53,19 @@ fn longer_body(arg: Option<u32>) -> u32 {
     }
 }
 
+fn impure_else(arg: Option<i32>) {
+    let side_effect = || {
+        println!("return 1");
+        1
+    };
+    let _ = if let Some(x) = arg {
+        x
+    } else {
+        // map_or_else must be suggested
+        side_effect()
+    };
+}
+
 fn test_map_or_else(arg: Option<u32>) {
     let _ = if let Some(x) = arg {
         x * x * x * x
@@ -89,4 +103,5 @@ fn main() {
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);
+    let _ = impure_else(None);
 }
diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr
@@ -1,5 +1,5 @@
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:5:5
+  --> $DIR/option_if_let_else.rs:6:5
    |
 LL | /     if let Some(x) = string {
 LL | |         (true, x)
@@ -11,7 +11,7 @@ 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:15:12
+  --> $DIR/option_if_let_else.rs:16:12
    |
 LL |       } else if let Some(x) = string {
    |  ____________^
@@ -22,19 +22,19 @@ LL | |     }
    | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:23:13
+  --> $DIR/option_if_let_else.rs:24: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:24:13
+  --> $DIR/option_if_let_else.rs:25: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:25:13
+  --> $DIR/option_if_let_else.rs:26:13
    |
 LL |       let _ = if let Some(s) = &mut num {
    |  _____________^
@@ -54,13 +54,13 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:31:13
+  --> $DIR/option_if_let_else.rs:32: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:32:13
+  --> $DIR/option_if_let_else.rs:33:13
    |
 LL |       let _ = if let Some(mut s) = num {
    |  _____________^
@@ -80,7 +80,7 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:38:13
+  --> $DIR/option_if_let_else.rs:39:13
    |
 LL |       let _ = if let Some(ref mut s) = num {
    |  _____________^
@@ -100,7 +100,7 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:47:5
+  --> $DIR/option_if_let_else.rs:48:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -119,7 +119,19 @@ LL |     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:56:13
+  --> $DIR/option_if_let_else.rs:61:13
+   |
+LL |       let _ = if let Some(x) = arg {
+   |  _____________^
+LL | |         x
+LL | |     } else {
+LL | |         // map_or_else must be suggested
+LL | |         side_effect()
+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:70:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -142,10 +154,10 @@ LL |     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:85:13
+  --> $DIR/option_if_let_else.rs:99: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: aborting due to 12 previous errors