Improve the error message for missing else clauses in if expressions

Author: Jakub Wieczorek

src/librustc/middle/typeck/check/mod.rs

@@ -2999,35 +2999,36 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
                        expected: Expectation) {
         check_expr_has_type(fcx, cond_expr, ty::mk_bool());
 
+        // Disregard "castable to" expectations because they
+        // can lead us astray. Consider for example `if cond
+        // {22} else {c} as u8` -- if we propagate the
+        // "castable to u8" constraint to 22, it will pick the
+        // type 22u8, which is overly constrained (c might not
+        // be a u8). In effect, the problem is that the
+        // "castable to" expectation is not the tightest thing
+        // we can say, so we want to drop it in this case.
+        // The tightest thing we can say is "must unify with
+        // else branch". Note that in the case of a "has type"
+        // constraint, this limitation does not hold.
+
+        // If the expected type is just a type variable, then don't use
+        // an expected type. Otherwise, we might write parts of the type
+        // when checking the 'then' block which are incompatible with the
+        // 'else' branch.
+        let expected = match expected.only_has_type() {
+            ExpectHasType(ety) => {
+                match infer::resolve_type(fcx.infcx(), Some(sp), ety, force_tvar) {
+                    Ok(rty) if !ty::type_is_ty_var(rty) => ExpectHasType(rty),
+                    _ => NoExpectation
+                }
+            }
+            _ => NoExpectation
+        };
+        check_block_with_expected(fcx, then_blk, expected);
+        let then_ty = fcx.node_ty(then_blk.id);
+
         let branches_ty = match opt_else_expr {
             Some(ref else_expr) => {
-                // Disregard "castable to" expectations because they
-                // can lead us astray. Consider for example `if cond
-                // {22} else {c} as u8` -- if we propagate the
-                // "castable to u8" constraint to 22, it will pick the
-                // type 22u8, which is overly constrained (c might not
-                // be a u8). In effect, the problem is that the
-                // "castable to" expectation is not the tightest thing
-                // we can say, so we want to drop it in this case.
-                // The tightest thing we can say is "must unify with
-                // else branch". Note that in the case of a "has type"
-                // constraint, this limitation does not hold.
-
-                // If the expected type is just a type variable, then don't use
-                // an expected type. Otherwise, we might write parts of the type
-                // when checking the 'then' block which are incompatible with the
-                // 'else' branch.
-                let expected = match expected.only_has_type() {
-                    ExpectHasType(ety) => {
-                        match infer::resolve_type(fcx.infcx(), Some(sp), ety, force_tvar) {
-                            Ok(rty) if !ty::type_is_ty_var(rty) => ExpectHasType(rty),
-                            _ => NoExpectation
-                        }
-                    }
-                    _ => NoExpectation
-                };
-                check_block_with_expected(fcx, then_blk, expected);
-                let then_ty = fcx.node_ty(then_blk.id);
                 check_expr_with_expectation(fcx, &**else_expr, expected);
                 let else_ty = fcx.expr_ty(&**else_expr);
                 infer::common_supertype(fcx.infcx(),
@@ -3037,8 +3038,11 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
                                         else_ty)
             }
             None => {
-                check_block_no_value(fcx, then_blk);
-                ty::mk_nil()
+                infer::common_supertype(fcx.infcx(),
+                                        infer::IfExpressionWithNoElse(sp),
+                                        false,
+                                        then_ty,
+                                        ty::mk_nil())
             }
         };
 

src/librustc/middle/typeck/infer/error_reporting.rs

@@ -366,6 +366,7 @@ impl<'a, 'tcx> ErrorReporting for InferCtxt<'a, 'tcx> {
             infer::RelateOutputImplTypes(_) => "mismatched types",
             infer::MatchExpressionArm(_, _) => "match arms have incompatible types",
             infer::IfExpression(_) => "if and else have incompatible types",
+            infer::IfExpressionWithNoElse(_) => "if may be missing an else clause",
         };
 
         self.tcx.sess.span_err(
@@ -1486,6 +1487,9 @@ impl<'a, 'tcx> ErrorReportingHelpers for InferCtxt<'a, 'tcx> {
                     infer::IfExpression(_) => {
                         format!("if and else have compatible types")
                     }
+                    infer::IfExpressionWithNoElse(_) => {
+                        format!("if may be missing an else clause")
+                    }
                 };
 
                 match self.values_str(&trace.values) {

src/librustc/middle/typeck/infer/mod.rs

@@ -121,6 +121,9 @@ pub enum TypeOrigin {
 
     // Computing common supertype in an if expression
     IfExpression(Span),
+
+    // Computing common supertype of an if expression with no else counter-part
+    IfExpressionWithNoElse(Span)
 }
 
 /// See `error_reporting.rs` for more details
@@ -1001,6 +1004,7 @@ impl TypeOrigin {
             RelateOutputImplTypes(span) => span,
             MatchExpressionArm(match_span, _) => match_span,
             IfExpression(span) => span,
+            IfExpressionWithNoElse(span) => span
         }
     }
 }
@@ -1030,6 +1034,9 @@ impl Repr for TypeOrigin {
             IfExpression(a) => {
                 format!("IfExpression({})", a.repr(tcx))
             }
+            IfExpressionWithNoElse(a) => {
+                format!("IfExpressionWithNoElse({})", a.repr(tcx))
+            }
         }
     }
 }

src/test/compile-fail/if-without-else-result.rs

@@ -8,11 +8,10 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// error-pattern:mismatched types: expected `()`, found `bool`
-
 extern crate debug;
 
 fn main() {
     let a = if true { true };
+//~^ ERROR if may be missing an else clause: expected `()`, found `bool` (expected (), found bool)
     println!("{:?}", a);
 }

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

@@ -0,0 +1,18 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn main() {
+    let a = if true {
+        0
+    } else if false {
+//~^ ERROR if may be missing an else clause: expected `()`, found `<generic integer #1>`
+        1
+    };
+}