Reparse `@import` as synthetic type-only import declaration #831

sandersn · 2025-04-28T15:21:19Z

It is slightly stricter than Strada's treatment because Corsa now treats the synthetic node exactly as a type-only declaration.

It is slightly stricter than Strada's treatment because Corsa now treats the synthetic node *exactly* as a type-only declaration.

Copilot

Pull Request Overview

This PR introduces a new syntax node (KindJSImportDeclaration) to support the re-parsing of @import annotations as synthetic type‑only import declarations. The updates ensure that all relevant components—including transformers, printers, checkers, AST utilities, and unit tests—properly recognize and handle the new Kind alongside the traditional import declaration.

Extended switch-case statements and error handling in multiple modules to include KindJSImportDeclaration.
Updated AST creation methods and reparse logic to correctly mark synthetic jsimport declarations as type‑only.

Reviewed Changes

Copilot reviewed 52 out of 53 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
testdata/baselines/reference/submodule/conformance/importTag15(module=esnext).errors.txt	Adjusted error output to account for the additional type‑only import errors.
testdata/baselines/reference/submodule/conformance/importTag15(module=es2015).errors.txt	Updated error messages and counts for invalid use of import attributes.
internal/transformers/typeeraser.go	Added KindJSImportDeclaration in the type eraser transformer to ensure proper traversal.
internal/transformers/importelision.go	Extended import elision logic to handle synthetic jsimport declarations.
internal/transformers/externalmoduleinfo.go	Modified external module info collection to include KindJSImportDeclaration.
internal/transformers/esnext.go	Updated transformer switch to recognize KindJSImportDeclaration.
internal/transformers/esmodule.go	Added a case to handle KindJSImportDeclaration during ES module transformation.
internal/transformers/commonjsmodule.go	Added handling for KindJSImportDeclaration as a no‑op, aligning with CommonJS semantics.
internal/printer/printer.go	Extended the printer to generate nodes for KindJSImportDeclaration.
internal/printer/namegenerator.go	Modified name generation logic to include KindJSImportDeclaration along with existing types.
internal/parser/reparser.go	Implemented reparse logic for @import JSDoc tags to create synthetic jsimport declarations marked as type‑only.
internal/ls/utilities.go	Updated language service utilities to recognize KindJSImportDeclaration in import specifier resolution.
internal/checker/grammarchecks.go	Adjusted grammar checks to include KindJSImportDeclaration when validating ambient import modifiers.
internal/checker/checker.go	Integrated KindJSImportDeclaration in multiple checking routines for import declarations.
internal/ast/utilities.go	Updated various AST utility functions to support KindJSImportDeclaration in their logic.
internal/ast/kind_stringer_generated.go	Revised generated string mappings to include KindJSImportDeclaration.
internal/ast/kind.go	Declared the new constant KindJSImportDeclaration.
internal/ast/ast.go	Updated factory methods for creating import declarations to allow creation of KindJSImportDeclaration.

Files not reviewed (1)

testdata/baselines/reference/submodule/conformance/importTag1.errors.txt: Language not supported

sandersn · 2025-04-28T15:39:20Z

testdata/baselines/reference/submoduleAccepted/conformance/importTag16.types.diff

+ export function foo(a, b) {}
+->foo : (a: Foo, b: I) => void
+->a : Foo
+>foo : (a: default, b: I) => void


I don't know how typescript behaves, but I've changed so little code that I guess that a: default is closer to TS than a: Foo was.

Isn’t this saying that TS has a: Foo where tsgo has a: default? We definitely shouldn’t be calling that type default.

testdata/baselines/reference/submoduleAccepted/conformance/importTag17.errors.txt.diff

No longer used; everything now uses JSImportDeclaration.

sandersn · 2025-04-28T17:59:35Z

@a-tarasyuk you wrote the original implementation of @import, so you might be interested to see how it looks in Corsa.

rbuckton · 2025-04-28T18:02:22Z

internal/ast/ast.go

+}
+
+func (f *NodeFactory) NewJSImportDeclaration(modifiers *ModifierList, importClause *ImportClauseNode, moduleSpecifier *Expression, attributes *ImportAttributesNode) *Node {
+	return f.newImportOrJSImportDeclaration(KindJSImportDeclaration, modifiers, importClause, moduleSpecifier, attributes)
 }

 func (f *NodeFactory) UpdateImportDeclaration(node *ImportDeclaration, modifiers *ModifierList, importClause *ImportClauseNode, moduleSpecifier *Expression, attributes *ImportAttributesNode) *Node {


VisitEachChild is going to have issues if you don't differentiate by Kind in UpdateImportDeclaration, since it would turn a JSImportDeclaration into a normal ImportDeclaration.

rbuckton · 2025-04-28T18:04:29Z

internal/printer/printer.go

@@ -3953,7 +3953,7 @@ func (p *Printer) emitStatement(node *ast.Statement) {
 		p.emitNamespaceExportDeclaration(node.AsNamespaceExportDeclaration())
 	case ast.KindImportEqualsDeclaration:
 		p.emitImportEqualsDeclaration(node.AsImportEqualsDeclaration())
-	case ast.KindImportDeclaration:


Printing KindJSImportDeclaration should either panic (if you expect it to always be transformed), or have its own distinct emit.

rbuckton · 2025-04-28T18:06:56Z

internal/transformers/commonjsmodule.go

@@ -63,6 +63,8 @@ func (tx *CommonJSModuleTransformer) visitTopLevel(node *ast.Node) *ast.Node {
 	switch node.Kind {
 	case ast.KindImportDeclaration:
 		node = tx.visitTopLevelImportDeclaration(node.AsImportDeclaration())
+	case ast.KindJSImportDeclaration:


IMO, these should be handled by something like typeeraser.go or importellision.go so that other transformers shouldn't have to deal with special cases like JSImportDeclaration. Handling them in the module transforms means they have to exist as part of all other transforms first, and its better if we limit syntax in those transforms to ones expected by that edition of JS.

rbuckton · 2025-04-28T18:07:26Z

internal/transformers/esmodule.go

@@ -39,6 +39,8 @@ func (tx *ESModuleTransformer) visit(node *ast.Node) *ast.Node {
 		node = tx.visitSourceFile(node.AsSourceFile())
 	case ast.KindImportDeclaration:
 		node = tx.visitImportDeclaration(node.AsImportDeclaration())
+	case ast.KindJSImportDeclaration:
+		node = nil


This should be handled in an earlier transform.

rbuckton · 2025-04-28T18:07:32Z

internal/transformers/esnext.go

@@ -368,6 +368,7 @@ func (tx *ESNextTransformer) transformUsingDeclarations(statementsIn []*ast.Stat

 		switch node.Kind {
 		case ast.KindImportDeclaration,
+			ast.KindJSImportDeclaration,


This should be handled in an earlier transform.

rbuckton · 2025-04-28T18:08:00Z

internal/transformers/externalmoduleinfo.go

@@ -47,11 +47,12 @@ func (c *externalModuleInfoCollector) collect() *externalModuleInfo {
 	hasImportDefault := false
 	for _, node := range c.sourceFile.Statements.Nodes {
 		switch node.Kind {
-		case ast.KindImportDeclaration:
+		case ast.KindImportDeclaration, ast.KindJSImportDeclaration:


Is this necessary? Aren't these imports type-only and just elided?

rbuckton · 2025-04-28T18:09:06Z

internal/transformers/importelision.go

@@ -28,7 +28,7 @@ func (tx *ImportElisionTransformer) visit(node *ast.Node) *ast.Node {
 			return nil
 		}
 		return tx.visitor.VisitEachChild(node)
-	case ast.KindImportDeclaration:
+	case ast.KindImportDeclaration, ast.KindJSImportDeclaration:


This should be handled by typeeraser.go and should never reach this point

rbuckton · 2025-04-28T18:09:22Z

internal/transformers/importelision.go

@@ -166,7 +166,7 @@ func (tx *ImportElisionTransformer) isElisionBlocked(node *ast.Node /*ImportDecl
 	}

 	switch node.Kind {
-	case ast.KindImportDeclaration:
+	case ast.KindImportDeclaration, ast.KindJSImportDeclaration:


This shouldn't be necessary as it should be handled by typeeraser.go

rbuckton · 2025-04-28T18:09:34Z

internal/transformers/typeeraser.go

@@ -255,7 +255,7 @@ func (tx *TypeEraserTransformer) visit(node *ast.Node) *ast.Node {
 		}
 		return tx.visitor.VisitEachChild(node)

-	case ast.KindImportDeclaration:
+	case ast.KindImportDeclaration, ast.KindJSImportDeclaration:


This should have its own branch and should just be elided.

Reparse @import as synthetic type-only import declaration

8ea1898

It is slightly stricter than Strada's treatment because Corsa now treats the synthetic node *exactly* as a type-only declaration.

Copilot AI review requested due to automatic review settings April 28, 2025 15:21

Copilot AI reviewed Apr 28, 2025

View reviewed changes

revert diff that should go in microsoft#826

1de7efe

sandersn commented Apr 28, 2025

View reviewed changes

testdata/baselines/reference/submoduleAccepted/conformance/importTag17.errors.txt.diff Show resolved Hide resolved

Remove JSDocImportTag code

097fa73

No longer used; everything now uses JSImportDeclaration.

sandersn requested review from andrewbranch and rbuckton April 28, 2025 17:54

Merge branch 'main' into add-jsdoc-import

b970689

rbuckton requested changes Apr 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reparse `@import` as synthetic type-only import declaration #831

Reparse `@import` as synthetic type-only import declaration #831

sandersn commented Apr 28, 2025

Copilot AI left a comment

sandersn Apr 28, 2025

andrewbranch Apr 28, 2025

sandersn commented Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

rbuckton Apr 28, 2025

Reparse @import as synthetic type-only import declaration #831

Are you sure you want to change the base?

Reparse @import as synthetic type-only import declaration #831

Conversation

sandersn commented Apr 28, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reparse `@import` as synthetic type-only import declaration #831

Reparse `@import` as synthetic type-only import declaration #831