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

Specify order of MirroredElemTypes for Mirror.SumOf #8390

Merged

Conversation

travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Feb 27, 2020

For sum types, the order of elements in MirrorElemTypes currently follows the order of definitions in the file. I'd like to be able to rely on this order (which wasn't exposed to macro authors in Scala 2), so I've specified it in the docs and added a couple of test cases.

Related discussion on Gitter:

@smarter:

Don't think it's specified, but you could make a PR to specify it in the doc and add a testcase :)
what's the usecase ?

@travisbrown:

In Haskell for example you often see code like this:

data N = One | Two | Three deriving (Eq, Ord)

In Scala that hasn't been possible because knownDirectSubclasses returned a Set (which Shapeless's Generic sorted by constructor name, so you'd have Three < Two).

Also comes up for things like deriving Semigroup for ADTs, where you want a stable and meaningful way to decide how to combine non-same-constructor values.

"First defined takes precedence" feels a lot more sensible than "keep the one with the name that comes first in the alphabet".

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, WDYT @milessabin ?

@mpilquist
Copy link
Contributor

FYI, I'm relying on this in scodec too. The derived codec for an ADT is a unsigned 8-bit integer field that stores the ordinal of the constructor, followed by the derived product codec of the constructor.

In the Scala 2 version, I used a very complex implicit mechanism where the root would provide the discriminator codec (implicitly) and each constructor would provide the value for the discriminator (implicitly). In hindsight, this was way too complex so I dropped it all when porting to Dotty.

@milessabin
Copy link
Contributor

If you can guarantee that the ordering will be stable across (incremental) builds, then go for it.

@milessabin
Copy link
Contributor

Digging a bit more, it's possible that there could be an issue with anonymous mirrors generated for Scala 2 ADTs, which necessarily don't have mirrors baked into their companions. I think there could well be an issue for stability of the ordering given incremental/separate compilation.

But like I said, if that's a solved problem now, by all means go for it. If it isn't then lexicographic ordering by constructor name, as done in shapeless, is the way to go.

@smarter
Copy link
Member

smarter commented Mar 2, 2020

If you can guarantee that the ordering will be stable across (incremental) builds, then go for it.

The order in which we store annotations is based on their position in the source code (https://github.com/lampepfl/dotty/blob/aba0c5822024bbdd761f8bbae76654884a4b2208/compiler/src/dotty/tools/dotc/typer/Namer.scala#L553), so that should be as stable as can be. For Scala 2 the order comes from the CHILDREN tag we get from pickling (https://github.com/lampepfl/dotty/blob/aba0c5822024bbdd761f8bbae76654884a4b2208/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala#L852), which is at least stable as long as the same classfile is used.

@odersky odersky merged commit 70a49f9 into scala:master Mar 6, 2020
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.

6 participants