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

cannot override generic Java method instantiated with value class with -Yexplicit-nulls #15194

Open
olhotak opened this issue May 16, 2022 · 4 comments

Comments

@olhotak
Copy link
Contributor

olhotak commented May 16, 2022

Compiler version

Scala compiler version 3.2.0-RC1-bin-SNAPSHOT-git-7fbbeef -- Copyright 2002-2022, LAMP/EPFL

Minimized code

import language.unsafeNulls

class PosZLong(value: Long) extends AnyVal

object PosZLongOrd extends Ordering[PosZLong]:
  def compare(x: PosZLong, y: PosZLong): Int = 0

Compile with scalac -Yexplicit-nulls PosZLong.scala

Minimized from scalatest/scalactic.

Output

-- Error: PosZLong.scala:4:7 --------------------------------------------------------------------------------------------------------------
4 |object PosZLongOrd extends Ordering[PosZLong]:
  |       ^
  |object creation impossible, since def compare(x$0: T | Null, x$1: T | Null): Int in trait Comparator in package java.util is not defined 
  |(The class implements a member with a different type: def compare(x: PosZLong, y: PosZLong): Int in object PosZLongOrd)
1 error found

Expectation

Should compile.

Fails to compile independently of language.unsafeNulls.

Probably related to #13975.

Blocker in community build for scalatest and the many projects that depend on it.

@olhotak
Copy link
Contributor Author

olhotak commented May 16, 2022

Related to #13837

@olhotak
Copy link
Contributor Author

olhotak commented Jun 7, 2022

A simpler example is:

object IntOrd extends Ordering[Int] {
  def compare(i: Int, j: Int) = ???
}
1 |object IntOrd extends Ordering[Int] {
  |       ^
  |object creation impossible, since def compare(x$0: T | Null, x$1: T | Null): Int in trait Comparator in package java.util is not defined 
  |(The class implements a member with a different type: def compare(i: Int, j: Int): Int in object IntOrd)

One cannot define compare to take parameters of type Int | Null either because then it would not override the compare in Ordering.

An even simpler example is for IntOrd to extend just Comparator[Int] directly instead of Ordering[Int]. That gives the same error as above.

Thinking about this some more, I think I have a solution:

For AnyRef types, we already ignore | Null when deciding whether a method overrides a method of a Java-defined supertype. We should also do that even for non-AnyRef types. So compare(Int, Int) would override a Java-defined compare(Int|Null, Int|Null) just as compare(String, String) overrides a Java-defined compare(String|Null, String|Null). This would preserve the behaviour without -Yexplicit-nulls, since there, the Java-defined signature does not have the |Null added to it.

A problem would then be that we allow Int and Int|Null to be distinct overloads. The following compiles:

class D {
  def foo(s: Int) = ???
  def foo(s: Int|Null) = ???
}

This creates an ambiguity if D extends a Java-defined class with foo(Int|Null): which of the two methods in D should override the foo of the superclass? I think we could make it an error to have both foos in the specific case that D overrides a Java-defined class with a foo(Int|Null).

If Int is replaced with String in this example, it's already an error:

16 |  def foo(s: String|Null) = ???
   |      ^
   |Double definition:
   |def foo(s: String): Nothing in class D at line 15 and
   |def foo(s: String | Null): Nothing in class D at line 16
   |have the same type after erasure.
   |
   |Consider adding a @targetName annotation to one of the conflicting definitions
   |for disambiguation.

I'd be interested in the thoughts of @odersky and @smarter as the experts on erasure and bridge methods.

@olhotak
Copy link
Contributor Author

olhotak commented Jun 9, 2022

As a side comment, I found an example in which we are already less sound with -Yexplicit-nulls than without. The following compiles with -Yexplicit-nulls but not without:

def f(o: Ordering[Int]) = {
  (o: java.util.Comparator[Int]).compare(null, null)
}

@martingd
Copy link

martingd commented Dec 6, 2022

It could seem related to #13975 and #13837 (as mentioned above) but these are both merged and released with Scala 3.2.2-RC1 (if I read correctly) but the issue is still with that version of the compiler.

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

3 participants