From 5d9e7ea72091f462c490a117a22693a89a31663f Mon Sep 17 00:00:00 2001 From: Victorien Elvinger Date: Fri, 10 May 2024 20:54:43 +0200 Subject: [PATCH] refactor: improve import namespace handling --- CHANGELOG.md | 25 +++ .../invalidNamesapceReference.ts | 5 + .../invalidNamesapceReference.ts.snap | 39 +++++ .../invalid-import-namespace.ts | 5 + .../invalid-import-namespace.ts.snap | 56 +++++++ .../validNamesapceExportType.ts | 2 + .../validNamesapceExportType.ts.snap | 9 ++ .../tests/specs/style/useExportType/valid.ts | 5 +- .../specs/style/useExportType/valid.ts.snap | 20 +++ .../specs/suspicious/noRedeclare/invalid.ts | 8 +- .../suspicious/noRedeclare/invalid.ts.snap | 53 +++++++ crates/biome_js_semantic/src/events.rs | 142 ++++++++++++++---- 12 files changed, 340 insertions(+), 29 deletions(-) create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts.snap create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts.snap create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts create mode 100644 crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts.snap diff --git a/CHANGELOG.md b/CHANGELOG.md index e1fd12e3a20..8c37c4ceb24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,31 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b #### Bug fixes +- [noUndeclaredVariables](https://biomejs.dev/linter/rules/no-undeclared-variables/) and [noUnusedImports](https://biomejs.dev/linter/rules/no-unused-imports) now correctly handle import namespaces ([#2796](https://github.com/biomejs/biome/issues/2796)). + + Previously, Biome bound unqualified type to import namespaces. + Import namespaces can only be used as qualified names in a type (ambient) context. + + ```ts + // Unused import + import * as Ns1 from ""; + // This doesn't reference the import namespace `Ns1` + type T1 = Ns1; // Undeclared variable `Ns1` + + // Unused import + import type * as Ns2 from ""; + // This doesn't reference the import namespace `Ns2` + type T2 = Ns2; // Undeclared variable `Ns2` + + import type * as Ns3 from ""; + // This references the import namespace because it is a qualified name. + type T3 = Ns3.Inner; + // This also references the import namespace. + export type { Ns3 } + ``` + + Contributed by @Conaclos + - `useJsxKeyInIterable` now handles more cases involving fragments. See the snippets below. Contributed by @dyc3 ```jsx // valid diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts b/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts new file mode 100644 index 00000000000..cee3f989f24 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts @@ -0,0 +1,5 @@ +import * as Ns1 from ""; +export type T1 = Ns1; // This doesn't reference the import namespace `Ns1` + +import type * as Ns2 from ""; +export type T2 = Ns2; // This doesn't reference the import namespace `Ns1` \ No newline at end of file diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts.snap b/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts.snap new file mode 100644 index 00000000000..90731907cfd --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidNamesapceReference.ts.snap @@ -0,0 +1,39 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: invalidNamesapceReference.ts +--- +# Input +```ts +import * as Ns1 from ""; +export type T1 = Ns1; // This doesn't reference the import namespace `Ns1` + +import type * as Ns2 from ""; +export type T2 = Ns2; // This doesn't reference the import namespace `Ns1` +``` + +# Diagnostics +``` +invalidNamesapceReference.ts:2:18 lint/correctness/noUndeclaredVariables ━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! The Ns1 variable is undeclared + + 1 │ import * as Ns1 from ""; + > 2 │ export type T1 = Ns1; // This doesn't reference the import namespace `Ns1` + │ ^^^ + 3 │ + 4 │ import type * as Ns2 from ""; + + +``` + +``` +invalidNamesapceReference.ts:5:18 lint/correctness/noUndeclaredVariables ━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! The Ns2 variable is undeclared + + 4 │ import type * as Ns2 from ""; + > 5 │ export type T2 = Ns2; // This doesn't reference the import namespace `Ns1` + │ ^^^ + + +``` diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts b/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts new file mode 100644 index 00000000000..5985b2d0ae2 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts @@ -0,0 +1,5 @@ +import * as Ns1 from "" +export type T1 = Ns1; + +import type * as Ns2 from "" +export type T2 = Ns2; \ No newline at end of file diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts.snap b/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts.snap new file mode 100644 index 00000000000..6ffbfd097b6 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/invalid-import-namespace.ts.snap @@ -0,0 +1,56 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: invalid-import-namespace.ts +--- +# Input +```ts +import * as Ns1 from "" +export type T1 = Ns1; + +import type * as Ns2 from "" +export type T2 = Ns2; +``` + +# Diagnostics +``` +invalid-import-namespace.ts:1:13 lint/correctness/noUnusedImports FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This import is unused. + + > 1 │ import * as Ns1 from "" + │ ^^^ + 2 │ export type T1 = Ns1; + 3 │ + + i Unused imports might be the result of an incomplete refactoring. + + i Safe fix: Remove the unused import. + + 1 │ import·*·as·Ns1·from·"" + │ ----------------------- + +``` + +``` +invalid-import-namespace.ts:4:18 lint/correctness/noUnusedImports FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This import is unused. + + 2 │ export type T1 = Ns1; + 3 │ + > 4 │ import type * as Ns2 from "" + │ ^^^ + 5 │ export type T2 = Ns2; + + i Unused imports might be the result of an incomplete refactoring. + + i Safe fix: Remove the unused import. + + 1 1 │ import * as Ns1 from "" + 2 2 │ export type T1 = Ns1; + 3 │ - + 4 │ - import·type·*·as·Ns2·from·"" + 5 3 │ export type T2 = Ns2; + + +``` diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts b/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts new file mode 100644 index 00000000000..18f82edcbe5 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts @@ -0,0 +1,2 @@ +namespace Ns {} +export type { Ns } \ No newline at end of file diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts.snap b/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts.snap new file mode 100644 index 00000000000..ad0fb724779 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamesapceExportType.ts.snap @@ -0,0 +1,9 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: validNamesapceExportType.ts +--- +# Input +```ts +namespace Ns {} +export type { Ns } +``` diff --git a/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts b/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts index f2163b32bdc..2b87d98e356 100644 --- a/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts +++ b/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts @@ -38,4 +38,7 @@ export type { f2, Class2, typeNs }; declare class AmbientClass {} declare enum AmbientEnum {} declare class AmbientFunction {} -export { AmbientClass, AmbientEnum, AmbientFunction } \ No newline at end of file +export { AmbientClass, AmbientEnum, AmbientFunction } + +import type * as Ns from "" +export { Ns } \ No newline at end of file diff --git a/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts.snap b/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts.snap index 615a20939da..d3c315ea9cd 100644 --- a/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts.snap +++ b/crates/biome_js_analyze/tests/specs/style/useExportType/valid.ts.snap @@ -45,6 +45,26 @@ declare class AmbientClass {} declare enum AmbientEnum {} declare class AmbientFunction {} export { AmbientClass, AmbientEnum, AmbientFunction } + +import type * as Ns from "" +export { Ns } +``` + +# Diagnostics ``` +valid.ts:44:8 lint/style/useExportType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ! All exports are only types and should thus use export type. + + 43 │ import type * as Ns from "" + > 44 │ export { Ns } + │ ^^^^^^ + + i Using export type allows transpilers to safely drop exports of types without looking for their definition. + + i Safe fix: Use a grouped export type. + + 44 │ export·type·{·Ns·} + │ +++++ +``` diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts b/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts index d139a32752e..7cf1e03bca3 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts +++ b/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts @@ -9,4 +9,10 @@ function f() {} function g() { type T = number; -} \ No newline at end of file +} + +import * as Ns1 from "" +namespace Ns1 {} + +import type * as Ns2 from "" +namespace Ns2 {} diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts.snap b/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts.snap index 3d3e2c73931..361496d0727 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts.snap +++ b/crates/biome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.ts.snap @@ -16,6 +16,13 @@ function f() {} function g() { type T = number; } + +import * as Ns1 from "" +namespace Ns1 {} + +import type * as Ns2 from "" +namespace Ns2 {} + ``` # Diagnostics @@ -76,6 +83,7 @@ invalid.ts:11:10 lint/suspicious/noRedeclare ━━━━━━━━━━━ > 11 │ type T = number; │ ^ 12 │ } + 13 │ i 'T' is defined here: @@ -88,3 +96,48 @@ invalid.ts:11:10 lint/suspicious/noRedeclare ━━━━━━━━━━━ ``` + +``` +invalid.ts:15:11 lint/suspicious/noRedeclare ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Shouldn't redeclare 'Ns1'. Consider to delete it or rename it. + + 14 │ import * as Ns1 from "" + > 15 │ namespace Ns1 {} + │ ^^^ + 16 │ + 17 │ import type * as Ns2 from "" + + i 'Ns1' is defined here: + + 12 │ } + 13 │ + > 14 │ import * as Ns1 from "" + │ ^^^ + 15 │ namespace Ns1 {} + 16 │ + + +``` + +``` +invalid.ts:18:11 lint/suspicious/noRedeclare ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Shouldn't redeclare 'Ns2'. Consider to delete it or rename it. + + 17 │ import type * as Ns2 from "" + > 18 │ namespace Ns2 {} + │ ^^^ + 19 │ + + i 'Ns2' is defined here: + + 15 │ namespace Ns1 {} + 16 │ + > 17 │ import type * as Ns2 from "" + │ ^^^ + 18 │ namespace Ns2 {} + 19 │ + + +``` diff --git a/crates/biome_js_semantic/src/events.rs b/crates/biome_js_semantic/src/events.rs index eeb65703908..0068ff24b9e 100644 --- a/crates/biome_js_semantic/src/events.rs +++ b/crates/biome_js_semantic/src/events.rs @@ -183,21 +183,31 @@ impl BindingName { #[derive(Debug, Clone)] struct BindingInfo { + /// range of the name range: TextRange, - is_imported: bool, + /// Kind of the declaration, + /// or in the acse of a bogus declaration, the kind of the name + declaration_kind: JsSyntaxKind, } impl BindingInfo { - fn new(range: TextRange) -> Self { + fn new(range: TextRange, declaration_kind: JsSyntaxKind) -> Self { Self { range, - is_imported: false, + declaration_kind, } } - fn into_imported(mut self) -> Self { - self.is_imported = true; - self + fn is_imported(&self) -> bool { + matches!( + self.declaration_kind, + JsSyntaxKind::TS_IMPORT_EQUALS_DECLARATION + | JsSyntaxKind::JS_DEFAULT_IMPORT_SPECIFIER + | JsSyntaxKind::JS_NAMESPACE_IMPORT_SPECIFIER + | JsSyntaxKind::JS_BOGUS_NAMED_IMPORT_SPECIFIER + | JsSyntaxKind::JS_SHORTHAND_NAMED_IMPORT_SPECIFIER + | JsSyntaxKind::JS_NAMED_IMPORT_SPECIFIER + ) } } @@ -399,9 +409,9 @@ impl SemanticEventExtractor { fn enter_identifier_binding(&mut self, node: &AnyJsIdentifierBinding) { let mut hoisted_scope_id = None; let is_exported = if let Ok(name_token) = node.name_token() { - let info = BindingInfo::new(name_token.text_range()); let name = name_token.token_text_trimmed(); if let Some(declaration) = node.declaration() { + let info = BindingInfo::new(name_token.text_range(), declaration.syntax().kind()); let is_exported = declaration.export().is_some(); match declaration { AnyJsBindingDeclaration::JsArrayBindingPatternElement(_) @@ -476,29 +486,35 @@ impl SemanticEventExtractor { self.push_binding(None, BindingName::Type(name), info); } AnyJsBindingDeclaration::TsImportEqualsDeclaration(declaration) => { - let info = info.into_imported(); if declaration.type_token().is_none() { self.push_binding(None, BindingName::Value(name.clone()), info.clone()); } self.push_binding(None, BindingName::Type(name), info); } - AnyJsBindingDeclaration::JsDefaultImportSpecifier(_) - | AnyJsBindingDeclaration::JsNamespaceImportSpecifier(_) => { + AnyJsBindingDeclaration::JsDefaultImportSpecifier(_) => { let type_token = declaration .parent::() .and_then(|clause| clause.type_token()); - let info = info.into_imported(); if type_token.is_none() { self.push_binding(None, BindingName::Value(name.clone()), info.clone()); } self.push_binding(None, BindingName::Type(name), info); } + AnyJsBindingDeclaration::JsNamespaceImportSpecifier(_) => { + let type_token = declaration + .parent::() + .and_then(|clause| clause.type_token()); + if type_token.is_none() { + self.push_binding(None, BindingName::Value(name.clone()), info.clone()); + } else { + self.push_binding(None, BindingName::Type(name), info); + } + } AnyJsBindingDeclaration::JsBogusNamedImportSpecifier(_) | AnyJsBindingDeclaration::JsShorthandNamedImportSpecifier(_) | AnyJsBindingDeclaration::JsNamedImportSpecifier(_) => { let specifier = AnyJsNamedImportSpecifier::unwrap_cast(declaration.into_syntax()); - let info = info.into_imported(); if !specifier.imports_only_types() { self.push_binding(None, BindingName::Value(name.clone()), info.clone()); } @@ -523,7 +539,8 @@ impl SemanticEventExtractor { } is_exported } else { - // Handle identifiers in bogus nodes, + // Handle identifiers in bogus nodes + let info = BindingInfo::new(name_token.text_range(), node.syntax().kind()); self.push_binding(None, BindingName::Value(name), info); false } @@ -704,7 +721,7 @@ impl SemanticEventExtractor { if let Ok(name_token) = infer.ident_token() { let name = name_token.token_text_trimmed(); let name_range = name_token.text_range(); - let binding_info = BindingInfo::new(name_range); + let binding_info = BindingInfo::new(name_range, JsSyntaxKind::TS_INFER_TYPE); self.push_binding(None, BindingName::Type(name), binding_info); let scope_id = self.current_scope_mut().scope_id; self.stash.push_back(SemanticEvent::DeclarationFound { @@ -739,15 +756,16 @@ impl SemanticEventExtractor { /// 2 - Unmatched references are promoted to its parent scope or become [UnresolvedReference] events; /// 3 - All declarations of this scope are removed; /// 4 - All shadowed declarations are restored. - fn pop_scope(&mut self, range: TextRange) { + fn pop_scope(&mut self, scope_range: TextRange) { debug_assert!(!self.scopes.is_empty()); let scope = self.scopes.pop().unwrap(); let scope_id = scope.scope_id; - // Match references and declarations + // Bind references to declarations for (name, mut references) in scope.references { if let Some(&BindingInfo { - range: declared_at, .. + range: declared_at, + declaration_kind, }) = self.bindings.get(&name) { // If we know the declaration of these reference push the correct events... @@ -772,9 +790,36 @@ impl SemanticEventExtractor { } } } - Reference::Read(range) - | Reference::Typeof(range) - | Reference::Qualified(range) => { + Reference::Read(range) | Reference::Typeof(range) => { + if declaration_kind == JsSyntaxKind::JS_NAMESPACE_IMPORT_SPECIFIER + && matches!(name, BindingName::Type(_)) + { + // An import namespace imported as a type can only be + // used in a qualified name, e.g `Namesapce.Type`. + // Thus, the reference is unresolved. + // Note that we don't need to forward the reference in a parent scope, + // becasue an import namespace is laready in the root scope. + self.stash.push_back(SemanticEvent::UnresolvedReference { + is_read: !reference.is_write(), + range: *reference.range(), + }); + continue; + } + if declaration_before_reference { + SemanticEvent::Read { + range, + declared_at, + scope_id, + } + } else { + SemanticEvent::HoistedRead { + range, + declared_at, + scope_id, + } + } + } + Reference::Qualified(range) => { if declaration_before_reference { SemanticEvent::Read { range, @@ -825,16 +870,59 @@ impl SemanticEventExtractor { // If a dual binding exists, then it exports to the dual binding. continue; } + Reference::ExportType(range) => { + if let Some(info) = &dual_binding { + // TypeScript namespaces and import namespaces can also be exported as a type. + if matches!( + info.declaration_kind, + JsSyntaxKind::JS_NAMESPACE_IMPORT_SPECIFIER + | TS_MODULE_DECLARATION + ) { + let declared_at = info.range; + let declaration_before_reference = + declared_at.start() < reference.range().start(); + self.stash + .push_back(SemanticEvent::Exported { range: declared_at }); + let event = if declaration_before_reference { + SemanticEvent::Read { + range, + declared_at, + scope_id: 0, + } + } else { + SemanticEvent::HoistedRead { + range, + declared_at, + scope_id: 0, + } + }; + self.stash.push_back(event); + continue; + } + } + } Reference::Typeof(range) | Reference::Qualified(range) => { // A typeof can only use a value, // but also an imported type (with `type` modifier) if let Some(info) = &dual_binding { - if info.is_imported { - self.stash.push_back(SemanticEvent::Read { - range, - declared_at: info.range, - scope_id: 0, - }); + if info.is_imported() { + let declared_at = info.range; + let declaration_before_reference = + declared_at.start() < reference.range().start(); + let event = if declaration_before_reference { + SemanticEvent::Read { + range, + declared_at, + scope_id: 0, + } + } else { + SemanticEvent::HoistedRead { + range, + declared_at, + scope_id: 0, + } + }; + self.stash.push_back(event); continue; } } @@ -858,7 +946,7 @@ impl SemanticEventExtractor { self.bindings.extend(scope.shadowed); self.stash.push_back(SemanticEvent::ScopeEnded { - range, + range: scope_range, scope_id: scope.scope_id, }); }