From d7ee55bfd0240095d0614ec5d33774171a177d86 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <chevalier@alum.wellesley.edu>
Date: Thu, 4 Aug 2011 18:34:05 -0700
Subject: [PATCH] (Almost) Always unify a function tail expr with the function
 result type

typeck::check_fn had an exception for the case where the tail expr
was compatible with type nil -- in that case, it doesn't unify the
tail expr's type with the enclosing function's result type. This
seems wrong to me. There are several test cases in Issue #719
that illustrate why. If the tail expr has type T, for some type
variable T that isn't resolved when this check happens, then T
never gets unified with anything, which is incorrect -- T should
be unified with the result type of the enclosing function. (The
bug was occurring because an unconstrained type variable is
compatible with type nil.)

Instead, I removed the check for type nil and added a check that
the function isn't an iterator -- if it's an iterator, I don't
check the tail expr's type against the function result type,
as that wouldn't make sense.

However, this broke two test cases, and after discussion with
brson, I understood that the purpose of the check was to allow
semicolons to be omitted in some cases. The whole thing seems
rather ad hoc. But I came up with a hacky compromise solution:
instead of checking whether the tailexpr type is *compatible*
with nil, we now just check whether it *is* nil. This also
necessitates calling resolve_type_vars_if_possible before
the check happens, which worries me. But, this fixes the bug
from Issue #719 without requiring changes to any test cases.

Closes #719 but I didn't try every variation -- so reopen the bug
if one of the variations still doesn't work.
---
 src/comp/middle/typeck.rs            | 37 +++++++++++++++++++++++-----
 src/test/run-pass/unify-return-ty.rs | 11 +++++++++
 2 files changed, 42 insertions(+), 6 deletions(-)
 create mode 100644 src/test/run-pass/unify-return-ty.rs

diff --git a/src/comp/middle/typeck.rs b/src/comp/middle/typeck.rs
index e7089faba257..99e10eafa3cf 100644
--- a/src/comp/middle/typeck.rs
+++ b/src/comp/middle/typeck.rs
@@ -1080,6 +1080,7 @@ fn variant_arg_types(ccx: &@crate_ctxt, sp: &span, vid: &ast::def_id,
 mod writeback {
 
     export resolve_type_vars_in_block;
+    export resolve_type_vars_in_expr;
 
     fn resolve_type_vars_in_type(fcx: &@fn_ctxt, sp: &span, typ: ty::t) ->
        option::t[ty::t] {
@@ -1168,6 +1169,20 @@ mod writeback {
         // Ignore items
     }
 
+    fn resolve_type_vars_in_expr(fcx: &@fn_ctxt, e: &@ast::expr) -> bool {
+        let wbcx = {fcx: fcx, mutable success: true};
+        let visit = visit::mk_vt
+            (@{visit_item: visit_item,
+               visit_stmt: visit_stmt,
+               visit_expr: visit_expr,
+               visit_block: visit_block,
+               visit_pat: visit_pat,
+               visit_local: visit_local
+               with *visit::default_visitor()});
+        visit::visit_expr(e, wbcx, visit);
+        ret wbcx.success;
+    }
+
     fn resolve_type_vars_in_block(fcx: &@fn_ctxt, blk: &ast::blk) -> bool {
         let wbcx = {fcx: fcx, mutable success: true};
         let visit = visit::mk_vt
@@ -2651,13 +2666,23 @@ fn check_fn(ccx: &@crate_ctxt, f: &ast::_fn, id: &ast::node_id,
       _ { }
     }
 
-    if option::is_some(body.node.expr) {
+// For non-iterator fns, we unify the tail expr's type with the
+// function result type, if there is a tail expr.
+// We don't do this check for an iterator, as the tail expr doesn't
+// have to have the result type of the iterator.
+    if option::is_some(body.node.expr) && f.proto != ast::proto_iter {
         let tail_expr = option::get(body.node.expr);
-        let tail_expr_ty = expr_ty(ccx.tcx, tail_expr);
-        // Have to exclude ty_nil to allow functions to end in
-        // while expressions, etc.
-        let nil = ty::mk_nil(fcx.ccx.tcx);
-        if !are_compatible(fcx, nil, tail_expr_ty) {
+        // The use of resolve_type_vars_if_possible makes me very
+        // afraid :-(
+        let tail_expr_ty = resolve_type_vars_if_possible(
+          fcx, expr_ty(ccx.tcx, tail_expr));
+        // Hacky compromise: use eq and not are_compatible
+        // This allows things like while loops and ifs with no
+        // else to appear in tail position without a trailing
+        // semicolon when the return type is non-nil, while
+        // making sure to unify the tailexpr-type with the result
+        // type when the tailexpr-type is just a type variable.
+        if !ty::eq_ty(tail_expr_ty, ty::mk_nil(ccx.tcx)) {
             demand::simple(fcx, tail_expr.span, fcx.ret_ty, tail_expr_ty);
         }
     }
diff --git a/src/test/run-pass/unify-return-ty.rs b/src/test/run-pass/unify-return-ty.rs
new file mode 100644
index 000000000000..f9f8e6725ca6
--- /dev/null
+++ b/src/test/run-pass/unify-return-ty.rs
@@ -0,0 +1,11 @@
+// Tests that the tail expr in null() has its type
+// unified with the type *T, and so the type variable
+// in that type gets resolved.
+use std;
+import std::unsafe;
+
+fn null[T]() -> *T { unsafe::reinterpret_cast(0) }
+
+fn main() {
+    null[int]();
+}
\ No newline at end of file