Refactor: move "delete x" handling into linter

strager · strager · commit a114d08c5fac · 2021-11-03T19:49:10.000-07:00
diff --git a/src/lint.cpp b/src/lint.cpp
@@ -345,8 +345,16 @@ void linter::visit_variable_assignment(identifier name) {
   }
 }
 
-void linter::visit_variable_delete_use(identifier name) {
+void linter::visit_variable_delete_use(identifier name,
+                                       source_code_span delete_keyword) {
   this->visit_variable_use(name, used_variable_kind::use);
+  QLJS_ASSERT(delete_keyword.end() <= name.span().begin());
+  // TODO(strager): What if the variable was parenthesized? We should include
+  // the closing parenthesis.
+  this->error_reporter_->report(error_redundant_delete_statement_on_variable{
+      .delete_expression =
+          source_code_span(delete_keyword.begin(), name.span().end()),
+  });
 }
 
 void linter::visit_variable_export_use(identifier name) {
diff --git a/src/main.cpp b/src/main.cpp
@@ -302,7 +302,8 @@ class debug_visitor {
               << '\n';
   }
 
-  void visit_variable_delete_use(identifier name) {
+  void visit_variable_delete_use(
+      identifier name, [[maybe_unused]] source_code_span delete_keyword) {
     std::cerr << "variable delete use: " << out_string8(name.normalized_name())
               << '\n';
   }
@@ -415,9 +416,10 @@ 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_delete_use(identifier name,
+                                 source_code_span delete_keyword) {
+    this->visitor_1_->visit_variable_delete_use(name, delete_keyword);
+    this->visitor_2_->visit_variable_delete_use(name, delete_keyword);
   }
 
   void visit_variable_export_use(identifier name) {
diff --git a/src/parse.cpp b/src/parse.cpp
@@ -243,13 +243,6 @@ expression* parser::parse_primary_expression(precedence prec) {
                                                                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(
-          error_redundant_delete_statement_on_variable{
-              .delete_expression = ast->span(),
-          });
-    }
     return ast;
   }
 
diff --git a/src/quick-lint-js/buffering-visitor.h b/src/quick-lint-js/buffering-visitor.h
@@ -84,7 +84,7 @@ class buffering_visitor {
         target.visit_variable_assignment(v.name);
         break;
       case visit_kind::variable_delete_use:
-        target.visit_variable_delete_use(v.name);
+        target.visit_variable_delete_use(v.name, v.extra_span);
         break;
       case visit_kind::variable_export_use:
         target.visit_variable_export_use(v.name);
@@ -175,8 +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_delete_use(identifier name,
+                                 source_code_span delete_keyword) {
+    this->visits_.emplace_back(visit_kind::variable_delete_use, name,
+                               delete_keyword);
   }
 
   void visit_variable_export_use(identifier name) {
@@ -227,6 +229,10 @@ class buffering_visitor {
                    variable_kind var_kind) noexcept
         : kind(kind), name(name), var_kind(var_kind) {}
 
+    explicit visit(visit_kind kind, identifier name,
+                   source_code_span extra_span) noexcept
+        : kind(kind), name(name), extra_span(extra_span) {}
+
     visit_kind kind;
 
     union {
@@ -240,6 +246,10 @@ class buffering_visitor {
       // variable_declaration
       variable_kind var_kind;
       static_assert(std::is_trivially_destructible_v<variable_kind>);
+
+      // variable_delete_use
+      source_code_span extra_span;
+      static_assert(std::is_trivially_destructible_v<source_code_span>);
     };
   };
 
diff --git a/src/quick-lint-js/expression.h b/src/quick-lint-js/expression.h
@@ -363,6 +363,10 @@ class expression::expression_with_prefix_operator_base : public expression {
                             this->child_->span().end());
   }
 
+  const char8 *unary_operator_begin() const noexcept {
+    return this->unary_operator_begin_;
+  }
+
  protected:
   const char8 *unary_operator_begin_;
   expression *child_;
@@ -375,7 +379,15 @@ class expression::_delete final
 
   explicit _delete(expression *child, source_code_span operator_span) noexcept
       : expression::expression_with_prefix_operator_base(kind, child,
-                                                         operator_span) {}
+                                                         operator_span) {
+    QLJS_ASSERT(operator_span.string_view() == u8"delete");
+  }
+
+  source_code_span unary_operator_span() {
+    const char8 *operator_begin = this->unary_operator_begin();
+    return source_code_span(operator_begin,
+                            operator_begin + strlen(u8"delete"));
+  }
 };
 static_assert(expression_arena::is_allocatable<expression::_delete>);
 
diff --git a/src/quick-lint-js/lint.h b/src/quick-lint-js/lint.h
@@ -82,7 +82,8 @@ 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_delete_use(identifier name,
+                                 source_code_span delete_keyword);
   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,7 +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_delete_use(identifier, source_code_span) {}
   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
@@ -31,6 +31,7 @@ namespace quick_lint_js {
 #if QLJS_HAVE_CXX_CONCEPTS
 template <class Visitor>
 concept parse_visitor = requires(Visitor v, identifier name,
+                                 source_code_span span,
                                  variable_kind var_kind) {
   {v.visit_end_of_module()};
   {v.visit_enter_block_scope()};
@@ -50,7 +51,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_delete_use(name, span)};
   {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
@@ -799,7 +799,9 @@ class parser {
     case expression_kind::_delete: {
       expression *child = ast->child_0();
       if (child->kind() == expression_kind::variable) {
-        v.visit_variable_delete_use(child->variable_identifier());
+        v.visit_variable_delete_use(
+            child->variable_identifier(),
+            static_cast<expression::_delete *>(ast)->unary_operator_span());
       } else {
         this->visit_expression(child, v, context);
       }
diff --git a/test/quick-lint-js/spy-visitor.h b/test/quick-lint-js/spy-visitor.h
@@ -158,7 +158,8 @@ struct spy_visitor : public error_collector {
   };
   std::vector<visited_variable_declaration> variable_declarations;
 
-  void visit_variable_delete_use(identifier name) {
+  void visit_variable_delete_use(
+      identifier name, [[maybe_unused]] source_code_span delete_keyword) {
     this->variable_uses.emplace_back(
         visited_variable_use{string8(name.normalized_name())});
     this->visits.emplace_back("visit_variable_delete_use");
diff --git a/test/test-buffering-visitor.cpp b/test/test-buffering-visitor.cpp
@@ -27,6 +27,7 @@ identifier identifier_of(const char8 (&name)[N]) {
 }
 
 TEST(test_buffering_visitor, buffers_all_visits) {
+  const char8 delete_keyword[] = u8"delete";
   const char8 function_name[] = u8"function";
   const char8 property_name[] = u8"property";
   const char8 variable_name[] = u8"variable";
@@ -51,7 +52,8 @@ 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_delete_use(identifier_of(variable_name),
+                              span_of(delete_keyword));
   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));
diff --git a/test/test-lint-parse.cpp b/test/test-lint-parse.cpp
@@ -192,6 +192,24 @@ TEST(test_lint, escape_sequence_in_keyword_identifier) {
                           error_keywords_cannot_contain_escape_sequences>(
                   ::testing::_)));
 }
+
+TEST(test_lint, delete_local_variable) {
+  padded_string input(
+      u8"function f(param) { let v; delete v; delete param; }"_sv);
+  error_collector v;
+  linter l(&v, &default_globals);
+  parser p(&input, &v);
+  p.parse_and_visit_module(l);
+  l.visit_end_of_module();
+
+  EXPECT_THAT(
+      v.errors,
+      ElementsAre(
+          ::testing::VariantWith<error_redundant_delete_statement_on_variable>(
+              ::testing::_),
+          ::testing::VariantWith<error_redundant_delete_statement_on_variable>(
+              ::testing::_)));
+}
 }
 }
 
diff --git a/test/test-lint.cpp b/test/test-lint.cpp
@@ -2705,6 +2705,37 @@ TEST(test_lint_magic_arguments, catch_variable_shadows_magic_arguments) {
 
 // TODO(#204): 'arguments' should not be declared in arrow functions.
 
+TEST(test_lint_delete, deleting_local_variable_is_a_warning) {
+  const char8 declaration[] = u8"v";
+  padded_string delete_expression(u8"delete v"_sv);
+  source_code_span delete_keyword_span(delete_expression.data(),
+                                       delete_expression.data() + 6);
+  ASSERT_EQ(delete_keyword_span.string_view(), u8"delete"_sv);
+  source_code_span deleted_variable_span(delete_expression.data() + 7,
+                                         delete_expression.data() + 8);
+  ASSERT_EQ(deleted_variable_span.string_view(), u8"v"_sv);
+
+  // (() => {
+  //   let v;
+  //   delete v;
+  // });
+  error_collector v;
+  linter l(&v, &default_globals);
+  l.visit_enter_function_scope();
+  l.visit_enter_function_scope_body();
+  l.visit_variable_declaration(identifier_of(declaration), variable_kind::_let);
+  l.visit_variable_delete_use(identifier(deleted_variable_span),
+                              delete_keyword_span);
+  l.visit_exit_function_scope();
+  l.visit_end_of_module();
+
+  EXPECT_THAT(
+      v.errors,
+      ElementsAre(ERROR_TYPE_FIELD(
+          error_redundant_delete_statement_on_variable, delete_expression,
+          offsets_matcher(&delete_expression, 0, u8"delete x"))));
+}
+
 TEST(test_lint_typeof, using_undeclared_variable_in_typeof_is_not_an_error) {
   const char8 use[] = u8"v";
 
diff --git a/test/test-parse-expression-statement.cpp b/test/test-parse-expression-statement.cpp
@@ -805,10 +805,7 @@ TEST(test_parse, delete_of_variable) {
   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"))));
+  EXPECT_THAT(v.errors, IsEmpty());
 }
 
 TEST(test_parse, delete_of_expression) {
diff --git a/test/test-parse-expression.cpp b/test/test-parse-expression.cpp
@@ -440,14 +440,8 @@ TEST_F(test_parse_expression, parse_typeof_conditional_operator) {
 
 TEST_F(test_parse_expression, delete_unary_operator) {
   {
-    test_parser p(u8"delete variable");
-    expression* ast = p.parse_expression();
+    expression* ast = this->parse_expression(u8"delete variable"_sv);
     EXPECT_EQ(summarize(ast), "delete(var variable)");
-    EXPECT_THAT(
-        p.errors(),
-        ElementsAre(ERROR_TYPE_FIELD(
-            error_redundant_delete_statement_on_variable, delete_expression,
-            offsets_matcher(p.code(), 0, u8"delete variable"))));
   }
 
   {

Original file line number	Diff line number	Diff line change
`@@ -345,8 +345,16 @@ void linter::visit_variable_assignment(identifier name) {`
`345`	`345`	`}`
`346`	`346`	`}`
`347`	`347`
`348`		`-void linter::visit_variable_delete_use(identifier name) {`
	`348`	`+void linter::visit_variable_delete_use(identifier name,`
	`349`	`+ source_code_span delete_keyword) {`
`349`	`350`	`this->visit_variable_use(name, used_variable_kind::use);`
	`351`	`+ QLJS_ASSERT(delete_keyword.end() <= name.span().begin());`
	`352`	`+ // TODO(strager): What if the variable was parenthesized? We should include`
	`353`	`+ // the closing parenthesis.`
	`354`	`+ this->error_reporter_->report(error_redundant_delete_statement_on_variable{`
	`355`	`+ .delete_expression =`
	`356`	`+ source_code_span(delete_keyword.begin(), name.span().end()),`
	`357`	`+ });`
`350`	`358`	`}`
`351`	`359`
`352`	`360`	`void linter::visit_variable_export_use(identifier name) {`
Original file line number	Diff line number	Diff line change
`@@ -302,7 +302,8 @@ class debug_visitor {`
`302`	`302`	`<< '\n';`
`303`	`303`	`}`
`304`	`304`
`305`		`- void visit_variable_delete_use(identifier name) {`
	`305`	`+ void visit_variable_delete_use(`
	`306`	`+ identifier name, [[maybe_unused]] source_code_span delete_keyword) {`
`306`	`307`	`std::cerr << "variable delete use: " << out_string8(name.normalized_name())`
`307`	`308`	`<< '\n';`
`308`	`309`	`}`
`@@ -415,9 +416,10 @@ class multi_visitor {`
`415`	`416`	`this->visitor_2_->visit_variable_declaration(name, kind);`
`416`	`417`	`}`
`417`	`418`
`418`		`- void visit_variable_delete_use(identifier name) {`
`419`		`- this->visitor_1_->visit_variable_delete_use(name);`
`420`		`- this->visitor_2_->visit_variable_delete_use(name);`
	`419`	`+ void visit_variable_delete_use(identifier name,`
	`420`	`+ source_code_span delete_keyword) {`
	`421`	`+ this->visitor_1_->visit_variable_delete_use(name, delete_keyword);`
	`422`	`+ this->visitor_2_->visit_variable_delete_use(name, delete_keyword);`
`421`	`423`	`}`
`422`	`424`
`423`	`425`	`void visit_variable_export_use(identifier name) {`