Auto merge of rust-lang#5266 - sinkuu:questionmark, r=flip1995

Lint `if let Some` and early return in question_mark lint

Fixes rust-lang#5260

changelog: lint `if let Some` and early return in `question_mark` lint

clippy_lints/src/question_mark.rs

@@ -1,13 +1,15 @@
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::{def, Block, Expr, ExprKind, StmtKind};
+use rustc_hir::{def, BindingAnnotation, Block, Expr, ExprKind, MatchSource, PatKind, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 use crate::utils::paths::{OPTION, OPTION_NONE};
 use crate::utils::sugg::Sugg;
-use crate::utils::{higher, match_def_path, match_type, span_lint_and_then, SpanlessEq};
+use crate::utils::{
+    higher, match_def_path, match_qpath, match_type, snippet_with_applicability, span_lint_and_sugg, SpanlessEq,
+};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for expressions that could be replaced by the question mark operator.
@@ -55,7 +57,8 @@ impl QuestionMark {
             if Self::is_option(cx, subject);
 
             then {
-                let receiver_str = &Sugg::hir(cx, subject, "..");
+                let mut applicability = Applicability::MachineApplicable;
+                let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
                 let mut replacement: Option<String> = None;
                 if let Some(else_) = else_ {
                     if_chain! {
@@ -74,25 +77,61 @@ impl QuestionMark {
                 }
 
                 if let Some(replacement_str) = replacement {
-                    span_lint_and_then(
+                    span_lint_and_sugg(
                         cx,
                         QUESTION_MARK,
                         expr.span,
                         "this block may be rewritten with the `?` operator",
-                        |db| {
-                            db.span_suggestion(
-                                expr.span,
-                                "replace_it_with",
-                                replacement_str,
-                                Applicability::MaybeIncorrect, // snippet
-                            );
-                        }
+                        "replace it with",
+                        replacement_str,
+                        applicability,
                     )
                }
             }
         }
     }
 
+    fn check_if_let_some_and_early_return_none(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
+        if_chain! {
+            if let ExprKind::Match(subject, arms, source) = &expr.kind;
+            if *source == MatchSource::IfLetDesugar { contains_else_clause: true };
+            if Self::is_option(cx, subject);
+
+            if let PatKind::TupleStruct(path1, fields, None) = &arms[0].pat.kind;
+            if match_qpath(path1, &["Some"]);
+            if let PatKind::Binding(annot, _, bind, _) = &fields[0].kind;
+            let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
+
+            if let ExprKind::Block(block, None) = &arms[0].body.kind;
+            if block.stmts.is_empty();
+            if let Some(trailing_expr) = &block.expr;
+            if let ExprKind::Path(path) = &trailing_expr.kind;
+            if match_qpath(path, &[&bind.as_str()]);
+
+            if let PatKind::Wild = arms[1].pat.kind;
+            if Self::expression_returns_none(cx, arms[1].body);
+            then {
+                let mut applicability = Applicability::MachineApplicable;
+                let receiver_str = snippet_with_applicability(cx, subject.span, "..", &mut applicability);
+                let replacement = format!(
+                    "{}{}?",
+                    receiver_str,
+                    if by_ref { ".as_ref()" } else { "" },
+                );
+
+                span_lint_and_sugg(
+                    cx,
+                    QUESTION_MARK,
+                    expr.span,
+                    "this if-let-else may be rewritten with the `?` operator",
+                    "replace it with",
+                    replacement,
+                    applicability,
+                )
+            }
+        }
+    }
+
     fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr<'_>) -> bool {
         let expr_ty = cx.tables.expr_ty(expression);
 
@@ -158,5 +197,6 @@ impl QuestionMark {
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMark {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
         Self::check_is_none_and_early_return_none(cx, expr);
+        Self::check_if_let_some_and_early_return_none(cx, expr);
     }
 }

clippy_lints/src/types.rs

@@ -1698,12 +1698,7 @@ fn detect_absurd_comparison<'a, 'tcx>(
         return None;
     }
 
-    let normalized = normalize_comparison(op, lhs, rhs);
-    let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized {
-        val
-    } else {
-        return None;
-    };
+    let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?;
 
     let lx = detect_extreme_expr(cx, normalized_lhs);
     let rx = detect_extreme_expr(cx, normalized_rhs);

tests/ui/question_mark.fixed

@@ -0,0 +1,113 @@
+// run-rustfix
+#![allow(unreachable_code)]
+
+fn some_func(a: Option<u32>) -> Option<u32> {
+    a?;
+
+    a
+}
+
+fn some_other_func(a: Option<u32>) -> Option<u32> {
+    if a.is_none() {
+        return None;
+    } else {
+        return Some(0);
+    }
+    unreachable!()
+}
+
+pub enum SeemsOption<T> {
+    Some(T),
+    None,
+}
+
+impl<T> SeemsOption<T> {
+    pub fn is_none(&self) -> bool {
+        match *self {
+            SeemsOption::None => true,
+            SeemsOption::Some(_) => false,
+        }
+    }
+}
+
+fn returns_something_similar_to_option(a: SeemsOption<u32>) -> SeemsOption<u32> {
+    if a.is_none() {
+        return SeemsOption::None;
+    }
+
+    a
+}
+
+pub struct CopyStruct {
+    pub opt: Option<u32>,
+}
+
+impl CopyStruct {
+    #[rustfmt::skip]
+    pub fn func(&self) -> Option<u32> {
+        (self.opt)?;
+
+        self.opt?;
+
+        let _ = Some(self.opt?);
+
+        let _ = self.opt?;
+
+        self.opt
+    }
+}
+
+#[derive(Clone)]
+pub struct MoveStruct {
+    pub opt: Option<Vec<u32>>,
+}
+
+impl MoveStruct {
+    pub fn ref_func(&self) -> Option<Vec<u32>> {
+        self.opt.as_ref()?;
+
+        self.opt.clone()
+    }
+
+    pub fn mov_func_reuse(self) -> Option<Vec<u32>> {
+        self.opt.as_ref()?;
+
+        self.opt
+    }
+
+    pub fn mov_func_no_use(self) -> Option<Vec<u32>> {
+        self.opt.as_ref()?;
+        Some(Vec::new())
+    }
+
+    pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
+        let v: &Vec<_> = self.opt.as_ref()?;
+
+        Some(v.clone())
+    }
+
+    pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
+        let v = self.opt?;
+
+        Some(v)
+    }
+}
+
+fn main() {
+    some_func(Some(42));
+    some_func(None);
+    some_other_func(Some(42));
+
+    let copy_struct = CopyStruct { opt: Some(54) };
+    copy_struct.func();
+
+    let move_struct = MoveStruct {
+        opt: Some(vec![42, 1337]),
+    };
+    move_struct.ref_func();
+    move_struct.clone().mov_func_reuse();
+    move_struct.mov_func_no_use();
+
+    let so = SeemsOption::Some(45);
+    returns_something_similar_to_option(so);
+}

tests/ui/question_mark.rs

@@ -1,3 +1,6 @@
+// run-rustfix
+#![allow(unreachable_code)]
+
 fn some_func(a: Option<u32>) -> Option<u32> {
     if a.is_none() {
         return None;
@@ -58,6 +61,12 @@ impl CopyStruct {
             self.opt
         };
 
+        let _ = if let Some(x) = self.opt {
+            x
+        } else {
+            return None;
+        };
+
         self.opt
     }
 }
@@ -90,11 +99,32 @@ impl MoveStruct {
         }
         Some(Vec::new())
     }
+
+    pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
+        let v: &Vec<_> = if let Some(ref v) = self.opt {
+            v
+        } else {
+            return None;
+        };
+
+        Some(v.clone())
+    }
+
+    pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
+        let v = if let Some(v) = self.opt {
+            v
+        } else {
+            return None;
+        };
+
+        Some(v)
+    }
 }
 
 fn main() {
     some_func(Some(42));
     some_func(None);
+    some_other_func(Some(42));
 
     let copy_struct = CopyStruct { opt: Some(54) };
     copy_struct.func();

tests/ui/question_mark.stderr

@@ -1,63 +1,96 @@
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:2:5
+  --> $DIR/question_mark.rs:5:5
    |
 LL | /     if a.is_none() {
 LL | |         return None;
 LL | |     }
-   | |_____^ help: replace_it_with: `a?;`
+   | |_____^ help: replace it with: `a?;`
    |
    = note: `-D clippy::question-mark` implied by `-D warnings`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:47:9
+  --> $DIR/question_mark.rs:50:9
    |
 LL | /         if (self.opt).is_none() {
 LL | |             return None;
 LL | |         }
-   | |_________^ help: replace_it_with: `(self.opt)?;`
+   | |_________^ help: replace it with: `(self.opt)?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:51:9
+  --> $DIR/question_mark.rs:54:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None
 LL | |         }
-   | |_________^ help: replace_it_with: `self.opt?;`
+   | |_________^ help: replace it with: `self.opt?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:55:17
+  --> $DIR/question_mark.rs:58:17
    |
 LL |           let _ = if self.opt.is_none() {
    |  _________________^
 LL | |             return None;
 LL | |         } else {
 LL | |             self.opt
 LL | |         };
-   | |_________^ help: replace_it_with: `Some(self.opt?)`
+   | |_________^ help: replace it with: `Some(self.opt?)`
+
+error: this if-let-else may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:64:17
+   |
+LL |           let _ = if let Some(x) = self.opt {
+   |  _________________^
+LL | |             x
+LL | |         } else {
+LL | |             return None;
+LL | |         };
+   | |_________^ help: replace it with: `self.opt?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:72:9
+  --> $DIR/question_mark.rs:81:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
-   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+   | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:80:9
+  --> $DIR/question_mark.rs:89:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
-   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+   | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:88:9
+  --> $DIR/question_mark.rs:97:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
-   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+   | |_________^ help: replace it with: `self.opt.as_ref()?;`
+
+error: this if-let-else may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:104:26
+   |
+LL |           let v: &Vec<_> = if let Some(ref v) = self.opt {
+   |  __________________________^
+LL | |             v
+LL | |         } else {
+LL | |             return None;
+LL | |         };
+   | |_________^ help: replace it with: `self.opt.as_ref()?`
+
+error: this if-let-else may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:114:17
+   |
+LL |           let v = if let Some(v) = self.opt {
+   |  _________________^
+LL | |             v
+LL | |         } else {
+LL | |             return None;
+LL | |         };
+   | |_________^ help: replace it with: `self.opt?`
 
-error: aborting due to 7 previous errors
+error: aborting due to 10 previous errors