Reverify token after parsing compiler log output. #1167
Conversation
| `${stderr}\n#Done()`, | ||
| ); | ||
|
|
||
| // Re-verify again after second async operation |
There was a problem hiding this comment.
This is the key change for the race problem.
After the second async call, we want to check again if there is no newer request.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1166 by adding a second token verification after parsing compiler log output to prevent race conditions in incremental compilation. The implementation refactors the compilation flow from callback-based to promise-based using AbortController for better async handling.
Key changes:
- Migrates from callback-based
execFileto promise-basedexecFilePromisewith AbortController for cleaner async/await flow - Adds second token verification after
parseCompilerLogOutputto catch compilations that became stale during parsing - Extracts repetitive code into helper functions (
remapCodeActionsToSourceFile,filterIncrementalDiagnostics,logIncrementalCompilationError,processAndPublishDiagnostics)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
What do you think, is it worth shipping this to the prerelease extension to get broader test coverage? We should really revamp this whole editor tooling integration now that Rewatch is first class. I'm not sure it has to be that difficult/much work either, but I assume it'd take a few planning sessions to get the structure right. But the end game really is that I think, to revamp it. Currently the extension is guessing about compilation from timestamps etc, but Rewatch already knows all of this and could just as easily drive how the editor tooling syncs/updates. One route for that is of course to do the LSP in Rust fully and launch it via Rewatch. But just the alternative of having Rewatch push changes to the editor tooling that then distributes them, in the current structure, would be enough to start making serious improvements, I think. |
Yeah maybe, but I really need to do some testing. So far, only did the fun coding part. As for the future, we can definitely craft something beautiful and need do some planning. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const notification: p.NotificationMessage = { | ||
| jsonrpc: c.jsonrpcVersion, | ||
| method: "textDocument/publishDiagnostics", | ||
| params: { | ||
| uri: fileUri, | ||
| diagnostics: res, | ||
| }, | ||
| }; | ||
| send(notification); |
There was a problem hiding this comment.
The refactored code no longer combines incremental diagnostics with compiler diagnostics from the main build. The old implementation called getCurrentCompilerDiagnosticsForFile(fileUri) and merged those diagnostics with the incremental ones before publishing. This logic should be restored to ensure diagnostics from the main build are not lost when incremental compilation completes.
There was a problem hiding this comment.
That is the whole point silly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hasIgnoredMessages = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The tracking of ignored messages has inverted logic. The variable hasIgnoredMessages is set to true inside the filter when returning true (meaning the diagnostic is KEPT), but it should be set when a diagnostic is FILTERED OUT (ignored).
For example:
- Messages starting with "Uninterpreted extension 'rescript." are filtered out (return false), but
hasIgnoredMessagesremains false - Messages that don't match the filter conditions are kept (return true), but
hasIgnoredMessagesis set to true
This causes the error logging condition at line 625 (!hasIgnoredMessages) to have opposite behavior: it will be false when diagnostics were kept (preventing legitimate error logging) and true when diagnostics were filtered out (causing incorrect error logging).
| hasIgnoredMessages = true; | |
| return true; | |
| } | |
| return true; | |
| } | |
| hasIgnoredMessages = true; |
There was a problem hiding this comment.
This code didn't change, it was merely extracted so I would not touch it.
| ); | ||
| await sendUpdatedDiagnostics(); | ||
|
|
||
| if (config.extensionConfiguration.incrementalTypechecking?.enable) { |
There was a problem hiding this comment.
This is the other relevant change: we check the file with incremental compilation versus looking at potentially stale .compiler.log
2 participants
Fixes #1166
I would test this PR for some time on my local machine to see if there are no regressions.
Feel free this review already.