Fix decorator design:types emit for type variables.#25190
Conversation
|
Let me know if this is remotely reasonable, or feel free to just take the tests and fix based on them. /CC @weswigham touched the code last, I think. |
7322ffa to
f817a9d
Compare
Previously, TypeScript would resolve the reified types for the
`design:types` decorator emit in the regular `currentScope`. That scope
does not include class declaration bodies.
However when reifying types, class declarations do introduce a new scope
for any `TypeVariable`s declared on them. Because TS resolved the
EntityName for such types against the parent scope (e.g. the source
file), not the class scope, TypeScript would either fail to resolve the type (giving `TypeReferenceSerializationKind.Unknown`), or
incorrectly resolve to a different, accidentally matching symbol in the outer scope (giving `TypeWithConstructSignatureAndValue`).
This would result in an emit referencing an undeclared symbol, or
mis-referencing the wrong symbol.
__metadata("design:type", typeof (_a = typeof TypeVariable !== "undefined" && TypeVariable) === "function" && _a || Object)
__metadata("design:type", TypeVariable)
This change special cases `currentScope` for
`serializeTypeReferenceNode` to use a class scope, if present. This
changes the emit for a `TypeVariable` back to `Object`:
__metadata("design:type", Object)
f817a9d to
55c3ec3
Compare
| // node might be a reference to type variable, which can be scoped to a class declaration, in addition to the regular | ||
| // TypeScript scopes. Walk up the AST to find the next class and use that as the lookup scope. | ||
| let scope: Node = node; | ||
| while (scope.parent && scope !== currentScope) { |
There was a problem hiding this comment.
We shouldn't be using .parent in a transform, as a before transform can unset parent pointers (usually by replacing nodes). The correct scope should be captured, like currentScope, as the transform descends through the tree.
There was a problem hiding this comment.
Given how currentScope is used, I think it is acceptable simply to add the class body to the list of things captured by the transform as a possible currentScope, I believe, since it is just used for resolving names (like for this serialization) and checking if we're at the top level of a file.
There was a problem hiding this comment.
That was my first idea, too. That does however break variable merging in emit, i.e. it adds in a duplicated var declaration:
class C {
constructor() {
this.foo = 10;
}
}
C.bar = 20;
var C; // <-- ADDED
(function (C) {
C.x = 10;
})(C || (C = {}));
I tried debugging, and got lost somewhere in merge declaration markers. plz send help ;-)
|
@mprobst a few failing baselines that need updating. |
|
@mhegazy I think my change is incorrect, and the baselines should not be updated. Problem is I don't know how to fix correctly, I don't understand the merge declaration markers. |
|
@mprobst I pushed a change to solve those problems, hope you don't mind! |
|
Thanks @weswigham. As I said, feel free to take the tests and do whatever works :-) |
|
thanks! |
Fix decorator design:types emit for type variables.
Previously, TypeScript would resolve the reified types for the
design:typesdecorator emit in the regularcurrentScope. That scopedoes not include class declaration bodies.
However when reifying types, class declarations do introduce a new scope
for any
TypeVariables declared on them. Because TS resolved theEntityName for such types against the parent scope (e.g. the source
file), not the class scope, TypeScript would either fail to resolve the type (giving
TypeReferenceSerializationKind.Unknown), orincorrectly resolve to a different, accidentally matching symbol in the outer scope (giving
TypeWithConstructSignatureAndValue).This would result in an emit referencing an undeclared symbol, or
mis-referencing the wrong symbol.
This change special cases
currentScopeforserializeTypeReferenceNodeto use a class scope, if present. Thischanges the emit for a
TypeVariableback toObject: