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 6 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
67 changes: 67 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,73 @@ 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()) {
if (has_potential_side_effects(*entry.property) ||
has_potential_side_effects(entry.value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we check entry.value regardless of entry.property.has_value()?

return true;
}
}
return false;
}
}
return false;
strager marked this conversation as resolved.
Show resolved Hide resolved
}

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
39 changes: 38 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,43 @@ 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;

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;
}

this->consume_semicolon();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test for this example.


if (this->peek().type == token_type::left_curly) {
strager marked this conversation as resolved.
Show resolved Hide resolved
if (!is_invalidating_if && !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
197 changes: 197 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,203 @@ TEST_F(test_parse_expression,
}
}

TEST_F(test_parse_expression, test_expression_for_potential_side_effects) {
{
expression* ast = this->parse_expression(u8"class {};"_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