diff --git a/tsc/internal/compiler/checkerpool.go b/tsc/internal/compiler/checkerpool.go index a7068a296d183..01db5aa06318c 100644 --- a/tsc/internal/compiler/checkerpool.go +++ b/tsc/internal/compiler/checkerpool.go @@ -17,6 +17,8 @@ import ( // request-scoped lifetime and reclamation. It returns a checker and a release // function that must be called when the caller is done with the checker. // The returned checker must not be accessed concurrently; each acquisition is exclusive. +// Acquisitions are not reentrant, even when they share a request ID. Callers must +// pass an already acquired checker to nested operations instead of acquiring again. // If file is non-nil, the pool may use it as an affinity hint to return the same // checker for the same file across calls. type CheckerPool interface { diff --git a/tsc/internal/fourslash/tests/contentMapperInlayHints_test.go b/tsc/internal/fourslash/tests/contentMapperInlayHints_test.go new file mode 100644 index 0000000000000..7d8f6cd03b739 --- /dev/null +++ b/tsc/internal/fourslash/tests/contentMapperInlayHints_test.go @@ -0,0 +1,42 @@ +package fourslash_test + +import ( + "testing" + + "github.com/microsoft/TypeScript/tsc/internal/core" + "github.com/microsoft/TypeScript/tsc/internal/ls/lsutil" + "github.com/microsoft/TypeScript/tsc/internal/testutil" + "github.com/microsoft/TypeScript/tsc/internal/testutil/contentmappertest" +) + +func TestContentMapperInlayHintsReleaseEachRange(t *testing.T) { + t.Parallel() + defer testutil.RecoverAndFail(t, "Panic on fourslash test") + // The mapper emits the script verbatim followed by: + // + // function __render() { + // void (value); + // void (value); + // void (value); + // void (value); + // } + // export default {}; + // + // The script and four template identifiers map to five disjoint virtual ranges. + // Each range must release its checker before processing the next range. + f, done := newContentMapperFourslash(t, `// @Filename: /app.vue + +{{value}} +{{value}} +{{value}} +{{value}} +`, contentmappertest.ComponentMapper, ".vue") + defer done() + + f.GoToFile(t, "/app.vue") + f.VerifyBaselineInlayHints(t, nil, &lsutil.UserPreferences{ + InlayHints: lsutil.InlayHintsPreferences{IncludeInlayVariableTypeHints: core.TSTrue}, + }) +} diff --git a/tsc/internal/ls/codeactions_fixclassincorrectlyimplementsinterface.go b/tsc/internal/ls/codeactions_fixclassincorrectlyimplementsinterface.go index 67a56d7e0a3d8..beb69a6675d09 100644 --- a/tsc/internal/ls/codeactions_fixclassincorrectlyimplementsinterface.go +++ b/tsc/internal/ls/codeactions_fixclassincorrectlyimplementsinterface.go @@ -67,6 +67,8 @@ func getCodeActionsToFixClassIncorrectlyImplementsInterface(context context.Cont } func getAllCodeActionsToFixClassIncorrectlyImplementsInterface(context context.Context, fixContext *CodeFixContext) (*CombinedCodeActions, error) { + allDiags := getAllDiagnostics(context, fixContext.Program, fixContext.SourceFile) + typeChecker, done := fixContext.Program.GetTypeCheckerForFile(context, fixContext.SourceFile) defer done() @@ -78,7 +80,7 @@ func getAllCodeActionsToFixClassIncorrectlyImplementsInterface(context context.C seenClassDeclarations := collections.Set[*ast.Node]{} - for _, diag := range getAllDiagnostics(context, fixContext.Program, fixContext.SourceFile) { + for _, diag := range allDiags { if isFixableDiagnostic(diag, fixClassIncorrectlyImplementsInterfaceErrorCodes) { classDeclaration := getClass(fixContext.SourceFile, core.NewTextRange(diag.Pos(), diag.End())) if classDeclaration == nil { diff --git a/tsc/internal/ls/codeactions_fixmissingtypeannotation.go b/tsc/internal/ls/codeactions_fixmissingtypeannotation.go index 50bea6bd45d9a..7f25f281c5609 100644 --- a/tsc/internal/ls/codeactions_fixmissingtypeannotation.go +++ b/tsc/internal/ls/codeactions_fixmissingtypeannotation.go @@ -126,6 +126,8 @@ func getIsolatedDeclarationsCodeActions(ctx context.Context, fixContext *CodeFix } func getAllIsolatedDeclarationsCodeActions(ctx context.Context, fixContext *CodeFixContext) (*CombinedCodeActions, error) { + allDiags := getAllDiagnostics(ctx, fixContext.Program, fixContext.SourceFile) + ch, done := fixContext.Program.GetTypeCheckerForFile(ctx, fixContext.SourceFile) defer done() @@ -141,7 +143,6 @@ func getAllIsolatedDeclarationsCodeActions(ctx context.Context, fixContext *Code typePrintMode: typePrintModeFull, } - allDiags := getAllDiagnostics(ctx, fixContext.Program, fixContext.SourceFile) for _, diag := range allDiags { if isFixableDiagnostic(diag, isolatedDeclarationsFixErrorCodes) { span := core.NewTextRange(diag.Loc().Pos(), diag.Loc().End()) diff --git a/tsc/internal/ls/codeactions_importfixes.go b/tsc/internal/ls/codeactions_importfixes.go index 5f186f90dda23..51890a608c452 100644 --- a/tsc/internal/ls/codeactions_importfixes.go +++ b/tsc/internal/ls/codeactions_importfixes.go @@ -61,7 +61,10 @@ type fixInfo struct { } func getImportCodeActions(ctx context.Context, fixContext *CodeFixContext) ([]*CodeAction, error) { - info, err := getFixInfos(ctx, fixContext, fixContext.ErrorCode, fixContext.Span.Pos()) + ch, done := fixContext.Program.GetTypeChecker(ctx) + defer done() + + info, err := getFixInfos(ch, fixContext, fixContext.ErrorCode, fixContext.Span.Pos()) if err != nil { return nil, err } @@ -133,7 +136,7 @@ func getAllImportCodeActions(ctx context.Context, fixContext *CodeFixContext) (* ) for _, diag := range importDiags { - if err := addImportFromDiagnostic(ctx, importAdder, diag, fixContext); err != nil { + if err := addImportFromDiagnostic(ch, importAdder, diag, fixContext); err != nil { return nil, err } } @@ -149,7 +152,7 @@ func getAllImportCodeActions(ctx context.Context, fixContext *CodeFixContext) (* } // addImportFromDiagnostic finds the best import fix for a diagnostic and adds it to the adder. -func addImportFromDiagnostic(ctx context.Context, importAdder autoimport.ImportAdder, diag *ast.Diagnostic, fixContext *CodeFixContext) error { +func addImportFromDiagnostic(ch *checker.Checker, importAdder autoimport.ImportAdder, diag *ast.Diagnostic, fixContext *CodeFixContext) error { diagFixContext := &CodeFixContext{ SourceFile: fixContext.SourceFile, Span: core.NewTextRange(diag.Pos(), diag.End()), @@ -158,7 +161,7 @@ func addImportFromDiagnostic(ctx context.Context, importAdder autoimport.ImportA LS: fixContext.LS, } - infos, err := getFixInfos(ctx, diagFixContext, diag.Code(), diag.Pos()) + infos, err := getFixInfos(ch, diagFixContext, diag.Code(), diag.Pos()) if err != nil { return err } @@ -168,7 +171,7 @@ func addImportFromDiagnostic(ctx context.Context, importAdder autoimport.ImportA return nil } -func getFixInfos(ctx context.Context, fixContext *CodeFixContext, errorCode int32, pos int) ([]*fixInfo, error) { +func getFixInfos(ch *checker.Checker, fixContext *CodeFixContext, errorCode int32, pos int) ([]*fixInfo, error) { // Can't compute import fixes for dynamic/untitled files since they don't have real file paths if tspath.IsDynamicFileName(fixContext.SourceFile.FileName()) { return nil, nil @@ -179,9 +182,6 @@ func getFixInfos(ctx context.Context, fixContext *CodeFixContext, errorCode int3 return nil, nil } - ch, done := fixContext.Program.GetTypeChecker(ctx) - defer done() - var view *autoimport.View var info []*fixInfo diff --git a/tsc/internal/ls/findallreferences.go b/tsc/internal/ls/findallreferences.go index a9ccd11270b81..d7faff3b7512d 100644 --- a/tsc/internal/ls/findallreferences.go +++ b/tsc/internal/ls/findallreferences.go @@ -1287,7 +1287,7 @@ func (l *LanguageService) getReferencedSymbolsForNode(ctx context.Context, posit } if moduleSymbol := checker.GetMergedSymbol(resolvedRef.file.Symbol); moduleSymbol != nil { - return l.getReferencedSymbolsForModule(ctx, program, moduleSymbol /*excludeImportTypeOfExportEquals*/, false, sourceFiles, sourceFilesSet) + return l.getReferencedSymbolsForModule(checker, program, moduleSymbol /*excludeImportTypeOfExportEquals*/, false, sourceFiles, sourceFilesSet) } // !!! not implemented @@ -1336,7 +1336,7 @@ func (l *LanguageService) getReferencedSymbolsForNode(ctx context.Context, posit if symbol.Parent == nil { return nil } - return l.getReferencedSymbolsForModule(ctx, program, symbol.Parent, false /*excludeImportTypeOfExportEquals*/, sourceFiles, sourceFilesSet) + return l.getReferencedSymbolsForModule(checker, program, symbol.Parent, false /*excludeImportTypeOfExportEquals*/, sourceFiles, sourceFilesSet) } moduleReferences := l.getReferencedSymbolsForModuleIfDeclaredBySourceFile(ctx, symbol, program, sourceFiles, checker, options, sourceFilesSet) @@ -1410,7 +1410,7 @@ func (l *LanguageService) getReferencedSymbolsForModuleIfDeclaredBySourceFile(ct } exportEquals := symbol.Exports[ast.InternalSymbolNameExportEquals] // If exportEquals != nil, we're about to add references to `import("mod")` anyway, so don't double-count them. - moduleReferences := l.getReferencedSymbolsForModule(ctx, program, symbol, exportEquals != nil, sourceFiles, sourceFilesSet) + moduleReferences := l.getReferencedSymbolsForModule(checker, program, symbol, exportEquals != nil, sourceFiles, sourceFilesSet) if exportEquals == nil || exportEquals.Flags&ast.SymbolFlagsAlias == 0 || !sourceFilesSet.Has(moduleSourceFileName) { return moduleReferences } @@ -1732,12 +1732,9 @@ func getMergedAliasedSymbolOfNamespaceExportDeclaration(node *ast.Node, symbol * return nil } -func (l *LanguageService) getReferencedSymbolsForModule(ctx context.Context, program *compiler.Program, symbol *ast.Symbol, excludeImportTypeOfExportEquals bool, sourceFiles []*ast.SourceFile, sourceFilesSet *collections.Set[string]) []*SymbolAndEntries { +func (l *LanguageService) getReferencedSymbolsForModule(checker *checker.Checker, program *compiler.Program, symbol *ast.Symbol, excludeImportTypeOfExportEquals bool, sourceFiles []*ast.SourceFile, sourceFilesSet *collections.Set[string]) []*SymbolAndEntries { debug.Assert(symbol.ValueDeclaration != nil) - checker, done := program.GetTypeChecker(ctx) - defer done() - moduleRefs := findModuleReferences(program, sourceFiles, symbol, checker) references := core.MapNonNil(moduleRefs, func(reference ModuleReference) *ReferenceEntry { switch reference.kind { diff --git a/tsc/internal/ls/inlay_hints.go b/tsc/internal/ls/inlay_hints.go index 0864f8df361e7..70cd5c990f1d4 100644 --- a/tsc/internal/ls/inlay_hints.go +++ b/tsc/internal/ls/inlay_hints.go @@ -38,20 +38,22 @@ func (l *LanguageService) ProvideInlayHint( mappedRanges := l.converters.FromLSPRangeIntersectingForSourceFile(file, params.Range, spanmap.FeatureInlayHints) result := make([]*lsproto.InlayHint, 0, len(mappedRanges)) for _, mapped := range mappedRanges { - projection := mapped.Script - checker, done := program.GetTypeCheckerForFile(ctx, projection) - defer done() - inlayHintState := &inlayHintState{ - ctx: ctx, - span: mapped.Span, - preferences: inlayHintPreferences, - quotePreference: quotePreference, - file: projection, - checker: checker, - converters: l.converters, - } - inlayHintState.visit(projection.AsNode()) - result = append(result, inlayHintState.result...) + func() { + projection := mapped.Script + checker, done := program.GetTypeCheckerForFile(ctx, projection) + defer done() + inlayHintState := &inlayHintState{ + ctx: ctx, + span: mapped.Span, + preferences: inlayHintPreferences, + quotePreference: quotePreference, + file: projection, + checker: checker, + converters: l.converters, + } + inlayHintState.visit(projection.AsNode()) + result = append(result, inlayHintState.result...) + }() } return lsproto.InlayHintsOrNull{InlayHints: &result}, nil } diff --git a/tsc/internal/lsp/server_flakydiagnostics_test.go b/tsc/internal/lsp/server_flakydiagnostics_test.go new file mode 100644 index 0000000000000..885155e62b0db --- /dev/null +++ b/tsc/internal/lsp/server_flakydiagnostics_test.go @@ -0,0 +1,71 @@ +package lsp_test + +import ( + "context" + "fmt" + "io" + "testing" + + "github.com/microsoft/TypeScript/tsc/internal/bundled" + "github.com/microsoft/TypeScript/tsc/internal/lsp" + "github.com/microsoft/TypeScript/tsc/internal/lsp/lsproto" + "github.com/microsoft/TypeScript/tsc/internal/testutil/lsptestutil" + "github.com/microsoft/TypeScript/tsc/internal/vfs/vfstest" + "gotest.tools/v3/assert" +) + +func TestFlakyDiagnosticTrackingParallelEmit(t *testing.T) { + t.Parallel() + if !bundled.Embedded { + t.Skip("bundled files are not embedded") + } + + for _, noEmitOnError := range []bool{false, true} { + t.Run(fmt.Sprintf("noEmitOnError=%t", noEmitOnError), func(t *testing.T) { + t.Parallel() + files := map[string]string{ + "/src/tsconfig.json": fmt.Sprintf(`{ + "compilerOptions": { "strict": true, "declaration": true, "noEmitOnError": %t, "outDir": "out" } + }`, noEmitOnError), + "/src/a.ts": `export function box(value: T) { return { value }; }`, + "/src/b.ts": `import { box } from "./a"; export const b = box("b");`, + "/src/c.ts": `import { box } from "./a"; export const c = box(1);`, + } + client, closeClient := lsptestutil.NewLSPClient(t, lsp.ServerOptions{ + Err: io.Discard, + Cwd: "/src", + FS: bundled.WrapFS(vfstest.FromMap(files, false)), + DefaultLibraryPath: bundled.LibPath(), + }, func(_ context.Context, req *lsproto.RequestMessage) *lsproto.ResponseMessage { + switch req.Method { + case lsproto.MethodClientRegisterCapability, lsproto.MethodClientUnregisterCapability, lsproto.MethodWindowWorkDoneProgressCreate: + return &lsproto.ResponseMessage{ID: req.ID, JSONRPC: req.JSONRPC, Result: lsproto.Null{}} + default: + return nil + } + }) + t.Cleanup(func() { _ = closeClient() }) + + msg, _, ok := client.SendRequest(t, lsproto.InitializeInfo, &lsproto.InitializeParams{ + Capabilities: &lsproto.ClientCapabilities{}, + InitializationOptions: &lsproto.InitializationOptionsOrNull{ + InitializationOptions: &lsproto.InitializationOptions{TrackFlakyDiagnostics: new(lsproto.DiagnosticFlakeLogLevelPanic)}, + }, + }) + assert.Assert(t, ok && msg.AsResponse().Error == nil, "initialize failed") + client.SendNotification(t, lsproto.InitializedInfo, &lsproto.InitializedParams{}) + <-client.Server.InitComplete() + + uri := lsproto.DocumentUri("file:///src/a.ts") + client.SendNotification(t, lsproto.TextDocumentDidOpenInfo, &lsproto.DidOpenTextDocumentParams{ + TextDocument: &lsproto.TextDocumentItem{Uri: uri, LanguageId: lsproto.LanguageKindTypeScript, Text: files["/src/a.ts"]}, + }) + msg, diagnostics, ok := client.SendRequest(t, lsproto.TextDocumentDiagnosticInfo, &lsproto.DocumentDiagnosticParams{ + TextDocument: lsproto.TextDocumentIdentifier{Uri: uri}, + }) + assert.Assert(t, ok && msg.AsResponse().Error == nil, "diagnostics request failed") + assert.Assert(t, diagnostics.FullDocumentDiagnosticReport != nil) + assert.Equal(t, len(diagnostics.FullDocumentDiagnosticReport.Items), 0) + }) + } +} diff --git a/tsc/internal/project/checkerpool.go b/tsc/internal/project/checkerpool.go index bed324f6cc3a5..3033f1c8bd4ad 100644 --- a/tsc/internal/project/checkerpool.go +++ b/tsc/internal/project/checkerpool.go @@ -142,29 +142,29 @@ func (p *checkerPool) GetChecker(ctx context.Context, file *ast.SourceFile) (*ch } } -// tryReacquireForRequest checks whether the given request already has an -// associated checker. If so, it either returns the checker directly (still held) -// or reacquires it by claiming a semaphore slot. The caller must provide the +// tryReacquireForRequest claims a semaphore slot, then checks whether the given +// request has an idle associated checker. The caller must provide the // appropriate semaphore channel and indicate whether this is a diagnostics // request (isDiag). If the associated checker is in the wrong category // (e.g. a diagnostics index for a query request), the association is deleted // and normal acquisition proceeds. // -// Returns (checker, release, true) if the request was served (either still held -// or reclaimed). Returns (nil, nil, false) if the caller must proceed with +// Request affinity is only a preference for an idle checker, not permission to +// reuse a held checker: concurrent acquisitions can share the same request ID. +// Returns (checker, release, true) if the checker was reclaimed. +// Returns (nil, nil, false) if the caller must proceed with // normal acquisition — in this case, a semaphore slot has already been claimed. // Must NOT be called with p.mu held. func (p *checkerPool) tryReacquireForRequest(requestID string, sem chan<- struct{}, isDiag bool) (*checker.Checker, func(), bool) { + sem <- struct{}{} if requestID == "" { - sem <- struct{}{} return nil, nil, false } p.mu.Lock() + defer p.mu.Unlock() index, ok := p.requestAssociations[requestID] if !ok { - p.mu.Unlock() - sem <- struct{}{} return nil, nil, false } @@ -172,46 +172,20 @@ func (p *checkerPool) tryReacquireForRequest(requestID string, sem chan<- struct // Index 0 is for diagnostics; indices 1+ are for queries. if (isDiag && index != 0) || (!isDiag && index == 0) { delete(p.requestAssociations, requestID) - p.mu.Unlock() - sem <- struct{}{} return nil, nil, false } c := p.checkers[index] if c == nil { delete(p.requestAssociations, requestID) - p.mu.Unlock() - sem <- struct{}{} return nil, nil, false } - held := p.heldBy[index] - if held == requestID { - // Same request, checker still held — return without claiming a slot. - p.mu.Unlock() - return c, noop, true - } - - if held == "" { - // Same request reacquiring after release — need a semaphore slot. - p.mu.Unlock() - sem <- struct{}{} - p.mu.Lock() - // Re-check: checker may have been disposed while waiting for the slot. - if cc := p.checkers[index]; cc == c && p.heldBy[index] == "" { - p.heldBy[index] = requestID - p.mu.Unlock() - return c, p.createRelease(requestID, index, c), true - } - p.mu.Unlock() - // Checker was replaced/disposed while waiting for the slot. - // The slot is still claimed; the caller will use it for normal acquisition. - return nil, nil, false + if p.heldBy[index] == "" { + p.heldBy[index] = requestID + return c, p.createRelease(requestID, index, c), true } - // Checker held by another request — claim a slot normally. - p.mu.Unlock() - sem <- struct{}{} return nil, nil, false } @@ -529,5 +503,3 @@ func (p *checkerPool) Discard() { p.cleanupTimer = nil } } - -func noop() {} diff --git a/tsc/internal/project/checkerpool_test.go b/tsc/internal/project/checkerpool_test.go index 860cc3490c2e7..3b8ffacc10b6d 100644 --- a/tsc/internal/project/checkerpool_test.go +++ b/tsc/internal/project/checkerpool_test.go @@ -96,18 +96,70 @@ func TestCheckerPoolRequestAffinity(t *testing.T) { // First call acquires. c1, release1 := pool.GetChecker(ctx, nil) - // Second call with same request ID while still held returns same checker (noop release). + release1() + + // After release, same request should still get the same checker (cross-release affinity). c2, release2 := pool.GetChecker(ctx, nil) release2() - release1() - assert.Assert(t, c1 == c2, "same request ID should return the same checker while held") + assert.Assert(t, c1 == c2, "same request ID should return the same checker after release") +} - // After release, same request should still get the same checker (cross-release affinity). - c3, release3 := pool.GetChecker(ctx, nil) - release3() +func TestCheckerPoolSameRequestContention(t *testing.T) { + t.Parallel() + session, _ := setupCheckerPoolSession(t, CheckerPoolOptions{MaxCheckers: 2}) + ls, err := session.GetLanguageService(context.Background(), "file:///src/index.ts") + assert.NilError(t, err) + for _, test := range []struct { + name string + lifetime core.CheckerLifetime + }{ + {"diagnostics", core.CheckerLifetimeDiagnostics}, + {"query", core.CheckerLifetimeTemporary}, + {"api", core.CheckerLifetimeAPI}, + } { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + pool := newTestCheckerPool(ls.GetProgram(), CheckerPoolOptions{MaxCheckers: 2}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ctx = core.WithRequestID(ctx, "same-request") + ctx = core.WithCheckerLifetime(ctx, test.lifetime) + + c1, release1 := pool.GetChecker(ctx, nil) + defer release1() + var acquired atomic.Bool + go func() { + c2, release2 := pool.GetChecker(ctx, nil) + defer release2() + assert.Assert(t, c1 == c2) + acquired.Store(true) + }() + + synctest.Wait() + assert.Check(t, !acquired.Load(), "the request ID must not bypass exclusive acquisition") + release1() + synctest.Wait() + assert.Assert(t, acquired.Load(), "waiting acquisition should finish after release") + }) + }) + } +} + +func TestCheckerPoolSameRequestConcurrentQueries(t *testing.T) { + t.Parallel() + _, pool := setupCheckerPoolSession(t, CheckerPoolOptions{MaxCheckers: 3}) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + ctx = core.WithRequestID(ctx, "same-request") - assert.Assert(t, c1 == c3, "same request ID should return the same checker after release") + c1, release1 := pool.GetChecker(ctx, nil) + defer release1() + c2, release2 := pool.GetChecker(ctx, nil) + defer release2() + assert.Check(t, c1 != c2, "overlapping acquisitions must use different checkers") + assert.Equal(t, len(pool.querySem), 2, "each acquisition must hold its own slot") } func TestCheckerPoolIdleCleanup(t *testing.T) { diff --git a/tsc/testdata/baselines/reference/fourslash/inlayHints/contentMapperInlayHintsReleaseEachRange.baseline b/tsc/testdata/baselines/reference/fourslash/inlayHints/contentMapperInlayHintsReleaseEachRange.baseline new file mode 100644 index 0000000000000..1b724611d72f1 --- /dev/null +++ b/tsc/testdata/baselines/reference/fourslash/inlayHints/contentMapperInlayHintsReleaseEachRange.baseline @@ -0,0 +1,28 @@ +// === Inlay Hints === +const value = () => 1; + ^ +{ + "position": { + "line": 1, + "character": 11 + }, + "label": [ + { + "value": ": " + }, + { + "value": "(" + }, + { + "value": ")" + }, + { + "value": " => " + }, + { + "value": "number" + } + ], + "kind": 1, + "paddingLeft": true +} \ No newline at end of file