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

Dotty allow overriding equals and hashCode for Value Classes #22494

Open
hamzaremmal opened this issue Feb 1, 2025 · 12 comments
Open

Dotty allow overriding equals and hashCode for Value Classes #22494

hamzaremmal opened this issue Feb 1, 2025 · 12 comments
Assignees

Comments

@hamzaremmal
Copy link
Member

Compiler version

590691b

Minimized code

class Bar(val self: Short) extends AnyVal:
    override def equals(that: Any): Boolean = ???
    override def hashCode: Int = ???

Output

[[syntax trees at end of                  genBCode]] // playground.scala
package <empty> {
  @SourceFile("playground.scala") final class Bar extends Object {
    def <init>(self: Short): Unit =
      {
        this.self = self
        super()
        ()
      }
    private val self: Short
    def self(): Short = this.self
    override def equals(that: Object): Boolean =
      Bar.equals$extension(this.self(), that)
    override def hashCode(): Int = Bar.hashCode$extension(this.self())
  }
  @SourceFile("playground.scala") final module class Bar extends Object {
    def <init>(): Unit =
      {
        super()
        ()
      }
    private def writeReplace(): Object =
      new scala.runtime.ModuleSerializationProxy(classOf[Bar])
    final def equals$extension($this: Short, that: Object): Boolean = ???()
    final def hashCode$extension($this: Short): Int = ???()
  }
  final lazy module val Bar: Bar = new Bar()
}

Compiles.

Expectation

Dotty should prohibit overriding equals and hashCode per the Value Classes's specification; SIP-15:

A value class C must satisfy the following criteria:
...
5. C may not define concrete equals or hashCode methods
...

@hamzaremmal hamzaremmal added itype:bug area:value-classes Issues tied to value classes. labels Feb 1, 2025
@hamzaremmal hamzaremmal self-assigned this Feb 1, 2025
@hamzaremmal hamzaremmal added the Spree Suitable for a future Spree label Feb 1, 2025
@soronpo
Copy link
Contributor

soronpo commented Feb 1, 2025

Other than it being out of spec, what is the issue? Maybe it's better to change the spec since it has been like this from the start.

@hamzaremmal
Copy link
Member Author

hamzaremmal commented Feb 1, 2025

@soronpo If the spec changes, we will have to implement the old behaviour anyway for #22480 (behind -Ycompile-scala2-library).

EDIT: Maybe this coment is more relevant for #22493 that flag the change of semantics

@hamzaremmal
Copy link
Member Author

Actually, for #22480, since we can define both these methods in source, we can just write them manually to preserve the semantics of those specific classes. So the question is open: Should the implementation change or the spec ?

@soronpo
Copy link
Contributor

soronpo commented Feb 1, 2025

I'm using this capability for utilizing the standard collection with my own custom comparison (e.g., grouping lists according to a different criteria than the default equality). I will admit that I'm not sure if indeed the value class saves on boxing/unboxing values for my use-case.

@mbovel
Copy link
Member

mbovel commented Feb 2, 2025

This issue was picked for the Scala Issue Spree of tomorrow, Monday, February 3rd. @hamzaremmal, @julian-a-avar-c and @nmcb will be working on it.

@sjrd
Copy link
Member

sjrd commented Feb 3, 2025

I have a vague recollection that this change was intentional. It's always been frustrating that we couldn't override equals and hashCode for value classes in Scala 2. Since this language restriction is not necessary for correctness, and has no impact on the implementation strategy, I believe we chose not to restrict in Scala 3.

At this point I think we should change the spec instead.

@nmcb
Copy link
Contributor

nmcb commented Feb 3, 2025

I'm not sure if indeed the value class saves on boxing/unboxing values for my use-case.

@soronpo can you link to your use-case ?

@mbovel
Copy link
Member

mbovel commented Feb 3, 2025

@hamzaremmal: if this is in the end should be a specification change, should we try to find an alternative issue for today's Spree?

@hamzaremmal
Copy link
Member Author

hamzaremmal commented Feb 3, 2025

@mbovel Yes please

@mbovel
Copy link
Member

mbovel commented Feb 3, 2025

@mbovel Yes please

I suggest that you tackle #22232 instead.

@sjrd
Copy link
Member

sjrd commented Feb 5, 2025

@soronpo can you link to your use-case ?

Here would be my use case ("would" because this code still cross-compiles for Scala 2.12, so ...):
https://github.com/scala-js/scala-js/blob/48ddd575a43eae47d6e96138e0e1749f0ea50b3c/ir/shared/src/main/scala/org/scalajs/ir/UTF8String.scala#L25-L29

@note
equals() and hashCode(), along with == and ##, are just as
broken for UTF8String as for Arrays. Use the methods in the
companion object instead. This is unavoidable because we cannot override
equals nor hashCode in an AnyVal.

https://github.com/scala-js/scala-js/blob/48ddd575a43eae47d6e96138e0e1749f0ea50b3c/ir/shared/src/main/scala/org/scalajs/ir/UTF8String.scala#L97-L101

  def equals(x: UTF8String, y: UTF8String): Boolean =
    java.util.Arrays.equals(x.bytes, y.bytes)

  def hashCode(x: UTF8String): Int =
    scala.util.hashing.MurmurHash3.bytesHash(x.bytes)

@hamzaremmal
Copy link
Member Author

In today's Core Meeting, we have decided to modify the specification of Value Classes to allow the definition of equals and hashCode. The decision is motivated by the fact that we don't see any reason why we should impose this limitation.
I will leave this ticket open for now until we change the specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants