-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
#265 Warn on redundant semicolon after else #290
base: master
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot Thank you for your contribution! Like many free software projects, you must sign our Contributor License Agreement before we can accept your contribution. EDIT: All contributors have signed quick-lint-js' Contributor License Agreement (CLA-v1.md). |
I have read and hereby agree to quick-lint-js' Contributor License Agreement (CLA-v1.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: It'd also be great if you could add documentation to docs/errors/ for this new error type. Not necessary, though.
src/quick-lint-js/error.h
Outdated
QLJS_ERROR_TYPE( \ | ||
error_redundant_semicolon_after_else, "E202", \ | ||
{ source_code_span semicolon; }, \ | ||
.warning(QLJS_TRANSLATABLE("redundant semicolon after else"), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: This error message is incorrect. The extra semicolon is not redundant; it changes behavior.
Example:
for (let b of [true, false]) {
if (b) {
console.log('t');
} else /* Maybe add a semicolon here */ {
console.log('f');
}
console.log('---');
}
Without semicolon:
t
---
f
---
With semicolon:
t
f
---
f
---
We should communicate that the semicolon is likely unintended, but also imply that removing the semicolon would change behavior.
Perhaps this message is better: "else has empty body; consider removing the semicolon or removing the 'else'" The more I think about it, the more I think we should have two different error messages:
if (b) {
} else; { // else has empty body; consider removing the semicolon to make it not empty
}
if (b) {
} else; // else has empty body; consider removing the redundant 'else'
{
}
(The first example has no line break between ;
and {
. The second example does.)
(Perhaps the second case doesn't deserve a diagnostic at all though. Or perhaps it should be of a lesser severity than "warning".)
test/test-parse-statement.cpp
Outdated
EXPECT_THAT(v.visits, ElementsAre("visit_variable_use", // cond | ||
"visit_enter_block_scope", // (if) | ||
"visit_variable_use", // body | ||
"visit_exit_block_scope")); // (else) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The comment is misleading. The visit_exit_block_scope
is for the if
body, not the else
body.
"visit_exit_block_scope")); // (else) | |
"visit_exit_block_scope")); // (if) |
a4ce1c3
to
82917f4
Compare
docs/errors/E220.md
Outdated
> true | ||
> & false | ||
|
||
To avoid this behavior, remove the semicolon between the else keyword and the opening brace of the else block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include an example of what working code looks like.
test/test-parse-statement.cpp
Outdated
EXPECT_THAT(v.errors, | ||
ElementsAre(ERROR_TYPE_FIELD( | ||
error_unexpected_semicolon_after_else, semicolon, | ||
offsets_matcher(&code, strlen(u8"if (cond) { body; } else"), u8";")))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
docs/errors/E202.md
Outdated
> else | ||
``` | ||
|
||
To avoid this behavior, remove the semicolon between the else keyword and the opening brace of the else block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
In case this is impacting your code, you should remove the semicolon next to the else keyword
src/quick-lint-js/parse.h
Outdated
if (this->peek().type == token_type::right_curly) { | ||
this->error_reporter_->report(error_else_has_empty_body{ | ||
.where = source_code_span(expect_left_curly.begin, this->peek().end) | ||
}); | ||
} else if (token_after_else.type == token_type::semicolon && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the check for an empty block on the else case. Now that I read it again, I see it should only be triggered if the else has a semicolon 🤦♂️
#290 (comment)
Is creating a transaction on the lexer and then rolling it back the correct way to peek forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is creating a transaction on the lexer and then rolling it back the correct way to peek forward?
We only need a transaction if you want to look ahead multiple tokens.
Can you give an example JS program where using this->peek()
won't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had gone through a rabbit hole of actually checking the body of the next scope to the else. That's why I was advancing the cursor and parsing the next scope after the else using a transaction 🤦♂️ . I've rolled the code back to only check for the newline and the opening brace.
Commits are now squashed, let me know what you think.
Did you want me to review your latest changes, or are you still hacking? |
0b6c261
to
81acb09
Compare
81acb09
to
79d5928
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. The logic looks much cleaner now!
if (true) { | ||
console.log("true"); | ||
} | ||
console.log("always"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely docs!
offsets_matcher(&code, strlen(u8"if (cond) { body; } else {"), | ||
u8"}")))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: This strlen string doesn't match the code. The u8"}"
looks wrong too. Did you mean the following instead?
offsets_matcher(&code, strlen(u8"if (cond) { body; } else {"), | |
u8"}")))); | |
offsets_matcher(&code, strlen(u8"if (cond) { body; } else;\n"), | |
u8"{")))); |
If so, the error looks misplaced. It should point at the else
token or the ;
, not at the following unrelated {
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it detects else;{}
(semicolon after else) and else;\n{}
(empty body), the following in parse.h:2534 should change to only trigger else has empty body when it is not preceded by a newline:
- if (token_after_else.type == token_type::semicolon &&
- this->peek().type == token_type::left_curly) {
+ if (token_after_else.type == token_type::semicolon &&
+ this->peek().type == token_type::left_curly &&
+ !this->peek().has_leading_newline) {
- if (this->peek().has_leading_newline) {
+ if (body is empty) {
this->error_reporter_->report(
error_else_has_empty_body{.where = this->peek().span()});
} else {
this->error_reporter_->report(error_unexpected_semicolon_after_else{
.semicolon = token_after_else.span()});
}
}
Should I omit the else has empty body warning altogether, or should I detect the empty body?
I do not know how to do it without making use of a transaction, advancing the peek and then doing a rollback afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I omit the else has empty body warning altogether
Yeah, let's only emit error_unexpected_semicolon_after_else in this PR.
I do not know how to do it without making use of a transaction, advancing the peek and then doing a rollback afterwards.
Would something like this work? Or am I missing an edge case?
if (token_after_else.type == token_type::semicolon &&
this->peek().type == token_type::left_curly &&
!this->peek().has_leading_newline) {
this->error_reporter_->report(error_unexpected_semicolon_after_else{
.semicolon = token_after_else.span(),
});
}
TEST(test_parse, else_has_empty_body) { | ||
{ | ||
spy_visitor v; | ||
padded_string code(u8"if (cond) { body; } else;\n{ }"_sv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't think it's worth having a diagnostic for this code. If there's a newline after the else;
, I think the empty body is intentional.
@@ -0,0 +1,30 @@ | |||
# E204: semicolon after else may be causing unexpected behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone else took error code E204. Choose a different error code.
Summary
Warn when a semicolon is found after else.
After this change a warning is reported for the semicolons at
7:7
12:15
and16:2
Output in VIM plugin format is:
else;;; {}
is also valid code but was left out for simplicity. (loop of peek and skip required).