Repository navigation
Compute CommonJSModuleIndicator in the parser - #65073
Jake Bailey (jakebailey) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Two critical reparsing defects leave CommonJS module indicators missing or stale, causing binding panics for valid JavaScript inputs.
2 open issues
What changed in this PR
Moves CommonJS module detection from the binder to the parser so program reuse decisions no longer depend on whether a file has been bound.
Changes:
- Tracks CommonJS constructs during parsing, including reparsed JSDoc nodes.
- Updates the binder to consume parser-owned metadata while preserving symbol creation order.
- Adds tests for module detection and program reuse across CommonJS edits.
| File | Description |
|---|---|
tsc/internal/parser/reparser.go |
Checks cloned JSDoc nodes for CommonJS constructs. |
tsc/internal/parser/parser.go |
Tracks and publishes CommonJS module indicators during parsing. |
tsc/internal/parser/parser_test.go |
Adds CommonJS detection cases. |
tsc/internal/compiler/program_test.go |
Tests reuse across CommonJS edits and script transitions. |
tsc/internal/binder/binder.go |
Uses parser metadata while retaining deferred module symbol creation. |
Review effort: Balanced
|
Uh. Interesting |
CommonJSModuleIndicator was only set by the binder, so any reader of an unbound file saw nil and treated a CommonJS JS file as a script. In particular, canReplaceFileInProgram compares module-ness of the old (usually bound) file against the freshly parsed new file, so editing any CommonJS file always rebuilt the whole program, while a script turning into CommonJS was wrongly reused. The binder also wrote this field on files shared between programs via the parse cache. Whether a file is CommonJS is purely syntactic, so the parser now records it as SourceFile.IsCommonJSModule. Nothing read the indicator node itself, so a bool is enough. Reparsed JSDoc nodes count too, since the binder visits them. The binder no longer writes the field, but still creates the file symbol at the first CommonJS construct to keep symbol creation order, and asserts that it agrees with the parser.
0a43778 to
fce61e8
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The parser/binder ownership change requires classification parity across JSDoc and top-level-await reparsing while preserving symbol order, but the necessary compiler, binder regression, and race-test validation is unavailable in this review environment.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
Go back to storing the indicator node rather than a bool. TypeScript 6.0 reported the "File is a CommonJS module; it may be converted to an ES module" suggestion at this node, and keeping it avoids having to find the construct again if that suggestion is ported.
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||

CommonJSModuleIndicatoris read on files when seeing if they can be reused. But, that field is only set when a file has been bound! This means that file reuse is racy, but also inconsistent depending on when something was bound.Instead of doing something drastic like forcing a bind just to check reuse, move this info gathering to the parser, which can calculate the same thing earlier.
Funnily, the
SourceFiletype already hadCommonJSModuleIndicatorin the parser-owned field section, which was wrong.Unfortunately,
CommonJSModuleIndicatordepends on JSDoc nodes, so this requires a bit more care. I think it using JSDoc nodes is utterly strange, but, that's what we've always done, even before TS 7.Arguably, we don't even need to set a node for CommonJSModuleIndicator or have one at all, it's not in the public JS API. I might push again to this PR and just make it a bool or tristate or something. Though it not being there but part of the reuse calculation makes me wonder if it should actually be in the AST API after all?Opted to not, because actually that node's normally used for the CJS-to-ESM fix, which, we never ported