Skip to content

Commit

Permalink
fix(typescript): don't report redeclaration of imported variable
Browse files Browse the repository at this point in the history
If an import imports a type, then re-declaring that imported type as a
function is valid in TypeScript. Similarly, if an import imports a
variable, then re-declaring that imported variable as an interface is
valid in TypeScript. Ensure quick-lint-js doesn't report false
diagnostics in these cases.
  • Loading branch information
strager committed Dec 28, 2023
1 parent adbaed8 commit 7122d67
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 23 deletions.
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ Semantic Versioning.
directly in its own definition") when using `extends ? :`.
* Making a type alias and a function with the same name no longer falsely
reports [E0034][] ("redeclaration of variable").
* Importing a type then declaring a function or variable with the same name no
longer falsely reports [E0034][] ("redeclaration of variable").
* Importing a function or variable then declaring a type with the same name no
longer falsely reports [E0034][] ("redeclaration of variable").
* `T extends () => RT ? A : B` no longer falsely reports [E0348][]
("unexpected '?' in type; use '| void' to make an optional type").
* `<T extends />` is now correctly parsed as a JSX element.
Expand Down
1 change: 1 addition & 0 deletions src/quick-lint-js/fe/linter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ void parse_and_lint(Padded_String_View code, Diag_Reporter& reporter,
.eval_can_declare_variables = !options.typescript,
// TODO(strager): Deduplicate with typescript_var_options in tests.
.can_assign_to_class = !options.typescript,
.import_variable_can_be_runtime_or_type = options.typescript,
});

#if defined(__EMSCRIPTEN__)
Expand Down
11 changes: 10 additions & 1 deletion src/quick-lint-js/fe/variable-analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ namespace quick_lint_js {
namespace {
bool is_runtime(Variable_Kind);
bool is_type(Variable_Kind);
bool is_runtime_and_type(Variable_Kind);
}

Variable_Analyzer::Variable_Analyzer(
Expand Down Expand Up @@ -1122,7 +1123,6 @@ bool Variable_Analyzer::report_error_if_variable_declaration_conflicts(
(kind == VK::_import_type && other_kind == VK::_var) ||
(kind == VK::_infer_type && other_kind == VK::_infer_type) ||
(kind == VK::_interface && other_kind == VK::_class) ||
(kind == VK::_interface && other_kind == VK::_import) ||
(kind == VK::_interface && other_kind == VK::_import_alias) ||
(kind == VK::_interface && other_kind == VK::_interface) ||
(kind == VK::_interface && other_kind == VK::_namespace) ||
Expand Down Expand Up @@ -1160,6 +1160,11 @@ bool Variable_Analyzer::report_error_if_variable_declaration_conflicts(
(kind_is_parameter && other_kind_is_parameter) ||
(!quick_lint_js::is_type(kind) && other_kind == VK::_interface) ||
// clang-format on
(this->options_.import_variable_can_be_runtime_or_type &&
((kind == VK::_import &&
!quick_lint_js::is_runtime_and_type(other_kind)) ||
(other_kind == VK::_import &&
!quick_lint_js::is_runtime_and_type(kind)))) ||
(other_kind == VK::_namespace &&
(kind == VK::_class || kind == VK::_function) &&
!(already_declared_var.flags &
Expand Down Expand Up @@ -1404,6 +1409,10 @@ bool is_type(Variable_Kind kind) {
}
QLJS_UNREACHABLE();
}

bool is_runtime_and_type(Variable_Kind kind) {
return is_type(kind) && is_runtime(kind);
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/quick-lint-js/fe/variable-analyzer.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ struct Variable_Analyzer_Options {
// If false, assigning to a 'class'-declared variable is invalid, like in
// TypeScript.
bool can_assign_to_class = true;

// If true, imported variables can be run-time variables (e.g. function, let)
// or type-only variables (e.g. TypeScript type alias or interface) or both
// (e.g. class).
//
// If false, imported variables can only be run-time variables, like in
// vanilla JavaScript.
bool import_variable_can_be_runtime_or_type = false;
};

// A variable_analyzer is a parse_visitor which implements variable lookup
Expand Down
1 change: 1 addition & 0 deletions test/quick-lint-js/variable-analyzer-support.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ constexpr Variable_Analyzer_Options typescript_var_options =
.allow_deleting_typescript_variable = false,
.eval_can_declare_variables = false,
.can_assign_to_class = false,
.import_variable_can_be_runtime_or_type = true,
};

struct Test_Parse_And_Analyze_Options {
Expand Down
87 changes: 65 additions & 22 deletions test/test-variable-analyzer-multiple-declarations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,71 @@ TEST(Test_Variable_Analyzer_Multiple_Declarations,
javascript_analyze_options, default_globals);
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
typescript_import_does_not_conflict_with_runtime_only_variables) {
for (String8_View other_thing : {
u8"const x = null;"_sv,
u8"function x() {}"_sv,
u8"let x;"_sv,
u8"var x;"_sv,
}) {
test_parse_and_analyze(concat(u8"import x from ''; "_sv, other_thing),
no_diags, typescript_analyze_options,
default_globals);
test_parse_and_analyze(concat(other_thing, u8" import x from '';"_sv),
no_diags, typescript_analyze_options,
default_globals);
}
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
typescript_import_does_not_conflict_with_type_only_variables) {
for (String8_View other_thing : {
u8"interface x {}"_sv,
u8"type x = null;"_sv,
u8"import {type x} from 'othermod';"_sv,
}) {
test_parse_and_analyze(concat(u8"import x from ''; "_sv, other_thing),
no_diags, typescript_analyze_options,
default_globals);
test_parse_and_analyze(concat(other_thing, u8" import x from '';"_sv),
no_diags, typescript_analyze_options,
default_globals);
}
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
typescript_import_conflicts_with_mixed_runtime_and_type_variables) {
for (String8_View other_thing : {
u8"class x {}"_sv,
u8"enum x {}"_sv,
}) {
test_parse_and_analyze(concat(u8"import x from ''; "_sv, other_thing),
u8"Diag_Redeclaration_Of_Variable"_diag,
typescript_analyze_options, default_globals);
test_parse_and_analyze(concat(other_thing, u8" import x from '';"_sv),
u8"Diag_Redeclaration_Of_Variable"_diag,
typescript_analyze_options, default_globals);
}
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
typescript_import_always_conflicts_with_another_import) {
for (String8_View other_thing : {
u8"import {x} from 'othermod';"_sv,
u8"import x from 'othermod';"_sv,
u8"import * as x from 'othermod';"_sv,
u8"import x = require('othermod');"_sv,
}) {
test_parse_and_analyze(concat(u8"import x from ''; "_sv, other_thing),
u8"Diag_Redeclaration_Of_Variable"_diag,
typescript_analyze_options, default_globals);
test_parse_and_analyze(concat(other_thing, u8" import x from '';"_sv),
u8"Diag_Redeclaration_Of_Variable"_diag,
typescript_analyze_options, default_globals);
}
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
import_alias_does_not_conflict_with_most_other_things) {
for (String8_View other_thing : {
Expand Down Expand Up @@ -355,28 +420,6 @@ TEST(Test_Variable_Analyzer_Multiple_Declarations,
typescript_analyze_options, default_globals);
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
imported_type_always_conflicts_with_things_declaring_types) {
for (String8_View other_thing : {
u8"class T {}"_sv,
u8"enum T {}"_sv,
u8"import T from 'othermod';"_sv,
u8"import T = require('othermod');"_sv,
u8"import {type T} from 'othermod';"_sv,
u8"interface T {}"_sv,
u8"type T = null;"_sv,
}) {
test_parse_and_analyze(
concat(u8"import {type T} from 'mod'; "_sv, other_thing),
u8"Diag_Redeclaration_Of_Variable"_diag, typescript_analyze_options,
default_globals);
test_parse_and_analyze(
concat(other_thing, u8" import {type T} from 'mod';"_sv),
u8"Diag_Redeclaration_Of_Variable"_diag, typescript_analyze_options,
default_globals);
}
}

TEST(Test_Variable_Analyzer_Multiple_Declarations,
imported_type_might_not_conflict_with_runtime_only_declarations) {
// These things conflict when importing a class, but do not conflict when
Expand Down

0 comments on commit 7122d67

Please sign in to comment.