-
Notifications
You must be signed in to change notification settings - Fork 150
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
Higher kinded type support #374
base: master
Are you sure you want to change the base?
Higher kinded type support #374
Conversation
Some code of the form `extends StringEnum[Entry[?]]` that worked in scala 2 no longer works in scala 3, as existential types were removed. The simplest way to get the types to work is to change the `?` to `Any`. But then `findValues` can't process all the enum entries because they don't extend `Entry[Any]`, they extend `Entry[X]` for some type `X`. The macro was previously quite strict about types. It would get the `MirroredElemTypes` from a `Mirror.SumOf[A]`, and then while processing the types, it would check each element to make sure it extended `A`. But the elements are from the mirror, which just has the things that extend `A`, so of course they extend `A`. It would also check while looking for the constructor to pull the value from that the constructor type extended `A`. I updated this check to unwrap both types if either was an `AppliedType`. I.e. instead of checking that `Entry[Int] <:< Entry[Any]` it checks that `Entry <:< Entry`. This is a slight change from the scala 2 macro behavior, but it allows higher kinded type parameters to be used in enums without needing existential types.
Thanks for putting this up. I'm not entirely opposed to this, even with the cast. I just have a couple of clarifying questions:
|
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #374 +/- ##
=======================================
Coverage 85.52% 85.52%
=======================================
Files 65 65
Lines 518 518
Branches 31 31
=======================================
Hits 443 443
Misses 75 75 |
|
Thanks; any chance you can give a concrete example of point 1 that couldn't be done with what's there today? I'm having some trouble visualising/understanding if the unsoundness is necessitated by the use case or the encoding itself. |
I can't figure out a good way to do 1 that compiles in scala 3 and has the entry and companion knowing each others types that works with what's there today |
Preface: I'm not sure if this is a change you would actually want. If you don't, that's fine, we can just use our fork.
Preface 2: This needs the changes from #373 and would conflict with #371, so I based the branch off of those two. The only new changes here are the most recent commit.
Basically, in scala 2 you could have an enum that had a higher kinded type where you could have
?
as the type parameter in theextends StringEnum[Entry[?]]
. In scala 3 this fails withunreducible application of higher-kinded type Enum to wildcard arguments
. The simplest fix to get the type system to accept this is to replace the?
s withAny
s. But thenfindValues
can't find the instances because they're not of the expected type. (See examples in the added tests. Note that the only difference is that two?
s in the scala 2 test were changed toAny
s in the scala 3 test, as it doesn't compile otherwise).I very slightly relaxed the type checking in
findValues
so that it could find instances even if they don't strictly extend the expected type because of variance. (If either type has a type parameter, the type parameters are dropped, i.e. instead of checkingEntry[String] <:< Entry[Any]
it just checksEntry <:< Entry
). I put more details in the commit message. This relaxation also necessitated adding a cast. Since the cast is only necessary if one or both types have type parameters, and type parameters are erased at runtime, the cast itself should be safe. It could allow unsafe things to be done with the individual values, but I'd hope that anyone who has a situation complicated enough where that would be the case would understand the underlying principles well enough to be able to avoid that. (Maybe we add a flag that enables the behavior and defaults to false? I would've gone ahead and just done it, but there are a few levels of separation between the behavior itself and thefindValues
signatures so I wanted to hear thoughts first)In the case I was looking at, the constructor was nested one layer deeper in an
Apply
call, so I also updatedcollect
to use aTreeAccumulator
to be more flexible.