-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Split TASTy reflect interface and implementation #4905
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
Conversation
4ee0011
to
fa0793b
Compare
fa0793b
to
ed73ef5
Compare
0e8565a
to
5c2ad6b
Compare
cca03c2
to
19aab74
Compare
Mostly to simplify the maintainability and documentation of distinct parts of the implementation. Abstract types are defined separately in TastyCore to avoid cake pattern (except for Printers). Extracted the `show` extension methods to avoid cakes.
19aab74
to
eb5360b
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.
Otherwise, LGTM
* +- Constant | ||
* | ||
* ``` | ||
*/ |
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.
Nice diagram 👍
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.
Thanks :)
type DefDef <: Definition | ||
type ValDef <: Definition | ||
type PackageDef <: Definition | ||
type Term <: Statement with Parent |
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.
In the TODO, it's implied type Parent = Term | TypeTree
. How do we write the constraint for Term
after the change?
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.
Just type Term <: Statement
. The TODO referes to the other places where Parent
is used, mainly in the return type of extractors. I will clarify the coment.
|
||
type CaseDef | ||
|
||
type Pattern |
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.
Should we constrain Pattern
, CaseDef
by <: Tree
?
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 tempting but no. If we do so we would encounter ambiguities when using the extractor for Term.Ident
and Pattern.Value
, both have an underlyingtpd.Ident
and cannot be distinguished at runtime. CaseDef
could be a Tree
but there is no place where something can return a CaseDef
or some Tree
.
|
||
type ImportSelector | ||
|
||
type Id |
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's the difference between Ident
and Id
? A doc comment can be helpful here.
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.
Id
is an untyped Ident
and Id
does not provide the tpe
method. It is used to give a position to a name such as in imports.
@Blaisorblade could you have a quick look to make sure I did not lose anything while rebasing of the last PR. |
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.
@nicolasstucki Our fixes from #4904 seem to have gotten here fine :-) — that's what you meant I guess? LGTM!
Yes, that is what I meant. |
Mostly to simplify the maintainability and documentation of distinct parts of the implementation.
Abstract types are defined separately in TastyCore to avoid cake pattern (except for Printers).
Extracted the
show
extension methods to avoid cakes.