-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add tasty.reflect.Kernel #5990
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
Add tasty.reflect.Kernel #5990
Conversation
c40f573
to
ec848ea
Compare
3663724
to
f127535
Compare
be6396e
to
8f04ba5
Compare
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. Well done 👍
This is a nice step towards the split between user-facing contracts & compiler-facing contracts. Some comments for further improvements (for later PRs, this PR is already huge):
-
Better naming scheme. Names are always a headache for programming, currently the method name
def ClassSymbol_companionClass
is not nice. -
Kernel
is now the interface between macro system & compiler, thus safe construction of trees should not be a design goal forKernel
, as the APIs are not used by programmers . A worthwhile design goal would be to minimize the contract & defend against compiler changes. For example,Kernel
does not need to define the same set of abstract types asCore
. In terms of minimizing interface, currently it's not ideal:
def Tree_pos(self: Tree)(implicit ctx: Context): Position
def Term_pos(self: Term)(implicit ctx: Context): Position
def Pattern_pos(self: Pattern)(implicit ctx: Context): Position
def TypeTree_pos(self: TypeTree)(implicit ctx: Context): Position
def Id_pos(self: Id)(implicit ctx: Context): Position
In the design of Kernel
, can we assume that Scala2 is going to implement it as well some day? That may help with the design.
@@ -116,48 +116,50 @@ package scala.tasty.reflect | |||
*/ | |||
trait Core { |
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 better to find another name for Core
, to avoid conflicts with Kernel
.
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 about ReflectionTypes
?
fa232e0
to
80028d9
Compare
Rebased |
This PR reduces compile time for the Dotty project. |
The idea is to move all logic from the TASTy reflect API that needs to be implemented by the compiler to the kernel. This will allow us to decouple the user-facing API from the compiler implementation.