Refactor: make 'delete x' call visit_variable_delete_use

strager · strager · commit d5adaec68376 · 2021-11-03T18:11:01.000-07:00
error_redundant_delete_statement_on_variable is handled by the parser.
The logic needs to know some linter state, so I want to move the logic
into the linter. Make this possible by having the parser call
visit_variable_delete_use instead of visit_variable_use for 'delete x'.
This makes 'delete' similar to 'typeof'.

This commit should not change behavior.
diff --git a/src/lint.cpp b/src/lint.cpp
@@ -345,6 +345,10 @@ void linter::visit_variable_assignment(identifier name) {
   }
 }
 
+void linter::visit_variable_delete_use(identifier name) {
+  this->visit_variable_use(name, used_variable_kind::use);
+}
+
 void linter::visit_variable_export_use(identifier name) {
   this->visit_variable_use(name, used_variable_kind::_export);
 }
diff --git a/src/main.cpp b/src/main.cpp
@@ -302,6 +302,11 @@ class debug_visitor {
               << '\n';
   }
 
+  void visit_variable_delete_use(identifier name) {
+    std::cerr << "variable delete use: " << out_string8(name.normalized_name())
+              << '\n';
+  }
+
   void visit_variable_export_use(identifier name) {
     std::cerr << "variable export use: " << out_string8(name.normalized_name())
               << '\n';
@@ -410,6 +415,11 @@ class multi_visitor {
     this->visitor_2_->visit_variable_declaration(name, kind);
   }
 
+  void visit_variable_delete_use(identifier name) {
+    this->visitor_1_->visit_variable_delete_use(name);
+    this->visitor_2_->visit_variable_delete_use(name);
+  }
+
   void visit_variable_export_use(identifier name) {
     this->visitor_1_->visit_variable_export_use(name);
     this->visitor_2_->visit_variable_export_use(name);
diff --git a/src/parse.cpp b/src/parse.cpp
@@ -236,10 +236,13 @@ expression* parser::parse_primary_expression(precedence prec) {
       });
     }
     expression* ast =
-        type == token_type::kw_typeof
-            ? this->make_expression<expression::_typeof>(child, operator_span)
-            : this->make_expression<expression::unary_operator>(child,
-                                                                operator_span);
+        type == token_type::kw_delete
+            ? this->make_expression<expression::_delete>(child, operator_span)
+            : type == token_type::kw_typeof
+                  ? this->make_expression<expression::_typeof>(child,
+                                                               operator_span)
+                  : this->make_expression<expression::unary_operator>(
+                        child, operator_span);
     if (type == token_type::kw_delete &&
         child->kind() == expression_kind::variable) {
       this->error_reporter_->report(
@@ -1264,6 +1267,7 @@ void parser::parse_arrow_function_expression_remainder(
     break;
 
   case expression_kind::_class:
+  case expression_kind::_delete:
   case expression_kind::_invalid:
   case expression_kind::_new:
   case expression_kind::_template:
diff --git a/src/quick-lint-js/buffering-visitor.h b/src/quick-lint-js/buffering-visitor.h
@@ -83,6 +83,9 @@ class buffering_visitor {
       case visit_kind::variable_assignment:
         target.visit_variable_assignment(v.name);
         break;
+      case visit_kind::variable_delete_use:
+        target.visit_variable_delete_use(v.name);
+        break;
       case visit_kind::variable_export_use:
         target.visit_variable_export_use(v.name);
         break;
@@ -172,6 +175,10 @@ class buffering_visitor {
     this->visits_.emplace_back(visit_kind::variable_declaration, name, kind);
   }
 
+  void visit_variable_delete_use(identifier name) {
+    this->visits_.emplace_back(visit_kind::variable_delete_use, name);
+  }
+
   void visit_variable_export_use(identifier name) {
     this->visits_.emplace_back(visit_kind::variable_export_use, name);
   }
@@ -203,6 +210,7 @@ class buffering_visitor {
     property_declaration_with_name,
     property_declaration_without_name,
     variable_assignment,
+    variable_delete_use,
     variable_export_use,
     variable_typeof_use,
     variable_use,
diff --git a/src/quick-lint-js/expression.h b/src/quick-lint-js/expression.h
@@ -36,6 +36,7 @@ class expression;
 
 enum class expression_kind {
   _class,
+  _delete,
   _invalid,
   _new,
   _template,
@@ -156,6 +157,7 @@ class expression {
   class expression_with_prefix_operator_base;
 
   class _class;
+  class _delete;
   class _invalid;
   class _new;
   class _template;
@@ -366,6 +368,17 @@ class expression::expression_with_prefix_operator_base : public expression {
   expression *child_;
 };
 
+class expression::_delete final
+    : public expression::expression_with_prefix_operator_base {
+ public:
+  static constexpr expression_kind kind = expression_kind::_delete;
+
+  explicit _delete(expression *child, source_code_span operator_span) noexcept
+      : expression::expression_with_prefix_operator_base(kind, child,
+                                                         operator_span) {}
+};
+static_assert(expression_arena::is_allocatable<expression::_delete>);
+
 class expression::_class : public expression {
  public:
   static constexpr expression_kind kind = expression_kind::_class;
@@ -1166,6 +1179,7 @@ inline auto expression::with_derived(Func &&func) {
 
   switch (this->kind()) {
     QLJS_EXPRESSION_CASE(_class)
+    QLJS_EXPRESSION_CASE(_delete)
     QLJS_EXPRESSION_CASE(_invalid)
     QLJS_EXPRESSION_CASE(_new)
     QLJS_EXPRESSION_CASE(_template)
diff --git a/src/quick-lint-js/lint.h b/src/quick-lint-js/lint.h
@@ -82,6 +82,7 @@ class linter {
   void visit_property_declaration(std::optional<identifier>);
   void visit_variable_declaration(identifier name, variable_kind kind);
   void visit_variable_assignment(identifier name);
+  void visit_variable_delete_use(identifier name);
   void visit_variable_export_use(identifier name);
   void visit_variable_typeof_use(identifier name);
   void visit_variable_use(identifier name);
diff --git a/src/quick-lint-js/null-visitor.h b/src/quick-lint-js/null-visitor.h
@@ -28,6 +28,7 @@ class null_visitor {
   void visit_property_declaration(std::optional<identifier>) {}
   void visit_variable_assignment(identifier) {}
   void visit_variable_declaration(identifier, variable_kind) {}
+  void visit_variable_delete_use(identifier) {}
   void visit_variable_export_use(identifier) {}
   void visit_variable_typeof_use(identifier) {}
   void visit_variable_use(identifier) {}
diff --git a/src/quick-lint-js/parse-visitor.h b/src/quick-lint-js/parse-visitor.h
@@ -50,6 +50,7 @@ concept parse_visitor = requires(Visitor v, identifier name,
   {v.visit_property_declaration(std::optional<identifier>(name))};
   {v.visit_variable_assignment(name)};
   {v.visit_variable_declaration(name, var_kind)};
+  {v.visit_variable_delete_use(name)};
   {v.visit_variable_export_use(name)};
   {v.visit_variable_typeof_use(name)};
   {v.visit_variable_use(name)};
diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h
@@ -796,6 +796,15 @@ class parser {
       }
       break;
     }
+    case expression_kind::_delete: {
+      expression *child = ast->child_0();
+      if (child->kind() == expression_kind::variable) {
+        v.visit_variable_delete_use(child->variable_identifier());
+      } else {
+        this->visit_expression(child, v, context);
+      }
+      break;
+    }
     case expression_kind::await:
     case expression_kind::spread:
     case expression_kind::unary_operator:
@@ -3554,6 +3563,7 @@ class parser {
     }
 
     case expression_kind::_class:
+    case expression_kind::_delete:
     case expression_kind::_new:
     case expression_kind::_template:
     case expression_kind::_typeof:
diff --git a/test/quick-lint-js/spy-visitor.h b/test/quick-lint-js/spy-visitor.h
@@ -158,6 +158,12 @@ struct spy_visitor : public error_collector {
   };
   std::vector<visited_variable_declaration> variable_declarations;
 
+  void visit_variable_delete_use(identifier name) {
+    this->variable_uses.emplace_back(
+        visited_variable_use{string8(name.normalized_name())});
+    this->visits.emplace_back("visit_variable_delete_use");
+  }
+
   void visit_variable_export_use(identifier name) {
     this->variable_uses.emplace_back(
         visited_variable_use{string8(name.normalized_name())});
diff --git a/test/test-buffering-visitor.cpp b/test/test-buffering-visitor.cpp
@@ -51,6 +51,7 @@ TEST(test_buffering_visitor, buffers_all_visits) {
   v.visit_variable_assignment(identifier_of(variable_name));
   v.visit_variable_declaration(identifier_of(variable_name),
                                variable_kind::_var);
+  v.visit_variable_delete_use(identifier_of(variable_name));
   v.visit_variable_export_use(identifier_of(variable_name));
   v.visit_variable_typeof_use(identifier_of(variable_name));
   v.visit_variable_use(identifier_of(variable_name));
@@ -76,6 +77,7 @@ TEST(test_buffering_visitor, buffers_all_visits) {
                           "visit_property_declaration",        //
                           "visit_variable_assignment",         //
                           "visit_variable_declaration",        //
+                          "visit_variable_delete_use",         //
                           "visit_variable_export_use",         //
                           "visit_variable_typeof_use",         //
                           "visit_variable_use"));
diff --git a/test/test-parse-expression-statement.cpp b/test/test-parse-expression-statement.cpp
@@ -779,20 +779,6 @@ TEST(test_parse, expression_statement) {
                                       "visit_exit_function_scope"));
   }
 
-  {
-    spy_visitor v;
-    padded_string code(u8"delete x;"_sv);
-    parser p(&code, &v);
-    p.parse_and_visit_expression(v);
-    EXPECT_THAT(v.visits, ElementsAre("visit_variable_use"));
-    EXPECT_THAT(v.variable_uses,
-                ElementsAre(spy_visitor::visited_variable_use{u8"x"}));
-    EXPECT_THAT(v.errors, ElementsAre(ERROR_TYPE_FIELD(
-                              error_redundant_delete_statement_on_variable,
-                              delete_expression,
-                              offsets_matcher(&code, 0, u8"delete x"))));
-  }
-
   {
     padded_string code(u8"#myArray[index] = rhs;"_sv);
     spy_visitor v;
@@ -811,6 +797,30 @@ TEST(test_parse, expression_statement) {
   }
 }
 
+TEST(test_parse, delete_of_variable) {
+  spy_visitor v;
+  padded_string code(u8"delete x;"_sv);
+  parser p(&code, &v);
+  p.parse_and_visit_expression(v);
+  EXPECT_THAT(v.visits, ElementsAre("visit_variable_delete_use"));
+  EXPECT_THAT(v.variable_uses,
+              ElementsAre(spy_visitor::visited_variable_use{u8"x"}));
+  EXPECT_THAT(v.errors,
+              ElementsAre(ERROR_TYPE_FIELD(
+                  error_redundant_delete_statement_on_variable,
+                  delete_expression, offsets_matcher(&code, 0, u8"delete x"))));
+}
+
+TEST(test_parse, delete_of_expression) {
+  spy_visitor v;
+  padded_string code(u8"delete x.p;"_sv);
+  parser p(&code, &v);
+  p.parse_and_visit_expression(v);
+  EXPECT_THAT(v.visits, ElementsAre("visit_variable_use"));
+  EXPECT_THAT(v.variable_uses,
+              ElementsAre(spy_visitor::visited_variable_use{u8"x"}));
+}
+
 TEST(test_parse, cannot_reference_private_identifier_outside_class) {
   {
     padded_string code(u8"this.#x = 10;"_sv);
diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp
@@ -442,7 +442,7 @@ TEST_F(test_parse_expression, delete_unary_operator) {
   {
     test_parser p(u8"delete variable");
     expression* ast = p.parse_expression();
-    EXPECT_EQ(summarize(ast), "unary(var variable)");
+    EXPECT_EQ(summarize(ast), "delete(var variable)");
     EXPECT_THAT(
         p.errors(),
         ElementsAre(ERROR_TYPE_FIELD(
@@ -452,7 +452,7 @@ TEST_F(test_parse_expression, delete_unary_operator) {
 
   {
     expression* ast = this->parse_expression(u8"delete variable.property"_sv);
-    EXPECT_EQ(summarize(ast), "unary(dot(var variable, property))");
+    EXPECT_EQ(summarize(ast), "delete(dot(var variable, property))");
   }
 }
 
@@ -959,6 +959,7 @@ TEST_F(test_parse_expression,
          {u8"await async () => {}"_sv, nullptr, "await(asyncarrowblock())"},
          {u8"await await x"_sv,        nullptr, "await(await(var x))"},
          {u8"await class{}"_sv,        nullptr, "await(class)"},
+         {u8"await delete x.p"_sv,     nullptr, "await(delete(dot(var x, p)))"},
          {u8"await function() {}"_sv,  nullptr, "await(function)"},
          {u8"await /regexp/"_sv,       nullptr, "await(literal)"},
          {u8"await /=regexp/"_sv,      nullptr, "await(literal)"},
@@ -973,7 +974,6 @@ TEST_F(test_parse_expression,
          {u8"await super"_sv,          nullptr, "await(super)"},
          {u8"await typeof x"_sv,       nullptr, "await(typeof(var x))"},
          {u8"await !x"_sv,             nullptr, "await(unary(var x))"},
-         {u8"await delete x.p"_sv,     nullptr, "await(unary(dot(var x, p)))"},
          {u8"await void x"_sv,         nullptr, "await(unary(var x))"},
          {u8"await ~x"_sv,             nullptr, "await(unary(var x))"},
          {u8"await as"_sv,             nullptr, "await(var as)"},
@@ -3326,6 +3326,8 @@ std::string summarize(const expression& expression) {
   switch (expression.kind()) {
   case expression_kind::_class:
     return "class";
+  case expression_kind::_delete:
+    return "delete(" + summarize(expression.child_0()) + ")";
   case expression_kind::_invalid:
     return "?";
   case expression_kind::_new:

Original file line number	Diff line number	Diff line change
`@@ -345,6 +345,10 @@ void linter::visit_variable_assignment(identifier name) {`
`345`	`345`	`}`
`346`	`346`	`}`
`347`	`347`
	`348`	`+void linter::visit_variable_delete_use(identifier name) {`
	`349`	`+ this->visit_variable_use(name, used_variable_kind::use);`
	`350`	`+}`
	`351`	`+`
`348`	`352`	`void linter::visit_variable_export_use(identifier name) {`
`349`	`353`	`this->visit_variable_use(name, used_variable_kind::_export);`
`350`	`354`	`}`