Skip to content

No reparsing of CommonJS constructs#3388

Open
ahejlsberg wants to merge 19 commits intomainfrom
no-cjs-reparsing
Open

No reparsing of CommonJS constructs#3388
ahejlsberg wants to merge 19 commits intomainfrom
no-cjs-reparsing

Conversation

@ahejlsberg
Copy link
Copy Markdown
Member

@ahejlsberg ahejlsberg commented Apr 11, 2026

In Strada, we recognize ESM constructs in the parser and set SourceFile.ExternalModuleIndicator. Then, for source files that aren't marked as ES modules, we recognize CJS constructs in the binder and set SourceFile.CommonJSModuleIndicator. This means that CJS constructs in ES modules are ignored and just treated as regular JS code.

In Corsa, we've been recognizing both ESM and CJS constructs in the parser and re-parsing CJS constructs into JSExportAssignment and CommonJSExport synthetic AST nodes . This causes issues when files contain both kinds of constructs because ESM doesn't take precedence and we get confused about which kind of module we're dealing with.

This PR removes re-parsing of CJS constructs, instead processing module.exports = ... and exports.foo = ... assignments in the binder and checker as we did in Strada. Specifically, the JSExportAssignment and CommonJSExport AST nodes are gone, and CJS constructs are processed using ast.GetAssignmentDeclarationKind classification. Re-parsing now purely deals with JSDoc constructs.

The baseline changes mainly have to do with us now ignoring CJS constructs in ESM files. A number of changes are also caused by logic we were missing to resolve exports and module.exports to the proper target symbol (these are the changes where exports changes to export= in the symbol baselines).

Fixes #2656.

Copilot AI review requested due to automatic review settings April 11, 2026 15:38
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@ahejlsberg
Copy link
Copy Markdown
Member Author

I had to locally disable the formatting step in the AST generation scripts. Here's what happens when I don't:

c:\ts-go>hereby generate:ast
Using c:\ts-go\Herebyfile.mjs to run generate:ast
Starting generate:ast
[09:23:20.213] [0] $ node --experimental-strip-types --no-warnings ./_scripts/generate.ts
Generating encoder/decoder code...
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
Warning: formatter failed for c:\ts-go\internal\api\encoder\encoder_generated.go
Wrote c:\ts-go\internal\api\encoder\encoder_generated.go
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
Warning: formatter failed for c:\ts-go\internal\api\encoder\decoder_generated.go
Wrote c:\ts-go\internal\api\encoder\decoder_generated.go
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
Warning: formatter failed for c:\ts-go\_packages\api\src\node\protocol.generated.ts
Wrote c:\ts-go\_packages\api\src\node\protocol.generated.ts
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
Warning: formatter failed for c:\ts-go\_packages\api\src\node\encoder.generated.ts
Wrote c:\ts-go\_packages\api\src\node\encoder.generated.ts
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
Warning: formatter failed for c:\ts-go\_packages\api\src\node\node.generated.ts
Wrote c:\ts-go\_packages\api\src\node\node.generated.ts
Done!
Generating Go AST code...
No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.
file:///c:/ts-go/node_modules/execa/lib/return/final-error.js:6
        return new ErrorClass(message, options);
               ^

ExecaSyncError: Command failed with exit code 14: dprint fmt "c:\ts-go\internal\ast\ast_generated.go"
    at getFinalError (file:///c:/ts-go/node_modules/execa/lib/return/final-error.js:6:9)
    at makeError (file:///c:/ts-go/node_modules/execa/lib/return/result.js:108:16)
    at getSyncResult (file:///c:/ts-go/node_modules/execa/lib/methods/main-sync.js:146:4)
    at spawnSubprocessSync (file:///c:/ts-go/node_modules/execa/lib/methods/main-sync.js:100:9)
    at execaCoreSync (file:///c:/ts-go/node_modules/execa/lib/methods/main-sync.js:18:17)
    at callBoundExeca (file:///c:/ts-go/node_modules/execa/lib/methods/create.js:43:5)
    at boundExeca (file:///c:/ts-go/node_modules/execa/lib/methods/create.js:15:44)
    at writeAndFormat (file:///c:/ts-go/_scripts/generate-go-ast.ts:960:5)
    at main (file:///c:/ts-go/_scripts/generate-go-ast.ts:969:5)
    at generate (file:///c:/ts-go/_scripts/generate.ts:8:5) {
  shortMessage: 'Command failed with exit code 14: dprint fmt "c:\\ts-go\\internal\\ast\\ast_generated.go"',
  command: 'dprint fmt c:\\ts-go\\internal\\ast\\ast_generated.go',
  escapedCommand: 'dprint fmt "c:\\ts-go\\internal\\ast\\ast_generated.go"',
  cwd: 'c:\\ts-go',
  durationMs: 92.288,
  failed: true,
  timedOut: false,
  isCanceled: false,
  isGracefullyCanceled: false,
  isTerminated: false,
  isMaxBuffer: false,
  isForcefullyTerminated: false,
  exitCode: 14,
  stdio: [ undefined, undefined, undefined ],
  ipcOutput: [],
  pipedFrom: []
}

Node.js v24.14.1
[09:23:25.163] [0]  Command failed with exit code 1: node --experimental-strip-types --no-warnings ./_scripts/generate.ts
[09:23:25.164] [0]  (done in 4.9s)
Error in generate:ast in 4.9s
ExecaError: Command failed with exit code 1: node --experimental-strip-types --no-warnings ./_scripts/generate.ts
Completed generate:ast with errors in 4.9s
Failed tasks: generate:ast

@ahejlsberg
Copy link
Copy Markdown
Member Author

@andrewbranch @jakebailey I'm not sure why hereby generate:ast fails for me locally. Could be a configuration issue, or could be that the script doesn't work properly on Windows. Ideas?

@ahejlsberg
Copy link
Copy Markdown
Member Author

@jakebailey I'm puzzled by the race mode failure. Only thing I can think of is that parsing used to set the CommonJSModuleIndicator, but that now happens in binding. Do we not fully bind files before computing their build info?

@jakebailey
Copy link
Copy Markdown
Member

jakebailey commented Apr 11, 2026

I assume the race is because affectsGlobalScope requires binding information for export stuff whereas before it did not, and so we need to make sure that we have bound files before then. Probably just throw binder.BindSourceFile(file) at the top of fileAffectsGlobalScope.

For reference, Strada:

export function create(newProgram: Program, oldState: Readonly<BuilderState> | undefined, disableUseFileVersionAsSignature: boolean): BuilderState {
    const fileInfos = new Map<Path, FileInfo>();
    const options = newProgram.getCompilerOptions();
    const referencedMap = createReferencedMap(options);
    const useOldState = canReuseOldState(referencedMap, oldState);

    // Ensure source files have parent pointers set
    newProgram.getTypeChecker();

    // Create the reference map, and set the file infos
    for (const sourceFile of newProgram.getSourceFiles()) {
        const version = Debug.checkDefined(sourceFile.version, "Program intended to be used with Builder should have source files with versions set");

It asks for a type checker as a way to make sure files are bound before doing the work.

@jakebailey
Copy link
Copy Markdown
Member

I cannot reproduce your ast generation issues on Windows, unfortunately...

@ahejlsberg
Copy link
Copy Markdown
Member Author

ahejlsberg commented Apr 11, 2026

I cannot reproduce your ast generation issues on Windows, unfortunately...

Apparently dprint is failing because it can't find any files to format. Which is really odd because dprint fmt works just fine from the command line. Ideas for what's wrong?

No files found to format with the specified plugins at C:\ts-go. You may want to try using `dprint output-file-paths` to see which files it's finding or run with `--allow-no-files`.

@ahejlsberg
Copy link
Copy Markdown
Member Author

@jakebailey @andrewbranch I figured out the dprint issue when running hereby generate:ast. It turns out that if your current directory is c:\ts-go (lower case c:) it fails. But when the directory is C:\ts-go (upper case C:) it succeeds. Lovely!

@jakebailey
Copy link
Copy Markdown
Member

jakebailey commented Apr 11, 2026

That's evil; I guess this is a cmd-ism as powershell does not let you cd into a lowercase drive.

If you shove this at the top of Herebyfile.mjs, does that make it work?

if (process.platform === "win32") {
    process.chdir(fs.realpathSync.native(process.cwd()));
}

@ahejlsberg
Copy link
Copy Markdown
Member Author

If you shove this at the top of Herebyfile.mjs, does that make it work?

Yes, that works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exports are no longer recognized in js modules which contain functions with parameter called module which have module.exports = in their body

3 participants