-
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
showextension methods to avoid cakes.