Emit JS this properties in declarations matching tsc's serializePropertySymbol#4396
Emit JS this properties in declarations matching tsc's serializePropertySymbol#4396Copilot wants to merge 10 commits into
this properties in declarations matching tsc's serializePropertySymbol#4396Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
this properties in declarations
There was a problem hiding this comment.
Pull request overview
This PR adjusts declaration emit so that this.<prop> = ... assignments inside JavaScript class constructors can synthesize class members in the emitted .d.ts even when a base class already declares the same property, aligning behavior with tsc for React-style this.state = ... initialization.
Changes:
- Update declaration transformer logic to not skip base-property
thisassignments when they occur in a constructor. - Add a compiler regression test covering parallel
.jsand.tsinputs for the same pattern. - Add new reference baselines (
.js,.types,.symbols) validating the emitted declaration differences.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/transformers/declarations/transform.go |
Allows constructor this assignments to synthesize properties even if the base type already declares the property. |
testdata/tests/cases/compiler/jsDeclarationEmitThisAssignmentWithBaseProperty.ts |
New regression test covering JS-vs-TS behavior for constructor this.state assignment with a base state property. |
testdata/baselines/reference/compiler/jsDeclarationEmitThisAssignmentWithBaseProperty.js |
New output + declaration baseline validating emitted state for JS C2 but not TS C2. |
testdata/baselines/reference/compiler/jsDeclarationEmitThisAssignmentWithBaseProperty.types |
New types baseline validating inferred type flow for the assignment pattern. |
testdata/baselines/reference/compiler/jsDeclarationEmitThisAssignmentWithBaseProperty.symbols |
New symbols baseline validating symbol ownership differences between JS and TS scenarios. |
| decls := tx.resolver.GetBaseDeclarationsForPropertyDeclaration(node) | ||
| if len(decls) > 0 { | ||
| break caseBlock // property lightly overrides a property in a base type - skip it | ||
| if len(decls) > 0 && !ast.IsConstructorDeclaration(thisContainer) { | ||
| break caseBlock // skip non-constructor assignments to base properties |
There was a problem hiding this comment.
Now skip when any base declaration is an accessor, even in constructors. Fixed in 9bf4b46.
| export class C2 extends Component { | ||
| constructor() { | ||
| super({}); | ||
| this.state = { count: 0 }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Added a non-constructor (method) assignment case (C3), plus a constructor-to-accessor case (C4), both verified skipped in the baselines. Fixed in 9bf4b46.
Even in a constructor, assigning to an inherited accessor via `this.prop = ...` is a setter call at runtime, so synthesizing a data property override can produce an invalid .d.ts. Continue skipping when any base declaration is an accessor. Also extend the regression test with non-constructor and accessor cases. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Improves readability of the compound condition guarding declaration emit for this-property assignments. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@copilot you need to fix the |
GetBaseDeclarationsForPropertyDeclaration only returned the first extends base type's declarations and ignored implements clauses. Collect property declarations from all base types and implements clause types so declaration emit of JS this-property assignments accounts for implemented members. Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
|
| } | ||
|
|
||
|
|
||
| //// [DtsFileErrors] |
There was a problem hiding this comment.
@copilot you've introduced a dts file error, which means something is wrong.
There was a problem hiding this comment.
Fixed in 5304bef. The invalid .d.ts came from feeding implements-clause property declarations into the override-compat skip logic: C5's value is an accessor on the implemented WithAccessor, so it got skipped, leaving C5 missing a required member (TS2720). tsc always emits implements-clause properties and only skips writes to extends base members (inherited accessors / non-constructor writes). I reverted the base-declaration lookup to extends base types only, so C5 now emits value: number; with no dts error, and the C2/C3/C4 cases are unchanged.
|
@copilot actually, go back and just remove the TODO - classes need to redeclare interface-inherited members anyway. |
| decls := tx.resolver.GetBaseDeclarationsForPropertyDeclaration(node) | ||
| if len(decls) > 0 { | ||
| break caseBlock // property lightly overrides a property in a base type - skip it | ||
| shouldSkipAssignment := len(decls) > 0 && (!ast.IsConstructorDeclaration(thisContainer) || core.Some(decls, ast.IsAccessor)) |
There was a problem hiding this comment.
I think this does need to include skipping inherited methods, too, though - this-expando props have the SymbolFlagsReplaceableByMethod tag, so a this.method = this.method.bind({}) in a subclass shouldn't override the method definition in the superclass. Skipping accessors is more of a necessity, since the symbol merges with the accessor and just... does weird stuff (and at runtime it just uses the accessor, ofc).
There was a problem hiding this comment.
I also don't think skipping expandos outside the constructor is at-all correct; we recognize this. assignments as adding js expando props in any method.
There was a problem hiding this comment.
The exact condition we used in strada (see serializePropertySymbol in checker.ts) was
if (
p.flags & SymbolFlags.Prototype || p.escapedName === "constructor" ||
(baseType && getPropertyOfType(baseType, p.escapedName)
&& isReadonlySymbol(getPropertyOfType(baseType, p.escapedName)!) === isReadonlySymbol(p)
&& (p.flags & SymbolFlags.Optional) === (getPropertyOfType(baseType, p.escapedName)!.flags & SymbolFlags.Optional)
&& isTypeIdenticalTo(getTypeOfSymbol(p), getTypeOfPropertyOfType(baseType, p.escapedName)!))
) {
return [];
}which is to say, skip prototype, skip constructor, and skip properties identical to those in the base type.
If we don't wanna have to come back to this again - we probably wanna replicate those conditions here.
There was a problem hiding this comment.
@copilot adjust the condition to match the above, rather than looking at the declaration kinds of the symbol or thisContainer to try and approximate the same thing. You can and should remove GetBaseDeclarationsForPropertyDeclaration and replace it with a new resolver method - IsPropertyIdenticalToBaseTypeProperty that implements the above logic.
There was a problem hiding this comment.
Reworked in 2559767 to replicate the strada serializePropertySymbol conditions in IsThisPropertyAssignmentDeclarationRedundant:
- No longer gated on the constructor —
this.assignments in any method are treated as expando props. - Skips when the inherited member is identical (same readonly-ness, optionality, and identical type).
- Always skips inherited accessors and inherited methods/functions (so
this.method = this.method.bind(this)no longer overrides the base method). - Only
extendsbase types are considered.
Added regression cases: C3 (method assignment, skipped via type identity) and C6 (method rebind, skipped via the method check). I kept the accessor/method overrides as skips rather than implementing the accessor-emit "bonus" branch.
Skip decisions for JS constructor `this` assignments are based only on extends base declarations; implements-clause props are always emitted, as tsc does. Reverting the implements collection fixes the invalid C5 .d.ts. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
Done in 2559767. Removed the TODO/ |
this properties in declarationsthis properties in declarations matching tsc's serializePropertySymbol
Declaration emit skipped JS
this-assigned properties when a base class already declared the same property. This made React-stylethis.state = ...initialization disappear from.jsdeclaration output, whiletscstill emits it.Declaration emit
this.<prop> = ...expando assignments now mirrors the checker'sserializePropertySymbol(EmitResolver.IsThisPropertyAssignmentDeclarationRedundant), rather than a declaration-based heuristic gated on the constructor.extendsbase member:this-expando props carry theReplaceableByMethodcontract, sothis.method = this.method.bind(this)must not override the base method).this.assignments add JS expando props in any method, and are emitted/skipped purely on the conditions above.extendsbase types are considered.implements-clause properties are always emitted (matchingtsc), since they are not inherited and the class must (re)declare them.Regression coverage
.jsand.tsinputs.state(member type differs from the base), while the TS constructor assignment remains omitted.this.method = this.method.bind(this)) to a base method (skipped).implementsclause type, which is emitted (not skipped) and produces no declaration error.Now emits in
.jsdeclaration output: