Support cancellation in checker #856
Conversation
ahejlsberg
requested review from
Copilot,
jakebailey and
andrewbranch
and removed request for
Copilot and
jakebailey
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for context‐based cancellation across key diagnostic and checking methods, updating several APIs to accept a context parameter. Key changes include propagating a context.Context in methods such as GetSyntacticDiagnostics, GetSemanticDiagnostics, CheckSourceFile and GetDiagnostics, along with adding helper methods in the Checker to handle cancellation and update existing tests accordingly.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/testutil/harnessutil/harnessutil.go | Updated diagnostic function calls to pass context.Background() where a context was previously omitted. |
| internal/ls/diagnostics.go | Modified diagnostic methods to use a context for obtaining diagnostics. |
| internal/execute/tsc.go | Updated tsc execution logic to pass context for diagnostic retrieval. |
| internal/compiler/program.go | Added context parameters to several methods (CheckSourceFiles, GetSyntacticDiagnostics, etc.) and updated the helper to pass cancellation checks. |
| internal/checker/utilities.go | Added cancellation helper methods and integrated them in Checker functions. |
| internal/checker/checker.go | Updated Checker’s diagnostic and checking methods to accept a context and handle cancellation via a new cancellation flag. |
| internal/ast/ast.go | Added helper methods to retrieve statement lists to support cancellation flow. |
| cmd/tsgo/main.go | Updated main execution to pass a context in calls to diagnostic methods. |
Comments suppressed due to low confidence (1)
internal/checker/checker.go:13155
- [nitpick] The current design calls c.checkNotCanceled() and panics if cancellation was previously detected; consider a more graceful approach (e.g. returning nil or an error) to allow callers to handle cancellation without a panic.
func (c *Checker) GetDiagnostics(ctx context.Context, sourceFile *ast.SourceFile) []*ast.Diagnostic {
internal/compiler/program.go
Outdated
| if ctx != nil && ctx.Err() != nil { | ||
| return nil | ||
| } | ||
| if len(sourceFile.CommentDirectives) == 0 { | ||
| return diags | ||
| } |
There was a problem hiding this comment.
Consider checking for cancellation earlier in getDiagnosticsHelper to avoid processing diagnostics when the context is already cancelled, which may save unnecessary computation.
| if ctx != nil && ctx.Err() != nil { | |
| return nil | |
| } | |
| if len(sourceFile.CommentDirectives) == 0 { | |
| return diags | |
| } |
Copilot uses AI. Check for mistakes.
| syntaxDiagnostics := program.GetSyntacticDiagnostics(context.Background(), file) | ||
| semanticDiagnostics := program.GetSemanticDiagnostics(context.Background(), file) |
There was a problem hiding this comment.
| syntaxDiagnostics := program.GetSyntacticDiagnostics(context.Background(), file) | |
| semanticDiagnostics := program.GetSemanticDiagnostics(context.Background(), file) | |
| syntaxDiagnostics := program.GetSyntacticDiagnostics(context.TODO(), file) | |
| semanticDiagnostics := program.GetSemanticDiagnostics(context.TODO(), file) |
I think this is one place we're going to want to plumb it.
There was a problem hiding this comment.
I'm going to leave that to a separate PR though.
There was a problem hiding this comment.
To be clear my suggestion wasn't to plumb it in this PR, but to just leave these as TODO calls to make it clear that we're intending on changing it later.
There was a problem hiding this comment.
I'll approve it in any case, this is just a nit and I'm sure Andrew will figure this out when he plugs it in
|
I should mention that I verified cancellation works by compiling the VS Code project with a context that times out after two seconds. It consistently bailed out within a few milliseconds. |
| syntaxDiagnostics := program.GetSyntacticDiagnostics(context.Background(), file) | ||
| semanticDiagnostics := program.GetSemanticDiagnostics(context.Background(), file) |
There was a problem hiding this comment.
I'll approve it in any case, this is just a nit and I'm sure Andrew will figure this out when he plugs it in
With this PR the
CheckSourceFileandGetDiagnosticsmethods onCheckerandGetSyntacticDiagnostics,GetBindDiagnostics, andGetSemanticDiagnosticsonProgramsupport cancellation through acontext.Contextwhich is passed as the first parameter to those methods.When an operation is cancelled, any subsequent call to
CheckSourceFile,GetDiagnostics,GetDiagnosticsWithoutCheck, orGetGlobalDiagnosticson that checker will panic regardless of which context the checker is invoked on. (Following cancellation, a checker contains only a partial list of diagnostics, and without more extensive work to track which source elements were checked and which weren't, that checker can no longer deliver a consistent set of diagnostics.)