lloydmeta / enumeratum

A type-safe, reflection-free, powerful enumeration implementation for Scala with exhaustive pattern match warnings and helpful integrations.
MIT License
1.18k stars 146 forks source link

Higher kinded type support #374

Open coreywoodfield opened 11 months ago

coreywoodfield commented 11 months ago

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.

lloydmeta commented 11 months ago

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-commenter commented 11 months ago

Codecov Report

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

:exclamation: 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 commented 11 months ago
lloydmeta commented 11 months ago

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 commented 11 months ago

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