search

softwaremill / magnolia

Easy, fast, transparent generic derivation of typeclass instances
https://softwaremill.com/open-source/
Apache License 2.0
754 stars 115 forks source link

Does Magnolia 1 for Scala 2 support getting the original index of a subtype in a sealed trait? #514

Open ThijsBroersen opened 4 months ago

ThijsBroersen commented 4 months ago

In Magnolia 1 for Scala 3 the index of the subtype is retrieved from the Mirror.SumOf. In Magnolia 1 for Scala 2 the index of the subtype is determined after doing a .sortBy(_.fullName) using .zipWithIndex. -> https://github.com/softwaremill/magnolia/blob/f406cfecc534b30fd9dfcc7da63dc96e89aeaa2d/core/src/main/scala/magnolia1/magnolia.scala#L656

Is the conclusion correct?

Is it possible to preserve the original index in the Scala 2 implementation?

adamw commented 4 months ago

Hm what would be the "original index" in case of Scala 2 implementation?

I'm not sure if such information is available ... plus, the hierarchies might be quite complex, so there might even not be a single good answer (there are some weird hierarchies in the tests I think)

ThijsBroersen commented 4 months ago

Are you saying knownSubclassesOf(classType.get).toList does not reflect the exact order in which the subtypes are written in the source code?

adamw commented 4 months ago

Hm ... this sounds reasonable. Though I'm not 100% sure how Symbol.knownDirectSubclasses works. Maybe it would be doable :) We'd just have to account for the fact that some children might appear twice (as they might implement multiple traits)

ThijsBroersen commented 4 months ago

Changing the behaviour will probably cause issues/pain with library users.

The main reason I raises this issue was because of Avro4s where the order of sealed trait and enums depend on Magnolia. The Avro-spec encodes enums to integers based the order of which they appear in the spec. So a change in the order will result in different avro encodings and wrongly decoding of old messages. Although IMHO deriving these types of specs is risky, it would be nice of it is possible to derive specs by the exact order in the Scale code.

The best approach would probably be to add a function which allows a user to get the subtypes as close to the order of the source code as possible.

adamw commented 4 months ago

Ah I see the problem. Yes, that would be a breaking change then. Not sure if adding another function is viable - it would have to be another top-level derive macro I guess?

ThijsBroersen commented 4 months ago

What about adding a field to Subtype?

adamw commented 4 months ago

Good idea :) Since it's a trait, we could probably add a new method in a binary-compatible way. Then we'd need to keep the old constructors and delegate to the new ones. Seems doable.