From 1e1e6ab2510edba0a3e18154234fc51ea8ce7596 Mon Sep 17 00:00:00 2001 From: keyehzy Date: Tue, 18 May 2021 16:24:26 -0300 Subject: [PATCH 1/8] Attempts to fix issue #266 We do this by treating `else (` separately. If `else` is followed by a (conditional + block) pattern, maybe the intention was to use an `else if` instead and a warning is emitted. --- src/quick-lint-js/error.h | 7 +++++++ src/quick-lint-js/parse.h | 20 ++++++++++++++++++- test/test-parse-statement.cpp | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/quick-lint-js/error.h b/src/quick-lint-js/error.h index 918e1c7e93..d17782752c 100644 --- a/src/quick-lint-js/error.h +++ b/src/quick-lint-js/error.h @@ -214,6 +214,13 @@ .error(QLJS_TRANSLATABLE("'else' has no corresponding 'if'"), \ else_token)) \ \ + QLJS_ERROR_TYPE( \ + error_else_with_conditional_missing_if, "E???", \ + { 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; }, \ diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index cdb2d3d6d5..0d16bcc1fa 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -2515,8 +2515,26 @@ 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: + expression *ast = this->parse_expression(precedence{}); + this->visit_expression(ast, v, variable_context::rhs); + + if (this->peek().type == token_type::left_curly) { + this->error_reporter_->report(error_else_with_conditional_missing_if{ + .else_token = else_span, + }); + parse_and_visit_body(); + } + break; + } } } diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp index 2277edd9ad..30f1ed46bc 100644 --- a/test/test-parse-statement.cpp +++ b/test/test-parse-statement.cpp @@ -578,6 +578,42 @@ 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", // + "visit_variable_use", // b + "visit_exit_block_scope", // + "visit_variable_use", // c + "visit_enter_block_scope", // + "visit_variable_use", // d + "visit_exit_block_scope")); // + } + + { + 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", // + "visit_variable_use", // b + "visit_exit_block_scope", // + "visit_variable_use", // c + "visit_enter_block_scope", // + "visit_variable_use", // d + "visit_exit_block_scope")); // + + 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")))); + } +} + TEST(test_parse, block_statement) { { spy_visitor v = parse_and_visit_statement(u8"{ }"_sv); From 532bc4f853dcdb51be906e50eb269009915eac45 Mon Sep 17 00:00:00 2001 From: keyehzy Date: Wed, 19 May 2021 19:40:10 -0300 Subject: [PATCH 2/8] Improved code from last commit Included function to check if the expression is side-effect-free, although I have to check more carefully if the cases are done correctly. The cases that explicitly not side-effect-free are: dot, index, function and named_functions. From testing I found that arrow functions invalidate placing an `if`, even if their children are side-effect-free. This case is treated seperately. Maybe there are more cases where this happens and I must think it through. --- src/quick-lint-js/error.h | 2 +- src/quick-lint-js/parse.h | 87 +++++++++++++++++++++++++++++++++-- test/test-parse-statement.cpp | 27 +++++++++++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/quick-lint-js/error.h b/src/quick-lint-js/error.h index d17782752c..4cd6c9bf62 100644 --- a/src/quick-lint-js/error.h +++ b/src/quick-lint-js/error.h @@ -215,7 +215,7 @@ else_token)) \ \ QLJS_ERROR_TYPE( \ - error_else_with_conditional_missing_if, "E???", \ + 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"), \ diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 0d16bcc1fa..4ee44728f7 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -2460,6 +2460,71 @@ class parser { v.visit_exit_with_scope(); } + bool is_side_effect_free(expression *ast) { + switch (ast->kind()) { + case expression_kind::_class: + case expression_kind::_invalid: + case expression_kind::_typeof: + case expression_kind::await: + case expression_kind::import: + 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: + case expression_kind::spread: + case expression_kind::super: + case expression_kind::unary_operator: + case expression_kind::variable: + case expression_kind::yield_many: + case expression_kind::yield_none: + case expression_kind::yield_one: + return true; + + case expression_kind::dot: + case expression_kind::index: + case expression_kind::function: + case expression_kind::named_function: + return false; + + case expression_kind::_new: + case expression_kind::_template: + case expression_kind::array: + case expression_kind::binary_operator: + case expression_kind::call: + case expression_kind::tagged_template_literal: + 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: + 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; + } + } + return true; + } + } + return false; + } + template void parse_and_visit_if(Visitor &v) { QLJS_ASSERT(this->peek().type == token_type::kw_if); @@ -2527,10 +2592,26 @@ class parser { 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? + case expression_kind::arrow_function_with_expression: + case expression_kind::arrow_function_with_statements: + is_invalidating_if = true; + break; + } + if (this->peek().type == token_type::left_curly) { - this->error_reporter_->report(error_else_with_conditional_missing_if{ - .else_token = else_span, - }); + 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; diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp index 30f1ed46bc..57aa6b0b3a 100644 --- a/test/test-parse-statement.cpp +++ b/test/test-parse-statement.cpp @@ -591,7 +591,9 @@ TEST(test_parse, else_if) { "visit_variable_use", // d "visit_exit_block_scope")); // } +} +TEST(test_parse, else_with_implicit_if) { { spy_visitor v; padded_string code(u8"if (a) { b; } else (c) { d; }"_sv); @@ -612,6 +614,31 @@ TEST(test_parse, else_if) { 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); + 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); + 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()); + } } TEST(test_parse, block_statement) { From 8be203a70e4f4fc3a70bc0bd84be67ccef1a1dea Mon Sep 17 00:00:00 2001 From: keyehzy Date: Fri, 21 May 2021 22:25:20 -0300 Subject: [PATCH 3/8] Testing function `parser::has_potential_side_effects` This function has been refactored and tested for edge cases. --- src/parse.cpp | 67 +++++++++ src/quick-lint-js/parse.h | 70 +-------- test/test-parse-expression.cpp | 267 +++++++++++++++++++++++++++++++++ test/test-parse-statement.cpp | 57 +++++-- 4 files changed, 381 insertions(+), 80 deletions(-) diff --git a/src/parse.cpp b/src/parse.cpp index 1e0809b740..00453ec9a9 100644 --- a/src/parse.cpp +++ b/src/parse.cpp @@ -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 this->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 (this->has_potential_side_effects(ast->child(i))) return true; + } + return false; + + case expression_kind::conditional: + return this->has_potential_side_effects(ast->child_0()) || + this->has_potential_side_effects(ast->child_1()) || + this->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 (this->has_potential_side_effects(*entry.property) || + this->has_potential_side_effects(entry.value)) + return true; + } + } + return false; + } + } + return false; +} + void parser::consume_semicolon() { switch (this->peek().type) { case token_type::semicolon: diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 4ee44728f7..2477877b74 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -658,6 +658,8 @@ class parser { return this->parse_expression(precedence{}); } + bool has_potential_side_effects(expression *ast); + private: enum class variable_context { lhs, @@ -2460,71 +2462,6 @@ class parser { v.visit_exit_with_scope(); } - bool is_side_effect_free(expression *ast) { - switch (ast->kind()) { - case expression_kind::_class: - case expression_kind::_invalid: - case expression_kind::_typeof: - case expression_kind::await: - case expression_kind::import: - 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: - case expression_kind::spread: - case expression_kind::super: - case expression_kind::unary_operator: - case expression_kind::variable: - case expression_kind::yield_many: - case expression_kind::yield_none: - case expression_kind::yield_one: - return true; - - case expression_kind::dot: - case expression_kind::index: - case expression_kind::function: - case expression_kind::named_function: - return false; - - case expression_kind::_new: - case expression_kind::_template: - case expression_kind::array: - case expression_kind::binary_operator: - case expression_kind::call: - case expression_kind::tagged_template_literal: - 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: - 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; - } - } - return true; - } - } - return false; - } - template void parse_and_visit_if(Visitor &v) { QLJS_ASSERT(this->peek().type == token_type::kw_if); @@ -2598,7 +2535,6 @@ class parser { default: break; - // Any other kind? case expression_kind::arrow_function_with_expression: case expression_kind::arrow_function_with_statements: is_invalidating_if = true; @@ -2606,7 +2542,7 @@ class parser { } if (this->peek().type == token_type::left_curly) { - if (!is_invalidating_if && this->is_side_effect_free(ast)) { + if (!is_invalidating_if && !this->has_potential_side_effects(ast)) { this->error_reporter_->report( error_else_with_conditional_missing_if{ .else_token = else_span, diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp index be380b3729..45d08df7df 100644 --- a/test/test-parse-expression.cpp +++ b/test/test-parse-expression.cpp @@ -3161,6 +3161,273 @@ TEST_F(test_parse_expression, } } +TEST(test_parse, test_expression_for_potential_side_effects) { + { + test_parser p(u8"class {};"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"typeof 42;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"typeof f();"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"await foo;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"import foo;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"++x;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"x++;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"...foo"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"yield foo;"_sv); + auto guard = p.parser().enter_function(function_attributes::generator); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"yield* foo();"_sv); + auto guard = p.parser().enter_function(function_attributes::generator); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"yield;"_sv); + auto guard = p.parser().enter_function(function_attributes::generator); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"function () { bar; };"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"(function () { bar; })();"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"function foo () { bar; };"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"(function foo () { bar; })();"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"() => { bar; };"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"(() => { bar; })();"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"() => bar;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"(() => bar)();"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"new foo;"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + // TODO(keyehzh): Need more specific tests as some binary operators might + // have side-effects (such as '+', '==', ...) and some don't + // (such as '===' and '!==') + test_parser p(u8"x + y"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"['foo', 'bar']"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"['foo', g()]"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"`foo`"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"`foo ${bar}`"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"`foo ${g()}`"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo`bar`"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo = bar"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"x += y"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"f() ? bar : 42"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo ? f() : g()"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo ? bar : g()"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo ? f() : bar"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"foo = bar ? x : y"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"{foo: 42}"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_FALSE(side_effects); + } + + { + test_parser p(u8"{key: f()}"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } + + { + test_parser p(u8"{[f()]: true}"_sv); + expression* ast = p.parse_expression(); + bool side_effects = p.parser().has_potential_side_effects(ast); + EXPECT_TRUE(side_effects); + } +} + std::string summarize(const expression& expression) { auto children = [&] { std::string result; diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp index 57aa6b0b3a..53d6288ebf 100644 --- a/test/test-parse-statement.cpp +++ b/test/test-parse-statement.cpp @@ -596,18 +596,21 @@ TEST(test_parse, else_if) { TEST(test_parse, else_with_implicit_if) { { spy_visitor v; - padded_string code(u8"if (a) { b; } else (c) { d; }"_sv); + padded_string code(u8"if (a) { b; } else (c)\n{ 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", // - "visit_variable_use", // b - "visit_exit_block_scope", // - "visit_variable_use", // c - "visit_enter_block_scope", // - "visit_variable_use", // d - "visit_exit_block_scope")); // + 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 (42)\n{ d; }"_sv); + parser p(&code, &v); + EXPECT_TRUE(p.parse_and_visit_statement(v)); EXPECT_THAT( v.errors, ElementsAre(ERROR_TYPE_FIELD( @@ -617,16 +620,44 @@ TEST(test_parse, else_with_implicit_if) { { spy_visitor v; - padded_string code(u8"if (a) { b; } else () => {} { c; }"_sv); + padded_string code(u8"if (a) { b; } else (function () { c; })\n{ d; }"_sv); parser p(&code, &v); EXPECT_TRUE(p.parse_and_visit_statement(v)); - EXPECT_THAT(v.errors, IsEmpty()); + 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 (() => console.log(c))() { d; }"_sv); + u8"if (a) { b; } else (function () { c; })()\n{ d; }"_sv); + 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 () => {}\n{ c; }"_sv); + 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 (() => c)()\n{ d; }"_sv); + 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())\n{ c; }"_sv); parser p(&code, &v); EXPECT_TRUE(p.parse_and_visit_statement(v)); EXPECT_THAT(v.errors, IsEmpty()); @@ -634,7 +665,7 @@ TEST(test_parse, else_with_implicit_if) { { spy_visitor v; - padded_string code(u8"if (a) { b; } else (o.m()) { c; }"_sv); + padded_string code(u8"if (a) { b; } else (b = a)\n{ c; }"_sv); parser p(&code, &v); EXPECT_TRUE(p.parse_and_visit_statement(v)); EXPECT_THAT(v.errors, IsEmpty()); From 546a8777e5c50ec4776a92abc5e9c4c723adb421 Mon Sep 17 00:00:00 2001 From: keyehzy Date: Sat, 22 May 2021 13:54:25 -0300 Subject: [PATCH 4/8] Make `bool has_potential_side_effects(expression*)` static --- src/parse.cpp | 14 +++++++------- src/quick-lint-js/parse.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/parse.cpp b/src/parse.cpp index 00453ec9a9..5c09471f5a 100644 --- a/src/parse.cpp +++ b/src/parse.cpp @@ -2100,7 +2100,7 @@ bool parser::has_potential_side_effects(expression* ast) { return false; case expression_kind::_typeof: - return this->has_potential_side_effects(ast->child(0)); + return has_potential_side_effects(ast->child(0)); case expression_kind::_template: case expression_kind::array: @@ -2108,21 +2108,21 @@ bool parser::has_potential_side_effects(expression* ast) { case expression_kind::arrow_function_with_statements: case expression_kind::trailing_comma: for (int i = 0; i < ast->child_count(); i++) { - if (this->has_potential_side_effects(ast->child(i))) return true; + if (has_potential_side_effects(ast->child(i))) return true; } return false; case expression_kind::conditional: - return this->has_potential_side_effects(ast->child_0()) || - this->has_potential_side_effects(ast->child_1()) || - this->has_potential_side_effects(ast->child_2()); + 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 (this->has_potential_side_effects(*entry.property) || - this->has_potential_side_effects(entry.value)) + if (has_potential_side_effects(*entry.property) || + has_potential_side_effects(entry.value)) return true; } } diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 2477877b74..9539fbc8a4 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -658,7 +658,7 @@ class parser { return this->parse_expression(precedence{}); } - bool has_potential_side_effects(expression *ast); + static bool has_potential_side_effects(expression *ast); private: enum class variable_context { From e17eb7c08a72b6092e2a07616f6fee0538ae8c5b Mon Sep 17 00:00:00 2001 From: keyehzy Date: Sat, 22 May 2021 13:57:37 -0300 Subject: [PATCH 5/8] Make tests more compact --- test/test-parse-expression.cpp | 216 +++++++++++---------------------- 1 file changed, 73 insertions(+), 143 deletions(-) diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp index 45d08df7df..b3cba7b494 100644 --- a/test/test-parse-expression.cpp +++ b/test/test-parse-expression.cpp @@ -3161,270 +3161,200 @@ TEST_F(test_parse_expression, } } -TEST(test_parse, test_expression_for_potential_side_effects) { +TEST_F(test_parse_expression, test_expression_for_potential_side_effects) { { - test_parser p(u8"class {};"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"class {};"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"typeof 42;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"typeof 42;"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"typeof f();"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"typeof f();"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"await foo;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"await foo;"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"import foo;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"import(foo);"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"++x;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"++x;"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"x++;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"x++;"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"...foo"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + 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(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + 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(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + 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(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"function () { bar; };"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"function () { bar; };"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"(function () { bar; })();"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"(function () { bar; })();"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"function foo () { bar; };"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"function foo () { bar; };"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"(function foo () { bar; })();"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = + this->parse_expression(u8"(function foo () { bar; })();"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"() => { bar; };"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"() => { bar; };"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"(() => { bar; })();"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"(() => { bar; })();"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"() => bar;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"() => bar;"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"(() => bar)();"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"(() => bar)();"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"new foo;"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + 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 '!==') - test_parser p(u8"x + y"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"x + y"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"['foo', 'bar']"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"['foo', 'bar']"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"['foo', g()]"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"['foo', g()]"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"`foo`"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"`foo`"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"`foo ${bar}`"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"`foo ${bar}`"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"`foo ${g()}`"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"`foo ${g()}`"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo`bar`"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo`bar`"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo = bar"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo = bar"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"x += y"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"x += y"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"f() ? bar : 42"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"f() ? bar : 42"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo ? f() : g()"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo ? f() : g()"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo ? bar : g()"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo ? bar : g()"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo ? f() : bar"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo ? f() : bar"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"foo = bar ? x : y"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"foo = bar ? x : y"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"{foo: 42}"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_FALSE(side_effects); + expression* ast = this->parse_expression(u8"{foo: 42}"_sv); + EXPECT_FALSE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"{key: f()}"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"{key: f()}"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } { - test_parser p(u8"{[f()]: true}"_sv); - expression* ast = p.parse_expression(); - bool side_effects = p.parser().has_potential_side_effects(ast); - EXPECT_TRUE(side_effects); + expression* ast = this->parse_expression(u8"{[f()]: true}"_sv); + EXPECT_TRUE(parser::has_potential_side_effects(ast)); } } From c2d812b06a9b54ffa04a7b965ddce3b7b1be3351 Mon Sep 17 00:00:00 2001 From: keyehzy Date: Sat, 22 May 2021 14:50:14 -0300 Subject: [PATCH 6/8] Consume semicolon after parsing statement and improve tests. --- src/quick-lint-js/parse.h | 2 + test/test-parse-statement.cpp | 87 ++++++++++++++++++++++++----------- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 9539fbc8a4..9d468c9607 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -2541,6 +2541,8 @@ class parser { break; } + this->consume_semicolon(); + if (this->peek().type == token_type::left_curly) { if (!is_invalidating_if && !this->has_potential_side_effects(ast)) { this->error_reporter_->report( diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp index 53d6288ebf..f518581f36 100644 --- a/test/test-parse-statement.cpp +++ b/test/test-parse-statement.cpp @@ -583,46 +583,75 @@ 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", // + "visit_enter_block_scope", // (if) "visit_variable_use", // b - "visit_exit_block_scope", // + "visit_exit_block_scope", // (if) "visit_variable_use", // c - "visit_enter_block_scope", // + "visit_enter_block_scope", // (else if) "visit_variable_use", // d - "visit_exit_block_scope")); // + "visit_exit_block_scope")); // (else if) } } TEST(test_parse, else_with_implicit_if) { { spy_visitor v; - padded_string code(u8"if (a) { b; } else (c)\n{ d; }"_sv); + 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")))); + ElementsAre( + ERROR_TYPE_FIELD( + error_missing_semicolon_after_statement, where, + offsets_matcher(&code, strlen(u8"if (a) { b; } else (c)"), + u8"")), + 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 (42)\n{ d; }"_sv); + padded_string code(u8"if (a) { b; } else (f()) { d; }"_sv); parser p(&code, &v); EXPECT_TRUE(p.parse_and_visit_statement(v)); - 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")))); + 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", // f + "visit_enter_block_scope", // (block) + "visit_variable_use", // d + "visit_exit_block_scope")); // (block) + EXPECT_THAT(v.errors, + ElementsAre(ERROR_TYPE_FIELD( + error_missing_semicolon_after_statement, where, + offsets_matcher(&code, strlen(u8"if (a) { b; } else (f())"), + u8"")))); } { spy_visitor v; - padded_string code(u8"if (a) { b; } else (function () { c; })\n{ d; }"_sv); + padded_string code(u8"if (a) { b; } else (c)\n{ 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( @@ -632,11 +661,25 @@ TEST(test_parse, else_with_implicit_if) { { spy_visitor v; - padded_string code( - u8"if (a) { b; } else (function () { c; })()\n{ d; }"_sv); + padded_string code(u8"if (a) { b; } else () => {} { c; }"_sv); parser p(&code, &v); EXPECT_TRUE(p.parse_and_visit_statement(v)); - EXPECT_THAT(v.errors, IsEmpty()); + EXPECT_THAT(v.visits, ElementsAre("visit_variable_use", // a + "visit_enter_block_scope", // (if) + "visit_variable_use", // b + "visit_exit_block_scope", // (if) + "visit_enter_function_scope", // (arrow) + "visit_enter_function_scope_body", // {} + "visit_exit_function_scope", // (arrow) + "visit_enter_block_scope", // (block) + "visit_variable_use", // d + "visit_exit_block_scope")); // (block) + EXPECT_THAT( + v.errors, + ElementsAre(ERROR_TYPE_FIELD( + error_missing_semicolon_after_statement, where, + offsets_matcher(&code, strlen(u8"if (a) { b; } else () => {}"), + u8"")))); } { @@ -662,14 +705,6 @@ TEST(test_parse, else_with_implicit_if) { EXPECT_TRUE(p.parse_and_visit_statement(v)); EXPECT_THAT(v.errors, IsEmpty()); } - - { - spy_visitor v; - padded_string code(u8"if (a) { b; } else (b = a)\n{ c; }"_sv); - parser p(&code, &v); - EXPECT_TRUE(p.parse_and_visit_statement(v)); - EXPECT_THAT(v.errors, IsEmpty()); - } } TEST(test_parse, block_statement) { From 9ea9fb36cb710dbdcdd56abe09d1a6e114c8daac Mon Sep 17 00:00:00 2001 From: keyehzy Date: Mon, 24 May 2021 18:02:31 -0300 Subject: [PATCH 7/8] Improve on previous commit --- src/parse.cpp | 7 +++---- src/quick-lint-js/parse.h | 4 ++-- test/test-parse-expression.cpp | 5 +++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/parse.cpp b/src/parse.cpp index 5c09471f5a..e5f5096d99 100644 --- a/src/parse.cpp +++ b/src/parse.cpp @@ -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); } } return false; } } - return false; + QLJS_UNREACHABLE(); } void parser::consume_semicolon() { diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 9d468c9607..332dc439fa 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -2541,9 +2541,9 @@ class parser { break; } - this->consume_semicolon(); - if (this->peek().type == token_type::left_curly) { + this->consume_semicolon(); + if (!is_invalidating_if && !this->has_potential_side_effects(ast)) { this->error_reporter_->report( error_else_with_conditional_missing_if{ diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp index b3cba7b494..9217d95299 100644 --- a/test/test-parse-expression.cpp +++ b/test/test-parse-expression.cpp @@ -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)); } @@ -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)); } From 6a7536aed5641ff74246ac12f107223c7976c993 Mon Sep 17 00:00:00 2001 From: keyehzy Date: Mon, 24 May 2021 18:16:23 -0300 Subject: [PATCH 8/8] Create `bool is_arrow_kind()` for expression. --- src/quick-lint-js/expression.h | 5 +++++ src/quick-lint-js/parse.h | 14 +------------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/quick-lint-js/expression.h b/src/quick-lint-js/expression.h index 57d8d479f7..89fedd49ce 100644 --- a/src/quick-lint-js/expression.h +++ b/src/quick-lint-js/expression.h @@ -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); } diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h index 332dc439fa..24deb53494 100644 --- a/src/quick-lint-js/parse.h +++ b/src/quick-lint-js/parse.h @@ -2529,22 +2529,10 @@ class parser { 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: - is_invalidating_if = true; - break; - } - if (this->peek().type == token_type::left_curly) { this->consume_semicolon(); - if (!is_invalidating_if && !this->has_potential_side_effects(ast)) { + 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,