Attempts to fix issue #266#317
Attempts to fix issue #266#317keyehzy wants to merge 8 commits intoquick-lint:masterfrom keyehzy:else_fix
Conversation
We do this by treating `else (` separately. If `else` is followed by a (conditional + block) pattern, maybe the intention was to use an `else if` instead and a warning is emitted.
Included function to check if the expression is side-effect-free, although I have to check more carefully if the cases are done correctly. The cases that explicitly not side-effect-free are: dot, index, function and named_functions. From testing I found that arrow functions invalidate placing an `if`, even if their children are side-effect-free. This case is treated seperately. Maybe there are more cases where this happens and I must think it through.
strager
left a comment
strager
left a comment
There was a problem hiding this comment.
Hmm, this issue is more complicated than I thought! Lots of edge cases.
This function has been refactored and tested for edge cases.
src/parse.cpp
Outdated
| auto entry = ast->object_entry(i); | ||
| if (entry.property.has_value()) { | ||
| if (this->has_potential_side_effects(*entry.property) || | ||
| this->has_potential_side_effects(entry.value)) |
There was a problem hiding this comment.
Shouldn't we check entry.value regardless of entry.property.has_value()?
test/test-parse-expression.cpp
Outdated
| } | ||
|
|
||
| { | ||
| test_parser p(u8"import foo;"_sv); |
There was a problem hiding this comment.
Blocking: This has syntax errors. I think you meant this:
import(foo);There was a problem hiding this comment.
This one is resolved.
strager
left a comment
strager
left a comment
There was a problem hiding this comment.
Blocking: if (a) { b; } else (c) { d; } is still an issue (false negative).
Other than that, this commit looks bueno!
test/test-parse-statement.cpp
Outdated
| TEST(test_parse, else_with_implicit_if) { | ||
| { | ||
| spy_visitor v; | ||
| padded_string code(u8"if (a) { b; } else (c)\n{ d; }"_sv); |
There was a problem hiding this comment.
We should check v.visits so we know we recovered from the error correctly.
src/parse.cpp
Outdated
| auto entry = ast->object_entry(i); | ||
| if (entry.property.has_value()) { | ||
| if (has_potential_side_effects(*entry.property) || | ||
| has_potential_side_effects(entry.value)) |
There was a problem hiding this comment.
Shouldn't we check entry.value regardless of entry.property.has_value()?
src/quick-lint-js/parse.h
Outdated
| break; | ||
| } | ||
|
|
||
| this->consume_semicolon(); |
There was a problem hiding this comment.
Blocking: This looks like it's in the wrong spot. Won't skipping a semicolon here cause the following code to have the error_else_with_conditional_missing_if warning?
if (a) { b; } else (c);
{ d; }I think we need to consume a semicolon after the check on line 2546.
There was a problem hiding this comment.
Please add a test for this example.
| return has_potential_side_effects(*entry.property) || | ||
| has_potential_side_effects(entry.value); |
There was a problem hiding this comment.
Blocking: You're only checking the first entry which has a value! We should check all entries.
src/quick-lint-js/parse.h
Outdated
| break; | ||
| } | ||
|
|
||
| this->consume_semicolon(); |
There was a problem hiding this comment.
Please add a test for this example.
|
There are merge conflicts because |
2 participants
We do this by treating
else (separately. Ifelseis followed bya (conditional + block) pattern, maybe the intention was to use an
else ifinstead and a warning is emitted.