Fix external helper crash after module-status updates#3994
Conversation
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Perfect - can you write this as a fourslash test instead? Make sure the fourslash test actually fails without the fix. |
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Addressed in f732c93. I replaced the checker-level regression with a fourslash test and verified it fails without the production fix with the |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes an incremental-program reuse edge case that could crash external helper resolution when a file changes from script to module (e.g., via adding an export) under importHelpers.
Changes:
- Mark program state as non-reusable when a file’s module-status indicators (
ExternalModuleIndicator/CommonJSModuleIndicator) change. - Add a fourslash regression test that edits a script into an exported decorated module under
importHelpersand verifies diagnostics remain error-free.
Show a summary per file
| File | Description |
|---|---|
| internal/fourslash/tests/importHelpersAfterScriptBecomesDecoratedModule_test.go | Adds regression test covering script→decorated module transition with importHelpers. |
| internal/compiler/program.go | Prevents incremental reuse when module-status indicators change, forcing rebuild of module-resolution/helper state. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
| return file2 != nil && | ||
| file1.ParseOptions() == file2.ParseOptions() && | ||
| file1.UsesUriStyleNodeCoreModules == file2.UsesUriStyleNodeCoreModules && | ||
| (file1.ExternalModuleIndicator != nil) == (file2.ExternalModuleIndicator != nil) && |
There was a problem hiding this comment.
Don't we need to check that they're equal in some way?
There was a problem hiding this comment.
I don't think so - my understanding is that these are used to determine if the program is sufficiently structurally the same to avoid rebuilding a full program - not that the files themselves are "equal" before/after. Below, we check that the rest of the structure is equal (e.g. same imports, global modifications, etc.)
From program.go's perspective, the external module indicator probably should not be used to determine anything about the file other than "is this a module, or is this a global?"
checkExternalEmitHelperscould panic after an incremental program update when a file changed from script to module. The reused program state could lack the synthetictslibimport specifier needed for helper resolution.Program reuse
ExternalModuleIndicatororCommonJSModuleIndicatoras non-reusable.Regression coverage
importHelpers.tslibstate and completes semantic diagnostics without errors.