diff --git a/INVESTIGATION.md b/INVESTIGATION.md new file mode 100644 index 00000000000..83932b26d9c --- /dev/null +++ b/INVESTIGATION.md @@ -0,0 +1,107 @@ +# Investigation: TestCodeFixPromoteTypeOnlyOrderingCrash + +## Summary + +Fixed a crash in `TestCodeFixPromoteTypeOnlyOrderingCrash` which occurred when promoting a type-only import to a value import with `verbatimModuleSyntax: true` and import reordering. + +## Root Causes + +The crash was caused by TWO separate bugs: + +### Bug 1: `getVisualListRange` finding wrong token (Critical) + +**Location**: `internal/format/indent.go`, function `getVisualListRange` + +**Problem**: The function was using `astnav.FindNextToken(prior, node, sourceFile)` to find the token after a list. However, this was finding the first child token WITHIN the list instead of the token AFTER the list (e.g., finding "AAA" instead of the closing brace `}`). + +**Impact**: When `GetContainingList` checked if a node was within the visual list range, it returned `nil` because the visual range was too small (e.g., 13-18 instead of 13-32), causing the crash with the assertion "containingList should not be nil". + +**Fix**: Changed the logic to use `astnav.FindPrecedingToken(sourceFile, searchPos)` with incremental searching to find the actual closing token after the list: + +```go +// Find the token that comes after the list ends +searchPos := list.End() + 1 +next := astnav.FindPrecedingToken(sourceFile, searchPos) +// Advance until we find a token that starts after the list end +maxSearchDistance := 10 // Limit for performance +for next != nil && next.End() <= list.End() && searchDistance < maxSearchDistance { + searchPos++ + next = astnav.FindPrecedingToken(sourceFile, searchPos) +} +``` + +### Bug 2: Incorrect sorting assumption during import promotion + +**Location**: `internal/ls/autoimport/fix.go`, function `promoteImportClause` + +**Problem**: When promoting an import specifier from type-only to value (e.g., promoting BBB in `import type { AAA, BBB }`), the code assumed value imports should always go first (index 0). This was incorrect because: +1. It didn't respect the actual sort order preference +2. When using inline type modifiers (required by `verbatimModuleSyntax`), alphabetical sorting is typically expected +3. The comparison was using incompatible sorting preferences (default "Last" vs needed "Inline") + +**Impact**: Specifiers were being moved to the wrong position, causing incorrect output with mangled formatting. + +**Fix**: +1. Changed to use `OrganizeImportsTypeOrderInline` when converting to inline type modifiers +2. Created synthetic specifiers with type modifiers to properly compute the correct insertion index +3. Only reorder if the insertion index actually changes +4. Extracted `createSyntheticImportSpecifier` helper to reduce code duplication + +```go +// Use inline type ordering when converting to inline type modifiers +prefsClone := *prefsForInlineType +prefsClone.OrganizeImportsTypeOrder = lsutil.OrganizeImportsTypeOrderInline + +// Create synthetic specifiers to find correct position +specsWithTypeModifiers := core.Map(namedImportsData.Elements.Nodes, func(e *ast.Node) *ast.Node { + // ... create specifiers with type modifiers ... + return createSyntheticImportSpecifier(changes.NodeFactory, s, true) +}) + +insertionIndex := organizeimports.GetImportSpecifierInsertionIndex( + specsWithTypeModifiers, newSpecifier, specifierComparer) + +// Only reorder if position changes +if insertionIndex != aliasIndex { + changes.Delete(sourceFile, aliasDeclaration) + changes.InsertImportSpecifierAtIndex(sourceFile, newSpecifier, namedImports, insertionIndex) +} +``` + +## Test Case + +The test promotes BBB from `import type { AAA, BBB }` to: +```typescript +import { + type AAA, + BBB, +} from "./bar"; +``` + +With `verbatimModuleSyntax: true`, this requires: +1. Removing the top-level `type` keyword +2. Adding inline `type` modifier to AAA +3. Keeping BBB as a value import +4. Maintaining alphabetical order (AAA before BBB) + +## Files Changed + +1. `internal/format/indent.go` - Fixed `getVisualListRange` to find correct closing token +2. `internal/ls/autoimport/fix.go` - Fixed import reordering logic to use correct sorting preference and added helper function + +## Code Review Feedback Addressed + +1. ✅ Added search distance limit (10 chars) to prevent performance issues in large files +2. ✅ Extracted `createSyntheticImportSpecifier` helper to reduce code duplication +3. ✅ Added clarifying comments + +## Verification + +- ✅ `TestCodeFixPromoteTypeOnlyOrderingCrash` now passes +- ✅ All autoimport tests pass (0.449s) +- ✅ All format tests pass (0.457s) +- ✅ CodeQL security scan: No issues detected + +## Security Summary + +No security vulnerabilities were introduced or discovered during this investigation. The changes are purely correctness fixes for existing functionality. diff --git a/internal/format/indent.go b/internal/format/indent.go index d5ccb949a32..09b62ef479e 100644 --- a/internal/format/indent.go +++ b/internal/format/indent.go @@ -322,7 +322,8 @@ func getList(list *ast.NodeList, r core.TextRange, node *ast.Node, sourceFile *a if list == nil { return nil } - if r.ContainedBy(getVisualListRange(node, list.Loc, sourceFile)) { + visualRange := getVisualListRange(node, list.Loc, sourceFile) + if r.ContainedBy(visualRange) { return list } return nil @@ -341,12 +342,27 @@ func getVisualListRange(node *ast.Node, list core.TextRange, sourceFile *ast.Sou } else { priorEnd = prior.End() } - next := astnav.FindNextToken(prior, node, sourceFile) + // Find the token that comes after the list ends + // We search from list.End() forward to find the first token that starts at or after list.End() + // Most lists have closing tokens immediately after, so this loop typically runs only 1-2 iterations + searchPos := list.End() + 1 + next := astnav.FindPrecedingToken(sourceFile, searchPos) + // Advance until we find a token that starts after the list end (or reach source file end) + // Limit the search to avoid performance issues with very large files + maxSearchDistance := 10 // Reasonable limit since closing tokens are typically very close + searchDistance := 0 + for next != nil && next.End() <= list.End() && searchDistance < maxSearchDistance && searchPos < sourceFile.End() { + searchPos++ + searchDistance++ + next = astnav.FindPrecedingToken(sourceFile, searchPos) + } var nextStart int - if next == nil { + if next == nil || next.Pos() < list.End() { + // If we didn't find a token after the list, or the token we found starts before the list ends, + // just use the list's end position nextStart = list.End() } else { - nextStart = next.Pos() + nextStart = astnav.GetStartOfNode(next, sourceFile, false) } return core.NewTextRange(priorEnd, nextStart) } diff --git a/internal/fourslash/tests/manual/codeFixPromoteTypeOnlyOrderingCrash_test.go b/internal/fourslash/tests/manual/codeFixPromoteTypeOnlyOrderingCrash_test.go new file mode 100644 index 00000000000..098d3e48445 --- /dev/null +++ b/internal/fourslash/tests/manual/codeFixPromoteTypeOnlyOrderingCrash_test.go @@ -0,0 +1,40 @@ +package fourslash_test + +import ( + "testing" + + "github.com/microsoft/typescript-go/internal/fourslash" + "github.com/microsoft/typescript-go/internal/testutil" +) + +// Test case for crash when promoting type-only import to value import +// when existing type imports precede the new value import +// https://github.com/microsoft/typescript-go/issues/XXX +func TestCodeFixPromoteTypeOnlyOrderingCrash(t *testing.T) { + fourslash.SkipIfFailing(t) + t.Parallel() + defer testutil.RecoverAndFail(t, "Panic on fourslash test") + const content = `// @module: node18 +// @verbatimModuleSyntax: true +// @Filename: /bar.ts +export interface AAA {} +export class BBB {} +// @Filename: /foo.ts +import type { + AAA, + BBB, +} from "./bar"; + +let x: AAA = new BBB()` + f, done := fourslash.NewFourslash(t, nil /*capabilities*/, content) + defer done() + f.GoToFile(t, "/foo.ts") + f.VerifyImportFixAtPosition(t, []string{ + `import { + type AAA, + BBB, +} from "./bar"; + +let x: AAA = new BBB()`, + }, nil /*preferences*/) +} diff --git a/internal/ls/autoimport/fix.go b/internal/ls/autoimport/fix.go index 4621112370e..bb72d449172 100644 --- a/internal/ls/autoimport/fix.go +++ b/internal/ls/autoimport/fix.go @@ -245,17 +245,7 @@ func addToExistingImport( specsToCompareAgainst := existingSpecifiers if promoteFromTypeOnly && len(existingSpecifiers) > 0 { specsToCompareAgainst = core.Map(existingSpecifiers, func(e *ast.Node) *ast.Node { - spec := e.AsImportSpecifier() - var propertyName *ast.Node - if spec.PropertyName != nil { - propertyName = spec.PropertyName - } - syntheticSpec := ct.NodeFactory.NewImportSpecifier( - true, // isTypeOnly - propertyName, - spec.Name(), - ) - return syntheticSpec + return createSyntheticImportSpecifier(ct.NodeFactory, e.AsImportSpecifier(), true) }) } @@ -1015,6 +1005,21 @@ func isIndexFileName(fileName string) bool { return false } +// createSyntheticImportSpecifier creates a synthetic import specifier with the given type-only status +// while preserving the property name and identifier from the original specifier. +// This is useful for comparison and sorting operations. +func createSyntheticImportSpecifier(factory *ast.NodeFactory, spec *ast.ImportSpecifier, isTypeOnly bool) *ast.Node { + var propertyName *ast.Node + if spec.PropertyName != nil { + propertyName = spec.PropertyName + } + return factory.NewImportSpecifier( + isTypeOnly, + propertyName, + spec.Name(), + ) +} + func promoteFromTypeOnly( changes *change.Tracker, aliasDeclaration *ast.Declaration, @@ -1116,14 +1121,23 @@ func promoteImportClause( namedImportsData := namedImports.AsNamedImports() if len(namedImportsData.Elements.Nodes) > 1 { // Check if the list is sorted and if we need to reorder - _, isSorted := organizeimports.GetNamedImportSpecifierComparerWithDetection( + // When converting to inline type modifiers, we should use inline type ordering + prefsForInlineType := preferences + if prefsForInlineType == nil { + prefsForInlineType = &lsutil.UserPreferences{} + } + // Clone the preferences and set type order to inline + prefsClone := *prefsForInlineType + prefsClone.OrganizeImportsTypeOrder = lsutil.OrganizeImportsTypeOrderInline + + specifierComparer, isSorted := organizeimports.GetNamedImportSpecifierComparerWithDetection( importClause.Parent, sourceFile, - preferences, + &prefsClone, ) // If the alias declaration is an ImportSpecifier and the list is sorted, - // move it to index 0 (since it will be the only non-type-only import) + // determine the correct position for the promoted (non-type-only) specifier if isSorted.IsFalse() == false && // isSorted !== false aliasDeclaration != nil && aliasDeclaration.Kind == ast.KindImportSpecifier { @@ -1135,12 +1149,45 @@ func promoteImportClause( break } } - // If not already at index 0, move it there - if aliasIndex > 0 { + // Create a new specifier node with the same properties + spec := aliasDeclaration.AsImportSpecifier() + var propertyName *ast.Node + if spec.PropertyName != nil { + propertyName = spec.PropertyName + } + newSpecifier := changes.NodeFactory.NewImportSpecifier( + false, // isTypeOnly - this specifier is being promoted to non-type-only + propertyName, + spec.Name(), + ) + + // Determine the correct insertion index for the promoted specifier + // We need to compute what the list will look like after adding type modifiers to existing elements + specsWithTypeModifiers := core.Map(namedImportsData.Elements.Nodes, func(e *ast.Node) *ast.Node { + if e == aliasDeclaration { + // This is the element being promoted, skip it for now + return nil + } + s := e.AsImportSpecifier() + if s.IsTypeOnly { + // Already type-only + return e + } + // Will have type modifier added + return createSyntheticImportSpecifier(changes.NodeFactory, s, true) + }) + // Filter out nils (the promoted element) + specsWithTypeModifiers = core.Filter(specsWithTypeModifiers, func(e *ast.Node) bool { return e != nil }) + + // Find the correct insertion index using the comparer + insertionIndex := organizeimports.GetImportSpecifierInsertionIndex(specsWithTypeModifiers, newSpecifier, specifierComparer) + + // Only delete and re-insert if the position changes + if insertionIndex != aliasIndex { // Delete the specifier from its current position changes.Delete(sourceFile, aliasDeclaration) - // Insert it at index 0 - changes.InsertImportSpecifierAtIndex(sourceFile, aliasDeclaration, namedImports, 0) + // Insert the new specifier at the correct index + changes.InsertImportSpecifierAtIndex(sourceFile, newSpecifier, namedImports, insertionIndex) } }