fix(64058): fix reparse jsdoc @extends type arguments for call expressions - #64072
fix(64058): fix reparse jsdoc @extends type arguments for call expressions#64072Oleksandr Tarasiuk (a-tarasyuk) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes JSDoc @extends type arguments for call-expression heritage clauses.
Changes:
- Copies JSDoc type arguments onto the parsed heritage node.
- Adds parser and compiler regression coverage.
- Removes the previous property-access matching helper.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/parser/reparser.go |
Reparses @extends type arguments. |
tsc/internal/ast/utilities.go |
Removes the old name-matching helper. |
tsc/internal/parser/parser_test.go |
Tests reparsed AST linkage. |
tsc/testdata/tests/cases/conformance/jsdoc/extendsTag7.ts |
Adds the compiler regression case. |
tsc/testdata/baselines/reference/conformance/extendsTag7.types |
Records inferred types. |
tsc/testdata/baselines/reference/conformance/extendsTag7.symbols |
Records resolved symbols. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| target.TypeArguments = p.newNodeList(source.TypeArguments.Loc, newArguments) | ||
| p.finishMutatedNode(target.AsNode()) | ||
| if target.TypeArguments == nil && source.TypeArguments != nil { |
There was a problem hiding this comment.
I think this problem has now been restored; does it need to still check that the call target is the right name?
(I haven't double checked Strada behavior here)
There was a problem hiding this comment.
This is a bit of a tricky case :), because Strada mostly relies on the Effective resolution, and for call-expression heritage, it doesn't compare the @extends name with the call receiver.
It treats them separately, Playground:
A.extend()is checked as the runtime expression.Other<string>from@extendsis used as the effective base type.
So class B extends A.extend() with @extends {Other<string>} inherits from Other<string>, not A<string>, and no mismatch diagnostic is produced.
f199fb7 to
459b499
Compare
| func HasSamePropertyAccessName(target, source *Node) bool { | ||
| target = SkipParentheses(target) | ||
| source = SkipParentheses(source) | ||
| if IsCallExpression(target) { |
There was a problem hiding this comment.
This is kind of a heuristic, since you could write something as goofy as Foo<string> and have extends Foo.Bar() and then it'd parameterize Foo?
Not sure how I feel about it. I actually didn't know the original code was so syntactic...
There was a problem hiding this comment.
Maybe the actual fix needs to be in the reparser more specifically?
0c1088c to
a315a46
Compare
| target.TypeArguments = p.newNodeList(source.TypeArguments.Loc, newArguments) | ||
| p.finishMutatedNode(target.AsNode()) | ||
| if target.TypeArguments == nil && source.TypeArguments != nil { | ||
| if ast.IsCallExpression(ast.SkipParentheses(target.Expression)) || ast.HasSamePropertyAccessName(target.Expression, source.Expression) { |
There was a problem hiding this comment.
Strada handles this by using the @extends type as the effective base and checking the call expression separately; it doesn’t apply the JSDoc arguments to the call’s returned constructor.
function mixin(Base) {
return class extends Base {};
}
/** @extends {A<string>} */
class B extends mixin(A) {}This change appears to apply <string> to the non-generic returned class. Could we resolve the call first and only attach the arguments when its returned constructor is compatible with the @extends target?
There was a problem hiding this comment.
Just to make sure I understand correctly: for @extends {A<string>} with extends mixin(A), should the JSDoc reparser synthesize the AST equivalent of:
class B extends (mixin(A) as typeof A<string>) {}so that normal heritage checking treats A<string> as the base? Or should it preserve mixin(A) in the heritage AST and let the checker resolve the call, consult the @extends tag, and apply <string> only if the resolved constructor is compatible with A, otherwise keeping the anonymous constructor as the base?
Fixes #64058