Allow finding values inheriting from a generic intermediary subtype

[Krever]

Aims to solve #424.

Original behavior: search among subclasses only of the enum type
New behavior: search among all subclasses, taking only those inheriting from the enum type

I’ve put NestedGenericEnum inside the test because I think it's more obvious/readable that way but I'm totally fine to move it into Models/scala to keep consistency.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.71%. Comparing base (c76f948) to head (6ad0df6).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #426   +/-   ##
=======================================
  Coverage   85.71%   85.71%           
=======================================
  Files          63       63           
  Lines         511      511           
  Branches       34       34           
=======================================
  Hits          438      438           
  Misses         73       73           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Krever]

I found a serious regression when tried to use the snapshot in my codebase. I added handling and a test case for it, but please let me know if that's enough. I was a bit frightened when the existing tests didn't catch it.

[lloydmeta]

Interesting...

I feel like this likely means we should have some logic/rationale for what subclasses get picked up or not picked up based on their type param.

[Krever]

Personally I wouldn't make it more complicated than it has to be. Fundamentally we need to

1. collect all subtypes of the enum type - already covered
2. make sure we don't report anything twice - ensured through added distinct

The only concern I have is that someone simultaneously removes the distinct and either removes a test I added or finds a different way to traverse the same object twice. But I don't see a way to prevent that.

While we could make traversal more defensive, I feel like it would only complicate the implementation without a real benefit. And the more general the traversal logic is, the less likely it is we will have other similar edge cases.

That being said, I'm just a guest here so I will try to follow whatever suggestions you might have.

[Krever]

Hey @lloydmeta, I just finished migrating my codebase, and this is the last blocker for me. Do you think it could be merged and released anytime soon?

[lloydmeta]

Yes, I still think we should have good reason other than "it's complex" for what members get picked up based on the type param.

[Krever]

Sure, but what would be the outline of the filtering logic? For me it seems that there is no inherent limitation to what members are being traversed, as long as the final leaf is a subtype of the enum type. If you have some idea for a stricter filtering, I can try to implement it.

[lloydmeta]

I'm thinking about the user perspective first; eg covariance? Self base trait? Etc. before worrying about implementation.

[Krever]

So you're considering scenarios where the user defines an enum subtype inside the enum object but doesn't expect it to be picked up by the macro?

From a variance perspective, I don't see a problem, I can imagine scenarios with either covariant, contravariant or invariant type parameter.
Regarding the self-trait, I would probably need a more elaborate explanation.

But in the meantime, maybe from a different perspective: as this is a fix for regression between scala 2 and 3, maybe we should stick to the approach present there?
I tried to look into scala 2 macros here and the algorithm seems to be very close (or the same?) as one in this PR - traverse everything, collect subtypes. So maybe its ok?

[lloydmeta]

Maintaining possibly odd behaviour is a non-goal between Scala 2 and Scala 3 versions. It's more constructive to make sure this actually makes sense going forward.

sealed trait Animal
sealed trait Dog extends Animal
sealed trait Cat extends Animal

sealed trait Vet[T] extends EnumEntry

object CatVets extends Enum[Vet[Cat]] {
  
  case object Joe extends Vet[Cat]
  case object CatHater extends Vet[Dog]

  lazy val values = findValues 
}


object DogVets extends Enum[Vet[Dog]] {
  
  case object John extends Vet[Dog]
  case object DogHater extends Vet[Cat]

  lazy val values = findValues 
}

That's the sort of thing I think we need to have a good answer for (well-reasoned + working implementation), before merging this.

[Krever]

Oh yes, that's almost exactly the scenario I have in my codebase and it works well. When I was referring to subtype relationship, I meant relation to the type passed to Enum[_], hence

CatVets:

1. Joe <: Vet[Cat] ✅
2. CatHater <: Vet[Cat] ❌

DogVets

1. John <: Vet[Dog] ✅
2. DogHater <: Vet[Dog] ❌

Would you like me to add test cases for those? And if so, is invariance enough or you think all 3 variances are better? (No preference on my side).

[lloydmeta]

Concretely I think we should have a good reason for why CatVets in my example should or should not work. IMO it should throw a compile time error. Yes, tests would help to show this.

I don't actually have the time or energy to come up with a good set of reasons (I've never had this type param requirement for enums), but I do have a strong preference that it should not (re)introduce surprising behaviour and if we can't think of a good reasoning, we should be as strict as possible.

[Krever]

I share your preference and agree that the absence of a good reason is enough not to support a particular case. That said, I think this specific situation is tricky if we aim to prevent surprising behavior at compile time.

What’s easy — and already implemented — is simply not collecting invalid data.

What you’re suggesting (as I understand it) is to disallow defining objects with “invalid” supertypes altogether. That might be difficult or even impossible to enforce at compile time.

From the compiler’s perspective, the following two cases are essentially equivalent:

sealed trait Animal
sealed trait Dog extends Animal
sealed trait Cat extends Animal

sealed trait Vet[T] extends EnumEntry

object CatVets extends Enum[Vet[Cat]] {
  case object Joe extends Vet[Cat]
  case object CatHater extends Vet[Dog]

  lazy val values = findValues
}

object Cats extends Enum[Cat] {
  case object Garfield extends Cat
  case object Snoopy extends Dog

  lazy val values = findValues
}

Currently, there’s no protection against defining a Dog inside an Enum[Cat].

What might be possible is emitting a warning (or error) when we see an object inside Enum[A] that:

• does not inherit from A
• but does share a common supertype with A

That would catch the “Dog among Cats” case. But:

• We’d need to handle common supertypes like Any, which could get messy.
• It wouldn’t help with mismatches involving type constructors.

An alternative idea would be to warn/error when an object inside Enum[A] doesn’t inherit from A, but shares the same type constructor:

• This avoids the Any issue
• It wouldn’t catch simple (non-generic) mistakes
• I’m also not confident I could implement this reliably — it’s quite outside my comfort zone

All that said, I share the general mindset of “be as restrictive as possible,” and I’m open to whatever checks you think are appropriate. But I’d really like to avoid a situation where perfect becomes the enemy of good. I just migrated a very large codebase over the past week, and I’d love to wrap that up. I also believe enumeratum is a great library — I’d much prefer to contribute here than fork it.

Let me know how you'd like to proceed. If you can define some semi-formal rule to implement, just let me know. Otherwise I'm not sure I will be able to do it myself.

[lloydmeta]

I'd be fine with not having invalid subtypes (incl in type params), with tests showing that.

To set expectations though: while I appreciate your effort here I maintain this lib in a small portion of my free time, most of which has been used up. If it does not line up with your needs and timeline, feel free to fork - it's open source for a reason.

How I would like to proceed remains unchanged - I'd like there to be good rationale as to why we think this should work this way, and have it documented and tested. If you work that out, with the above tests, great - otherwise I'm happy to let this PR sit until someone does.