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 all 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
66 changes: 66 additions & 0 deletions src/parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,6 +2065,72 @@ expression* parser::maybe_wrap_erroneous_arrow_function(
}
}

bool parser::has_potential_side_effects(expression* ast) {
switch (ast->kind()) {
case expression_kind::_class:
case expression_kind::_new:
case expression_kind::assignment:
case expression_kind::await:
case expression_kind::binary_operator: // TODO(keyehzh): '===' and '!==' are
// side-effect-free
case expression_kind::call:
case expression_kind::compound_assignment:
case expression_kind::conditional_assignment:
case expression_kind::dot:
case expression_kind::import:
case expression_kind::index:
case expression_kind::rw_unary_prefix:
case expression_kind::rw_unary_suffix:
case expression_kind::spread:
case expression_kind::tagged_template_literal:
case expression_kind::unary_operator:
case expression_kind::yield_many:
case expression_kind::yield_none:
case expression_kind::yield_one:
return true;

case expression_kind::_invalid:
case expression_kind::function:
case expression_kind::literal:
case expression_kind::named_function:
case expression_kind::new_target:
case expression_kind::private_variable:
case expression_kind::super:
case expression_kind::variable:
return false;

case expression_kind::_typeof:
return has_potential_side_effects(ast->child(0));

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

case expression_kind::conditional:
return has_potential_side_effects(ast->child_0()) ||
has_potential_side_effects(ast->child_1()) ||
has_potential_side_effects(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()) {
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;
}
}
QLJS_UNREACHABLE();
}

void parser::consume_semicolon() {
switch (this->peek().type) {
case token_type::semicolon:
Expand Down
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
5 changes: 5 additions & 0 deletions src/quick-lint-js/expression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ class expression {

int child_count() const noexcept;

bool is_arrow_kind() const noexcept {
return this->kind_ == expression_kind::arrow_function_with_statements ||
this->kind_ == expression_kind::arrow_function_with_expression;
}

expression *child_0() const noexcept { return this->child(0); }
expression *child_1() const noexcept { return this->child(1); }
expression *child_2() const noexcept { return this->child(2); }
Expand Down
27 changes: 26 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 @@ -658,6 +658,8 @@ class parser {
return this->parse_expression(precedence{});
}

static bool has_potential_side_effects(expression *ast);

private:
enum class variable_context {
lhs,
Expand Down Expand Up @@ -2515,8 +2517,31 @@ 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);

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

if (!ast->is_arrow_kind() && !this->has_potential_side_effects(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
198 changes: 198 additions & 0 deletions test/test-parse-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3161,6 +3161,204 @@ TEST_F(test_parse_expression,
}
}

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

{
expression* ast = this->parse_expression(u8"typeof 42;"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"typeof f();"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

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

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

{
expression* ast = this->parse_expression(u8"++x;"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"x++;"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

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

{
test_parser p(u8"yield foo;"_sv);
auto guard = p.parser().enter_function(function_attributes::generator);
expression* ast = p.parse_expression();
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
test_parser p(u8"yield* foo();"_sv);
auto guard = p.parser().enter_function(function_attributes::generator);
expression* ast = p.parse_expression();
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
test_parser p(u8"yield;"_sv);
auto guard = p.parser().enter_function(function_attributes::generator);
expression* ast = p.parse_expression();
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"function () { bar; };"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"(function () { bar; })();"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"function foo () { bar; };"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

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

{
expression* ast = this->parse_expression(u8"() => { bar; };"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

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

{
expression* ast = this->parse_expression(u8"() => bar;"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

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

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

{
// TODO(keyehzh): Need more specific tests as some binary operators might
// have side-effects (such as '+', '==', ...) and some don't
// (such as '===' and '!==')
strager marked this conversation as resolved.
Show resolved Hide resolved
expression* ast = this->parse_expression(u8"x + y"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"['foo', 'bar']"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

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

{
expression* ast = this->parse_expression(u8"`foo`"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"`foo ${bar}`"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"`foo ${g()}`"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

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

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

{
expression* ast = this->parse_expression(u8"x += y"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"f() ? bar : 42"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"foo ? f() : g()"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"foo ? bar : g()"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"foo ? f() : bar"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"foo = bar ? x : y"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"{foo: 42}"_sv);
EXPECT_FALSE(parser::has_potential_side_effects(ast));
}

{
expression* ast = this->parse_expression(u8"{key: f()}"_sv);
EXPECT_TRUE(parser::has_potential_side_effects(ast));
}

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

std::string summarize(const expression& expression) {
auto children = [&] {
std::string result;
Expand Down
Loading