Port TS#63043: Implement walkBindingPattern for declaration emit of destructured parameter properties#2751
Conversation
Implement walkBindingPattern in declaration emit to properly emit property declarations for destructured parameter properties. While parameter properties with binding patterns are an error (TS1187), the declaration emitter should still emit them correctly. The runtime transform crash fix was already ported (IsIdentifier checks in runtimesyntax.go). This change ports the remaining declaration emit behavior to match TypeScript. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Ports TypeScript PR microsoft/TypeScript#63043 into the Go declaration transformer so .d.ts emit includes class property declarations for destructured parameter properties, aligning tsgo’s output with upstream TypeScript baselines.
Changes:
- Implemented
walkBindingPatternin the declarations transformer and wired it intotransformClassDeclarationfor parameter properties with binding patterns. - Updated reference baselines to include the emitted property declarations and removed now-unnecessary
.js.diffdivergence files.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/transformers/declarations/transform.go | Adds walkBindingPattern and uses it to emit PropertyDeclarations for destructured parameter properties during .d.ts emit. |
| testdata/baselines/reference/submodule/compiler/declarationEmitDestructuringParameterProperties.js | Baseline update showing emitted x/y/z properties in generated .d.ts. |
| testdata/baselines/reference/submodule/compiler/declarationEmitDestructuringParameterProperties2.js | Baseline update showing emitted x/y/z properties in generated .d.ts. |
| testdata/baselines/reference/submodule/compiler/declarationEmitDestructuringParameterProperties.js.diff | Removes accepted-diff file since output now matches reference behavior. |
| testdata/baselines/reference/submodule/compiler/declarationEmitDestructuringParameterProperties2.js.diff | Removes accepted-diff file since output now matches reference behavior. |
| for _, elem := range pattern.Elements.Nodes { | ||
| if ast.IsOmittedExpression(elem) { | ||
| continue | ||
| } | ||
| if ast.IsBindingPattern(elem.Name()) { | ||
| elems = append(elems, tx.walkBindingPattern(elem.Name().AsBindingPattern(), param)...) | ||
| } | ||
| elems = append(elems, tx.Factory().NewPropertyDeclaration( | ||
| tx.ensureModifiers(param), | ||
| elem.Name(), | ||
| nil, /*questionOrExclamationToken*/ | ||
| tx.ensureType(elem, false), | ||
| nil, /*initializer*/ | ||
| )) |
There was a problem hiding this comment.
walkBindingPattern unconditionally calls ast.IsBindingPattern(elem.Name()) and then emits a PropertyDeclaration with elem.Name().
- In array binding patterns, the parser represents elisions (leading
,) asBindingElementnodes withname == nil(seeBindingElement.namecomment). In that caseelem.Name()is nil andast.IsBindingPattern(nil)will panic. - For nested destructuring (e.g.
{ a: { b } }),elem.Name()can be aBindingPattern. Emitting aPropertyDeclarationwith a binding pattern name will later panic in the printer (emitPropertyNameonly handles identifiers/literals/computed/private identifiers).
Consider: (1) skip elements with elem.Name() == nil, and (2) when elem.Name() is a binding pattern, recurse and continue (only emit a property when the binding name is an identifier/property-name node).
There was a problem hiding this comment.
I guess Copilot caught the same thing I called out but I didn't see the comment.
|
It's sort of debatable whether or not this is a good behavior or not. This is a type error we somehow still produce a decent declaration file for. |
| if ast.IsBindingPattern(elem.Name()) { | ||
| elems = append(elems, tx.walkBindingPattern(elem.Name().AsBindingPattern(), param)...) | ||
| } | ||
| elems = append(elems, tx.Factory().NewPropertyDeclaration( | ||
| tx.ensureModifiers(param), | ||
| elem.Name(), |
There was a problem hiding this comment.
There's a bug in the original code and the port - I assume the IsBindingPattern branch is supposed to continue. I'm pretty sure that this code will crash in the new compiler:
export class C {
constructor(public [[x]]: any[]) {}
}I've opened up microsoft/TypeScript#63153
There was a problem hiding this comment.
PR is at microsoft/TypeScript#63154
Pretty sure no test is required for this (we'll get one after we update the submodule), but basically we need a continue statement in the IsBindingPattern branch.
There was a problem hiding this comment.
@copilot Read microsoft/TypeScript#63154 and port it to this PR
There was a problem hiding this comment.
Added the continue statement in 598ba830. The walkBindingPattern now recurses into nested binding patterns and skips emitting a PropertyDeclaration for the pattern itself, matching the fix in TS#63154.
When walkBindingPattern encounters a nested binding pattern, it should recurse and continue to the next element, not fall through to also emit a PropertyDeclaration with the binding pattern as a name (which would crash in the printer). Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
DanielRosenwasser
deleted the
copilot/fix-transform-crash-destructured-property
branch
…estructured parameter properties (#2751) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
walkBindingPatternin declarations/transform.go for declaration emit of destructured parameter properties.js.difffiles eliminated (baselines now match TypeScript reference)continueafter recursing into nested binding patterns to prevent crash with e.g.constructor(public [[x]]: any[])✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.