Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report error for assignment in if condition #42649

Merged
merged 2 commits into from
Jun 17, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/librustc_typeck/check/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, formatting: can you move the sp to the next line so it's easier to see how many arguments this function takes?

expected: Ty<'tcx>,
actual: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
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))
}
}
}
Expand Down
68 changes: 51 additions & 17 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>),

Expand Down Expand Up @@ -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))
}
Expand All @@ -328,6 +330,7 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> {
fn to_option(self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> Option<Ty<'tcx>> {
match self.resolve(fcx) {
NoExpectation => None,
ExpectIfCondition => Some(fcx.tcx.types.bool),
ExpectCastableToType(ty) |
ExpectHasType(ty) |
ExpectRvalueLikeUnsized(ty) => Some(ty),
Expand All @@ -341,6 +344,7 @@ impl<'a, 'gcx, 'tcx> Expectation<'tcx> {
fn only_has_type(self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> Option<Ty<'tcx>> {
match self.resolve(fcx) {
ExpectHasType(ty) => Some(ty),
ExpectIfCondition => Some(fcx.tcx.types.bool),
_ => None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Pre-existing, but I'd rather this match were exhaustive.

}
}
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can call this check_expr_meets_expectation_or_error? I think this zoo of functions is sort of confusing, and the existing naming convention isn't the clearest (for bonus points, we could apply the _or_error convention elsewhere, e.g. check_expr_has_type_or_error)

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`.
Expand All @@ -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
}

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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 => (),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me mildly nervous. Can you add a call like self.tcx.sess.delay_span_bug(lhs.span, "invalid lhs expression in if; expected error elsehwere")? That way, in case there is some edge condition, compilation will abort and not just succeed.

_ => {
// 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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,25 @@ fn main() {
// `x { ... }` should not be interpreted as a struct literal here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a 'regression test for obscure scenario'. Let's rename the test to assignment-in-if.rs. While we're at it, can you add some other obscure cases, like this one:

if (if true { x = 4 } else { x = 5 }) { ... }

I don't really care what message it emits, although I think (if I read the code right...) that the expectation will be propagated. I just wanted to test it.

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);
}
}
50 changes: 50 additions & 0 deletions src/test/ui/type-check/issue-17283.stderr
Original file line number Diff line number Diff line change
@@ -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)