Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

coreywoodfield
Copy link
Contributor

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 the extends StringEnum[Entry[?]]. In scala 3 this fails with unreducible 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 with Anys. But then findValues 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 to Anys 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 checking Entry[String] <:< Entry[Any] it just checks Entry <:< 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 the findValues 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 updated collect to use a TreeAccumulator to be more flexible.

coreywoodfield and others added 4 commits July 19, 2023 12:26
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.
@lloydmeta
Copy link
Owner

Thanks for putting this up.

I'm not entirely opposed to this, even with the cast. I just have a couple of clarifying questions:

  • Can you give a real world example where something like this is needed?
  • Can you give a concrete example where relaxing the type check would cause issues with safety?
  • In terms of enabling the less-safe behaviour via a flag, what did you have in mind?

@codecov-commenter
Copy link

Codecov Report

Merging #374 (0496db6) into master (74a7b36) will not change coverage.
The diff coverage is n/a.

❗ 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           

@coreywoodfield
Copy link
Contributor Author

  • It's useful for being able to enumerate types and distinguish between them when serialized. Each enum entry corresponds to its companion which holds data of type A. You parse the enum first and then you know what type the data is and can parse the data
  • Suppose the AbstractEnumEntry had an apply(thing: A): Comp method. Then if findValues were of type IndexedSeq[AbstractEnumEntry[Any]], you could do something like values.map(entry => entry("I am a string")) which would fail
  • I was thinking just something like findValues(higherKindedTypes = true) or findHigherKindedTypeValues

@lloydmeta
Copy link
Owner

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.

@coreywoodfield
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants