Fix crash when this is typed as a generic T with no type constraints#47991
Fix crash when this is typed as a generic T with no type constraints#47991sandersn merged 6 commits intomicrosoft:mainfrom
this is typed as a generic T with no type constraints#47991Conversation
T as assigned to this with no constraintsthis is typed as a generic T with no type constraints
This comment was marked as resolved.
This comment was marked as resolved.
|
Nevermind, found the issues talking about private's behavior and protected's behavior in relation to |
176a44a to
87ad30b
Compare
sandersn
left a comment
There was a problem hiding this comment.
I think the basic fixes are good. Just some minor style requests.
| let enclosingClass = forEachEnclosingClass(location, enclosingDeclaration => { | ||
| const enclosingClass = getDeclaredTypeOfSymbol(getSymbolOfNode(enclosingDeclaration)!) as InterfaceType; | ||
| return isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing) ? enclosingClass : undefined; | ||
| return isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing); |
There was a problem hiding this comment.
This looks like an unrelated driveby cleanup, not part of either fix. Is that correct?
There was a problem hiding this comment.
correct just a cleanup, shouldn't change the behavior at all.
src/compiler/checker.ts
Outdated
| return undefined; | ||
| } | ||
|
|
||
| function getThisTypeFromNodeContext(node: Node) { |
There was a problem hiding this comment.
I think I would inline this since it's only two lines and only called once.
src/compiler/checker.ts
Outdated
| if ( | ||
| flags & ModifierFlags.Static | ||
| || !(enclosingClass = getEnclosingClassFromThisParameter(location)) | ||
| || !(enclosingClass = isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing)) | ||
| ) { |
There was a problem hiding this comment.
If it's OK to run the checks even for static methods (and I think it is), then the below is much less surprising to read:
| if ( | |
| flags & ModifierFlags.Static | |
| || !(enclosingClass = getEnclosingClassFromThisParameter(location)) | |
| || !(enclosingClass = isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing)) | |
| ) { | |
| enclosingClass = getEnclosingClassFromThisParameter(location)) || isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing); | |
| if (flags & ModifierFlags.Static || !enclosingClass) { |
There was a problem hiding this comment.
Yeah should be ok, at best it was just a very minor optimization to avoid fetching this's parameter if the field was already static.
There was a problem hiding this comment.
getEnclosingClassFromThisParameter can return undefined while isClassDerivedFromDeclaringClasses doesn't accept undefined, so I'll have to do some type checking with this style instead:
enclosingClass = getEnclosingClassFromThisParameter(location);
enclosingClass = enclosingClass && isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing);
if (flags & ModifierFlags.Static || !enclosingClass) {There was a problem hiding this comment.
Also noticed that the name isClassDerivedFromDeclaringClasses may be a bit misleading, as it doesn't return a boolean, but instead it returns the first parameter if checks succeeds, otherwise it returns undefined.
Not sure if we'd want to change the name or not.
There was a problem hiding this comment.
Maybe as a follow-up, I'd rather get this change in.
src/compiler/checker.ts
Outdated
|
|
||
| function getEnclosingClassFromThisParameter(node: Node): InterfaceType | undefined { | ||
| let thisType = getThisTypeFromNodeContext(node); | ||
|
|
There was a problem hiding this comment.
extremely minor style preference: I think the blank lines in this function should be deleted.
Fixes #47965
While checking a protected property's accessibility, the crash occurs because
getConstraintOfTypeParametercan returnundefinedwhenthisis typed as the genericTandThas no type constraints on it. The fix for this was to check forundefinedand refuse access if it was, as we can't guarantee that an instantiation of a genericTcan access protected properties whenTisn't constrained to any class/type.Additionally, the explicit
thistype was also skipping through derivation checks done byisClassDerivedFromDeclaringClasses, which led to some inconsistent behavior shown below, so changes were also made to address this, but please do correct me if I'm mistaken.