Fix crash when shadowing built-in globals

If an import (or other variable declaration) happens to shadow an automatically-declared global variable (such as NaN), quick-lint-js crashes: // JavaScript code: import { TextEncoder } from "util"; # quick-lint-js output: src/lint.cpp:503: internal check failed in report_error_if_variable_declaration_conflicts_in_scope: already_declared_variable->declaration.has_value() Fix the crash by putting user-declared global variables in a scope separate from automatically-declared global variables.
jeduardofr · Sep 11, 2020 · c219874 · c219874
1 parent 3871474
commit c219874
Show file tree

Hide file tree

Showing 2 changed files with 153 additions and 80 deletions.
diff --git a/src/lint.cpp b/src/lint.cpp
@@ -28,8 +28,9 @@
 namespace quick_lint_js {
 linter::linter(error_reporter *error_reporter)
     : error_reporter_(error_reporter) {
-  this->scopes_.emplace_back();
-  scope &global_scope = this->scopes_.back();
+  this->scopes_.emplace_back();  // Global scope.
+  this->scopes_.emplace_back();  // Module scope.
+  scope &global_scope = this->scopes_.front();
 
   const char8 *writable_global_variables[] = {
       // ECMA-262 18.1 Value Properties of the Global Object
@@ -293,6 +294,11 @@ void linter::visit_variable_use(identifier name, used_variable_kind use_kind) {
 }
 
 void linter::visit_end_of_module() {
+  this->propagate_variable_uses_to_parent_scope(
+      /*allow_variable_use_before_declaration=*/false,
+      /*consume_arguments=*/false);
+  this->scopes_.pop_back();
+
   std::vector<identifier> typeof_variables;
   for (const used_variable &used_var : this->scopes_.back().variables_used) {
     if (used_var.kind == used_variable_kind::_typeof) {

diff --git a/test/test-lint.cpp b/test/test-lint.cpp
@@ -140,87 +140,109 @@ TEST(test_lint, immutable_global_variables_are_not_assignable) {
               error_collector::error_assignment_to_const_global_variable);
     EXPECT_EQ(v.errors[0].where.begin(), global_variable);
   }
+
+  for (const char8 *global_variable : non_writable_global_variables) {
+    SCOPED_TRACE(out_string8(global_variable));
+
+    // (() => {
+    //   NaN = null;  // ERROR
+    // });
+    error_collector v;
+    linter l(&v);
+    l.visit_enter_function_scope();
+    l.visit_enter_function_scope_body();
+    l.visit_variable_assignment(identifier_of(global_variable));
+    l.visit_exit_function_scope();
+    l.visit_end_of_module();
+
+    ASSERT_EQ(v.errors.size(), 1);
+    EXPECT_EQ(v.errors[0].kind,
+              error_collector::error_assignment_to_const_global_variable);
+    EXPECT_EQ(v.errors[0].where.begin(), global_variable);
+  }
 }
 
-TEST(test_lint, nodejs_global_variables_are_usable) {
-  static constexpr const char8 *nodejs_global_variables[] = {
-      u8"Array",
-      u8"ArrayBuffer",
-      u8"Atomics",
-      u8"BigInt",
-      u8"BigInt64Array",
-      u8"BigUint64Array",
-      u8"Boolean",
-      u8"Buffer",
-      u8"DataView",
-      u8"Date",
-      u8"Error",
-      u8"EvalError",
-      u8"Float32Array",
-      u8"Float64Array",
-      u8"Function",
-      u8"GLOBAL",
-      u8"Infinity",
-      u8"Int16Array",
-      u8"Int32Array",
-      u8"Int8Array",
-      u8"Intl",
-      u8"JSON",
-      u8"Map",
-      u8"Math",
-      u8"NaN",
-      u8"Number",
-      u8"Object",
-      u8"Promise",
-      u8"Proxy",
-      u8"RangeError",
-      u8"ReferenceError",
-      u8"Reflect",
-      u8"RegExp",
-      u8"Set",
-      u8"SharedArrayBuffer",
-      u8"String",
-      u8"Symbol",
-      u8"SyntaxError",
-      u8"TextDecoder",
-      u8"TextEncoder",
-      u8"TypeError",
-      u8"URIError",
-      u8"URL",
-      u8"URLSearchParams",
-      u8"Uint16Array",
-      u8"Uint32Array",
-      u8"Uint8Array",
-      u8"Uint8ClampedArray",
-      u8"WeakMap",
-      u8"WeakSet",
-      u8"WebAssembly",
-      u8"clearImmediate",
-      u8"clearInterval",
-      u8"clearTimeout",
-      u8"console",
-      u8"decodeURI",
-      u8"decodeURIComponent",
-      u8"encodeURI",
-      u8"encodeURIComponent",
-      u8"escape",
-      u8"eval",
-      u8"global",
-      u8"globalThis",
-      u8"isFinite",
-      u8"isNaN",
-      u8"parseFloat",
-      u8"parseInt",
-      u8"process",
-      u8"queueMicrotask",
-      u8"root",
-      u8"setImmediate",
-      u8"setInterval",
-      u8"setTimeout",
-      u8"undefined",
-      u8"unescape",
-  };
+namespace {
+constexpr const char8 *nodejs_global_variables[] = {
+    u8"Array",
+    u8"ArrayBuffer",
+    u8"Atomics",
+    u8"BigInt",
+    u8"BigInt64Array",
+    u8"BigUint64Array",
+    u8"Boolean",
+    u8"Buffer",
+    u8"DataView",
+    u8"Date",
+    u8"Error",
+    u8"EvalError",
+    u8"Float32Array",
+    u8"Float64Array",
+    u8"Function",
+    u8"GLOBAL",
+    u8"Infinity",
+    u8"Int16Array",
+    u8"Int32Array",
+    u8"Int8Array",
+    u8"Intl",
+    u8"JSON",
+    u8"Map",
+    u8"Math",
+    u8"NaN",
+    u8"Number",
+    u8"Object",
+    u8"Promise",
+    u8"Proxy",
+    u8"RangeError",
+    u8"ReferenceError",
+    u8"Reflect",
+    u8"RegExp",
+    u8"Set",
+    u8"SharedArrayBuffer",
+    u8"String",
+    u8"Symbol",
+    u8"SyntaxError",
+    u8"TextDecoder",
+    u8"TextEncoder",
+    u8"TypeError",
+    u8"URIError",
+    u8"URL",
+    u8"URLSearchParams",
+    u8"Uint16Array",
+    u8"Uint32Array",
+    u8"Uint8Array",
+    u8"Uint8ClampedArray",
+    u8"WeakMap",
+    u8"WeakSet",
+    u8"WebAssembly",
+    u8"clearImmediate",
+    u8"clearInterval",
+    u8"clearTimeout",
+    u8"console",
+    u8"decodeURI",
+    u8"decodeURIComponent",
+    u8"encodeURI",
+    u8"encodeURIComponent",
+    u8"escape",
+    u8"eval",
+    u8"global",
+    u8"globalThis",
+    u8"isFinite",
+    u8"isNaN",
+    u8"parseFloat",
+    u8"parseInt",
+    u8"process",
+    u8"queueMicrotask",
+    u8"root",
+    u8"setImmediate",
+    u8"setInterval",
+    u8"setTimeout",
+    u8"undefined",
+    u8"unescape",
+};
+}
 
+TEST(test_lint, nodejs_global_variables_are_usable) {
   error_collector v;
   linter l(&v);
   for (const char8 *global_variable : nodejs_global_variables) {
@@ -231,6 +253,18 @@ TEST(test_lint, nodejs_global_variables_are_usable) {
   EXPECT_THAT(v.errors, IsEmpty());
 }
 
+TEST(test_lint, nodejs_global_variables_are_shadowable) {
+  error_collector v;
+  linter l(&v);
+  for (const char8 *global_variable : nodejs_global_variables) {
+    l.visit_variable_declaration(identifier_of(global_variable),
+                                 variable_kind::_let);
+  }
+  l.visit_end_of_module();
+
+  EXPECT_THAT(v.errors, IsEmpty());
+}
+
 TEST(test_lint, let_or_const_or_class_variable_use_before_declaration) {
   for (variable_kind kind :
        {variable_kind::_class, variable_kind::_const, variable_kind::_let}) {
@@ -2108,6 +2142,39 @@ TEST(test_lint, let_shadows_named_function_name) {
   }
 }
 
+TEST(test_lint, let_shadows_global_variable) {
+  const char8 var_declaration[] = u8"Array";
+  const char8 var_use[] = u8"Array";
+
+  {
+    // let Array;
+    error_collector v;
+    linter l(&v);
+    l.visit_variable_declaration(identifier_of(var_declaration),
+                                 variable_kind::_let);
+    l.visit_end_of_module();
+
+    EXPECT_THAT(v.errors, IsEmpty());
+  }
+
+  {
+    // Array;
+    // let Array;
+    error_collector v;
+    linter l(&v);
+    l.visit_variable_use(identifier_of(var_use));
+    l.visit_variable_declaration(identifier_of(var_declaration),
+                                 variable_kind::_let);
+    l.visit_end_of_module();
+
+    ASSERT_EQ(v.errors.size(), 1);
+    EXPECT_EQ(v.errors[0].kind,
+              error_collector::error_variable_used_before_declaration);
+    EXPECT_EQ(v.errors[0].where.begin(), var_use);
+    EXPECT_EQ(v.errors[0].other_where.begin(), var_declaration);
+  }
+}
+
 TEST(test_lint_magic_arguments,
      arguments_magic_variable_is_usable_within_functions) {
   const char8 arguments_use[] = u8"arguments";