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

fix #7227: allow custom toString on enum #9549

Merged

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Aug 13, 2020

fixes #7227

productPrefix is now overridden using the enum constant's name and used in the by-name lookup in runtime.EnumValues.

enumLabel is added as an abstract method to scala.Enum and implemented using the enum constant's name. productPrefix is also overridden in concrete enum values to point to enumLabel, as it is used to implement string lookup of enums.

We then also add a default toString to an enum value implementation if an override is not provided in the parent class

java based enum values are optimised so that productPrefix and enumLabel will
forward to this.name in the simple enum case, avoiding an extra field

@bishabosha bishabosha force-pushed the topic/enum-value-custom-tostring branch 2 times, most recently from 0dd4b22 to d3cf186 Compare August 13, 2020 14:24
@bishabosha bishabosha marked this pull request as ready for review August 13, 2020 14:29
Copy link
Member

@TheElectronWill TheElectronWill left a comment

Choose a reason for hiding this comment

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

I find productPrefix much less user-friendly than name, especially since the latter is familiar for users coming from Python, Java, Kotlin and probably more. It can also be surprising that name is the only method that requires extends java.lang.Enum to be available. You can call ordinal, productPrefix, etc. on all enums but not name.

Could name be handled in a similar way to toString, i.e. redirect to productPrefix for pure-dotty enums and to the name method for java enums?

@bishabosha
Copy link
Member Author

bishabosha commented Aug 14, 2020

@TheElectronWill name is more tricky to add to all enums, for example:

enum User:
  case Basic(name: String)
  case Unknown

we won't be able to define a field name for Basic if we want name to always be the identifier it was declared with, whereas productPrefix is not likely to clash with a field name

it could potentially be added to concrete enums if they have no parameterised cases, but then that API is again not generic unless we make a new mixin trait.

We can definitely explore options though, like do the above and have an annotation to force checking of only value cases, or a type class that adds an extension and does the same checking

@TheElectronWill
Copy link
Member

TheElectronWill commented Aug 14, 2020

Doesn't ordinal have the same problem as name? Maybe the real problem is that name is too common to be reserved by the language, and caseName or rawName (or enumName?) would be better.

@bishabosha
Copy link
Member Author

Doesn't ordinal have the same problem as name?

Thats true, but I do suspect ordinal is rare enough to make the user select a different name.

I'll try an implementation with enumLabel

@smarter smarter assigned bishabosha and unassigned smarter Aug 14, 2020
@bishabosha bishabosha assigned smarter and unassigned bishabosha Aug 14, 2020
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.

LGTM, but it'd be good to make a summary of all these enum changes and start a discussion in https://contributors.scala-lang.org/ to inform everyone about them.

docs/docs/reference/enums/enums.md Outdated Show resolved Hide resolved
@bishabosha bishabosha force-pushed the topic/enum-value-custom-tostring branch 2 times, most recently from c761e90 to eefc1ff Compare August 18, 2020 08:00
@bishabosha
Copy link
Member Author

rebased

productPrefix is now overriden using the enum constant's name and
used in the by-name lookup in EnumValues.

java based enum values are optimised so that productPrefix will
forward to .name in the simple enum case, avoiding an extra field
As a rule, even class cases override it to be consistent with
singleton cases which must have a value matching the identifier.

this also moves runtime.EnumValues into src-bootstrapped so that
fromName can directly call .enumLabel

This commit also removes the forced override of productPrefix in
EnumValue and individial cases, this allows the user
to customise productPrefix as with typical case classes.
An override of productPrefix will be generated in SyntheticMembers
to forward to enumLabel if a class parent does not define one.
@bishabosha bishabosha force-pushed the topic/enum-value-custom-tostring branch from eefc1ff to 9c9db17 Compare August 19, 2020 12:48
@bishabosha
Copy link
Member Author

rebased again

@julienrf
Copy link
Collaborator

julienrf commented Aug 19, 2020

Doesn't ordinal have the same problem as name? Maybe the real problem is that name is too common to be reserved by the language, and caseName or rawName (or enumName?) would be better.

I agree.

An alternative solution would be to synthesize a method def ordinal(enumValue: A): Int into the companion object of an enum A definition:

enum Foo:
  case Bar, Baz

Desugars to:

sealed trait Foo
object Foo:
  case object Bar extends Foo
  case object Baz extends Foo

  def ordinal(value: Foo): Int =
    value match:
      case Bar => 0
      case Baz => 1
end Foo

(the same could be done with enumLabel)

My second idea is to drop the support for enum cases that are not singleton values. (this also solves the problem of values not being always synthesized)

@bishabosha
Copy link
Member Author

bishabosha commented Aug 20, 2020

@julienrf so that ordinal method is already generated for the Mirror framework, and so that could definitely be enhanced for case label too, and so we're less likely to have method name collisions, but I am concerned about the linear lookup for large enums, but as its in the companion it could call a private non-colliding method on the enum class

@bishabosha bishabosha requested a review from sjrd August 20, 2020 08:33
@bishabosha bishabosha merged commit 80d9dcc into scala:master Aug 20, 2020
@bishabosha bishabosha deleted the topic/enum-value-custom-tostring branch August 20, 2020 09:17
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.

Enum implementation overrides custom toString
6 participants