-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Instantiate this
in non-super property/element access expressions
#29468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instantiate this
in non-super property/element access expressions
#29468
Conversation
@ahejlsberg per your offline review, this now does some more convoluted things with making a this-type type reference and passing that around with a refined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions about globalThisType
.
@@ -6685,6 +6688,23 @@ namespace ts { | |||
} | |||
|
|||
function resolveTypeReferenceMembers(type: TypeReference): void { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra newline is extra
@@ -6631,6 +6631,9 @@ namespace ts { | |||
else if (type.flags & TypeFlags.Intersection) { | |||
return getIntersectionType(map((<IntersectionType>type).types, t => getTypeWithThisArgument(t, thisArgument, needApparentType))); | |||
} | |||
else if (type.flags & TypeFlags.TypeParameter && thisArgument && (type as TypeParameter).isThisType) { | |||
return getTypeWithThisArgument(createTypeReference(globalThisType, [type]), needApparentType ? instantiateType(thisArgument, createTypeMapper([type], [getApparentType(type)])) : thisArgument, needApparentType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can you format the conditional one more than one line? It's not readable as-is. (I would extract the middle argument to a temp.)
- Why create a reference to
globalThisType
?
@@ -6685,6 +6688,23 @@ namespace ts { | |||
} | |||
|
|||
function resolveTypeReferenceMembers(type: TypeReference): void { | |||
|
|||
if (type.target === globalThisType && type.typeArguments && type.typeArguments[0].flags & TypeFlags.TypeParameter && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't understand what globalThisType
is doing here (unless I'm completely mistaken and it's a marker type rather than the type of globalThis
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the ThisType<T>
marker type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix looks ok now that I have been reminded that globalThisType
is ThisType<T>
not typeof globalThis
. It's pretty confusing to reuse ThisType
like this, though, so I think you should think again whether the fix is worth it before merging.
@ahejlsberg, can you take a look at this PR since @weswigham changed the way it is implemented?
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
Fixes #28159