Disable variable lookup errors in with blocks

singalhimanshu · web-flow · commit 2ad855f561d9 · 2021-05-11T01:33:49.000-07:00
Fixes #94
diff --git a/src/lint.cpp b/src/lint.cpp
@@ -219,6 +219,8 @@ linter::linter(error_reporter *error_reporter)
 
 void linter::visit_enter_block_scope() { this->scopes_.push(); }
 
+void linter::visit_enter_with_scope() { this->scopes_.push(); }
+
 void linter::visit_enter_class_scope() { this->scopes_.push(); }
 
 void linter::visit_enter_for_scope() { this->scopes_.push(); }
@@ -247,6 +249,13 @@ void linter::visit_exit_block_scope() {
   this->scopes_.pop();
 }
 
+void linter::visit_exit_with_scope() {
+  QLJS_ASSERT(!this->scopes_.empty());
+  // Don't propagate variable uses, only declarations
+  this->propagate_variable_declarations_to_parent_scope();
+  this->scopes_.pop();
+}
+
 void linter::visit_exit_class_scope() {
   QLJS_ASSERT(!this->scopes_.empty());
   this->propagate_variable_uses_to_parent_scope(
diff --git a/src/main.cpp b/src/main.cpp
@@ -180,6 +180,8 @@ class debug_visitor {
 
   void visit_enter_block_scope() { std::cerr << "entered block scope\n"; }
 
+  void visit_enter_with_scope() { std::cerr << "entered with scope\n"; }
+
   void visit_enter_class_scope() { std::cerr << "entered class scope\n"; }
 
   void visit_enter_for_scope() { std::cerr << "entered for scope\n"; }
@@ -196,6 +198,8 @@ class debug_visitor {
 
   void visit_exit_block_scope() { std::cerr << "exited block scope\n"; }
 
+  void visit_exit_with_scope() { std::cerr << "exited with scope\n"; }
+
   void visit_exit_class_scope() { std::cerr << "exited class scope\n"; }
 
   void visit_exit_for_scope() { std::cerr << "exited for scope\n"; }
@@ -253,6 +257,11 @@ class multi_visitor {
     this->visitor_2_->visit_enter_block_scope();
   }
 
+  void visit_enter_with_scope() {
+    this->visitor_1_->visit_enter_with_scope();
+    this->visitor_2_->visit_enter_with_scope();
+  }
+
   void visit_enter_class_scope() {
     this->visitor_1_->visit_enter_class_scope();
     this->visitor_2_->visit_enter_class_scope();
@@ -283,6 +292,11 @@ class multi_visitor {
     this->visitor_2_->visit_exit_block_scope();
   }
 
+  void visit_exit_with_scope() {
+    this->visitor_1_->visit_exit_with_scope();
+    this->visitor_2_->visit_exit_with_scope();
+  }
+
   void visit_exit_class_scope() {
     this->visitor_1_->visit_exit_class_scope();
     this->visitor_2_->visit_exit_class_scope();
diff --git a/src/quick-lint-js/buffering-visitor.h b/src/quick-lint-js/buffering-visitor.h
@@ -38,6 +38,9 @@ class buffering_visitor {
       case visit_kind::enter_block_scope:
         target.visit_enter_block_scope();
         break;
+      case visit_kind::enter_with_scope:
+        target.visit_enter_with_scope();
+        break;
       case visit_kind::enter_class_scope:
         target.visit_enter_class_scope();
         break;
@@ -56,6 +59,9 @@ class buffering_visitor {
       case visit_kind::exit_block_scope:
         target.visit_exit_block_scope();
         break;
+      case visit_kind::exit_with_scope:
+        target.visit_exit_with_scope();
+        break;
       case visit_kind::exit_class_scope:
         target.visit_exit_class_scope();
         break;
@@ -98,6 +104,10 @@ class buffering_visitor {
     this->visits_.emplace_back(visit_kind::enter_block_scope);
   }
 
+  void visit_enter_with_scope() {
+    this->visits_.emplace_back(visit_kind::enter_with_scope);
+  }
+
   void visit_enter_class_scope() {
     this->visits_.emplace_back(visit_kind::enter_class_scope);
   }
@@ -122,6 +132,10 @@ class buffering_visitor {
     this->visits_.emplace_back(visit_kind::exit_block_scope);
   }
 
+  void visit_exit_with_scope() {
+    this->visits_.emplace_back(visit_kind::exit_with_scope);
+  }
+
   void visit_exit_class_scope() {
     this->visits_.emplace_back(visit_kind::exit_class_scope);
   }
@@ -167,12 +181,14 @@ class buffering_visitor {
   enum class visit_kind {
     end_of_module,
     enter_block_scope,
+    enter_with_scope,
     enter_class_scope,
     enter_for_scope,
     enter_function_scope,
     enter_function_scope_body,
     enter_named_function_scope,
     exit_block_scope,
+    exit_with_scope,
     exit_class_scope,
     exit_for_scope,
     exit_function_scope,
diff --git a/src/quick-lint-js/lint.h b/src/quick-lint-js/lint.h
@@ -27,12 +27,14 @@ class linter {
   explicit linter(error_reporter *error_reporter);
 
   void visit_enter_block_scope();
+  void visit_enter_with_scope();
   void visit_enter_class_scope();
   void visit_enter_for_scope();
   void visit_enter_function_scope();
   void visit_enter_function_scope_body();
   void visit_enter_named_function_scope(identifier);
   void visit_exit_block_scope();
+  void visit_exit_with_scope();
   void visit_exit_class_scope();
   void visit_exit_for_scope();
   void visit_exit_function_scope();
diff --git a/src/quick-lint-js/null-visitor.h b/src/quick-lint-js/null-visitor.h
@@ -13,12 +13,14 @@ class null_visitor {
  public:
   void visit_end_of_module() {}
   void visit_enter_block_scope() {}
+  void visit_enter_with_scope() {}
   void visit_enter_class_scope() {}
   void visit_enter_for_scope() {}
   void visit_enter_function_scope() {}
   void visit_enter_function_scope_body() {}
   void visit_enter_named_function_scope(identifier) {}
   void visit_exit_block_scope() {}
+  void visit_exit_with_scope() {}
   void visit_exit_class_scope() {}
   void visit_exit_for_scope() {}
   void visit_exit_function_scope() {}
diff --git a/src/quick-lint-js/parse-visitor.h b/src/quick-lint-js/parse-visitor.h
@@ -34,12 +34,14 @@ concept parse_visitor = requires(Visitor v, identifier name,
                                  variable_kind var_kind) {
   {v.visit_end_of_module()};
   {v.visit_enter_block_scope()};
+  {v.visit_enter_with_scope()};
   {v.visit_enter_class_scope()};
   {v.visit_enter_for_scope()};
   {v.visit_enter_function_scope()};
   {v.visit_enter_function_scope_body()};
   {v.visit_enter_named_function_scope(name)};
   {v.visit_exit_block_scope()};
+  {v.visit_exit_with_scope()};
   {v.visit_exit_class_scope()};
   {v.visit_exit_for_scope()};
   {v.visit_exit_function_scope()};
diff --git a/src/quick-lint-js/parse.h b/src/quick-lint-js/parse.h
@@ -2450,11 +2450,14 @@ class parser {
     this->error_on_class_statement(statement_kind::with_statement);
     this->error_on_function_statement(statement_kind::with_statement);
     this->error_on_lexical_declaration(statement_kind::with_statement);
+
+    v.visit_enter_with_scope();
     bool parsed_body =
         this->parse_and_visit_statement_disallowing_declaration(v);
     if (!parsed_body) {
       QLJS_PARSER_UNIMPLEMENTED();
     }
+    v.visit_exit_with_scope();
   }
 
   template <QLJS_PARSE_VISITOR Visitor>
diff --git a/test/quick-lint-js/spy-visitor.h b/test/quick-lint-js/spy-visitor.h
@@ -27,6 +27,10 @@ struct spy_visitor : public error_collector {
     this->visits.emplace_back("visit_enter_block_scope");
   }
 
+  void visit_enter_with_scope() {
+    this->visits.emplace_back("visit_enter_with_scope");
+  }
+
   void visit_enter_class_scope() {
     this->visits.emplace_back("visit_enter_class_scope");
   }
@@ -66,6 +70,10 @@ struct spy_visitor : public error_collector {
     this->visits.emplace_back("visit_exit_block_scope");
   }
 
+  void visit_exit_with_scope() {
+    this->visits.emplace_back("visit_exit_with_scope");
+  }
+
   void visit_exit_class_scope() {
     this->visits.emplace_back("visit_exit_class_scope");
   }
diff --git a/test/test-buffering-visitor.cpp b/test/test-buffering-visitor.cpp
@@ -34,12 +34,14 @@ TEST(test_buffering_visitor, buffers_all_visits) {
   buffering_visitor v(new_delete_resource());
   v.visit_end_of_module();
   v.visit_enter_block_scope();
+  v.visit_enter_with_scope();
   v.visit_enter_class_scope();
   v.visit_enter_for_scope();
   v.visit_enter_named_function_scope(identifier_of(function_name));
   v.visit_enter_function_scope();
   v.visit_enter_function_scope_body();
   v.visit_exit_block_scope();
+  v.visit_exit_with_scope();
   v.visit_exit_class_scope();
   v.visit_exit_for_scope();
   v.visit_exit_function_scope();
@@ -57,12 +59,14 @@ TEST(test_buffering_visitor, buffers_all_visits) {
   EXPECT_THAT(spy.visits,
               ElementsAre("visit_end_of_module",               //
                           "visit_enter_block_scope",           //
+                          "visit_enter_with_scope",            //
                           "visit_enter_class_scope",           //
                           "visit_enter_for_scope",             //
                           "visit_enter_named_function_scope",  //
                           "visit_enter_function_scope",        //
                           "visit_enter_function_scope_body",   //
                           "visit_exit_block_scope",            //
+                          "visit_exit_with_scope",             //
                           "visit_exit_class_scope",            //
                           "visit_exit_for_scope",              //
                           "visit_exit_function_scope",         //
diff --git a/test/test-lint.cpp b/test/test-lint.cpp
@@ -2386,6 +2386,110 @@ TEST(
          "with 'b'";
 }
 
+TEST(test_lint, with_does_not_propagate_variable_uses) {
+  const char8 declaration[] = u8"a";
+  const char8 assignment[] = u8"a";
+  const char8 use[] = u8"a";
+
+  {
+    // with({})
+    //   a;
+    error_collector v;
+    linter l(&v);
+    l.visit_enter_with_scope();
+    l.visit_variable_use(identifier_of(use));
+    l.visit_exit_with_scope();
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, IsEmpty()) << "use of undeclared variable should not "
+                                        "be an error inside with scope";
+  }
+
+  {
+    // const a = 1;
+    // with ({})
+    //   a = 2;
+
+    error_collector v;
+    linter l(&v);
+    l.visit_variable_declaration(identifier_of(declaration),
+                                 variable_kind::_const);
+    l.visit_enter_with_scope();
+    l.visit_variable_assignment(identifier_of(assignment));
+    l.visit_exit_with_scope();
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, IsEmpty()) << "assigning to 'a' should not "
+                                        "be an error inside with scope";
+  }
+
+  {
+    // with ({})
+    //   a = 2;
+    // let a;
+
+    error_collector v;
+    linter l(&v);
+    l.visit_enter_with_scope();
+    l.visit_variable_assignment(identifier_of(assignment));
+    l.visit_exit_with_scope();
+    l.visit_variable_declaration(identifier_of(declaration),
+                                 variable_kind::_let);
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, IsEmpty()) << "assigning to 'a' should not "
+                                        "be an error inside with scope";
+  }
+
+  {
+    // with ({}) {
+    //   const a = 1;
+    //   a = 2;
+    // }
+
+    error_collector v;
+    linter l(&v);
+    l.visit_enter_with_scope();
+    l.visit_enter_block_scope();
+    l.visit_variable_declaration(identifier_of(declaration),
+                                 variable_kind::_const);
+    l.visit_variable_assignment(identifier_of(assignment));
+    l.visit_exit_block_scope();
+    l.visit_exit_with_scope();
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, ElementsAre(ERROR_TYPE_3_FIELDS(
+                              error_assignment_to_const_variable,      //
+                              assignment, span_matcher(assignment),    //
+                              declaration, span_matcher(declaration),  //
+                              var_kind, variable_kind::_const)));
+  }
+
+  {
+    // with ({}) {
+    //   function f() {
+    //     a;
+    //   }
+    // }
+
+    error_collector v;
+    linter l(&v);
+    l.visit_enter_with_scope();
+    l.visit_enter_block_scope();
+    l.visit_enter_function_scope();
+    l.visit_enter_function_scope_body();
+    l.visit_variable_use(identifier_of(use));
+    l.visit_exit_function_scope();
+    l.visit_exit_block_scope();
+    l.visit_exit_with_scope();
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, IsEmpty()) << "use of undeclared variable should not "
+                                        "be an error inside a function inside a"
+                                        "with scope";
+  }
+}
+
 TEST(test_lint_magic_arguments,
      arguments_magic_variable_is_usable_within_functions) {
   const char8 arguments_use[] = u8"arguments";
diff --git a/test/test-parse-class.cpp b/test/test-parse-class.cpp
@@ -1062,9 +1062,11 @@ TEST(test_parse, class_statement_as_with_statement_body_is_disallowed) {
     parser p(&code, &v);
     EXPECT_TRUE(p.parse_and_visit_statement(v));
     EXPECT_THAT(v.visits, ElementsAre("visit_variable_use",          // obj
+                                      "visit_enter_with_scope",      // with
                                       "visit_variable_declaration",  // C
                                       "visit_enter_class_scope",     // C
-                                      "visit_exit_class_scope"));    // C
+                                      "visit_exit_class_scope",      // C
+                                      "visit_exit_with_scope"));
     EXPECT_THAT(
         v.errors,
         ElementsAre(ERROR_TYPE_3_FIELDS(
diff --git a/test/test-parse-expression-statement.cpp b/test/test-parse-expression-statement.cpp
@@ -1340,8 +1340,10 @@ TEST(test_parse, let_as_statement_body_allows_asi) {
 
   {
     spy_visitor v = parse_and_visit_module(u8"with (obj) let\nx = y;");
-    EXPECT_THAT(v.visits, ElementsAre("visit_variable_use",         // obj
-                                      "visit_variable_use",         // let
+    EXPECT_THAT(v.visits, ElementsAre("visit_variable_use",      // obj
+                                      "visit_enter_with_scope",  // with
+                                      "visit_variable_use",      // let
+                                      "visit_exit_with_scope",
                                       "visit_variable_use",         // y
                                       "visit_variable_assignment",  // x
                                       "visit_end_of_module"));
diff --git a/test/test-parse-function.cpp b/test/test-parse-function.cpp
diff --git a/test/test-parse-statement.cpp b/test/test-parse-statement.cpp
diff --git a/test/test-parse-var.cpp b/test/test-parse-var.cpp

Original file line number	Diff line number	Diff line change
`@@ -2450,11 +2450,14 @@ class parser {`
`2450`	`2450`	`this->error_on_class_statement(statement_kind::with_statement);`
`2451`	`2451`	`this->error_on_function_statement(statement_kind::with_statement);`
`2452`	`2452`	`this->error_on_lexical_declaration(statement_kind::with_statement);`
	`2453`	`+`
	`2454`	`+ v.visit_enter_with_scope();`
`2453`	`2455`	`bool parsed_body =`
`2454`	`2456`	`this->parse_and_visit_statement_disallowing_declaration(v);`
`2455`	`2457`	`if (!parsed_body) {`
`2456`	`2458`	`QLJS_PARSER_UNIMPLEMENTED();`
`2457`	`2459`	`}`
	`2460`	`+ v.visit_exit_with_scope();`
`2458`	`2461`	`}`
`2459`	`2462`
`2460`	`2463`	`template <QLJS_PARSE_VISITOR Visitor>`
Original file line number	Diff line number	Diff line change
`@@ -27,6 +27,10 @@ struct spy_visitor : public error_collector {`
`27`	`27`	`this->visits.emplace_back("visit_enter_block_scope");`
`28`	`28`	`}`
`29`	`29`
	`30`	`+ void visit_enter_with_scope() {`
	`31`	`+ this->visits.emplace_back("visit_enter_with_scope");`
	`32`	`+ }`
	`33`	`+`
`30`	`34`	`void visit_enter_class_scope() {`
`31`	`35`	`this->visits.emplace_back("visit_enter_class_scope");`
`32`	`36`	`}`
`@@ -66,6 +70,10 @@ struct spy_visitor : public error_collector {`
`66`	`70`	`this->visits.emplace_back("visit_exit_block_scope");`
`67`	`71`	`}`
`68`	`72`
	`73`	`+ void visit_exit_with_scope() {`
	`74`	`+ this->visits.emplace_back("visit_exit_with_scope");`
	`75`	`+ }`
	`76`	`+`
`69`	`77`	`void visit_exit_class_scope() {`
`70`	`78`	`this->visits.emplace_back("visit_exit_class_scope");`
`71`	`79`	`}`