Revert "Make Program diagnostic API clearer (#2197)"

[jakebailey]

This reverts commit c86b860.

Fixes #2249

I need to think about this harder. Best to revert.

[Copilot]

Pull request overview

This PR reverts commit c86b860 which attempted to make the Program diagnostic API clearer. The revert is being done to address issue #2249, as the maintainer needs to reconsider the approach.

Key changes being reverted:

• Re-introduces context.Context parameter to ForEachCheckerParallel methods
• Renames GetSemanticDiagnosticsWithoutNoEmitFiltering back to GetSemanticDiagnosticsNoFilter
• Restores previous diagnostic collection logic with helper methods
• Re-adds CheckSourceFiles method
• Reverts changes to diagnostic comparison optimizations

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/testrunner/compiler_runner.go	Adds context parameter to ForEachCheckerParallel call in union ordering test
internal/project/checkerpool.go	Re-adds ForEachCheckerParallel method with context parameter and request ID validation
internal/execute/incremental/program.go	Reverts method name from GetSemanticDiagnosticsWithoutNoEmitFiltering to GetSemanticDiagnosticsNoFilter
internal/compiler/program.go	Restores previous diagnostic collection architecture with helper methods, re-adds CheckSourceFiles, and changes back to using slices.Concat
internal/compiler/checkerpool.go	Adds context parameter to ForEachCheckerParallel interface and implementation
internal/checker/checker_test.go	Adds test for checking TypeScript compiler source files
internal/ast/diagnostic.go	Removes pointer equality optimization checks from diagnostic comparison functions

[Copilot]

Calling ForEachCheckerParallel with context.Background() will cause a panic in the project CheckerPool implementation. The project CheckerPool's ForEachCheckerParallel method (line 106-108 in internal/project/checkerpool.go) panics when the context has no request ID.

The InstantiationCount() method doesn't receive a context parameter, so it uses context.Background(). This will work fine with the compiler's checkerPool but will panic when used with a project CheckerPool.

Consider either:

1. Adding a context parameter to this method
2. Creating a variant of ForEachCheckerParallel that doesn't require a request ID for read-only operations like counting
3. Using the compiler's checkerPool directly instead of the interface when the implementation is known

[Copilot]

This function will panic if sourceFile is nil. The code on lines 1047-1052 attempts to handle the nil case by conditionally getting a file checker, but then line 1053 unconditionally accesses sourceFile.BindDiagnostics(), which will cause a nil pointer dereference.

Either remove the nil handling code (lines 1047-1052) if this function should never be called with nil, or add a nil check before line 1053 to handle the nil case properly.

Suggested change

	diags := slices.Clip(sourceFile.BindDiagnostics())
	var diags []*ast.Diagnostic
	if sourceFile != nil {
	diags = slices.Clip(sourceFile.BindDiagnostics())
	} else {
	diags = nil
	}

[Copilot]

This function will panic if sourceFile is nil. The code on lines 1150-1155 attempts to handle the nil case by conditionally getting a file checker, but then line 1157 unconditionally accesses sourceFile.BindSuggestionDiagnostics, which will cause a nil pointer dereference.

Either remove the nil handling code (lines 1150-1155) if this function should never be called with nil, or add a nil check before line 1157 to handle the nil case properly.

Suggested change

	diags := slices.Clip(sourceFile.BindSuggestionDiagnostics)
	var diags []*ast.Diagnostic
	if sourceFile != nil {
	diags = slices.Clip(sourceFile.BindSuggestionDiagnostics)
	} else {
	diags = []*ast.Diagnostic{}
	}

[Copilot]

The call to p.GetChecker(ctx) on line 119 will deadlock because the mutex p.mu is already locked on line 102. The GetChecker method (line 86-91) attempts to acquire the same mutex with p.mu.Lock(), which will cause a deadlock.

This code path is only reached when there is no existing request checker, which means this implementation is fundamentally broken. Consider either:

1. Implementing a getCheckerLocked variant that assumes the lock is already held
2. Panicking instead, as suggested by the TODO comment on line 117-118

Suggested change

	// TODO: Does this ever work without deadlocking? `p.GetChecker` also tries to lock this mutex.
	// Should this just be a panic?
	c, release := p.GetChecker(ctx)
	defer release()
	cb(0, c)
	// Use getCheckerLocked to avoid deadlock; GetChecker would try to lock p.mu again.
	c, idx := p.getCheckerLocked(requestID)
	release := func() {
	p.inUse[c] = false
	if requestID != "" {
	delete(p.requestAssociations, requestID)
	}
	}
	defer release()
	cb(idx, c)

[Copilot]

Calling ForEachCheckerParallel with context.Background() will cause a panic in the project CheckerPool implementation. The project CheckerPool's ForEachCheckerParallel method (line 106-108 in internal/project/checkerpool.go) panics when the context has no request ID.

The counting methods SymbolCount(), TypeCount(), and InstantiationCount() don't receive a context parameter, so they use context.Background(). This will work fine with the compiler's checkerPool but will panic when used with a project CheckerPool.

Consider either:

1. Adding a context parameter to these counting methods
2. Creating a variant of ForEachCheckerParallel that doesn't require a request ID for read-only operations like counting
3. Using the compiler's checkerPool directly instead of the interface when the implementation is known

[Copilot]

Calling ForEachCheckerParallel with context.Background() will cause a panic in the project CheckerPool implementation. The project CheckerPool's ForEachCheckerParallel method (line 106-108 in internal/project/checkerpool.go) panics when the context has no request ID.

The TypeCount() method doesn't receive a context parameter, so it uses context.Background(). This will work fine with the compiler's checkerPool but will panic when used with a project CheckerPool.

Consider either:

1. Adding a context parameter to this method
2. Creating a variant of ForEachCheckerParallel that doesn't require a request ID for read-only operations like counting
3. Using the compiler's checkerPool directly instead of the interface when the implementation is known