Fix: skip CommonJS export binding when module or exports is a local variable

[Muhammad-Bin-Ali]

Fixes #2656

Summary

In .js files, module.exports = x or exports.foo = x inside a function where module or exports is a local variable (parameter, const, var, etc.) was incorrectly treated as a real CommonJS export. This caused the file's actual ESM exports to be shadowed by a false export= symbol, producing spurious TS1192/TS2305/TS2309 errors on imports.

The root cause is that the reparser creates JSExportAssignment/CommonJSExport nodes based on purely syntactic matching (IsModuleIdentifier just checks node.Text() == "module"), without verifying whether the identifier actually refers to the CommonJS global or a local binding. Since the parser has no scope information, this check can't happen there.

The fix adds a scope check in the binder (where scope information is available) before binding these nodes. If module or exports resolves to a local symbol in b.blockScopeContainer via lookupName, the binder skips the CJS export binding entirely. Rather than clearing CommonJSModuleIndicator mid-walk (which risks re-entry via later setCommonJSModuleIndicator calls), the indicator is left intact during the walk and corrected once afterward: if the indicator pointed to a skipped node, it is replaced with the first valid CJS export that was actually bound, or cleared if none exists.

Also fixes a pre-existing false TS6425 diagnostic on amdLikeInputDeclarationEmit, where const module = {}; module.exports = X inside an AMD callback was incorrectly flagged as a nested CommonJS export.

[Muhammad-Bin-Ali]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Fixes incorrect CommonJS export detection in JavaScript when module/exports is a local binding (parameter/variable), which previously caused false export= symbols and downstream import diagnostics (e.g. TS1192/TS2305/TS2309) in this TypeScript compiler/language-service port.

Changes:

• Add binder-time scope checks to skip binding JSExportAssignment/CommonJSExport as CommonJS exports when module/exports resolves to a local symbol, and clear a falsely-set CommonJSModuleIndicator.
• Add new compiler baseline tests covering module.exports / exports.* inside functions with module/exports parameters (and a regression test ensuring top-level module.exports still behaves as CommonJS).
• Update submodule baseline expectations to remove a pre-existing false TS6425 in amdLikeInputDeclarationEmit.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/binder/binder.go	Adds scope-based skip logic for binding synthetic CommonJS export nodes when module/exports is locally declared.
testdata/tests/cases/compiler/moduleExportsTopLevelStillCJS.ts	New regression test ensuring top-level module.exports still produces CommonJS export= behavior.
testdata/tests/cases/compiler/moduleExportsInFunctionParamNamed.ts	New test ensuring named ESM exports are not shadowed by module.exports when module is a function parameter.
testdata/tests/cases/compiler/moduleExportsInFunctionParam.ts	New test ensuring default ESM export is not shadowed by module.exports when module is a function parameter.
testdata/tests/cases/compiler/moduleExportsInArrowFunctionParam.ts	New test ensuring default ESM export is not shadowed by module.exports when module is an arrow parameter.
testdata/tests/cases/compiler/exportsInFunctionParam.ts	New test ensuring named ESM exports are not shadowed by exports.foo = ... when exports is a function parameter.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.js.diff	Updates reference diff baseline to reflect removal of false CommonJS-related declaration emit behavior.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.js	Updates reference baseline output to remove now-incorrect export {} artifact in the emitted d.ts section.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.errors.txt.diff	Removes reference diff baseline indicating the prior TS6425 error.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.errors.txt	Removes the reference baseline error output for the prior TS6425 diagnostic.
testdata/baselines/reference/compiler/moduleExportsTopLevelStillCJS.types	Adds expected types baseline for the new top-level CJS indicator regression test.
testdata/baselines/reference/compiler/moduleExportsTopLevelStillCJS.symbols	Adds expected symbols baseline for the new top-level CJS indicator regression test.
testdata/baselines/reference/compiler/moduleExportsInFunctionParamNamed.types	Adds expected types baseline for the “local module param” named-export scenario.
testdata/baselines/reference/compiler/moduleExportsInFunctionParamNamed.symbols	Adds expected symbols baseline for the “local module param” named-export scenario.
testdata/baselines/reference/compiler/moduleExportsInFunctionParamNamed.errors.txt	Adds expected errors baseline (TS7006 only) for the “local module param” named-export scenario.
testdata/baselines/reference/compiler/moduleExportsInFunctionParam.types	Adds expected types baseline for the “local module param” default-export scenario.
testdata/baselines/reference/compiler/moduleExportsInFunctionParam.symbols	Adds expected symbols baseline for the “local module param” default-export scenario.
testdata/baselines/reference/compiler/moduleExportsInFunctionParam.errors.txt	Adds expected errors baseline (TS7006 only) for the “local module param” default-export scenario.
testdata/baselines/reference/compiler/moduleExportsInArrowFunctionParam.types	Adds expected types baseline for the “arrow module param” default-export scenario.
testdata/baselines/reference/compiler/moduleExportsInArrowFunctionParam.symbols	Adds expected symbols baseline for the “arrow module param” default-export scenario.
testdata/baselines/reference/compiler/moduleExportsInArrowFunctionParam.errors.txt	Adds expected errors baseline (TS7006 only) for the “arrow module param” default-export scenario.
testdata/baselines/reference/compiler/exportsInFunctionParam.types	Adds expected types baseline for the “local exports param” named-export scenario.
testdata/baselines/reference/compiler/exportsInFunctionParam.symbols	Adds expected symbols baseline for the “local exports param” named-export scenario.
testdata/baselines/reference/compiler/exportsInFunctionParam.errors.txt	Adds expected errors baseline (TS7006 only) for the “local exports param” named-export scenario.

[jakebailey]

This seems plausible (I was hoping we could do this), but, I don't think it's right to clear the CJS indicator mid walk. My gut says that it could be readded by a later call.

Shouldn't it be better to instead guard where we set it in the first place, if that assignment is related to one of these patterns?

[Muhammad-Bin-Ali]

This seems plausible (I was hoping we could do this), but, I don't think it's right to clear the CJS indicator mid walk. My gut says that it could be readded by a later call.

Shouldn't it be better to instead guard where we set it in the first place, if that assignment is related to one of these patterns?

Yeah, I see what you're getting at, and I think I agree with that. Let me see if I can implement that.

[Muhammad-Bin-Ali]

@jakebailey

Upon looking into it further, I don't think there's a clean way of solving this problem earlier in the walk. Whether we choose to address it earlier in the binder's walk or in the reparser, we need information about what symbols exists before the JSExportAssignment node is visited, which is exactly what's determined in the walk. That being said, I did modify the approach a little (as well as added tests) to address the problem you mentioned.