Auto merge of #81458 - estebank:match-stmt-remove-semi, r=oli-obk

Detect match statement intended to be tail expression

CC #24157

compiler/rustc_typeck/src/check/_match.rs

@@ -1,10 +1,11 @@
 use crate::check::coercion::{AsCoercionSite, CoerceMany};
 use crate::check::{Diverges, Expectation, FnCtxt, Needs};
+use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir::{self as hir, ExprKind};
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_infer::traits::Obligation;
 use rustc_middle::ty::{self, ToPredicate, Ty, TyS};
-use rustc_span::Span;
+use rustc_span::{MultiSpan, Span};
 use rustc_trait_selection::opaque_types::InferCtxtExt as _;
 use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
 use rustc_trait_selection::traits::{
@@ -206,7 +207,64 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     ),
                 };
                 let cause = self.cause(span, code);
-                coercion.coerce(self, &cause, &arm.body, arm_ty);
+                let can_coerce_to_return_ty = match self.ret_coercion.as_ref() {
+                    Some(ret_coercion) if self.in_tail_expr => {
+                        let ret_ty = ret_coercion.borrow().expected_ty();
+                        let ret_ty = self.inh.infcx.shallow_resolve(ret_ty);
+                        self.can_coerce(arm_ty, ret_ty)
+                            && prior_arm_ty.map_or(true, |t| self.can_coerce(t, ret_ty))
+                            // The match arms need to unify for the case of `impl Trait`.
+                            && !matches!(ret_ty.kind(), ty::Opaque(..))
+                    }
+                    _ => false,
+                };
+
+                // This is the moral equivalent of `coercion.coerce(self, cause, arm.body, arm_ty)`.
+                // We use it this way to be able to expand on the potential error and detect when a
+                // `match` tail statement could be a tail expression instead. If so, we suggest
+                // removing the stray semicolon.
+                coercion.coerce_inner(
+                    self,
+                    &cause,
+                    Some(&arm.body),
+                    arm_ty,
+                    Some(&mut |err: &mut DiagnosticBuilder<'_>| {
+                        if let (Expectation::IsLast(stmt), Some(ret), true) =
+                            (orig_expected, self.ret_type_span, can_coerce_to_return_ty)
+                        {
+                            let semi_span = expr.span.shrink_to_hi().with_hi(stmt.hi());
+                            let mut ret_span: MultiSpan = semi_span.into();
+                            ret_span.push_span_label(
+                                expr.span,
+                                "this could be implicitly returned but it is a statement, not a \
+                                 tail expression"
+                                    .to_owned(),
+                            );
+                            ret_span.push_span_label(
+                                ret,
+                                "the `match` arms can conform to this return type".to_owned(),
+                            );
+                            ret_span.push_span_label(
+                                semi_span,
+                                "the `match` is a statement because of this semicolon, consider \
+                                 removing it"
+                                    .to_owned(),
+                            );
+                            err.span_note(
+                                ret_span,
+                                "you might have meant to return the `match` expression",
+                            );
+                            err.tool_only_span_suggestion(
+                                semi_span,
+                                "remove this semicolon",
+                                String::new(),
+                                Applicability::MaybeIncorrect,
+                            );
+                        }
+                    }),
+                    false,
+                );
+
                 other_arms.push(arm_span);
                 if other_arms.len() > 5 {
                     other_arms.remove(0);

compiler/rustc_typeck/src/check/coercion.rs

@@ -1237,7 +1237,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
     /// The inner coercion "engine". If `expression` is `None`, this
     /// is a forced-unit case, and hence `expression_ty` must be
     /// `Nil`.
-    fn coerce_inner<'a>(
+    crate fn coerce_inner<'a>(
         &mut self,
         fcx: &FnCtxt<'a, 'tcx>,
         cause: &ObligationCause<'tcx>,

compiler/rustc_typeck/src/check/expectation.rs

@@ -21,6 +21,8 @@ pub enum Expectation<'tcx> {
     /// This rvalue expression will be wrapped in `&` or `Box` and coerced
     /// to `&Ty` or `Box<Ty>`, respectively. `Ty` is `[A]` or `Trait`.
     ExpectRvalueLikeUnsized(Ty<'tcx>),
+
+    IsLast(Span),
 }
 
 impl<'a, 'tcx> Expectation<'tcx> {
@@ -79,19 +81,20 @@ impl<'a, 'tcx> Expectation<'tcx> {
 
     // Resolves `expected` by a single level if it is a variable. If
     // there is no expected type or resolution is not possible (e.g.,
-    // no constraints yet present), just returns `None`.
+    // no constraints yet present), just returns `self`.
     fn resolve(self, fcx: &FnCtxt<'a, 'tcx>) -> Expectation<'tcx> {
         match self {
             NoExpectation => NoExpectation,
             ExpectCastableToType(t) => ExpectCastableToType(fcx.resolve_vars_if_possible(t)),
             ExpectHasType(t) => ExpectHasType(fcx.resolve_vars_if_possible(t)),
             ExpectRvalueLikeUnsized(t) => ExpectRvalueLikeUnsized(fcx.resolve_vars_if_possible(t)),
+            IsLast(sp) => IsLast(sp),
         }
     }
 
     pub(super) fn to_option(self, fcx: &FnCtxt<'a, 'tcx>) -> Option<Ty<'tcx>> {
         match self.resolve(fcx) {
-            NoExpectation => None,
+            NoExpectation | IsLast(_) => None,
             ExpectCastableToType(ty) | ExpectHasType(ty) | ExpectRvalueLikeUnsized(ty) => Some(ty),
         }
     }
@@ -103,7 +106,9 @@ impl<'a, 'tcx> Expectation<'tcx> {
     pub(super) fn only_has_type(self, fcx: &FnCtxt<'a, 'tcx>) -> Option<Ty<'tcx>> {
         match self.resolve(fcx) {
             ExpectHasType(ty) => Some(ty),
-            NoExpectation | ExpectCastableToType(_) | ExpectRvalueLikeUnsized(_) => None,
+            NoExpectation | ExpectCastableToType(_) | ExpectRvalueLikeUnsized(_) | IsLast(_) => {
+                None
+            }
         }
     }
 

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

@@ -539,7 +539,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         self.overwrite_local_ty_if_err(local, ty, pat_ty);
     }
 
-    pub fn check_stmt(&self, stmt: &'tcx hir::Stmt<'tcx>) {
+    pub fn check_stmt(&self, stmt: &'tcx hir::Stmt<'tcx>, is_last: bool) {
         // Don't do all the complex logic below for `DeclItem`.
         match stmt.kind {
             hir::StmtKind::Item(..) => return,
@@ -567,7 +567,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 });
             }
             hir::StmtKind::Semi(ref expr) => {
-                self.check_expr(&expr);
+                // All of this is equivalent to calling `check_expr`, but it is inlined out here
+                // in order to capture the fact that this `match` is the last statement in its
+                // function. This is done for better suggestions to remove the `;`.
+                let expectation = match expr.kind {
+                    hir::ExprKind::Match(..) if is_last => IsLast(stmt.span),
+                    _ => NoExpectation,
+                };
+                self.check_expr_with_expectation(expr, expectation);
             }
         }
 
@@ -626,8 +633,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let ctxt = BreakableCtxt { coerce: Some(coerce), may_break: false };
 
         let (ctxt, ()) = self.with_breakable_ctxt(blk.hir_id, ctxt, || {
-            for s in blk.stmts {
-                self.check_stmt(s);
+            for (pos, s) in blk.stmts.iter().enumerate() {
+                self.check_stmt(s, blk.stmts.len() - 1 == pos);
             }
 
             // check the tail expression **without** holding the

src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.rs

@@ -0,0 +1,30 @@
+pub trait Foo {}
+
+struct Bar;
+struct Baz;
+
+impl Foo for Bar { }
+impl Foo for Baz { }
+
+fn not_all_paths(a: &str) -> u32 { //~ ERROR mismatched types
+    match a {
+        "baz" => 0,
+        _ => 1,
+    };
+}
+
+fn right(b: &str) -> Box<dyn Foo> {
+    match b {
+        "baz" => Box::new(Baz),
+        _ => Box::new(Bar),
+    }
+}
+
+fn wrong(c: &str) -> Box<dyn Foo> { //~ ERROR mismatched types
+    match c {
+        "baz" => Box::new(Baz),
+        _ => Box::new(Bar), //~ ERROR `match` arms have incompatible types
+    };
+}
+
+fn main() {}

src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.stderr

@@ -0,0 +1,51 @@
+error[E0308]: mismatched types
+  --> $DIR/match-with-different-arm-types-as-stmt-instead-of-expr.rs:9:30
+   |
+LL | fn not_all_paths(a: &str) -> u32 {
+   |    -------------             ^^^ expected `u32`, found `()`
+   |    |
+   |    implicitly returns `()` as its body has no tail or `return` expression
+...
+LL |     };
+   |      - help: consider removing this semicolon
+
+error[E0308]: `match` arms have incompatible types
+  --> $DIR/match-with-different-arm-types-as-stmt-instead-of-expr.rs:26:14
+   |
+LL | /     match c {
+LL | |         "baz" => Box::new(Baz),
+   | |                  ------------- this is found to be of type `Box<Baz>`
+LL | |         _ => Box::new(Bar),
+   | |              ^^^^^^^^^^^^^ expected struct `Baz`, found struct `Bar`
+LL | |     };
+   | |_____- `match` arms have incompatible types
+   |
+   = note: expected type `Box<Baz>`
+            found struct `Box<Bar>`
+note: you might have meant to return the `match` expression
+  --> $DIR/match-with-different-arm-types-as-stmt-instead-of-expr.rs:27:6
+   |
+LL |   fn wrong(c: &str) -> Box<dyn Foo> {
+   |                        ------------ the `match` arms can conform to this return type
+LL | /     match c {
+LL | |         "baz" => Box::new(Baz),
+LL | |         _ => Box::new(Bar),
+LL | |     };
+   | |     -^ the `match` is a statement because of this semicolon, consider removing it
+   | |_____|
+   |       this could be implicitly returned but it is a statement, not a tail expression
+
+error[E0308]: mismatched types
+  --> $DIR/match-with-different-arm-types-as-stmt-instead-of-expr.rs:23:22
+   |
+LL | fn wrong(c: &str) -> Box<dyn Foo> {
+   |    -----             ^^^^^^^^^^^^ expected struct `Box`, found `()`
+   |    |
+   |    implicitly returns `()` as its body has no tail or `return` expression
+   |
+   = note: expected struct `Box<(dyn Foo + 'static)>`
+           found unit type `()`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0308`.