Report errors on unsupported CJS constructs in declaration emit

[ahejlsberg]

With this PR we report errors in the following cases when generating declaration files from CommonJS modules:

• Multiple module.exports assignments.
• Nested module.exports.XXX assignments.
• Nested exports.XXX assignments.
• Nested Object.defineProperty(module.exports, ...) calls.
• Nested Object.defineProperty(exports, ...) calls.

The potential error locations for nested CJS constructs are collected by the binder in a new SourceFile.NestedCJSExports list such that the declaration transformer doesn't have to visit subtrees.

[DanielRosenwasser]

This looks good to me, though I'd rather get a second pair of eyes to take a look at the symbol tracker stuff (it's been a while and I'm less familiar).

[ahejlsberg]

though I'd rather get a second pair of eyes to take a look at the symbol tracker stuff

The only change there is the addition of two error reporting methods (the symbol tracker is the conduit for error reporting in the declaration transformer).

[Copilot]

Pull request overview

This PR introduces error reporting for unsupported CommonJS constructs during declaration emit, covering multiple module.exports assignments and nested CJS export patterns.

Changes:

• New diagnostic messages (TS6424, TS6425) for multiple module.exports assignments and nested CJS export constructs.
• Binder now collects nested CJS export nodes into SourceFile.NestedCJSExports.
• Declaration transformer reports these diagnostics via SymbolTracker methods.

Reviewed changes

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

Show a summary per file

File	Description
internal/ast/ast.go	Adds NestedCJSExports field to SourceFile
internal/binder/binder.go	Collects nested CJS export nodes during binding
internal/diagnostics/extraDiagnosticMessages.json	Defines new error messages TS6424 and TS6425
internal/diagnostics/diagnostics_generated.go	Generated diagnostics for TS6424 and TS6425
internal/nodebuilder/types.go	Adds new methods to SymbolTracker interface
internal/checker/symboltracker.go	Implements new SymbolTracker methods
internal/transformers/declarations/tracker.go	Implements reporting for new diagnostics
internal/transformers/declarations/transform.go	Triggers diagnostic reporting during source file transform
testdata/baselines/...	Baseline test files for the new error cases

[jakebailey]

Isn't this not the same module? I guess the comment above says that we treat it as such (yay...)

[ahejlsberg]

Yeah, we don't resolve the module in module.exports assignments to ensure it isn't a local. The parser isn't really equipped to do that, and that's where the re-parsing into ExportAssignment nodes happens.

[weswigham]

Otherwise, this looks good. I think a list on the file object like this would need to be invalidated if we did incremental reparsing, but we don't anymore, so 🤷‍♂️

[weswigham]

A comment elsewhere asking why this was done via the symbol tracker was pertinent - the symbol tracker is the error reporter for the node builder (so the checker node creation context has access to the declaration diagnostic collection) - for inferred type nodes. The declaration transform can issue diagnostics directly. This call to the tracker and the one below can be inlined and report diagnostics directly via tx.state.addDiagnostic (many ID diagnostics ultimately do this) - no need to defer through the tracker like this and add a bunch of methods to its' type.

[jakebailey]

Yeah, I don't think we need the symbol tracker for this

[ahejlsberg]

The declaration transformer currently doesn't call tx.state.addDiagnostic directly anywhere, but for example has an indirection to tx.tracker.ReportInferenceFallback(node) to report errors. And that method doesn't appear to be called by anyone but the declaration transformer, so I figured that was the desired pattern.

But happy to just inline instead.

[weswigham]

Yeah, in that case, ReportInferenceFallback is called by the node builder, so it also calls it when it wants to make the same error, rather than duplicating the diagnostic logic, since it does have access to the tracker.

[ahejlsberg]

Yeah, in that case, ReportInferenceFallback is called by the node builder

Is it? I might be missing something, but I'm not seeing that anywhere.