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.

Sign up for GitHub

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

Attempts to fix issue #266 #317

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Attempts to fix issue #266 #317

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1e1e6ab
Attempts to fix issue #266
keyehzy May 18, 2021
532bc4f
Improved code from last commit
keyehzy May 19, 2021
8be203a
Testing function `parser::has_potential_side_effects`
keyehzy May 22, 2021
546a877
Make `bool has_potential_side_effects(expression*)` static
keyehzy May 22, 2021
e17eb7c
Make tests more compact
keyehzy May 22, 2021
c2d812b
Consume semicolon after parsing statement and improve tests.
keyehzy May 22, 2021
9ea9fb3
Improve on previous commit
keyehzy May 24, 2021
6a7536a
Create `bool is_arrow_kind()` for expression.
keyehzy May 24, 2021
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
7 changes: 3 additions & 4 deletions src/parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2121,15 +2121,14 @@ bool parser::has_potential_side_effects(expression* ast) {
for (int i = 0; i < ast->object_entry_count(); i++) {
auto entry = ast->object_entry(i);
if (entry.property.has_value()) {
if (has_potential_side_effects(*entry.property) ||
has_potential_side_effects(entry.value))
return true;
return has_potential_side_effects(*entry.property) ||
has_potential_side_effects(entry.value);
Comment on lines +2124 to +2125
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: You're only checking the first entry which has a value! We should check all entries.

}
}
return false;
}
}
return false;
QLJS_UNREACHABLE();
}

void parser::consume_semicolon() {
Expand Down
4 changes: 2 additions & 2 deletions src/quick-lint-js/parse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2541,9 +2541,9 @@ class parser {
break;
}

this->consume_semicolon();

if (this->peek().type == token_type::left_curly) {
strager marked this conversation as resolved.
Show resolved Hide resolved
this->consume_semicolon();

if (!is_invalidating_if && !this->has_potential_side_effects(ast)) {
this->error_reporter_->report(
error_else_with_conditional_missing_if{
Expand Down
5 changes: 3 additions & 2 deletions test/test-parse-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3163,7 +3163,8 @@ TEST_F(test_parse_expression,

TEST_F(test_parse_expression, test_expression_for_potential_side_effects) {
{
expression* ast = this->parse_expression(u8"class {};"_sv);
expression* ast =
this->parse_expression(u8"class { static foo = bar(); };"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

Expand Down Expand Up @@ -3198,7 +3199,7 @@ TEST_F(test_parse_expression, test_expression_for_potential_side_effects) {
}

{
expression* ast = this->parse_expression(u8"...foo"_sv);
expression* ast = this->parse_expression(u8"[...foo]"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

Expand Down