Skip to content

Update Tasty Reflect match type code #10690

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Update Tasty Reflect match type code #10690

wants to merge 1 commit into from

Conversation

abgruszecki
Copy link
Contributor

No description provided.

@abgruszecki abgruszecki mentioned this pull request Dec 7, 2020
Closed
nicolasstucki
nicolasstucki approved these changes Dec 8, 2020
View reviewed changes
scala3doc/src/dotty/dokka/tasty/SyntheticSupport.scala
object MatchTypeCase:
def unapply(tpe: TypeRepr): Option[(TypeRepr, TypeRepr)] =
tpe match
case AppliedType(t, Seq(from, to)) /*if t == MatchCaseType*/ =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these reflect extractor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was code that we used in the doctool to take apart match types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indicates that MatchType are already fully supported. It is just a matter of knowing what needs to go there. This will need some documentation.

@abgruszecki
Copy link
Contributor Author

abgruszecki commented Dec 8, 2020 •
edited
Loading

@nicolasstucki I see that in tests we actually have code that created match types by accessing the internal MatchCase constructor and manually creating TypeLambdas. If that was acceptable before, then I can write an analogous API for creaing MatchCase-s and MatchType-s from MatchCase-s, without requiring the user to understand internals of how match types are represented.

nicolasstucki
nicolasstucki reviewed Dec 8, 2020
View reviewed changes
scala3doc/src/dotty/dokka/tasty/SyntheticSupport.scala
object MatchTypeCase:
def unapply(tpe: TypeRepr): Option[(TypeRepr, TypeRepr)] =
tpe match
case AppliedType(t, Seq(from, to)) /*if t == MatchCaseType*/ =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indicates that MatchType are already fully supported. It is just a matter of knowing what needs to go there. This will need some documentation.

library/src/scala/quoted/Quotes.scala
end extension
end MatchTypeMethods

type MatchTypeCase <: TypeRepr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the abstractions aligned with what is in TASTy and not add new ones.

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala Outdated
type MatchTypeCase = dotc.core.Types.Type

object MatchTypeCase extends MatchTypeCaseModule:
def unapply(x: MatchTypeCase): Option[(List[String], List[TypeBounds], TypeRepr, TypeRepr)] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can implement this in userspace, this should not be added here. It is unclear what the ideal API should be, it would be better to implement such custom extractors in a separate library.

@nicolasstucki
Copy link
Contributor

I do not think we should do these changes. We should keep it as it is.

@abgruszecki abgruszecki force-pushed the reflect-match-type-case branch 2 times, most recently from 6a481a9 to d43c5a2 Compare December 8, 2020 16:23
@abgruszecki abgruszecki force-pushed the reflect-match-type-case branch from d43c5a2 to 20ac708 Compare December 8, 2020 18:41
@abgruszecki
Copy link
Contributor Author

@nicolasstucki can take a look at the PR now? This is a full version of the API. I think we need to have a type for a case of a match type, it's the same thing as with summonFrom - morally it ought to be a separate thing, but in reality it's not. I also think we should expose ways for constructing and destructuring match types and match type cases without forcing people to understand their internal structure. We can document them, sure, though code is far better than documentation, especially in cases like this one. And if we have code around the internal structure, it will be much easier to change it in the future. Say, if we wanted to modify the binding structure of match type cases, we'd have issues because all macros creating/destructing them have hardcoded said binding structure. With this PR, that's not the case.

@nicolasstucki nicolasstucki mentioned this pull request Dec 10, 2020
Merged
@nicolasstucki
Copy link
Contributor

Replaced with #10735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abgruszecki @nicolasstucki