Attempts to fix issue #266

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 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))
+          return true;
+      }
+    }
+    return false;
+  }
+  }
+  return false;
+}
+
 void parser::consume_semicolon() {
   switch (this->peek().type) {
   case token_type::semicolon:

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

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,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:
+        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;
+        }
+
+        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(
+                error_else_with_conditional_missing_if{
+                    .else_token = else_span,
+                });
+          }
+          parse_and_visit_body();
+        }
+        break;
+      }
     }
   }
 

test/test-parse-expression.cpp

@@ -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 '!==')
+    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;