-
Notifications
You must be signed in to change notification settings - Fork 80
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
Hide internal implementation details in type objects #2103
Conversation
return InstanceKind.kRecordType; | ||
case RuntimeObjectKind.object: | ||
case RuntimeObjectKind.type: | ||
case RuntimeObjectKind.wrappedType: |
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.
What does RuntimeObjectKind.wrappedType
represent in this context? There might be a different name that causes less confusion.
I really dislike how DDC uses the term "WrappedType" because it always leads to confusion when discussing the types. We have an internal type that is used for all type operations. When we need to return a value from .runtimeType
calls we pass our internal type object to a method called wrapType()
that returns a Dart Type
object that wraps the internal representation. Then in various comments we refer to the wrapping Type
object as the "wrapped type". It continually confuses me because I also tend to think of the type inside as the "wrapped type" because it has a wrapper around it. Good news is that all of this is on the list to be deleted when we switch type systems.
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 is the dart._Type
object we get from DDC on x.runtimeType
calls. It has an _type
field with the actual DartType. Other runtime kinds (RuntimeObjectKind.type
and RuntimeObjectKind.recordType
) are for the internal types (RecordType
, class types, and primitive types objects). Would ExternalType
or TypeWrapper
work better than WrappedType
?
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.
Replaced by TypeWrapper
, to make the 'container' intention clearer.
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.
Actually, I realized I can optimize the type wrapper case away (by unrolling the recursion in JS code for getting metadata), and minimize the number of wrapType
calls. Updated.
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.
Generally LGTM, but I'll let a DWDS maintainer give final approval :-)
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.
LGTM with the minor changes from our discussion today.
In this PR:
core:Type
classBefore
After
VSCode (consistent with VM)
DevTools (consistent with VM)
Closes: #2086
Towards: #1949