extends functionality of question mark lint

Author: J-ZhengLi

clippy_lints/src/lib.rs

@@ -5,6 +5,7 @@
 #![feature(control_flow_enum)]
 #![feature(drain_filter)]
 #![feature(iter_intersperse)]
+#![feature(let_chains)]
 #![feature(let_else)]
 #![feature(once_cell)]
 #![feature(rustc_private)]

clippy_lints/src/question_mark.rs

@@ -6,11 +6,11 @@ use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::{eq_expr_value, is_lang_ctor, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::LangItem::{OptionNone, OptionSome, ResultOk};
-use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind};
+use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind, PathSegment, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::sym;
+use rustc_span::{sym, symbol::Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -61,21 +61,21 @@ impl QuestionMark {
             if let ExprKind::MethodCall(segment, args, _) = &cond.kind;
             if let Some(subject) = args.get(0);
             if (Self::option_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_none)) ||
-                (Self::result_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_err));
+                (Self::result_check_and_early_return(cx, subject, then, None) && segment.ident.name == sym!(is_err));
             then {
                 let mut applicability = Applicability::MachineApplicable;
-                let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
+                let suggestion = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
                 let mut replacement: Option<String> = None;
                 if let Some(else_inner) = r#else {
                     if eq_expr_value(cx, subject, peel_blocks(else_inner)) {
-                        replacement = Some(format!("Some({}?)", receiver_str));
+                        replacement = Some(format!("Some({}?)", suggestion));
                     }
                 } else if Self::moves_by_default(cx, subject)
                     && !matches!(subject.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
                 {
-                    replacement = Some(format!("{}.as_ref()?;", receiver_str));
+                    replacement = Some(format!("{}.as_ref()?;", suggestion));
                 } else {
-                    replacement = Some(format!("{}?;", receiver_str));
+                    replacement = Some(format!("{}?;", suggestion));
                 }
 
                 if let Some(replacement_str) = replacement {
@@ -93,21 +93,52 @@ impl QuestionMark {
         }
     }
 
+    /// Checks if the given expression on the given context matches the following structure:
+    ///
+    /// ```ignore
+    /// if let Some(x) = option {
+    ///    return x;
+    /// }
+    /// ```
+    ///
+    /// ```ignore
+    /// if let Err(err) = result {
+    ///     return result;
+    /// }
+    /// ```
+    ///
+    /// ```ignore
+    /// if let Ok(x) = result {
+    ///     return x;
+    /// }
+    /// ```
+    ///
+    /// If it matches, it will suggest to use the question mark operator instead
     fn check_if_let_some_or_err_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>) {
         if_chain! {
-            if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) })
+            if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else })
                 = higher::IfLet::hir(cx, expr);
+            // Make sure this expression is not part of `.. else if ..`,
+            // otherwise it will cause trouble when replacing blocks with question mark.
+            if !Self::expr_is_else_if(cx, expr);
             if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
-            if (Self::option_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, OptionSome)) ||
-                (Self::result_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, ResultOk));
-
-            if let PatKind::Binding(annot, bind_id, _, _) = fields[0].kind;
+            let nested_expr = if_else.unwrap_or(if_then);
+            if let PatKind::Binding(annot, bind_id, ident, _) = fields[0].kind;
+            if (Self::result_check_and_early_return(cx, let_expr, nested_expr, Some(ident.name)) &&
+                (is_lang_ctor(cx, path1, ResultOk) || is_lang_ctor(cx, path1, ResultErr))) ||
+                (Self::option_check_and_early_return(cx, let_expr, nested_expr) &&
+                is_lang_ctor(cx, path1, OptionSome)) && path_to_local_id(peel_blocks(if_then), bind_id);
             let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
-            if path_to_local_id(peel_blocks(if_then), bind_id);
             then {
                 let mut applicability = Applicability::MachineApplicable;
                 let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
-                let replacement = format!("{}{}?", receiver_str, if by_ref { ".as_ref()" } else { "" },);
+                let append_semi = Self::is_semicolon_needed(if_then, if_else);
+                let replacement = format!(
+                    "{}{}?{}",
+                    receiver_str,
+                    if by_ref { ".as_ref()" } else { "" },
+                    if append_semi { ";" } else { "" }
+                );
 
                 span_lint_and_sugg(
                     cx,
@@ -122,8 +153,54 @@ impl QuestionMark {
         }
     }
 
-    fn result_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
-        Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr)
+    /// Check whether the encountered expression contains `else if`
+    fn expr_is_else_if(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+        if let ExprKind::If(..) = expr.kind
+            && let Some(parent_expr) = clippy_utils::get_parent_expr(cx, expr)
+            && let ExprKind::If(..) = parent_expr.kind
+        {
+            return true;
+        }
+        false
+    }
+
+    /// Check whether the fixed code needs semicolon after `?`
+    ///
+    /// The fixed code of this expression doesn't need semi
+    /// ```ignore
+    /// let _ = if let Some(x) = Some(0) {
+    ///     x
+    /// } else {
+    ///     return None;
+    /// };
+    /// ```
+    ///
+    /// The fixed code of this expression needs semi
+    /// ```ignore
+    /// if let Err(err) = foo() {
+    ///     return Err(err);
+    /// }
+    /// ```
+    fn is_semicolon_needed(if_then: &Expr<'_>, if_else: Option<&'_ Expr<'_>>) -> bool {
+        let if_then_kind = &peel_blocks_with_stmt(if_then).kind;
+        let if_then_is_ret_stmt = matches!(if_then_kind, ExprKind::Ret(_));
+
+        if if_else.is_none() {
+            return if_then_is_ret_stmt;
+        }
+        if let ExprKind::Ret(_) = peel_blocks_with_stmt(if_else.unwrap()).kind {
+            return if_then_is_ret_stmt;
+        }
+        false
+    }
+
+    fn result_check_and_early_return(
+        cx: &LateContext<'_>,
+        expr: &Expr<'_>,
+        nested_expr: &Expr<'_>,
+        pat_symbol: Option<Symbol>,
+    ) -> bool {
+        Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, pat_symbol)
     }
 
     fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
@@ -156,10 +233,32 @@ impl QuestionMark {
         }
     }
 
-    fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
-        match peel_blocks_with_stmt(expr).kind {
-            ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr),
+    fn expression_returns_unmodified_err(
+        cx: &LateContext<'_>,
+        expr: &Expr<'_>,
+        cond_expr: &Expr<'_>,
+        pat_symbol: Option<Symbol>,
+    ) -> bool {
+        match &peel_blocks_with_stmt(expr).kind {
+            ExprKind::Ret(Some(ret_expr)) => {
+                Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, pat_symbol)
+            },
             ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
+            ExprKind::Call(call_expr, args_expr) => {
+                if let ExprKind::Path(qpath) = &call_expr.kind
+                    && let QPath::Resolved(_, path) = qpath
+                    && let Some(segment) = path.segments.first()
+                    && let Some(pat_sym) = pat_symbol
+                    && let Some(arg) = args_expr.first()
+                    && let ExprKind::Path(arg_qpath) = &arg.kind
+                    && let QPath::Resolved(_, arg_path) = arg_qpath
+                    && let Some(PathSegment { ident, .. }) = arg_path.segments.first()
+                {
+                    return segment.ident.name == sym::Err && pat_sym == ident.name
+                }
+
+                false
+            },
             _ => false,
         }
     }

tests/ui/question_mark.fixed

@@ -154,6 +154,64 @@ fn f() -> NotOption {
     NotOption::First
 }
 
+#[allow(dead_code)]
+mod issue8288 {
+    fn do_something() {}
+
+    fn foo() -> Result<(), i32> {
+        Ok(())
+    }
+
+    fn bar() -> Result<(), i32> {
+        foo()?;
+
+        Ok(())
+    }
+
+    fn baz() -> Result<(), i32> {
+        foo()?;
+        do_something();
+
+        Ok(())
+    }
+
+    // No warning
+    fn qux() -> Result<(), i32> {
+        if let Err(err) = foo() {
+            do_something();
+            return Err(err);
+        }
+
+        Ok(())
+    }
+
+    // No warning
+    fn quux() -> Option<i32> {
+        if let Err(err) = foo() {
+            return Some(err);
+        }
+
+        None
+    }
+
+    // No warning
+    fn quuz() -> Result<(), i32> {
+        if true {
+            Ok(())
+        } else if let Err(e) = foo() {
+            Err(e)
+        } else {
+            Err(-1)
+        }
+    }
+
+    // No warning
+    #[allow(clippy::manual_map)]
+    fn option_map() -> Option<bool> {
+        if let Some(a) = Some(false) { Some(!a) } else { None }
+    }
+}
+
 fn main() {
     some_func(Some(42));
     some_func(None);

tests/ui/question_mark.rs

@@ -186,6 +186,68 @@ fn f() -> NotOption {
     NotOption::First
 }
 
+#[allow(dead_code)]
+mod issue8288 {
+    fn do_something() {}
+
+    fn foo() -> Result<(), i32> {
+        Ok(())
+    }
+
+    fn bar() -> Result<(), i32> {
+        if let Err(err) = foo() {
+            return Err(err);
+        }
+
+        Ok(())
+    }
+
+    fn baz() -> Result<(), i32> {
+        if let Err(err) = foo() {
+            return Err(err);
+        }
+        do_something();
+
+        Ok(())
+    }
+
+    // No warning
+    fn qux() -> Result<(), i32> {
+        if let Err(err) = foo() {
+            do_something();
+            return Err(err);
+        }
+
+        Ok(())
+    }
+
+    // No warning
+    fn quux() -> Option<i32> {
+        if let Err(err) = foo() {
+            return Some(err);
+        }
+
+        None
+    }
+
+    // No warning
+    fn quuz() -> Result<(), i32> {
+        if true {
+            Ok(())
+        } else if let Err(e) = foo() {
+            Err(e)
+        } else {
+            Err(-1)
+        }
+    }
+
+    // No warning
+    #[allow(clippy::manual_map)]
+    fn option_map() -> Option<bool> {
+        if let Some(a) = Some(false) { Some(!a) } else { None }
+    }
+}
+
 fn main() {
     some_func(Some(42));
     some_func(None);

tests/ui/question_mark.stderr

@@ -114,5 +114,21 @@ LL | |         return x;
 LL | |     }
    | |_____^ help: replace it with: `x?;`
 
-error: aborting due to 13 previous errors
+error: this if-let-else may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:198:9
+   |
+LL | /         if let Err(err) = foo() {
+LL | |             return Err(err);
+LL | |         }
+   | |_________^ help: replace it with: `foo()?;`
+
+error: this if-let-else may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:206:9
+   |
+LL | /         if let Err(err) = foo() {
+LL | |             return Err(err);
+LL | |         }
+   | |_________^ help: replace it with: `foo()?;`
+
+error: aborting due to 15 previous errors