From 1d24f07271f7be9f67c7ba7edb7071f58995e294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 27 Jan 2021 18:27:13 -0800 Subject: [PATCH] Detect match statement intended to be tail expression CC #24157 --- compiler/rustc_typeck/src/check/_match.rs | 62 ++++++++++++++++++- compiler/rustc_typeck/src/check/coercion.rs | 2 +- .../rustc_typeck/src/check/expectation.rs | 11 +++- .../rustc_typeck/src/check/fn_ctxt/checks.rs | 15 +++-- ...erent-arm-types-as-stmt-instead-of-expr.rs | 30 +++++++++ ...t-arm-types-as-stmt-instead-of-expr.stderr | 51 +++++++++++++++ 6 files changed, 161 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.rs create mode 100644 src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.stderr diff --git a/compiler/rustc_typeck/src/check/_match.rs b/compiler/rustc_typeck/src/check/_match.rs index 30e0e3eecd4..cfd4e0830ef 100644 --- a/compiler/rustc_typeck/src/check/_match.rs +++ b/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); diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs index 159c97d8bfa..e3aacc44a02 100644 --- a/compiler/rustc_typeck/src/check/coercion.rs +++ b/compiler/rustc_typeck/src/check/coercion.rs @@ -1236,7 +1236,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>, diff --git a/compiler/rustc_typeck/src/check/expectation.rs b/compiler/rustc_typeck/src/check/expectation.rs index 5a5fc893d65..22be10a731f 100644 --- a/compiler/rustc_typeck/src/check/expectation.rs +++ b/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`, 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> { 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> { match self.resolve(fcx) { ExpectHasType(ty) => Some(ty), - NoExpectation | ExpectCastableToType(_) | ExpectRvalueLikeUnsized(_) => None, + NoExpectation | ExpectCastableToType(_) | ExpectRvalueLikeUnsized(_) | IsLast(_) => { + None + } } } diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs index 155c10e8916..5b8b25c2100 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs +++ b/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 diff --git a/src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.rs b/src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.rs new file mode 100644 index 00000000000..0360ce6e6b8 --- /dev/null +++ b/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 { + match b { + "baz" => Box::new(Baz), + _ => Box::new(Bar), + } +} + +fn wrong(c: &str) -> Box { //~ ERROR mismatched types + match c { + "baz" => Box::new(Baz), + _ => Box::new(Bar), //~ ERROR `match` arms have incompatible types + }; +} + +fn main() {} diff --git a/src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.stderr b/src/test/ui/suggestions/match-with-different-arm-types-as-stmt-instead-of-expr.stderr new file mode 100644 index 00000000000..7dce97468b6 --- /dev/null +++ b/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` +LL | | _ => Box::new(Bar), + | | ^^^^^^^^^^^^^ expected struct `Baz`, found struct `Bar` +LL | | }; + | |_____- `match` arms have incompatible types + | + = note: expected type `Box` + found struct `Box` +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 { + | ------------ 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 { + | ----- ^^^^^^^^^^^^ 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`.