Mark type arguments as used even if used in an invalid way#15557
Conversation
ghost
assigned
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Just one question about performance.
| } | ||
|
|
||
| function callLikeExpressionMayHaveTypeArguments(node: CallLikeExpression): node is CallExpression | NewExpression { | ||
| return node.kind === SyntaxKind.CallExpression || node.kind === SyntaxKind.NewExpression; |
There was a problem hiding this comment.
there no predicate like this utilities or wherever they live? I'm kind of surprised there's not a isCallLikeLikeExpression or something.
There was a problem hiding this comment.
I couldn't find one, but I've added one now. Still want to keep isCallOrNewExpression separate from callLikeExpressionMayHaveTypeArgumets since those may not always be the same thing (#11947).
| } | ||
|
|
||
| function getTypeReferenceType(node: TypeReferenceNode | ExpressionWithTypeArguments | JSDocTypeReference, symbol: Symbol) { | ||
| const typeArguments = typeArgumentsFromTypeReferenceNode(node); // Do unconditionally so we mark type arguments as referenced. |
There was a problem hiding this comment.
Are you sure it's not cheap enough to just call typeArgumentsFromTypeReferenceNode multiple times and avoid the argument-threading churn below?
There was a problem hiding this comment.
Depending on what's in the arguments, this could presumably do an arbitrary amount of work.
There was a problem hiding this comment.
getTypeFromTypeNode dispatches to a bunch of functions that are all cached, so the only danger is an arbitrarily long list of type arguments. Either way is fine with me, but I'd prefer to have some measurement that says the duplicate way is measurably slower.
There was a problem hiding this comment.
Found some duplicate code, so we call typeArgumentsFromTypeReferenceNode in fewer places now.
(The two parts of duplicate code were even added in the same commit: e223b2e)
As for performance, here are my measurements on a sample script:
{
{ type T = X<A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P>; }
{ type T = X<A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P>; }
... 1000 times ...
}
Tested with node ..\..\TypeScript\built\local\tsc.js --noEmit --diagnostics --noLib, and shimmed lib with e.g. interface Array {} so it would compile.
This PR:
Memory Used: 30702K 30379K 30134K 30140K 30114K (average: 30293.8K)
Check Time: 0.11s 0.11s 0.10s 0.10s 0.10s (average: 0.104s)
This PR with 1000 extra calls to typeArgumentsFromTypeReferenceNode:
Memory Used: 37329K 37266K 37830K 37825K 37247K (average: 37499.4K)
Check Time: 0.85s 0.83s 0.84s 0.84s 0.83s (average: 0.838s)
The memory difference is 7205K, a 0.02% difference per iteration.
The time difference is 0.734s, a 0.7% difference per iteration.
ghost
merged commit 
2 participants
Fixes #14953
Previously we would only check type arguments if there were matching type parameters. Now we always check type arguments. This gives additional diagnostics and makes sure they are marked as used.