have coercion supply back the target type

The `try_coerce` method coerces from a source to a target
type, possibly inserting adjustments. It should guarantee
that the post-adjustment type is a subtype of the target type
(or else that some side-constraint has been registered which will lead
to an error). However, it used to return the (possibly adjusted) source
as the type of the expression rather than the target. This led to
less good downstream errors.

To work around this, the code around blocks -- and particular tail
expressions in blocks -- had some special case manipulation. However,
since that code is now using the more general `CoerceMany` construct (to
account for breaks), it can no longer take advantage of that. This lead
to some regressions in compile-fail tests were errors were reported at
"less good" locations than before.

This change modifies coercions to return the target type when successful
rather the source type. This extends the behavior from blocks to all
coercions. Typically this has limited effect but on a few tests yielded
better errors results (and avoided regressions, of course).

This change also restores the hint about removing semicolons which went
missing (by giving 'force-unit' coercions a chance to add notes etc).

Author: nikomatsakis

src/librustc_typeck/check/_match.rs

@@ -498,7 +498,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             if is_if_let_fallback {
                 let cause = self.cause(expr.span, ObligationCauseCode::IfExpressionWithNoElse);
                 assert!(arm_ty.is_nil());
-                coercion.coerce_forced_unit(self, &cause);
+                coercion.coerce_forced_unit(self, &cause, &mut |_| ());
             } else {
                 let cause = self.cause(expr.span, ObligationCauseCode::MatchExpressionArm {
                     arm_span: arm.body.span,

src/librustc_typeck/check/coercion.rs

@@ -74,6 +74,7 @@ use rustc::ty::fold::TypeFoldable;
 use rustc::ty::error::TypeError;
 use rustc::ty::relate::RelateResult;
 use rustc::ty::subst::Subst;
+use errors::DiagnosticBuilder;
 use syntax::abi;
 use syntax::feature_gate;
 use syntax::ptr::P;
@@ -718,7 +719,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                 }
                 self.write_adjustment(expr.id, adjustment);
             }
-            Ok(adjustment.target)
+
+            // We should now have added sufficient adjustments etc to
+            // ensure that the type of expression, post-adjustment, is
+            // a subtype of target.
+            Ok(target)
         })
     }
 
@@ -1000,7 +1005,7 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
                       expression_ty: Ty<'tcx>,
                       expression_diverges: Diverges)
     {
-        self.coerce_inner(fcx, cause, Some(expression), expression_ty, expression_diverges)
+        self.coerce_inner(fcx, cause, Some(expression), expression_ty, expression_diverges, None)
     }
 
     /// Indicates that one of the inputs is a "forced unit". This
@@ -1011,15 +1016,21 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
     /// purposes. Note that these tend to correspond to cases where
     /// the `()` expression is implicit in the source, and hence we do
     /// not take an expression argument.
+    ///
+    /// The `augment_error` gives you a chance to extend the error
+    /// message, in case any results (e.g., we use this to suggest
+    /// removing a `;`).
     pub fn coerce_forced_unit<'a>(&mut self,
                                   fcx: &FnCtxt<'a, 'gcx, 'tcx>,
-                                  cause: &ObligationCause<'tcx>)
+                                  cause: &ObligationCause<'tcx>,
+                                  augment_error: &mut FnMut(&mut DiagnosticBuilder))
     {
         self.coerce_inner(fcx,
                           cause,
                           None,
                           fcx.tcx.mk_nil(),
-                          Diverges::Maybe)
+                          Diverges::Maybe,
+                          Some(augment_error))
     }
 
     /// The inner coercion "engine". If `expression` is `None`, this
@@ -1030,7 +1041,8 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
                         cause: &ObligationCause<'tcx>,
                         expression: Option<&'gcx hir::Expr>,
                         mut expression_ty: Ty<'tcx>,
-                        expression_diverges: Diverges)
+                        expression_diverges: Diverges,
+                        augment_error: Option<&mut FnMut(&mut DiagnosticBuilder)>)
     {
         // Incorporate whatever type inference information we have
         // until now; in principle we might also want to process
@@ -1126,19 +1138,25 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
                     (self.final_ty.unwrap_or(self.expected_ty), expression_ty)
                 };
 
+                let mut db;
                 match cause.code {
                     ObligationCauseCode::ReturnNoExpression => {
-                        struct_span_err!(fcx.tcx.sess, cause.span, E0069,
-                                         "`return;` in a function whose return type is not `()`")
-                            .span_label(cause.span, &format!("return type is not ()"))
-                            .emit();
+                        db = struct_span_err!(
+                            fcx.tcx.sess, cause.span, E0069,
+                            "`return;` in a function whose return type is not `()`");
+                        db.span_label(cause.span, &format!("return type is not ()"));
                     }
                     _ => {
-                        fcx.report_mismatched_types(cause, expected, found, err)
-                           .emit();
+                        db = fcx.report_mismatched_types(cause, expected, found, err);
                     }
                 }
 
+                if let Some(mut augment_error) = augment_error {
+                    augment_error(&mut db);
+                }
+
+                db.emit();
+
                 self.final_ty = Some(fcx.tcx.types.err);
             }
         }

src/librustc_typeck/check/mod.rs

@@ -87,7 +87,7 @@ use fmt_macros::{Parser, Piece, Position};
 use hir::def::{Def, CtorKind};
 use hir::def_id::{CrateNum, DefId, LOCAL_CRATE};
 use rustc_back::slice::ref_slice;
-use rustc::infer::{self, InferCtxt, InferOk, RegionVariableOrigin, TypeTrace};
+use rustc::infer::{self, InferCtxt, InferOk, RegionVariableOrigin};
 use rustc::infer::type_variable::{self, TypeVariableOrigin};
 use rustc::ty::subst::{Kind, Subst, Substs};
 use rustc::traits::{self, ObligationCause, ObligationCauseCode, Reveal};
@@ -99,6 +99,7 @@ use rustc::ty::adjustment;
 use rustc::ty::fold::{BottomUpFolder, TypeFoldable};
 use rustc::ty::maps::Providers;
 use rustc::ty::util::{Representability, IntTypeExt};
+use errors::DiagnosticBuilder;
 use require_c_abi_if_variadic;
 use session::{Session, CompileResult};
 use TypeAndSubsts;
@@ -2875,7 +2876,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             self.diverges.set(cond_diverges | then_diverges & else_diverges);
         } else {
             let else_cause = self.cause(sp, ObligationCauseCode::IfExpressionWithNoElse);
-            coerce.coerce_forced_unit(self, &else_cause);
+            coerce.coerce_forced_unit(self, &else_cause, &mut |_| ());
 
             // If the condition is false we can't diverge.
             self.diverges.set(cond_diverges);
@@ -3591,7 +3592,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                           coerce.coerce(self, &cause, e, e_ty, e_diverges);
                       } else {
                           assert!(e_ty.is_nil());
-                          coerce.coerce_forced_unit(self, &cause);
+                          coerce.coerce_forced_unit(self, &cause, &mut |_| ());
                       }
                   } else {
                       // If `ctxt.coerce` is `None`, we can just ignore
@@ -3626,7 +3627,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             } else {
                 let mut coercion = self.ret_coercion.as_ref().unwrap().borrow_mut();
                 let cause = self.cause(expr.span, ObligationCauseCode::ReturnNoExpression);
-                coercion.coerce_forced_unit(self, &cause);
+                coercion.coerce_forced_unit(self, &cause, &mut |_| ());
             }
             tcx.types.never
           }
@@ -4158,8 +4159,23 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                               tail_expr,
                               tail_expr_ty,
                               self.diverges.get()); // TODO
-            } else if !self.diverges.get().always() {
-                coerce.coerce_forced_unit(self, &self.misc(blk.span));
+            } else {
+                // Subtle: if there is no explicit tail expression,
+                // that is typically equivalent to a tail expression
+                // of `()` -- except if the block diverges. In that
+                // case, there is no value supplied from the tail
+                // expression (assuming there are no other breaks,
+                // this implies that the type of the block will be
+                // `!`).
+                if !self.diverges.get().always() {
+                    coerce.coerce_forced_unit(self, &self.misc(blk.span), &mut |err| {
+                        if let Some(expected_ty) = expected.only_has_type(self) {
+                            self.consider_hint_about_removing_semicolon(blk,
+                                                                        expected_ty,
+                                                                        err);
+                        }
+                    });
+                }
             }
         });
 
@@ -4175,43 +4191,42 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
         ty
     }
 
-    pub fn check_block_no_expr(&self, blk: &'gcx hir::Block, ty: Ty<'tcx>, ety: Ty<'tcx>) {
-        // We're not diverging and there's an expected type, which,
-        // in case it's not `()`, could result in an error higher-up.
-        // We have a chance to error here early and be more helpful.
-        let cause = self.misc(blk.span);
-        let trace = TypeTrace::types(&cause, false, ty, ety);
-        match self.sub_types(false, &cause, ty, ety) {
-            Ok(InferOk { obligations, .. }) => {
-                // FIXME(#32730) propagate obligations
-                assert!(obligations.is_empty());
-            },
-            Err(err) => {
-                let mut err = self.report_and_explain_type_error(trace, &err);
-
-                // Be helpful when the user wrote `{... expr;}` and
-                // taking the `;` off is enough to fix the error.
-                let mut extra_semi = None;
-                if let Some(stmt) = blk.stmts.last() {
-                    if let hir::StmtSemi(ref e, _) = stmt.node {
-                        if self.can_sub_types(self.node_ty(e.id), ety).is_ok() {
-                            extra_semi = Some(stmt);
-                        }
-                    }
-                }
-                if let Some(last_stmt) = extra_semi {
-                    let original_span = original_sp(last_stmt.span, blk.span);
-                    let span_semi = Span {
-                        lo: original_span.hi - BytePos(1),
-                        hi: original_span.hi,
-                        ctxt: original_span.ctxt,
-                    };
-                    err.span_help(span_semi, "consider removing this semicolon:");
-                }
-
-                err.emit();
-            }
+    /// A common error is to add an extra semicolon:
+    ///
+    /// ```
+    /// fn foo() -> usize {
+    ///     22;
+    /// }
+    /// ```
+    ///
+    /// This routine checks if the final statement in a block is an
+    /// expression with an explicit semicolon whose type is compatible
+    /// with `expected_ty`. If so, it suggests removing the semicolon.
+    fn consider_hint_about_removing_semicolon(&self,
+                                              blk: &'gcx hir::Block,
+                                              expected_ty: Ty<'tcx>,
+                                              err: &mut DiagnosticBuilder) {
+        // Be helpful when the user wrote `{... expr;}` and
+        // taking the `;` off is enough to fix the error.
+        let last_stmt = match blk.stmts.last() {
+            Some(s) => s,
+            None => return,
+        };
+        let last_expr = match last_stmt.node {
+            hir::StmtSemi(ref e, _) => e,
+            _ => return,
+        };
+        let last_expr_ty = self.expr_ty(last_expr);
+        if self.can_sub_types(last_expr_ty, expected_ty).is_err() {
+            return;
         }
+        let original_span = original_sp(last_stmt.span, blk.span);
+        let span_semi = Span {
+            lo: original_span.hi - BytePos(1),
+            hi: original_span.hi,
+            ctxt: original_span.ctxt,
+        };
+        err.span_help(span_semi, "consider removing this semicolon:");
     }
 
     // Instantiates the given path, which must refer to an item with the given

src/test/compile-fail/issue-15965.rs

@@ -11,7 +11,7 @@
 fn main() {
     return
         { return () }
-//~^ ERROR expected function, found `!`
+//~^ ERROR the type of this value must be known in this context
     ()
     ;
 }

src/test/compile-fail/issue-2149.rs

@@ -21,5 +21,5 @@ impl<A> vec_monad<A> for Vec<A> {
 }
 fn main() {
     ["hi"].bind(|x| [x] );
-    //~^ ERROR no method named `bind` found for type `[&'static str; 1]` in the current scope
+    //~^ ERROR no method named `bind` found for type `[&str; 1]` in the current scope
 }

src/test/compile-fail/region-invariant-static-error-reporting.rs

@@ -13,9 +13,6 @@
 // over time, but this test used to exhibit some pretty bogus messages
 // that were not remotely helpful.
 
-// error-pattern:cannot infer
-// error-pattern:cannot outlive the lifetime 'a
-// error-pattern:must be valid for the static lifetime
 // error-pattern:cannot infer
 // error-pattern:cannot outlive the lifetime 'a
 // error-pattern:must be valid for the static lifetime

src/test/compile-fail/regions-bounds.rs

@@ -16,11 +16,11 @@ struct an_enum<'a>(&'a isize);
 struct a_class<'a> { x:&'a isize }
 
 fn a_fn1<'a,'b>(e: an_enum<'a>) -> an_enum<'b> {
-    return e; //~^ ERROR mismatched types
+    return e; //~ ERROR mismatched types
 }
 
 fn a_fn3<'a,'b>(e: a_class<'a>) -> a_class<'b> {
-    return e; //~^ ERROR mismatched types
+    return e; //~ ERROR mismatched types
 }
 
 fn main() { }