Point out missing if conditional #43854
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I feel like it'd be better if the span for the missing conditional pointed at one character, probably either the |
carols10cents
added
the
S-waiting-on-review
estebank
force-pushed
the
missing-cond
branch
2 times, most recently
from
f543e5b
to
45bffde
Compare
src/test/ui/issue-13483.stderr
Outdated
| error: missing conditional | ||
| --> $DIR/issue-13483.rs:13:15 | ||
| | | ||
| 13 | } else if { //ERROR: MISSING CONDITIONAL |
There was a problem hiding this comment.
I agree with @Mark-Simulacrum that the span here is not ideal -- also, the term "conditional" doesn't quite feel right. I feel like I would prefer:
error: missing condition for `if` statemement
--> $DIR/issue-13483.rs:20:15
|
20 | } else if {
| ^ expected if condition here
21 | | };
estebank
force-pushed
the
missing-cond
branch
2 times, most recently
from
59ff234
to
005dcc9
Compare
src/libsyntax/parse/parser.rs
Outdated
| err.cancel(); | ||
| let sp = lo.next_point(); | ||
| let mut err = self.diagnostic() | ||
| .struct_span_err(ps, "missing condition for `if` statemement"); |
There was a problem hiding this comment.
You mean sp here?
[00:04:00] error[E0425]: cannot find value `ps` in this scope
[00:04:00] --> /checkout/src/libsyntax/parse/parser.rs:2977:38
[00:04:00] |
[00:04:00] 2977 | .struct_span_err(ps, "missing condition for `if` statemement");
[00:04:00] | ^^ not found in this scope
[00:04:00]
[00:04:07] error: aborting due to previous error
[00:04:07]
[00:04:07] error: Could not compile `syntax`.On a case where an else conditional is missing, point this out
instead of the token immediately after the (incorrect) else block:
```
error: missing condition for `if` statemementt push fork -f
--> $DIR/issue-13483.rs:16:5
|
13 | } else if {
| ^ expected if condition here
```
instead of
```
error: expected `{`, found `else`
--> ../../src/test/ui/issue-13483.rs:14:7
|
14 | } else {
| ^^^^
```
src/libsyntax/parse/parser.rs
Outdated
| if self.check_keyword(keywords::Let) { | ||
| return self.parse_if_let_expr(attrs); | ||
| } | ||
| let lo = self.prev_span; | ||
| let cond = self.parse_expr_res(RESTRICTION_NO_STRUCT_LITERAL, None)?; | ||
| let thn = self.parse_block()?; | ||
| let thn = self.parse_block().map_err(|mut err| { |
There was a problem hiding this comment.
I'm mildly worried that this error might trigger on cases where it shouldn't -- this will trigger whenever parsing the block failed, right? Even if there was a condition, potentially?
For example, something like this if true else { ... } will signal that there is a missing condition, which isn't quite right, no?
Maybe we should check whether we see the keyword else, instead of invoking parse_block()?
There was a problem hiding this comment.
Changed, but that won't catch the following case:
fn foo() {
if true {
} else if {
}
}
src/test/ui/issue-13483.rs
Outdated
|
|
||
| fn foo() { | ||
| if true { | ||
| } else if { //ERROR: MISSING CONDITIONAL |
There was a problem hiding this comment.
Hmm, so I guess my change means we no longer catch this case? That does seem unfortunate -- I hadn't quite thought that far ahead. :) We could handle this by expanding the set of things we check for, I guess -- e.g., including ; and } and a few other such tokens. Not sure if that's worth it.
There was a problem hiding this comment.
By then we wouldn't catch, for example:
fn foo() {
if true {
} else if {
}
bar();
}Alternatively, I could expand this PR to check if cond.node is Block and the last stmts is StmtKind::Expr(_) or StmtKind::Semi(Expr{node: ExprKind::Ret(Some(_)),..}). If false, present the proposed suggestion.
After this, if we don't check that the next token is {, the output for:
fn main() {
if true {
} else if {
true
}
}would be the same as it is now, which is in my eyes reasonable:
error: expected `{`, found `}`
--> file.rs:7:1
|
7 | }
| ^
There was a problem hiding this comment.
@nikomatsakis changed. It doesn't catch the following case, but I feel it is an acceptable tradeoff, given the alternatives:
if true {
} else if {
true
}
|
@bors r+ |
|
📌 Commit f063233 has been approved by |
Point out missing if conditional
On a case where an else conditional is missing, point this out
instead of the token immediately after the (incorrect) else block:
```
error: missing condition for `if` statemementt push fork -f
--> $DIR/issue-13483.rs:16:5
|
13 | } else if {
| ^ expected if condition here
```
instead of
```
error: expected `{`, found `else`
--> ../../src/test/ui/issue-13483.rs:14:7
|
14 | } else {
| ^^^^
```
Fix #13483.
|
☀️ Test successful - status-appveyor, status-travis |
On a case where an else conditional is missing, point this out
instead of the token immediately after the (incorrect) else block:
instead of
Fix #13483.