Uh oh!
There was an error while loading. Please reload this page.
Report error for assignment in if condition - #42649
Conversation
rust-highfive
commented
Jun 13, 2017
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
petrochenkov
commented
Jun 14, 2017
The "expected bool, found ()" message was kinda educational - "it's not like assignments are special and can't be used in some contexts, it's just the result has inappropriate type". |
bors
commented
Jun 14, 2017
☔ The latest upstream changes (presumably #42644) made this pull request unmergeable. Please resolve the merge conflicts. |
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 `()`
```estebank
commented
Jun 15, 2017
@petrochenkov changed |
estebank
commented
Jun 16, 2017
nikomatsakis
left a comment
There was a problem hiding this comment.
r=me modulo the nits below -- this is nice
| let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty); | ||
| match expected { | ||
| ExpectIfCondition => (), |
There was a problem hiding this comment.
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.
| self.check_expr_expect_type(expr, ExpectHasType(expected)) | ||
| } | ||
| fn check_expr_expect_type(&self, |
There was a problem hiding this comment.
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)
| match self.resolve(fcx) { | ||
| ExpectHasType(ty) => Some(ty), | ||
| ExpectIfCondition => Some(fcx.tcx.types.bool), | ||
| _ => None |
There was a problem hiding this comment.
Nit: Pre-existing, but I'd rather this match were exhaustive.
| self.demand_suptype_diag(sp, expected, actual).map(|mut e| e.emit()); | ||
| } | ||
| pub fn demand_suptype_diag(&self, sp: Span, |
There was a problem hiding this comment.
Nit, formatting: can you move the sp to the next line so it's easier to see how many arguments this function takes?
| @@ -24,25 +24,25 @@ fn main() { | |||
| // `x { ... }` should not be interpreted as a struct literal here | |||
There was a problem hiding this comment.
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(iftrue{ 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.
There was a problem hiding this comment.
Nit: we are not (currently) checking these annotations in ui tests (I want to...), but it doesn't appear that we emit a HELP in this case?
- exhaustive match - rename method to `check_expr_meets_expectation_or_error` - formatting - add `delay_span_bug` - add test
estebank
commented
Jun 17, 2017
@bors r=nikomatsakis |
bors
commented
Jun 17, 2017
📌 Commit da78b4d has been approved by |
bors
commented
Jun 17, 2017
⌛ Testing commit da78b4d with merge 78d8416... |
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 `()`
```
Fix#40926.bors
commented
Jun 17, 2017
☀️ Test successful - status-appveyor, status-travis |
For code like
if x = 3 {}, output:Fix#40926.