From 43e5d10428dd2b74e8afe367c46bb0f2b6c37246 Mon Sep 17 00:00:00 2001 From: Jakub Wieczorek Date: Mon, 13 Oct 2014 22:26:23 +0200 Subject: [PATCH] Improve the error message for missing else clauses in if expressions --- src/librustc/middle/typeck/check/mod.rs | 62 ++++++++++--------- .../middle/typeck/infer/error_reporting.rs | 4 ++ src/librustc/middle/typeck/infer/mod.rs | 7 +++ .../compile-fail/if-without-else-result.rs | 3 +- src/test/compile-fail/issue-4201.rs | 18 ++++++ 5 files changed, 63 insertions(+), 31 deletions(-) create mode 100644 src/test/compile-fail/issue-4201.rs diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index ed663cb85a2..2c47c8845cb 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/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()) } }; diff --git a/src/librustc/middle/typeck/infer/error_reporting.rs b/src/librustc/middle/typeck/infer/error_reporting.rs index 2a8a695b63e..1a79837c03a 100644 --- a/src/librustc/middle/typeck/infer/error_reporting.rs +++ b/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) { diff --git a/src/librustc/middle/typeck/infer/mod.rs b/src/librustc/middle/typeck/infer/mod.rs index 2e4987c76d4..7c04b371aae 100644 --- a/src/librustc/middle/typeck/infer/mod.rs +++ b/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)) + } } } } diff --git a/src/test/compile-fail/if-without-else-result.rs b/src/test/compile-fail/if-without-else-result.rs index c22a8e3f782..0754d273a9b 100644 --- a/src/test/compile-fail/if-without-else-result.rs +++ b/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); } diff --git a/src/test/compile-fail/issue-4201.rs b/src/test/compile-fail/issue-4201.rs new file mode 100644 index 00000000000..4b2be9e58ac --- /dev/null +++ b/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 or the MIT license +// , 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 `` + 1 + }; +}