From 3f8b78f5a43e07d10e0bd20998a15835a0a2486a Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Thu, 21 Dec 2023 00:15:22 -0500 Subject: [PATCH] fix(typescript): allow 'declare global' vars to be shadowed 'declare global' should declare global variables, not module-level variables. This means that module-level variables shadow 'declare global' variables; they do not conflict. For example, the following is valid TypeScript: let x; // Shadows 'const x' below. declare global { const x; } x = 42; // Uses 'let x;', not 'const x;'. export {}; --- docs/CHANGELOG.md | 4 +++ src/quick-lint-js/fe/variable-analyzer.cpp | 31 +++++++++++++++++++--- src/quick-lint-js/fe/variable-analyzer.h | 20 ++++++++++++++ test/test-variable-analyzer-declare.cpp | 21 +++++++++++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f6a91b9a43..51f14c81ab 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -91,6 +91,10 @@ Semantic Versioning. that the inferred variable was redeclared. * `class implements I {}` is now parsed as a class with no name rather than a class with the name `implements`. + * Variables declared inside a `declare global` block are now correctly + declared as global variables instead of module variables. This means that + variables inside a `declare global` block can be shadowed by module + variables without diagnostics. ## 2.18.0 (2023-11-03) diff --git a/src/quick-lint-js/fe/variable-analyzer.cpp b/src/quick-lint-js/fe/variable-analyzer.cpp index c7e06e65c2..cb75690f3f 100644 --- a/src/quick-lint-js/fe/variable-analyzer.cpp +++ b/src/quick-lint-js/fe/variable-analyzer.cpp @@ -139,6 +139,7 @@ void Variable_Analyzer::visit_enter_conditional_type_scope() { void Variable_Analyzer::visit_enter_declare_global_scope() { this->visit_enter_declare_scope(); + this->typescript_declare_global_scope_depth_ += 1; } void Variable_Analyzer::visit_enter_declare_scope() { @@ -221,6 +222,8 @@ void Variable_Analyzer::visit_exit_conditional_type_scope() { } void Variable_Analyzer::visit_exit_declare_global_scope() { + QLJS_ASSERT(this->typescript_declare_global_scope_depth_ > 0); + this->typescript_declare_global_scope_depth_ -= 1; this->visit_exit_declare_scope(); } @@ -296,7 +299,9 @@ void Variable_Analyzer::visit_property_declaration( void Variable_Analyzer::visit_variable_declaration( Identifier name, Variable_Kind kind, Variable_Declaration_Flags flags) { this->declare_variable( - /*scope=*/this->current_scope(), + /*scope=*/this->in_typescript_declare_global_scope() + ? this->scopes_.shadow_global_scope() + : this->current_scope(), /*name=*/name, /*kind=*/kind, /*declared_scope=*/Declared_Variable_Scope::declared_in_current_scope, @@ -511,8 +516,8 @@ void Variable_Analyzer::add_variable_use_to_current_scope(Used_Variable &&var) { } void Variable_Analyzer::visit_end_of_module() { - // We expect only the module scope. - QLJS_ASSERT(this->scopes_.size() == 1); + // We expect only the module scope and the shadow global scope. + QLJS_ASSERT(this->scopes_.size() == 2); Variable_Analyzer::Global_Scope &global_scope = this->global_scope_; @@ -522,6 +527,13 @@ void Variable_Analyzer::visit_end_of_module() { var); } + // Move variables from the module scope to the shadow global scope. + this->propagate_variable_uses_to_parent_scope( + /*allow_variable_use_before_declaration=*/false, + /*consume_arguments=*/false); + this->scopes_.pop(); + + // Move variables from the shadow global scope to the immutable global scope. this->propagate_variable_uses_to_parent_scope( /*parent_scope=*/global_scope, /*allow_variable_use_before_declaration=*/false, @@ -1005,6 +1017,10 @@ void Variable_Analyzer::report_error_if_variable_declaration_conflicts_in_scope( void Variable_Analyzer::report_error_if_variable_declaration_conflicts_in_scope( const Global_Scope &scope, const Declared_Variable &var) const { + // FIXME(#1129): Imagine a global variable is declared as non-shadowable in + // quick-lint-js.config, and is also declared with 'declare global' in the + // source file. Should we treat that variable as shadowable? Or should we + // error on the 'declare global'? std::optional already_declared_variable = scope.declared_variables.find(var.declaration); if (already_declared_variable) { @@ -1193,6 +1209,10 @@ bool Variable_Analyzer::in_typescript_ambient_context() const { return this->typescript_ambient_context_depth_ > 0; } +bool Variable_Analyzer::in_typescript_declare_global_scope() const { + return this->typescript_declare_global_scope_depth_ > 0; +} + bool Variable_Analyzer::Declared_Variable::is_runtime() const { return quick_lint_js::is_runtime(kind); } @@ -1304,10 +1324,15 @@ void Variable_Analyzer::Scope::clear() { } Variable_Analyzer::Scopes::Scopes() { + this->push(); // shadow_global_scope this->push(); // module_scope } Variable_Analyzer::Scope &Variable_Analyzer::Scopes::module_scope() { + return this->scopes_[1]; +} + +Variable_Analyzer::Scope &Variable_Analyzer::Scopes::shadow_global_scope() { return this->scopes_[0]; } diff --git a/src/quick-lint-js/fe/variable-analyzer.h b/src/quick-lint-js/fe/variable-analyzer.h index e692c47105..94e8bde94f 100644 --- a/src/quick-lint-js/fe/variable-analyzer.h +++ b/src/quick-lint-js/fe/variable-analyzer.h @@ -261,6 +261,19 @@ class Variable_Analyzer final : public Parse_Visitor_Base { // The module scope always exists, except possibly at the end of linting. Scope &module_scope(); + // An augmentation of Variable_Analyzer::global_scope_ which is modifiable + // by the user's code. + // + // Variables are declared into the shadow global scope using TypeScript's + // 'declare global' feature. + // + // Variables in the shadow global scope shadow/override global variables, + // hence its name. + // + // The shadow global scope always exists, except possibly at the end of + // linting. + Scope &shadow_global_scope(); + Scope ¤t_scope(); Scope &parent_scope(); @@ -338,6 +351,9 @@ class Variable_Analyzer final : public Parse_Visitor_Base { // 'declare' keyword. bool in_typescript_ambient_context() const; + // Returns true if we are inside a TypeScript 'declare global' scope. + bool in_typescript_declare_global_scope() const; + Scopes scopes_; // The scope which holds properties of the globalThis object. @@ -351,6 +367,10 @@ class Variable_Analyzer final : public Parse_Visitor_Base { // with the 'declare' keyword. unsigned typescript_ambient_context_depth_ = 0; + // If greater than zero, we are inside a TypeScript 'declare global' scope, + // thus declared variables should be hoisted into the global scope. + unsigned typescript_declare_global_scope_depth_ = 0; + Diag_Reporter *diag_reporter_; Variable_Analyzer_Options options_; diff --git a/test/test-variable-analyzer-declare.cpp b/test/test-variable-analyzer-declare.cpp index e207fa7b3e..8f7bb6fd42 100644 --- a/test/test-variable-analyzer-declare.cpp +++ b/test/test-variable-analyzer-declare.cpp @@ -34,6 +34,27 @@ TEST(Test_Variable_Analyzer_Declare, u8"class Base {}"_sv, no_diags, typescript_analyze_options, default_globals); } + +TEST( + Test_Variable_Analyzer_Declare, + typescript_declare_global_variables_are_usable_before_or_after_declaration) { + test_parse_and_analyze(u8"x; declare global { const x; }"_sv, no_diags, + typescript_analyze_options, default_globals); + test_parse_and_analyze(u8"declare global { const x; } x;"_sv, no_diags, + typescript_analyze_options, default_globals); +} + +TEST(Test_Variable_Analyzer_Declare, + typescript_declare_global_variables_are_shadowable) { + // This should not report Diag_Redeclaration_Of_Variable or + // Diag_Assignment_To_Const_Variable. + test_parse_and_analyze(u8"let x; x = 42; declare global { const x; }"_sv, + no_diags, typescript_analyze_options, default_globals); + test_parse_and_analyze(u8"let x; declare global { const x; } x = 42;"_sv, + no_diags, typescript_analyze_options, default_globals); + test_parse_and_analyze(u8"declare global { const x; } let x; x = 42;"_sv, + no_diags, typescript_analyze_options, default_globals); +} } }