Remove GetOptionsDiagnostics, expose global diags in LSP#2322
Remove GetOptionsDiagnostics, expose global diags in LSP#2322jakebailey wants to merge 15 commits intomainfrom
Conversation
jakebailey
requested review from
DanielRosenwasser,
andrewbranch,
Copilot and
sheetalkamat
There was a problem hiding this comment.
Pull request overview
This PR removes the GetOptionsDiagnostics method, which redundantly combined config file diagnostics and global diagnostics. The code now directly uses GetConfigFileParsingDiagnostics and GetGlobalDiagnostics instead, eliminating unnecessary duplication and improving diagnostic handling clarity.
Key changes:
- Removed
GetOptionsDiagnosticsmethod and its helper from theProgramstruct - Updated diagnostic collection logic to call
GetConfigFileParsingDiagnosticsandGetGlobalDiagnosticsdirectly - Added a safety check in
GetGlobalDiagnosticsto handle editor scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testdata/baselines/reference/tsbuildWatch/configFileErrors/reports-syntax-errors-in-config-file.js | Updated test baseline to reflect removal of semanticDiagnosticsPerFile field and changes to diagnostic caching behavior |
| testdata/baselines/reference/tsbuild/configFileErrors/reports-syntax-errors-in-config-file.js | Updated test baseline with same changes as watch variant |
| internal/execute/incremental/program.go | Removed GetOptionsDiagnostics method wrapper and its usage in error state checking |
| internal/compiler/program.go | Removed GetOptionsDiagnostics and helper methods, added type assertion safety check, updated GetDiagnosticsOfAnyProgram to call diagnostics methods directly |
jakebailey
changed the title
jakebailey
changed the title
Isn't it the case that some diagnostics are generated during checker initialization due to symbol table merging? Or are those only ever duplicate declaration diagnostics, which always have a location? It seems like maybe a distinction we should have instead of whatever mess we inherited from Strada is diagnostics that got generated during initialization, because those (whether they have a location or not) will always reliably be there regardless of what files have been checked in what order. |
|
Yes, there are diagnostics that are generated without a location, so can't be exposed any other way. |
|
I know that some diagnostics are generated without a location; I'm asking whether "has a location" is orthogonal to "is dependent on what files have been checked in what order" (I think it is, at least in part) |
|
Ah. I think we still can report global diagnostics on things like "Iterable is missing", which only happens when you finally ask for it somewhere. Would need to recheck. |
|
I think this change is fine as-is; I was just suggesting that it could be useful to have one bucket of diagnostics specifically from checker initialization because that would be known to be populated the exact same way no matter how many files you have or haven't checked. If that happened to include any global (location-less) diagnostics, we could reliably report them in the LSP without jumping through extra hoops. But I recognize that it wouldn't contain all the global diagnostics (and maybe in practice it wouldn't include any). The broader question of global diagnostics in the LSP will have to be figured out later—as you said, it's not new in this PR. I think Sheetal meant that it's new in Corsa, which is true I guess. I would approve this if you're ready to undraft it. |
| p.globalDiagCheckerCount[index] = len(globals) | ||
| before := len(p.globalDiagAccumulated) | ||
| p.globalDiagAccumulated = compiler.SortAndDeduplicateDiagnostics(append(p.globalDiagAccumulated, globals...)) | ||
| if len(p.globalDiagAccumulated) != before { | ||
| p.globalDiagChanged = true | ||
| } |
There was a problem hiding this comment.
Okay, this is my new fresh idea. The language server keeps track of global diagnostics, merging them into a big list that's kept even if checkers go away. Those get exposed as diags on the tsconfig, and it's all checked periodically.
jakebailey
changed the title
| GetChecker(ctx context.Context) (*checker.Checker, func()) | ||
| GetCheckerForFile(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func()) | ||
| GetCheckerForFileExclusive(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func()) | ||
| GetGlobalDiagnostics() []*ast.Diagnostic |
There was a problem hiding this comment.
There's technically not a great reason to bother putting this in here. In a future PR, I'm going to have to redo the way the pool works again anyway, but I can undo this specific bit and shove it back into Program as a special case.
|
Probably should just take #3285 |
"Options diagnostics" were just config file diagnostics + global diagnostics.
This was odd, because we then took that in places and combined it with... config file diags and global diags. And then not deduping it, sometimes.
This PR just removes
GetOptionsDiagnosticsaltogether, instead leaving behind justGetConfigFileParsingDiagnosticsandGetGlobalDiagnostics.The language server still does not ever callGetGlobalDiagnostics. I am not sure where such a thing would go, since global diagnostics are generated as a side effect of doing checking (e.g., when we figure out thatAsyncIterablewas never declared or something).The language server now keeps track of global diagnostics on the fly as it creates and destroys checkers. The diags are eventually bubbled out as diags on the tsconfig.
There's a baseline change, but I think it's correct and positive.