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 2 commits
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: 7 additions & 0 deletions src/quick-lint-js/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,13 @@
.error(QLJS_TRANSLATABLE("'else' has no corresponding 'if'"), \
else_token)) \
\
QLJS_ERROR_TYPE( \
error_else_with_conditional_missing_if, "E202", \
{ source_code_span else_token; }, \
.warning(QLJS_TRANSLATABLE("'else' with condition followed by block; " \
"maybe 'else if' was intended"), \
else_token)) \
\
QLJS_ERROR_TYPE( \
error_escaped_character_disallowed_in_identifiers, "E012", \
{ source_code_span escape_sequence; }, \
Expand Down
101 changes: 100 additions & 1 deletion src/quick-lint-js/parse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2460,6 +2460,71 @@ class parser {
v.visit_exit_with_scope();
}

bool is_side_effect_free(expression *ast) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd put this function's implementation in parse.cpp.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe it'd be better to invert the meaning of this function and call it has_potential_side_effects? !is_side_effect_free(ast) sounds like a double negative to me.

strager marked this conversation as resolved.
Show resolved Hide resolved
switch (ast->kind()) {
case expression_kind::_class:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: Class expressions might have side effects. For example:

function f() { console.log("yo"); }
let C = class { [f()]() {} };

We don't have enough information in the AST to know, so let's be conservative and assume that side effects might happen.

case expression_kind::_invalid:
case expression_kind::_typeof:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: typeof might have a side effect, depending on its argument. For example:

typeof 42; // no side effect
typeof f(); // side effect

case expression_kind::await:
case expression_kind::import:
strager marked this conversation as resolved.
Show resolved Hide resolved
case expression_kind::literal:
case expression_kind::new_target:
case expression_kind::private_variable:
case expression_kind::rw_unary_prefix:
case expression_kind::rw_unary_suffix:
strager marked this conversation as resolved.
Show resolved Hide resolved
case expression_kind::spread:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: spread might have side effects, because a spread would call an iterator's next function.

case expression_kind::super:
case expression_kind::unary_operator:
strager marked this conversation as resolved.
Show resolved Hide resolved
case expression_kind::variable:
case expression_kind::yield_many:
case expression_kind::yield_none:
case expression_kind::yield_one:
strager marked this conversation as resolved.
Show resolved Hide resolved
return true;

case expression_kind::dot:
case expression_kind::index:
case expression_kind::function:
case expression_kind::named_function:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost-blocking: function and named_function should be treated the same as arrow functions. The functions themselves don't have any side effects, but calling the functions would.

return false;

case expression_kind::_new:
strager marked this conversation as resolved.
Show resolved Hide resolved
case expression_kind::_template:
case expression_kind::array:
case expression_kind::binary_operator:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: binary_operator might have side effects. Some binary operators never have side effects (such as ===), but others might (such as == and +). We don't track which operator the binary_operator expression is talking about, so we cannot distinguish + from ===. Let's be on the safe side and say that any binary_operator might have side effects.

I think we should write a TODO comment to treat === and !== as side-effect-free.

case expression_kind::call:
case expression_kind::tagged_template_literal:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: call and tagged_template_literal always have side effects.

case expression_kind::trailing_comma:
case expression_kind::arrow_function_with_expression:
case expression_kind::arrow_function_with_statements:
for (int i = 0; i < ast->child_count(); i++) {
if (!this->is_side_effect_free(ast->child(i))) return false;
}
return true;

case expression_kind::assignment:
case expression_kind::compound_assignment:
case expression_kind::conditional_assignment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: Assignments always have side effects.

return this->is_side_effect_free(ast->child_0()) &&
this->is_side_effect_free(ast->child_1());

case expression_kind::conditional:
return this->is_side_effect_free(ast->child_0()) &&
this->is_side_effect_free(ast->child_1()) &&
this->is_side_effect_free(ast->child_2());

case expression_kind::object: {
for (int i = 0; i < ast->object_entry_count(); i++) {
auto entry = ast->object_entry(i);
if (entry.property.has_value()) {
if (!this->is_side_effect_free(*entry.property)) return false;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: Some object literal keys can have side effects. For example:

function f() { console.log("yo"); }
let o = { [f()]: true };

return true;
}
}
return false;
}

template <QLJS_PARSE_VISITOR Visitor>
void parse_and_visit_if(Visitor &v) {
QLJS_ASSERT(this->peek().type == token_type::kw_if);
Expand Down Expand Up @@ -2515,8 +2580,42 @@ class parser {
}

if (this->peek().type == token_type::kw_else) {
source_code_span else_span = this->peek().span();
this->skip();
parse_and_visit_body();

switch (this->peek().type) {
default:
parse_and_visit_body();
break;

case token_type::left_paren:
strager marked this conversation as resolved.
Show resolved Hide resolved
expression *ast = this->parse_expression(precedence{});
this->visit_expression(ast, v, variable_context::rhs);

bool is_invalidating_if = false;

switch (ast->kind()) {
default:
break;

// Any other kind?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think not.

case expression_kind::arrow_function_with_expression:
case expression_kind::arrow_function_with_statements:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should make expression::is_arrow_function?

is_invalidating_if = true;
break;
}

if (this->peek().type == token_type::left_curly) {
strager marked this conversation as resolved.
Show resolved Hide resolved
if (!is_invalidating_if && this->is_side_effect_free(ast)) {
this->error_reporter_->report(
error_else_with_conditional_missing_if{
.else_token = else_span,
});
}
parse_and_visit_body();
}
break;
strager marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down
63 changes: 63 additions & 0 deletions test/test-parse-statement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,69 @@ TEST(test_parse, else_without_if) {
}
}

TEST(test_parse, else_if) {
{
spy_visitor v =
parse_and_visit_statement(u8"if (a) { b; } else if (c) { d; }"_sv);
EXPECT_THAT(v.visits, ElementsAre("visit_variable_use", // a
"visit_enter_block_scope", // <if>
"visit_variable_use", // b
"visit_exit_block_scope", // </if>
"visit_variable_use", // c
"visit_enter_block_scope", // <else>
"visit_variable_use", // d
"visit_exit_block_scope")); // </else>
}
}

TEST(test_parse, else_with_implicit_if) {
{
spy_visitor v;
padded_string code(u8"if (a) { b; } else (c) { d; }"_sv);
parser p(&code, &v);
EXPECT_TRUE(p.parse_and_visit_statement(v));
EXPECT_THAT(v.visits, ElementsAre("visit_variable_use", // a
"visit_enter_block_scope", // <if>
"visit_variable_use", // b
"visit_exit_block_scope", // </if>
"visit_variable_use", // c
"visit_enter_block_scope", // <block>
"visit_variable_use", // d
"visit_exit_block_scope")); // </block>

EXPECT_THAT(
v.errors,
ElementsAre(ERROR_TYPE_FIELD(
error_else_with_conditional_missing_if, else_token,
offsets_matcher(&code, strlen(u8"if (a) { b; } "), u8"else"))));
}

{
spy_visitor v;
padded_string code(u8"if (a) { b; } else () => {} { c; }"_sv);
strager marked this conversation as resolved.
Show resolved Hide resolved
parser p(&code, &v);
EXPECT_TRUE(p.parse_and_visit_statement(v));
EXPECT_THAT(v.errors, IsEmpty());
}

{
spy_visitor v;
padded_string code(
u8"if (a) { b; } else (() => console.log(c))() { d; }"_sv);
strager marked this conversation as resolved.
Show resolved Hide resolved
parser p(&code, &v);
EXPECT_TRUE(p.parse_and_visit_statement(v));
EXPECT_THAT(v.errors, IsEmpty());
}

{
spy_visitor v;
padded_string code(u8"if (a) { b; } else (o.m()) { c; }"_sv);
parser p(&code, &v);
EXPECT_TRUE(p.parse_and_visit_statement(v));
EXPECT_THAT(v.errors, IsEmpty());
strager marked this conversation as resolved.
Show resolved Hide resolved
}
}
strager marked this conversation as resolved.
Show resolved Hide resolved

TEST(test_parse, block_statement) {
{
spy_visitor v = parse_and_visit_statement(u8"{ }"_sv);
Expand Down