From 028b5f94e3aef435c626131e5b571272f2d3d52c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 13 Jun 2017 16:26:20 -0700 Subject: [PATCH 1/2] Report error for assignment in `if` condition For code like `if x = 3 {}`, output: ``` error[E0308]: mismatched types --> $DIR/issue-17283.rs:25:8 | 25 | if x = x { | ^^^^^ | | | help: did you mean to compare equality? `x == x` | expected bool, found () | = note: expected type `bool` found type `()` ``` --- src/librustc_typeck/check/demand.rs | 9 ++- src/librustc_typeck/check/mod.rs | 68 ++++++++++++++----- .../type-check}/issue-17283.rs | 18 ++--- src/test/ui/type-check/issue-17283.stderr | 50 ++++++++++++++ 4 files changed, 118 insertions(+), 27 deletions(-) rename src/test/{compile-fail => ui/type-check}/issue-17283.rs (76%) create mode 100644 src/test/ui/type-check/issue-17283.stderr diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 9ed50dd1e4d40..732b9be81a398 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -26,13 +26,20 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // Requires that the two types unify, and prints an error message if // they don't. pub fn demand_suptype(&self, sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) { + self.demand_suptype_diag(sp, expected, actual).map(|mut e| e.emit()); + } + + pub fn demand_suptype_diag(&self, sp: Span, + expected: Ty<'tcx>, + actual: Ty<'tcx>) -> Option> { let cause = &self.misc(sp); match self.at(cause, self.param_env).sup(expected, actual) { Ok(InferOk { obligations, value: () }) => { self.register_predicates(obligations); + None }, Err(e) => { - self.report_mismatched_types(&cause, expected, actual, e).emit(); + Some(self.report_mismatched_types(&cause, expected, actual, e)) } } } diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index c12df083c30e6..5bba79a176eee 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -232,6 +232,9 @@ pub enum Expectation<'tcx> { /// We know nothing about what type this expression should have. NoExpectation, + /// This expression is an `if` condition, it must resolve to `bool`. + ExpectIfCondition, + /// This expression should have the type given (or some subtype) ExpectHasType(Ty<'tcx>), @@ -310,9 +313,8 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> { // no constraints yet present), just returns `None`. fn resolve(self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> Expectation<'tcx> { match self { - NoExpectation => { - NoExpectation - } + NoExpectation => NoExpectation, + ExpectIfCondition => ExpectIfCondition, ExpectCastableToType(t) => { ExpectCastableToType(fcx.resolve_type_vars_if_possible(&t)) } @@ -328,6 +330,7 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> { fn to_option(self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> Option> { match self.resolve(fcx) { NoExpectation => None, + ExpectIfCondition => Some(fcx.tcx.types.bool), ExpectCastableToType(ty) | ExpectHasType(ty) | ExpectRvalueLikeUnsized(ty) => Some(ty), @@ -341,6 +344,7 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> { fn only_has_type(self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> Option> { match self.resolve(fcx) { ExpectHasType(ty) => Some(ty), + ExpectIfCondition => Some(fcx.tcx.types.bool), _ => None } } @@ -2646,7 +2650,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn check_expr_has_type(&self, expr: &'gcx hir::Expr, expected: Ty<'tcx>) -> Ty<'tcx> { - let mut ty = self.check_expr_with_hint(expr, expected); + self.check_expr_expect_type(expr, ExpectHasType(expected)) + } + + fn check_expr_expect_type(&self, + expr: &'gcx hir::Expr, + expected: Expectation<'tcx>) -> Ty<'tcx> { + let expected_ty = expected.to_option(&self).unwrap_or(self.tcx.types.bool); + let mut ty = self.check_expr_with_expectation(expr, expected); // While we don't allow *arbitrary* coercions here, we *do* allow // coercions from ! to `expected`. @@ -2662,7 +2673,24 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ty = adj_ty; } - self.demand_suptype(expr.span, expected, ty); + if let Some(mut err) = self.demand_suptype_diag(expr.span, expected_ty, ty) { + // Add help to type error if this is an `if` condition with an assignment + match (expected, &expr.node) { + (ExpectIfCondition, &hir::ExprAssign(ref lhs, ref rhs)) => { + let msg = "did you mean to compare equality?"; + if let (Ok(left), Ok(right)) = ( + self.tcx.sess.codemap().span_to_snippet(lhs.span), + self.tcx.sess.codemap().span_to_snippet(rhs.span)) + { + err.span_suggestion(expr.span, msg, format!("{} == {}", left, right)); + } else { + err.help(msg); + } + } + _ => (), + } + err.emit(); + } ty } @@ -2837,7 +2865,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { opt_else_expr: Option<&'gcx hir::Expr>, sp: Span, expected: Expectation<'tcx>) -> Ty<'tcx> { - let cond_ty = self.check_expr_has_type(cond_expr, self.tcx.types.bool); + let cond_ty = self.check_expr_expect_type(cond_expr, ExpectIfCondition); let cond_diverges = self.diverges.get(); self.diverges.set(Diverges::Maybe); @@ -3637,19 +3665,25 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { hir::ExprAssign(ref lhs, ref rhs) => { let lhs_ty = self.check_expr_with_lvalue_pref(&lhs, PreferMutLvalue); - let tcx = self.tcx; - if !tcx.expr_is_lval(&lhs) { - struct_span_err!( - tcx.sess, expr.span, E0070, - "invalid left-hand side expression") - .span_label( - expr.span, - "left-hand of expression not valid") - .emit(); - } - let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty); + match expected { + ExpectIfCondition => (), + _ => { + // Only check this if not in an `if` condition, as the + // mistyped comparison help is more appropriate. + if !self.tcx.expr_is_lval(&lhs) { + struct_span_err!( + self.tcx.sess, expr.span, E0070, + "invalid left-hand side expression") + .span_label( + expr.span, + "left-hand of expression not valid") + .emit(); + } + } + } + self.require_type_is_sized(lhs_ty, lhs.span, traits::AssignmentLhsSized); if lhs_ty.references_error() || rhs_ty.references_error() { diff --git a/src/test/compile-fail/issue-17283.rs b/src/test/ui/type-check/issue-17283.rs similarity index 76% rename from src/test/compile-fail/issue-17283.rs rename to src/test/ui/type-check/issue-17283.rs index 98208bcfdbdee..f06888b8ec469 100644 --- a/src/test/compile-fail/issue-17283.rs +++ b/src/test/ui/type-check/issue-17283.rs @@ -24,25 +24,25 @@ fn main() { // `x { ... }` should not be interpreted as a struct literal here if x = x { //~^ ERROR mismatched types - //~| expected type `bool` - //~| found type `()` - //~| expected bool, found () + //~| HELP did you mean to compare equality? println!("{}", x); } // Explicit parentheses on the left should match behavior of above if (x = x) { //~^ ERROR mismatched types - //~| expected type `bool` - //~| found type `()` - //~| expected bool, found () + //~| HELP did you mean to compare equality? println!("{}", x); } // The struct literal interpretation is fine with explicit parentheses on the right if y = (Foo { foo: x }) { //~^ ERROR mismatched types - //~| expected type `bool` - //~| found type `()` - //~| expected bool, found () + //~| HELP did you mean to compare equality? + println!("{}", x); + } + // "invalid left-hand side expression" error is suppresed + if 3 = x { + //~^ ERROR mismatched types + //~| HELP did you mean to compare equality? println!("{}", x); } } diff --git a/src/test/ui/type-check/issue-17283.stderr b/src/test/ui/type-check/issue-17283.stderr new file mode 100644 index 0000000000000..f258caecd300e --- /dev/null +++ b/src/test/ui/type-check/issue-17283.stderr @@ -0,0 +1,50 @@ +error[E0308]: mismatched types + --> $DIR/issue-17283.rs:25:8 + | +25 | if x = x { + | ^^^^^ + | | + | help: did you mean to compare equality? `x == x` + | expected bool, found () + | + = note: expected type `bool` + found type `()` + +error[E0308]: mismatched types + --> $DIR/issue-17283.rs:31:8 + | +31 | if (x = x) { + | ^^^^^^^ + | | + | help: did you mean to compare equality? `x == x` + | expected bool, found () + | + = note: expected type `bool` + found type `()` + +error[E0308]: mismatched types + --> $DIR/issue-17283.rs:37:8 + | +37 | if y = (Foo { foo: x }) { + | ^^^^^^^^^^^^^^^^^^^^ + | | + | help: did you mean to compare equality? `y == (Foo { foo: x })` + | expected bool, found () + | + = note: expected type `bool` + found type `()` + +error[E0308]: mismatched types + --> $DIR/issue-17283.rs:43:8 + | +43 | if 3 = x { + | ^^^^^ + | | + | help: did you mean to compare equality? `3 == x` + | expected bool, found () + | + = note: expected type `bool` + found type `()` + +error: aborting due to previous error(s) + From da78b4d88e1ba4598428a92a6c8d5f9c399fede0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 16 Jun 2017 10:34:17 -0700 Subject: [PATCH 2/2] Review comments - exhaustive match - rename method to `check_expr_meets_expectation_or_error` - formatting - add `delay_span_bug` - add test --- src/librustc_typeck/check/_match.rs | 4 +-- src/librustc_typeck/check/demand.rs | 3 +- src/librustc_typeck/check/mod.rs | 31 ++++++++++--------- .../{issue-17283.rs => assignment-in-if.rs} | 4 +++ ...e-17283.stderr => assignment-in-if.stderr} | 17 +++++++--- 5 files changed, 38 insertions(+), 21 deletions(-) rename src/test/ui/type-check/{issue-17283.rs => assignment-in-if.rs} (93%) rename src/test/ui/type-check/{issue-17283.stderr => assignment-in-if.stderr} (73%) diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index 33f0b0282d173..bdd8169b84fe7 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -413,7 +413,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // discriminant. This is sort of a workaround, see note (*) in // `check_pat` for some details. discrim_ty = self.next_ty_var(TypeVariableOrigin::TypeInference(discrim.span)); - self.check_expr_has_type(discrim, discrim_ty); + self.check_expr_has_type_or_error(discrim, discrim_ty); }; // If the discriminant diverges, the match is pointless (e.g., @@ -480,7 +480,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { for (i, (arm, pats_diverge)) in arms.iter().zip(all_arm_pats_diverge).enumerate() { if let Some(ref e) = arm.guard { self.diverges.set(pats_diverge); - self.check_expr_has_type(e, tcx.types.bool); + self.check_expr_has_type_or_error(e, tcx.types.bool); } self.diverges.set(pats_diverge); diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 732b9be81a398..1b6f96cf65137 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -29,7 +29,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { self.demand_suptype_diag(sp, expected, actual).map(|mut e| e.emit()); } - pub fn demand_suptype_diag(&self, sp: Span, + pub fn demand_suptype_diag(&self, + sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) -> Option> { let cause = &self.misc(sp); diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 5bba79a176eee..d663d8f0d2b5a 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -345,7 +345,7 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> { match self.resolve(fcx) { ExpectHasType(ty) => Some(ty), ExpectIfCondition => Some(fcx.tcx.types.bool), - _ => None + NoExpectation | ExpectCastableToType(_) | ExpectRvalueLikeUnsized(_) => None, } } @@ -2647,15 +2647,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { self.demand_eqtype(expr.span, expected, ty); } - pub fn check_expr_has_type(&self, - expr: &'gcx hir::Expr, - expected: Ty<'tcx>) -> Ty<'tcx> { - self.check_expr_expect_type(expr, ExpectHasType(expected)) + pub fn check_expr_has_type_or_error(&self, + expr: &'gcx hir::Expr, + expected: Ty<'tcx>) -> Ty<'tcx> { + self.check_expr_meets_expectation_or_error(expr, ExpectHasType(expected)) } - fn check_expr_expect_type(&self, - expr: &'gcx hir::Expr, - expected: Expectation<'tcx>) -> Ty<'tcx> { + fn check_expr_meets_expectation_or_error(&self, + expr: &'gcx hir::Expr, + expected: Expectation<'tcx>) -> Ty<'tcx> { let expected_ty = expected.to_option(&self).unwrap_or(self.tcx.types.bool); let mut ty = self.check_expr_with_expectation(expr, expected); @@ -2865,7 +2865,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { opt_else_expr: Option<&'gcx hir::Expr>, sp: Span, expected: Expectation<'tcx>) -> Ty<'tcx> { - let cond_ty = self.check_expr_expect_type(cond_expr, ExpectIfCondition); + let cond_ty = self.check_expr_meets_expectation_or_error(cond_expr, ExpectIfCondition); let cond_diverges = self.diverges.get(); self.diverges.set(Diverges::Maybe); @@ -3352,7 +3352,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { self.check_expr_struct_fields(struct_ty, expected, expr.id, path_span, variant, fields, base_expr.is_none()); if let &Some(ref base_expr) = base_expr { - self.check_expr_has_type(base_expr, struct_ty); + self.check_expr_has_type_or_error(base_expr, struct_ty); match struct_ty.sty { ty::TyAdt(adt, substs) if adt.is_struct() => { let fru_field_types = adt.struct_variant().fields.iter().map(|f| { @@ -3668,7 +3668,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty); match expected { - ExpectIfCondition => (), + ExpectIfCondition => { + self.tcx.sess.delay_span_bug(lhs.span, "invalid lhs expression in if;\ + expected error elsehwere"); + } _ => { // Only check this if not in an `if` condition, as the // mistyped comparison help is more appropriate. @@ -3704,7 +3707,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { }; self.with_breakable_ctxt(expr.id, ctxt, || { - self.check_expr_has_type(&cond, tcx.types.bool); + self.check_expr_has_type_or_error(&cond, tcx.types.bool); let cond_diverging = self.diverges.get(); self.check_block_no_value(&body); @@ -3842,7 +3845,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } None => { let t: Ty = self.next_ty_var(TypeVariableOrigin::MiscVariable(element.span)); - let element_ty = self.check_expr_has_type(&element, t); + let element_ty = self.check_expr_has_type_or_error(&element, t); (element_ty, t) } }; @@ -4097,7 +4100,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } hir::StmtExpr(ref expr, _) => { // Check with expected type of () - self.check_expr_has_type(&expr, self.tcx.mk_nil()); + self.check_expr_has_type_or_error(&expr, self.tcx.mk_nil()); } hir::StmtSemi(ref expr, _) => { self.check_expr(&expr); diff --git a/src/test/ui/type-check/issue-17283.rs b/src/test/ui/type-check/assignment-in-if.rs similarity index 93% rename from src/test/ui/type-check/issue-17283.rs rename to src/test/ui/type-check/assignment-in-if.rs index f06888b8ec469..98dc55c666303 100644 --- a/src/test/ui/type-check/issue-17283.rs +++ b/src/test/ui/type-check/assignment-in-if.rs @@ -45,4 +45,8 @@ fn main() { //~| HELP did you mean to compare equality? println!("{}", x); } + if (if true { x = 4 } else { x = 5 }) { + //~^ ERROR mismatched types + println!("{}", x); + } } diff --git a/src/test/ui/type-check/issue-17283.stderr b/src/test/ui/type-check/assignment-in-if.stderr similarity index 73% rename from src/test/ui/type-check/issue-17283.stderr rename to src/test/ui/type-check/assignment-in-if.stderr index f258caecd300e..2943999927322 100644 --- a/src/test/ui/type-check/issue-17283.stderr +++ b/src/test/ui/type-check/assignment-in-if.stderr @@ -1,5 +1,5 @@ error[E0308]: mismatched types - --> $DIR/issue-17283.rs:25:8 + --> $DIR/assignment-in-if.rs:25:8 | 25 | if x = x { | ^^^^^ @@ -11,7 +11,7 @@ error[E0308]: mismatched types found type `()` error[E0308]: mismatched types - --> $DIR/issue-17283.rs:31:8 + --> $DIR/assignment-in-if.rs:31:8 | 31 | if (x = x) { | ^^^^^^^ @@ -23,7 +23,7 @@ error[E0308]: mismatched types found type `()` error[E0308]: mismatched types - --> $DIR/issue-17283.rs:37:8 + --> $DIR/assignment-in-if.rs:37:8 | 37 | if y = (Foo { foo: x }) { | ^^^^^^^^^^^^^^^^^^^^ @@ -35,7 +35,7 @@ error[E0308]: mismatched types found type `()` error[E0308]: mismatched types - --> $DIR/issue-17283.rs:43:8 + --> $DIR/assignment-in-if.rs:43:8 | 43 | if 3 = x { | ^^^^^ @@ -46,5 +46,14 @@ error[E0308]: mismatched types = note: expected type `bool` found type `()` +error[E0308]: mismatched types + --> $DIR/assignment-in-if.rs:48:8 + | +48 | if (if true { x = 4 } else { x = 5 }) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected bool, found () + | + = note: expected type `bool` + found type `()` + error: aborting due to previous error(s)