Instantiate this when used only in type parameter constraints #19655
Conversation
Previously, when checking for this references, the compiler did not check type parameters. This caused it to miss some instantiations that were necessary. Also some cleanup: 1. Rename isIndependent* to is*FreeOfThisReference. 'Independent' was a one-off concept that was unique to Typescript (and isn't referenced in the spec), so I think the new name is friendlier to new readers. 2. Use `Array.every` whenever possible, since Typescript added a dependency on ES5 since the code was first written. 3. Switch to JSDoc so that it shows up nicely in editors.
Make sure that `this` gets instantiated when it's used as a constraint of a type parameter, and nowhere else in a signature.
|
|
||
| class Test extends Base { | ||
| m() { | ||
| this.check(this); |
There was a problem hiding this comment.
previously the second this reference was not assignable to prop: TProp.
src/compiler/checker.ts
Outdated
| * to "this" in its body, if all base types are interfaces, | ||
| * and if none of the base interfaces have a "this" type. | ||
| */ | ||
| function isInterfaceFreeOfThisReference(symbol: Symbol): boolean { |
There was a problem hiding this comment.
Or name it isInterfaceWithThisReference and don't negate it.
function isInterfaceWithThisReference(symbol: Symbol): boolean {
return some(symbol.declarations, declaration =>
isInterfaceDeclaration(declaration) && (
!!(declaration.flags & NodeFlags.ContainsThis)
|| some(getInterfaceBaseTypeNodes(declaration), baseTypeReferencesThis)));
}
function baseTypeReferencesThis({ expression }: ExpressionWithTypeArguments): boolean {
if (!isEntityNameExpression(expression)) {
return false;
}
const baseSymbol = resolveEntityName(expression, SymbolFlags.Type, /*ignoreErrors*/ true);
return !baseSymbol || !(baseSymbol.flags & SymbolFlags.Interface) || !!getDeclaredTypeOfClassOrInterface(baseSymbol).thisType;
}
src/compiler/checker.ts
Outdated
| * literal type, an array with an element type that is free of this references, or a type reference that is | ||
| * free of this references. | ||
| */ | ||
| function isTypeFreeOfThisReference(node: TypeNode): boolean { |
There was a problem hiding this comment.
I would negate all of these functions to make them xHasThisReference.
There was a problem hiding this comment.
I'll try that in a separate PR. I'm afraid that if I introduce any bugs with further refactoring they'll only show up as performance regressions, and I want to isolate that from any required performance regressions that result from the additional instantiations here.
src/compiler/checker.ts
Outdated
| case SyntaxKind.TypeReference: | ||
| return isIndependentTypeReference(<TypeReferenceNode>node); | ||
| return isTypeReferenceFreeOfThisReference(<TypeReferenceNode>node); |
There was a problem hiding this comment.
return some((node as TypeReferenceNode).typeArguments, typeHasThisReference);, doesn't really deserve its own function.
src/compiler/checker.ts
Outdated
| case SyntaxKind.TypeReference: | ||
| return isIndependentTypeReference(<TypeReferenceNode>node); | ||
| return isTypeReferenceFreeOfThisReference(<TypeReferenceNode>node); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
So all other types are presumed guilty? Could you try for a complete switch and Debug.fail() in the default case?
There was a problem hiding this comment.
In a word, yes. I'm doing Definitely Typed duty this week, so I ran tests on the assert until they all passed. I had to add all the type kinds except for MappedType.
There may be some efficiency we could pick up here by inspecting more types — union and intersection especially — but I think it's fine to have the default case fall through to a return false.
ghost
deleted the
ghost
mentioned this pull request
Check function type parameters for this references. Previously, when checking for this references, the compiler did not check type parameters. This caused it to miss some instantiations that were necessary.
Also some cleanup:
Array.everywhenever possible, since Typescript added a dependency on ES5 since the code was first written.Fixes #16930