Walk class children to find this property assignments beyond top level#4264
Open
weswigham wants to merge 1 commit into
Open
Walk class children to find this property assignments beyond top level#4264weswigham wants to merge 1 commit into
weswigham wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a behavioral gap in JS-targeted declaration emit by walking deeper into class member bodies to discover this.<prop> = ... assignments (including those nested in arrow-function callbacks like new Promise((resolve) => { ... })). This brings tsgo’s emitted .d.ts closer to upstream TypeScript behavior for inferred, undeclared JS class members (Fixes #4206).
Changes:
- Add a dedicated AST walk (visitor) to collect
this-property assignments beyond top-level statements in constructors/methods/static blocks. - Add
EmitResolver.GetBaseDeclarationsForPropertyDeclarationto detect/avoid emitting properties that would “lightly override” a base member. - Update affected reference baselines to reflect newly emitted inferred class properties.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/transformers/declarations/transform.go |
Adds visitor-based collection of nested this.x = ... assignments and base-member override guard logic. |
internal/printer/emitresolver.go |
Extends EmitResolver interface with GetBaseDeclarationsForPropertyDeclaration. |
internal/checker/emitresolver.go |
Implements base-declaration lookup for a property symbol across base types. |
testdata/baselines/reference/** |
Updates baselines to include newly emitted inferred class properties in JS declaration emit scenarios. |
| GetEnumMemberValue(node *ast.Node) evaluator.Result | ||
| IsLateBound(node *ast.Node) bool | ||
| IsOptionalParameter(node *ast.Node) bool | ||
| GetBaseDeclarationsForPropertyDeclaration(ndoe *ast.Node) []*ast.Node |
Comment on lines
+1716
to
+1727
| // problem: this prop might be overriding a prop from a base type. The checker has special bails for override compat comparisons for binary expression properties, | ||
| // but what we transform to won't - so we either need to match the base type (for example, if it's a getter/setter) or emit nothing | ||
| // See `checkKindsOfPropertyMemberOverrides` in the checker for what we're trying to satisfy here | ||
| if thisTarget.ClassLikeData().HeritageClauses != nil && len(thisTarget.ClassLikeData().HeritageClauses.Nodes) > 0 && !isClassExtendingNull(thisTarget) { | ||
| // there is a base type any assignments might be "from" | ||
| tx.tracker.ReportInferenceFallback(thisTarget) // Add an isolated declarations error on this class - we can't know how to transform this prop into an assignment without referring to type information | ||
| decls := tx.resolver.GetBaseDeclarationsForPropertyDeclaration(node) | ||
| if len(decls) > 0 { | ||
| break caseBlock // property lightly overrides a property in a base type - skip it | ||
| // TODO: If the property has an explicit `@type` annotation, we should probably emit it (maybe with an `override` modifier) instead of skipping it | ||
| } | ||
| } |
Comment on lines
+1761
to
+1779
| func isClassExtendingNull(node *ast.Node) bool { | ||
| if node == nil { | ||
| return false | ||
| } | ||
| heritage := node.ClassLikeData().HeritageClauses | ||
| if heritage == nil { | ||
| return false | ||
| } | ||
| if len(heritage.Nodes) > 1 || len(heritage.Nodes) == 0 { | ||
| return false | ||
| } | ||
| for _, expA := range heritage.Nodes[0].AsHeritageClause().Types.Nodes { | ||
| expr := expA.AsExpressionWithTypeArguments().Expression | ||
| if expr != nil && expr.Kind == ast.KindNullKeyword { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
gabritto
reviewed
Jun 10, 2026
| tx.tracker.ReportInferenceFallback(thisTarget) // Add an isolated declarations error on this class - we can't know how to transform this prop into an assignment without referring to type information | ||
| decls := tx.resolver.GetBaseDeclarationsForPropertyDeclaration(node) | ||
| if len(decls) > 0 { | ||
| break caseBlock // property lightly overrides a property in a base type - skip it |
Member
There was a problem hiding this comment.
property lightly overrides a property in a base type is "lightly" a typo? I was confused
jakebailey
reviewed
Jun 10, 2026
| * @property {typeof import("./Test.js").default} [test] | ||
| */ | ||
| declare class X extends Test { | ||
| + test: import("./Test.js").default | undefined; |
Member
There was a problem hiding this comment.
Aren't we making this worse?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In JS-targeted declaration emit.
Fixes #4206