diff --git a/src/parse.cpp b/src/parse.cpp index 1e0809b740..e5f5096d99 100644 --- a/src/parse.cpp +++ b/src/parse.cpp @@ -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); + } + } + return false; + } + } + QLJS_UNREACHABLE(); +} + void parser::consume_semicolon() { switch (this->peek().type) { case token_type::semicolon: diff --git a/src/quick-lint-js/error.h b/src/quick-lint-js/error.h index 918e1c7e93..4cd6c9bf62 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, "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; }, \ 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 cdb2d3d6d5..24deb53494 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{}); } + static bool has_potential_side_effects(expression *ast); + private: enum class variable_context { lhs, @@ -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: + expression *ast = this->parse_expression(precedence{}); + this->visit_expression(ast, v, variable_context::rhs); + + if (this->peek().type == token_type::left_curly) { + 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; + } } } diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp index be380b3729..9217d95299 100644 --- a/test/test-parse-expression.cpp +++ b/test/test-parse-expression.cpp @@ -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 '!==') + 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; diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp index 2277edd9ad..f518581f36 100644 --- a/test/test-parse-statement.cpp +++ b/test/test-parse-statement.cpp @@ -578,6 +578,135 @@ 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 if) + "visit_variable_use", // d + "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) { 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_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 (f()) { 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", // 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 (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( + 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.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"")))); + } + + { + 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()); + } +} + TEST(test_parse, block_statement) { { spy_visitor v = parse_and_visit_statement(u8"{ }"_sv);