Prevent deadlock in getCompletionsAtPosition(..., { includeSymbol: true }) API (#64178)

Author: auvred

tsc/internal/api/session.go

@@ -2285,6 +2285,7 @@ func (s *Session) handleGetImportAdderEdits(ctx context.Context, params *GetImpo
 		sourceFile,
 		projectPath,
 		program,
+		ch,
 		userPreferences.ModuleSpecifierPreferences(),
 	)
 	importAdder := autoimport.NewImportAdder(

tsc/internal/api/session_completion_test.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"testing"
+	"time"
 
 	"github.com/microsoft/TypeScript/tsc/internal/bundled"
 	"github.com/microsoft/TypeScript/tsc/internal/core"
@@ -185,3 +186,71 @@ func TestCompletionRetriesWithAutoImports(t *testing.T) {
 	}
 	t.Fatal("expected auto-import completion for someValue")
 }
+
+func TestCompletionWithSymbolsAndExistingImportDoesNotDeadlock(t *testing.T) {
+	t.Parallel()
+	if !bundled.Embedded {
+		t.Skip("bundled files are not embedded")
+	}
+
+	const fileName = "/home/projects/p/src/index.ts"
+	const content = "import { otherValue } from \"./export\";\nsomeV"
+	projectSession, _ := projecttestutil.Setup(map[string]any{
+		"/home/projects/p/tsconfig.json": `{ "compilerOptions": { "module": "esnext", "target": "esnext" } }`,
+		"/home/projects/p/src/export.ts": "export const otherValue = 0; export const someValue = 1;",
+		fileName:                         content,
+	})
+	defer projectSession.Close()
+	projectSession.Configure(lsutil.UserPreferences{
+		IncludeCompletionsForModuleExports:    core.TSTrue,
+		IncludeCompletionsForImportStatements: core.TSTrue,
+	})
+
+	session := NewLSPSession(projectSession, nil)
+	defer session.Close()
+
+	snapshotResp, err := session.handleUpdateSnapshot(t.Context(), &UpdateSnapshotParams{
+		OpenFiles: []DocumentIdentifier{{FileName: fileName}},
+	})
+	assert.NilError(t, err)
+	proj, err := session.handleGetDefaultProjectForFile(t.Context(), &GetDefaultProjectForFileParams{
+		Snapshot: snapshotResp.Snapshot,
+		File:     DocumentIdentifier{FileName: fileName},
+	})
+	assert.NilError(t, err)
+	assert.Assert(t, proj != nil, "file should resolve to a default project")
+
+	// IncludeSymbol pins completion to the single persistent API checker. When
+	// ranking the auto-import completion, the existing import makes the view
+	// consult that checker. This used to try to acquire the same checker again
+	// and deadlock.
+	type completionResult struct {
+		completions *CompletionInfoResponse
+		err         error
+	}
+	result := make(chan completionResult, 1)
+	go func() {
+		completions, e := session.handleGetCompletionsAtPosition(t.Context(), &GetCompletionsAtPositionParams{
+			Snapshot:      snapshotResp.Snapshot,
+			Project:       proj.Id,
+			File:          DocumentIdentifier{FileName: fileName},
+			Position:      uint32(len(content)),
+			IncludeSymbol: true,
+		})
+		result <- completionResult{completions: completions, err: e}
+	}()
+
+	select {
+	case completion := <-result:
+		assert.NilError(t, completion.err)
+		assert.Assert(t, completion.completions != nil, "expected a completion list")
+		for _, entry := range completion.completions.Entries {
+			if entry.Name == "someValue" {
+				return
+			}
+		}
+		t.Fatal("expected auto-import completion for someValue")
+	case <-time.After(10 * time.Second):
+		t.Fatal("completion request deadlocked while examining an existing import")
+	}
+}

tsc/internal/ls/autoimport/fix.go

@@ -551,13 +551,13 @@ func makeImport(ct *change.Tracker, defaultImport *ast.IdentifierNode, namedImpo
 	return ct.NodeFactory.NewImportDeclaration( /*modifiers*/ nil, importClause, moduleSpecifier, nil /*attributes*/)
 }
 
-func (v *View) GetFixes(ctx context.Context, export *Export, forJSX bool, isValidTypeOnlyUseSite bool, usagePosition *lsproto.Position) []*Fix {
+func (v *View) GetFixes(export *Export, forJSX bool, isValidTypeOnlyUseSite bool, usagePosition *lsproto.Position) []*Fix {
 	var fixes []*Fix
-	if namespaceFix := v.tryUseExistingNamespaceImport(ctx, export, usagePosition); namespaceFix != nil {
+	if namespaceFix := v.tryUseExistingNamespaceImport(export, usagePosition); namespaceFix != nil {
 		fixes = append(fixes, namespaceFix)
 	}
 
-	if fix := v.tryAddToExistingImport(ctx, export, isValidTypeOnlyUseSite); fix != nil {
+	if fix := v.tryAddToExistingImport(export, isValidTypeOnlyUseSite); fix != nil {
 		return append(fixes, fix)
 	}
 
@@ -633,7 +633,7 @@ func getAddAsTypeOnly(isValidTypeOnlyUseSite bool, export *Export, compilerOptio
 	return lsproto.AddAsTypeOnlyAllowed
 }
 
-func (v *View) tryUseExistingNamespaceImport(ctx context.Context, export *Export, usagePosition *lsproto.Position) *Fix {
+func (v *View) tryUseExistingNamespaceImport(export *Export, usagePosition *lsproto.Position) *Fix {
 	if usagePosition == nil {
 		return nil
 	}
@@ -642,7 +642,7 @@ func (v *View) tryUseExistingNamespaceImport(ctx context.Context, export *Export
 		return nil
 	}
 
-	existingImports := v.getExistingImports(ctx)
+	existingImports := v.getExistingImports()
 	matchingDeclarations := existingImports.Get(export.ModuleID)
 	for _, existingImport := range matchingDeclarations {
 		namespacePrefix := getNamespaceLikeImportText(existingImport.node)
@@ -688,11 +688,10 @@ func getNamespaceLikeImportText(declaration *ast.Node) string {
 }
 
 func (v *View) tryAddToExistingImport(
-	ctx context.Context,
 	export *Export,
 	isValidTypeOnlyUseSite bool,
 ) *Fix {
-	existingImports := v.getExistingImports(ctx)
+	existingImports := v.getExistingImports()
 	matchingDeclarations := existingImports.Get(export.ModuleID)
 	if len(matchingDeclarations) == 0 {
 		return nil
@@ -844,27 +843,25 @@ type existingImport struct {
 	index           int
 }
 
-func (v *View) getExistingImports(ctx context.Context) *collections.MultiMap[ModuleID, existingImport] {
+func (v *View) getExistingImports() *collections.MultiMap[ModuleID, existingImport] {
 	if v.existingImports != nil {
 		return v.existingImports
 	}
 
 	result := collections.NewMultiMapWithSizeHint[ModuleID, existingImport](len(v.importingFile.Imports()))
-	ch, done := v.program.GetTypeChecker(ctx)
-	defer done()
 
 	for i, moduleSpecifier := range v.importingFile.Imports() {
 		node := ast.TryGetImportFromModuleSpecifier(moduleSpecifier)
 		if node == nil {
 			panic("error: did not expect node kind " + moduleSpecifier.Kind.String())
 		} else if ast.IsVariableDeclarationInitializedToRequire(node.Parent) {
-			if moduleSymbol := ch.ResolveExternalModuleName(moduleSpecifier, nil /*importAttributesType*/); moduleSymbol != nil {
+			if moduleSymbol := v.checker.ResolveExternalModuleName(moduleSpecifier, nil /*importAttributesType*/); moduleSymbol != nil {
 				if moduleID, _, ok := tryGetModuleIDAndFileNameOfModuleSymbol(moduleSymbol); ok {
 					result.Add(moduleID, existingImport{node: node.Parent, moduleSpecifier: moduleSpecifier.Text(), index: i})
 				}
 			}
 		} else if node.Kind == ast.KindImportDeclaration || node.Kind == ast.KindImportEqualsDeclaration || node.Kind == ast.KindJSDocImportTag {
-			if moduleSymbol := ch.GetSymbolAtLocation(moduleSpecifier); moduleSymbol != nil {
+			if moduleSymbol := v.checker.GetSymbolAtLocation(moduleSpecifier); moduleSymbol != nil {
 				if moduleID, _, ok := tryGetModuleIDAndFileNameOfModuleSymbol(moduleSymbol); ok {
 					result.Add(moduleID, existingImport{node: node, moduleSpecifier: moduleSpecifier.Text(), index: i})
 				}

tsc/internal/ls/autoimport/import_adder.go

@@ -108,7 +108,7 @@ func (adder *importAdder) AddImportFromExportedSymbol(exportedSymbol *ast.Symbol
 		// debug.Assert(len(adder.ls.UserPreferences().AutoImportFileExcludePatterns) > 0)
 		return
 	}
-	fix := adder.getImportFixForSymbol(adder.view, adder.view.importingFile, exportInfos, isValidTypeOnlyUseSite)
+	fix := adder.getImportFixForSymbol(adder.view, exportInfos, isValidTypeOnlyUseSite)
 	if fix != nil {
 		// !!! referenceImport -> propertyName
 		adder.AddImportFix(fix)
@@ -488,9 +488,9 @@ func replaceFirstIdentifierOfEntityName(factory *ast.NodeFactory, name *ast.Enti
 	)
 }
 
-func (adder *importAdder) getImportFixForSymbol(view *View, file *ast.SourceFile, exports []*Export, isValidTypeOnlyUseSite bool) *Fix {
+func (adder *importAdder) getImportFixForSymbol(view *View, exports []*Export, isValidTypeOnlyUseSite bool) *Fix {
 	fixes := core.FlatMap(exports, func(export *Export) []*Fix {
-		return view.GetFixes(adder.ctx, export, false /*forJSX*/, isValidTypeOnlyUseSite, nil /*usagePosition*/)
+		return view.GetFixes(export, false /*forJSX*/, isValidTypeOnlyUseSite, nil /*usagePosition*/)
 	})
 	slices.SortFunc(fixes, func(a, b *Fix) int {
 		return view.CompareFixesForRanking(a, b)

tsc/internal/ls/autoimport/view.go

@@ -1,12 +1,12 @@
 package autoimport
 
 import (
-	"context"
 	"slices"
 	"strings"
 	"unicode"
 
 	"github.com/microsoft/TypeScript/tsc/internal/ast"
+	"github.com/microsoft/TypeScript/tsc/internal/checker"
 	"github.com/microsoft/TypeScript/tsc/internal/collections"
 	"github.com/microsoft/TypeScript/tsc/internal/compiler"
 	"github.com/microsoft/TypeScript/tsc/internal/core"
@@ -23,6 +23,7 @@ type View struct {
 	importingFile     *ast.SourceFile
 	importingFilePath tspath.Path
 	program           *compiler.Program
+	checker           *checker.Checker
 	preferences       modulespecifiers.UserPreferences
 	projectKey        tspath.Path
 
@@ -33,7 +34,7 @@ type View struct {
 	shouldUseRequireForFixes         *bool
 }
 
-func NewView(registry *Registry, importingFile *ast.SourceFile, projectKey tspath.Path, program *compiler.Program, preferences modulespecifiers.UserPreferences) *View {
+func NewView(registry *Registry, importingFile *ast.SourceFile, projectKey tspath.Path, program *compiler.Program, typeChecker *checker.Checker, preferences modulespecifiers.UserPreferences) *View {
 	importingFilePath := importingFile.Path()
 	if canonical := importingFile.CanonicalSourceFile(); canonical != nil {
 		importingFilePath = canonical.Path()
@@ -43,6 +44,7 @@ func NewView(registry *Registry, importingFile *ast.SourceFile, projectKey tspat
 		importingFile:     importingFile,
 		importingFilePath: importingFilePath,
 		program:           program,
+		checker:           typeChecker,
 		projectKey:        projectKey,
 		preferences:       preferences,
 		conditions: collections.NewSetFromItems(
@@ -175,7 +177,7 @@ type FixAndExport struct {
 	Export *Export
 }
 
-func (v *View) GetCompletions(ctx context.Context, prefix string, position lsproto.Position, forJSX bool, isTypeOnlyLocation bool) []*FixAndExport {
+func (v *View) GetCompletions(prefix string, position lsproto.Position, forJSX bool, isTypeOnlyLocation bool) []*FixAndExport {
 	results := v.Search(prefix, QueryKindWordPrefix)
 
 	type exportGroupKey struct {
@@ -240,7 +242,7 @@ outer:
 	for _, exps := range grouped {
 		fixesForGroup := make([]*FixAndExport, 0, len(exps))
 		for _, e := range exps {
-			for _, fix := range v.GetFixes(ctx, e, forJSX, isTypeOnlyLocation, &position) {
+			for _, fix := range v.GetFixes(e, forJSX, isTypeOnlyLocation, &position) {
 				fixesForGroup = append(fixesForGroup, &FixAndExport{
 					Fix:    fix,
 					Export: e,

tsc/internal/ls/codeactions_fixclassincorrectlyimplementsinterface.go

@@ -229,7 +229,7 @@ func getInheritedMembers(typeChecker *checker.Checker, classDeclaration *ast.Nod
 }
 
 func createImportAdder(context context.Context, fixContext *CodeFixContext, typeChecker *checker.Checker) (autoimport.ImportAdder, error) {
-	view, err := fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile)
+	view, err := fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile, typeChecker)
 	if err != nil {
 		return nil, err
 	}

tsc/internal/ls/codeactions_importfixes.go

@@ -8,7 +8,6 @@ import (
 	"github.com/microsoft/TypeScript/tsc/internal/ast"
 	"github.com/microsoft/TypeScript/tsc/internal/astnav"
 	"github.com/microsoft/TypeScript/tsc/internal/checker"
-	"github.com/microsoft/TypeScript/tsc/internal/compiler"
 	"github.com/microsoft/TypeScript/tsc/internal/core"
 	"github.com/microsoft/TypeScript/tsc/internal/diagnostics"
 	"github.com/microsoft/TypeScript/tsc/internal/locale"
@@ -111,17 +110,17 @@ func getAllImportCodeActions(ctx context.Context, fixContext *CodeFixContext) (*
 		return nil, nil
 	}
 
-	view, err := fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile)
+	ch, done := fixContext.Program.GetTypeChecker(ctx)
+	defer done()
+
+	view, err := fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile, ch)
 	if err != nil {
 		return nil, err
 	}
 	if view == nil {
-		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile)
+		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile, ch)
 	}
 
-	ch, done := fixContext.Program.GetTypeChecker(ctx)
-	defer done()
-
 	importAdder := autoimport.NewImportAdder(
 		ctx,
 		fixContext.Program,
@@ -176,18 +175,20 @@ func getFixInfos(ctx context.Context, fixContext *CodeFixContext, errorCode int3
 	}
 
 	symbolToken := astnav.GetTokenAtPosition(fixContext.SourceFile, pos)
+	if errorCode != diagnostics.X_0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.Code() && !ast.IsIdentifier(symbolToken) {
+		return nil, nil
+	}
+
+	ch, done := fixContext.Program.GetTypeChecker(ctx)
+	defer done()
 
 	var view *autoimport.View
 	var info []*fixInfo
 
 	if errorCode == diagnostics.X_0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.Code() {
-		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile)
-		info = getFixesInfoForUMDImport(ctx, fixContext, symbolToken, view)
-	} else if !ast.IsIdentifier(symbolToken) {
-		return nil, nil
+		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile, ch)
+		info = getFixesInfoForUMDImport(symbolToken, view, ch)
 	} else if errorCode == diagnostics.X_0_cannot_be_used_as_a_value_because_it_was_imported_using_import_type.Code() {
-		ch, done := fixContext.Program.GetTypeChecker(ctx)
-		defer done()
 		compilerOptions := fixContext.Program.Options()
 		symbolNames := getSymbolNamesToImport(fixContext.SourceFile, ch, symbolToken, compilerOptions)
 
@@ -196,7 +197,7 @@ func getFixInfos(ctx context.Context, fixContext *CodeFixContext, errorCode int3
 			if !sn.isTypeOnly {
 				continue
 			}
-			fix := getTypeOnlyPromotionFix(ctx, fixContext.SourceFile, symbolToken, sn.name, fixContext.Program)
+			fix := getTypeOnlyPromotionFix(fixContext.SourceFile, symbolToken, sn.name, ch)
 			if fix != nil {
 				allTypeOnlyFixes = append(allTypeOnlyFixes, &fixInfo{fix: fix, symbolName: sn.name, errorIdentifierText: symbolToken.Text()})
 			}
@@ -224,26 +225,23 @@ func getFixInfos(ctx context.Context, fixContext *CodeFixContext, errorCode int3
 		return info, nil
 	} else {
 		var err error
-		view, err = fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile)
+		view, err = fixContext.LS.getPreparedAutoImportView(fixContext.SourceFile, ch)
 		if err != nil {
 			return nil, err
 		}
 		if view != nil {
-			info = getFixesInfoForNonUMDImport(ctx, fixContext, symbolToken, view)
+			info = getFixesInfoForNonUMDImport(fixContext, symbolToken, view, ch)
 		}
 	}
 
 	// Sort fixes by preference
 	if view == nil {
-		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile)
+		view = fixContext.LS.getCurrentAutoImportView(fixContext.SourceFile, ch)
 	}
 	return sortFixInfo(info, fixContext, view), nil
 }
 
-func getFixesInfoForUMDImport(ctx context.Context, fixContext *CodeFixContext, token *ast.Node, view *autoimport.View) []*fixInfo {
-	ch, done := fixContext.Program.GetTypeChecker(ctx)
-	defer done()
-
+func getFixesInfoForUMDImport(token *ast.Node, view *autoimport.View, ch *checker.Checker) []*fixInfo {
 	umdSymbol := getUmdSymbol(token, ch)
 	if umdSymbol == nil {
 		return nil
@@ -253,7 +251,7 @@ func getFixesInfoForUMDImport(ctx context.Context, fixContext *CodeFixContext, t
 	isValidTypeOnlyUseSite := ast.IsValidTypeOnlyAliasUseSite(token)
 
 	var result []*fixInfo
-	for _, fix := range view.GetFixes(ctx, export, false, isValidTypeOnlyUseSite, nil) {
+	for _, fix := range view.GetFixes(export, false, isValidTypeOnlyUseSite, nil) {
 		errorIdentifierText := ""
 		if ast.IsIdentifier(token) {
 			errorIdentifierText = token.Text()
@@ -302,9 +300,7 @@ func isUMDExportSymbol(symbol *ast.Symbol) bool {
 		ast.IsNamespaceExportDeclaration(symbol.Declarations[0])
 }
 
-func getFixesInfoForNonUMDImport(ctx context.Context, fixContext *CodeFixContext, symbolToken *ast.Node, view *autoimport.View) []*fixInfo {
-	ch, done := fixContext.Program.GetTypeChecker(ctx)
-	defer done()
+func getFixesInfoForNonUMDImport(fixContext *CodeFixContext, symbolToken *ast.Node, view *autoimport.View, ch *checker.Checker) []*fixInfo {
 	compilerOptions := fixContext.Program.Options()
 
 	isValidTypeOnlyUseSite := ast.IsValidTypeOnlyAliasUseSite(symbolToken)
@@ -341,7 +337,7 @@ func getFixesInfoForNonUMDImport(ctx context.Context, fixContext *CodeFixContext
 				continue
 			}
 
-			fixes := view.GetFixes(ctx, export, isJSXTagName, isValidTypeOnlyUseSite, &usagePosition)
+			fixes := view.GetFixes(export, isJSXTagName, isValidTypeOnlyUseSite, &usagePosition)
 			for _, fix := range fixes {
 				allInfo = append(allInfo, &fixInfo{
 					fix:               fix,
@@ -355,10 +351,7 @@ func getFixesInfoForNonUMDImport(ctx context.Context, fixContext *CodeFixContext
 	return allInfo
 }
 
-func getTypeOnlyPromotionFix(ctx context.Context, sourceFile *ast.SourceFile, symbolToken *ast.Node, symbolName string, program *compiler.Program) *autoimport.Fix {
-	ch, done := program.GetTypeChecker(ctx)
-	defer done()
-
+func getTypeOnlyPromotionFix(sourceFile *ast.SourceFile, symbolToken *ast.Node, symbolName string, ch *checker.Checker) *autoimport.Fix {
 	// Get the symbol at the token location
 	symbol := ch.ResolveName(symbolName, symbolToken, ast.SymbolFlagsValue, true /* excludeGlobals */)
 	if symbol == nil {