Skip to content

Commit

Permalink
Auto merge of #81458 - estebank:match-stmt-remove-semi, r=oli-obk
Browse files Browse the repository at this point in the history
Detect match statement intended to be tail expression

CC #24157
  • Loading branch information
bors committed Feb 26, 2021
2 parents d95d304 + 1d24f07 commit cecdb18
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 10 deletions.
62 changes: 60 additions & 2 deletions compiler/rustc_typeck/src/check/_match.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
11 changes: 8 additions & 3 deletions compiler/rustc_typeck/src/check/expectation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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),
}
}
Expand All @@ -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
}
}
}

Expand Down
15 changes: 11 additions & 4 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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() {}
Original file line number Diff line number Diff line change
@@ -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`.

0 comments on commit cecdb18

Please sign in to comment.