Rollup merge of rust-lang#120479 - estebank:issue-61788, r=wesleywiser

Suggest turning `if let` into irrefutable `let` if appropriate

When encountering an `if let` tail expression without an `else` arm for an enum with a single variant, suggest writing an irrefutable `let` binding instead.

```
error[E0317]: `if` may be missing an `else` clause
  --> $DIR/irrefutable-if-let-without-else.rs:8:5
   |
LL |   fn foo(x: Enum) -> i32 {
   |                      --- expected `i32` because of this return type
LL | /     if let Enum::Variant(value) = x {
LL | |         value
LL | |     }
   | |_____^ expected `i32`, found `()`
   |
   = note: `if` expressions without `else` evaluate to `()`
   = help: consider adding an `else` block that evaluates to the expected type
help: consider using an irrefutable `let` binding instead
   |
LL ~     let Enum::Variant(value) = x;
LL ~         value
   |
```

Fix rust-lang#61788.

Author: Nadrieril

compiler/rustc_hir_typeck/src/_match.rs

@@ -1,7 +1,11 @@
 use crate::coercion::{AsCoercionSite, CoerceMany};
 use crate::{Diverges, Expectation, FnCtxt, Needs};
-use rustc_errors::Diagnostic;
-use rustc_hir::{self as hir, ExprKind};
+use rustc_errors::{Applicability, Diagnostic};
+use rustc_hir::{
+    self as hir,
+    def::{CtorOf, DefKind, Res},
+    ExprKind, PatKind,
+};
 use rustc_hir_pretty::ty_to_string;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_infer::traits::Obligation;
@@ -273,7 +277,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// Returns `true` if there was an error forcing the coercion to the `()` type.
     pub(super) fn if_fallback_coercion<T>(
         &self,
-        span: Span,
+        if_span: Span,
+        cond_expr: &'tcx hir::Expr<'tcx>,
         then_expr: &'tcx hir::Expr<'tcx>,
         coercion: &mut CoerceMany<'tcx, '_, T>,
     ) -> bool
@@ -283,29 +288,106 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // If this `if` expr is the parent's function return expr,
         // the cause of the type coercion is the return type, point at it. (#25228)
         let hir_id = self.tcx.hir().parent_id(self.tcx.hir().parent_id(then_expr.hir_id));
-        let ret_reason = self.maybe_get_coercion_reason(hir_id, span);
-        let cause = self.cause(span, ObligationCauseCode::IfExpressionWithNoElse);
+        let ret_reason = self.maybe_get_coercion_reason(hir_id, if_span);
+        let cause = self.cause(if_span, ObligationCauseCode::IfExpressionWithNoElse);
         let mut error = false;
         coercion.coerce_forced_unit(
             self,
             &cause,
-            |err| {
-                if let Some((span, msg)) = &ret_reason {
-                    err.span_label(*span, msg.clone());
-                } else if let ExprKind::Block(block, _) = &then_expr.kind
-                    && let Some(expr) = &block.expr
-                {
-                    err.span_label(expr.span, "found here");
-                }
-                err.note("`if` expressions without `else` evaluate to `()`");
-                err.help("consider adding an `else` block that evaluates to the expected type");
-                error = true;
-            },
+            |err| self.explain_if_expr(err, ret_reason, if_span, cond_expr, then_expr, &mut error),
             false,
         );
         error
     }
 
+    /// Explain why `if` expressions without `else` evaluate to `()` and detect likely irrefutable
+    /// `if let PAT = EXPR {}` expressions that could be turned into `let PAT = EXPR;`.
+    fn explain_if_expr(
+        &self,
+        err: &mut Diagnostic,
+        ret_reason: Option<(Span, String)>,
+        if_span: Span,
+        cond_expr: &'tcx hir::Expr<'tcx>,
+        then_expr: &'tcx hir::Expr<'tcx>,
+        error: &mut bool,
+    ) {
+        if let Some((if_span, msg)) = ret_reason {
+            err.span_label(if_span, msg.clone());
+        } else if let ExprKind::Block(block, _) = then_expr.kind
+            && let Some(expr) = block.expr
+        {
+            err.span_label(expr.span, "found here");
+        }
+        err.note("`if` expressions without `else` evaluate to `()`");
+        err.help("consider adding an `else` block that evaluates to the expected type");
+        *error = true;
+        if let ExprKind::Let(hir::Let { span, pat, init, .. }) = cond_expr.kind
+            && let ExprKind::Block(block, _) = then_expr.kind
+            // Refutability checks occur on the MIR, so we approximate it here by checking
+            // if we have an enum with a single variant or a struct in the pattern.
+            && let PatKind::TupleStruct(qpath, ..) | PatKind::Struct(qpath, ..) = pat.kind
+            && let hir::QPath::Resolved(_, path) = qpath
+        {
+            match path.res {
+                Res::Def(DefKind::Ctor(CtorOf::Struct, _), _) => {
+                    // Structs are always irrefutable. Their fields might not be, but we
+                    // don't check for that here, it's only an approximation.
+                }
+                Res::Def(DefKind::Ctor(CtorOf::Variant, _), def_id)
+                    if self
+                        .tcx
+                        .adt_def(self.tcx.parent(self.tcx.parent(def_id)))
+                        .variants()
+                        .len()
+                        == 1 =>
+                {
+                    // There's only a single variant in the `enum`, so we can suggest the
+                    // irrefutable `let` instead of `if let`.
+                }
+                _ => return,
+            }
+
+            let mut sugg = vec![
+                // Remove the `if`
+                (if_span.until(*span), String::new()),
+            ];
+            match (block.stmts, block.expr) {
+                ([first, ..], Some(expr)) => {
+                    let padding = self
+                        .tcx
+                        .sess
+                        .source_map()
+                        .indentation_before(first.span)
+                        .unwrap_or_else(|| String::new());
+                    sugg.extend([
+                        (init.span.between(first.span), format!(";\n{padding}")),
+                        (expr.span.shrink_to_hi().with_hi(block.span.hi()), String::new()),
+                    ]);
+                }
+                ([], Some(expr)) => {
+                    let padding = self
+                        .tcx
+                        .sess
+                        .source_map()
+                        .indentation_before(expr.span)
+                        .unwrap_or_else(|| String::new());
+                    sugg.extend([
+                        (init.span.between(expr.span), format!(";\n{padding}")),
+                        (expr.span.shrink_to_hi().with_hi(block.span.hi()), String::new()),
+                    ]);
+                }
+                // If there's no value in the body, then the `if` expression would already
+                // be of type `()`, so checking for those cases is unnecessary.
+                (_, None) => return,
+            }
+            err.multipart_suggestion(
+                "consider using an irrefutable `let` binding instead",
+                sugg,
+                Applicability::MaybeIncorrect,
+            );
+        }
+    }
+
     pub fn maybe_get_coercion_reason(
         &self,
         hir_id: hir::HirId,

compiler/rustc_hir_typeck/src/expr.rs

@@ -1118,7 +1118,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             // We won't diverge unless both branches do (or the condition does).
             self.diverges.set(cond_diverges | then_diverges & else_diverges);
         } else {
-            self.if_fallback_coercion(sp, then_expr, &mut coerce);
+            self.if_fallback_coercion(sp, cond_expr, then_expr, &mut coerce);
 
             // If the condition is false we can't diverge.
             self.diverges.set(cond_diverges);

tests/ui/binding/irrefutable-if-let-without-else.fixed

@@ -0,0 +1,25 @@
+// run-rustfix
+enum Enum {
+    Variant(i32),
+}
+struct Struct(i32);
+
+fn foo(x: Enum) -> i32 {
+    let Enum::Variant(value) = x;
+        value
+}
+fn bar(x: Enum) -> i32 {
+    let Enum::Variant(value) = x;
+        let x = value + 1;
+        x
+}
+fn baz(x: Struct) -> i32 {
+    let Struct(value) = x;
+        let x = value + 1;
+        x
+}
+fn main() {
+    let _ = foo(Enum::Variant(42));
+    let _ = bar(Enum::Variant(42));
+    let _ = baz(Struct(42));
+}

tests/ui/binding/irrefutable-if-let-without-else.rs

@@ -0,0 +1,28 @@
+// run-rustfix
+enum Enum {
+    Variant(i32),
+}
+struct Struct(i32);
+
+fn foo(x: Enum) -> i32 {
+    if let Enum::Variant(value) = x { //~ ERROR `if` may be missing an `else` clause
+        value
+    }
+}
+fn bar(x: Enum) -> i32 {
+    if let Enum::Variant(value) = x { //~ ERROR `if` may be missing an `else` clause
+        let x = value + 1;
+        x
+    }
+}
+fn baz(x: Struct) -> i32 {
+    if let Struct(value) = x { //~ ERROR `if` may be missing an `else` clause
+        let x = value + 1;
+        x
+    }
+}
+fn main() {
+    let _ = foo(Enum::Variant(42));
+    let _ = bar(Enum::Variant(42));
+    let _ = baz(Struct(42));
+}

tests/ui/binding/irrefutable-if-let-without-else.stderr

@@ -0,0 +1,61 @@
+error[E0317]: `if` may be missing an `else` clause
+  --> $DIR/irrefutable-if-let-without-else.rs:8:5
+   |
+LL |   fn foo(x: Enum) -> i32 {
+   |                      --- expected `i32` because of this return type
+LL | /     if let Enum::Variant(value) = x {
+LL | |         value
+LL | |     }
+   | |_____^ expected `i32`, found `()`
+   |
+   = note: `if` expressions without `else` evaluate to `()`
+   = help: consider adding an `else` block that evaluates to the expected type
+help: consider using an irrefutable `let` binding instead
+   |
+LL ~     let Enum::Variant(value) = x;
+LL ~         value
+   |
+
+error[E0317]: `if` may be missing an `else` clause
+  --> $DIR/irrefutable-if-let-without-else.rs:13:5
+   |
+LL |   fn bar(x: Enum) -> i32 {
+   |                      --- expected `i32` because of this return type
+LL | /     if let Enum::Variant(value) = x {
+LL | |         let x = value + 1;
+LL | |         x
+LL | |     }
+   | |_____^ expected `i32`, found `()`
+   |
+   = note: `if` expressions without `else` evaluate to `()`
+   = help: consider adding an `else` block that evaluates to the expected type
+help: consider using an irrefutable `let` binding instead
+   |
+LL ~     let Enum::Variant(value) = x;
+LL ~         let x = value + 1;
+LL ~         x
+   |
+
+error[E0317]: `if` may be missing an `else` clause
+  --> $DIR/irrefutable-if-let-without-else.rs:19:5
+   |
+LL |   fn baz(x: Struct) -> i32 {
+   |                        --- expected `i32` because of this return type
+LL | /     if let Struct(value) = x {
+LL | |         let x = value + 1;
+LL | |         x
+LL | |     }
+   | |_____^ expected `i32`, found `()`
+   |
+   = note: `if` expressions without `else` evaluate to `()`
+   = help: consider adding an `else` block that evaluates to the expected type
+help: consider using an irrefutable `let` binding instead
+   |
+LL ~     let Struct(value) = x;
+LL ~         let x = value + 1;
+LL ~         x
+   |
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0317`.