Skip to content

signature-based override check is too restrictive #8929

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

Closed
smarter opened this issue May 9, 2020 · 3 comments
Closed

signature-based override check is too restrictive #8929

smarter opened this issue May 9, 2020 · 3 comments
Assignees

Comments

@smarter
Copy link
Member

smarter commented May 9, 2020

This compiles with Scala 2, but not with Dotty:

class A {
  def foo[T <: Cloneable with Serializable](x: T): Unit = {}
}

class B extends A {
  override def foo[T <: Serializable](x: T): Unit = {}
}
error overriding method foo in class A of type [T <: Cloneable & Serializable](x: T): Unit;
  method foo of type [T <: Serializable](x: T): Unit has incompatible type

This happens because TypeComparer requires matching signatures for subtypes:
https://github.com/lampepfl/dotty/blob/b0147ea5e44ce34a986b2858532fc390e4625f61/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L656
https://github.com/lampepfl/dotty/blob/b0147ea5e44ce34a986b2858532fc390e4625f61/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L666

This particular example is perhaps not very important, but the same issue can happen when overriding a Java method which is much more problematic: because Java erases some types differently, in some situation it might not be possible to write a method with a matching signature that is also a valid override.

To fix this, I think that we can just remove the consistentParams checks from TypeComparer. As far as I can tell, the subsequent matchingPolyParams/matchingMethodParams should be enough to guarantee soundness, so it's just a (hopefully insignificant) performance hit. /cc @odersky

@smarter smarter self-assigned this May 9, 2020
@smarter
Copy link
Member Author

smarter commented May 9, 2020

Ah, I spoke too soon, if overrides have non-matching signature then they're not considered overrides at use-site:

object Test {
  def test(x: B) = x.foo(???)
}
-- [E051] Reference Error: try/B.scala:13:21 -----------------------------------
13 |  def test(x: B) = x.foo(???)
   |                   ^^^^^
   |Ambiguous overload. The overloaded alternatives of method foo in class B with types
   | [T <: Serializable](x: T): Unit
   | [T <: Cloneable & Serializable](x: T): Unit
   |both match arguments ((??? : => Nothing))

Because denotation matching requires that parameter matches:
https://github.com/lampepfl/dotty/blob/b0147ea5e44ce34a986b2858532fc390e4625f61/compiler/src/dotty/tools/dotc/core/Denotations.scala#L1126
So the d != Signature.NoMatch check would have to be removed, which might be more expensive.

Alternatively, we could keep the check but only in situations where a Scala method is not overriding a Java method. That way we can still have a fast-path without compromising interop. WDYT?

@smarter
Copy link
Member Author

smarter commented May 10, 2020

(Note that the same sort of issues can arise when overriding Scala 2 methods from Dotty, which is what motivated me looking into this in the first place)

smarter added a commit to smarter/dotty that referenced this issue Jun 8, 2020
Use Denotation#matches instead of Types#matches, the former takes
signatures into account, in the testcase this is needed to ensure that
RefChecks does not emit an error about
`def foo[T <: Serializable](x: T): Unit` being an invalid override of
`def foo[T <: Cloneable](x: T): Unit`.

Note that our treatment of polymorphic methods is different Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see scala#8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.
smarter added a commit to smarter/dotty that referenced this issue Jun 8, 2020
Use Denotation#matches instead of Types#matches, the former takes
signatures into account, in the testcase this is needed to ensure that
RefChecks does not emit an error about
`def foo[T <: Serializable](x: T): Unit` being an invalid override of
`def foo[T <: Cloneable](x: T): Unit`.

Note that our treatment of polymorphic methods differs from Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see scala#8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.
smarter added a commit to smarter/dotty that referenced this issue Jun 9, 2020
Use Denotation#matches instead of Types#matches, the former takes
signatures into account, in the testcase this is needed to ensure that
RefChecks does not emit an error about
`def foo[T <: Serializable](x: T): Unit` being an invalid override of
`def foo[T <: Cloneable](x: T): Unit`.

Note that our treatment of polymorphic methods differs from Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see scala#8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.
smarter added a commit to smarter/dotty that referenced this issue Jun 9, 2020
Use Denotation#matches instead of Types#matches, the former takes
signatures into account, in the testcase this is needed to ensure that
RefChecks does not emit an error about
`def foo[T <: Serializable](x: T): Unit` being an invalid override of
`def foo[T <: Cloneable](x: T): Unit`.

Note that our treatment of polymorphic methods differs from Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see scala#8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.
smarter added a commit to dotty-staging/dotty that referenced this issue Jun 9, 2020
Use Denotation#matches instead of Types#matches, the former takes
signatures into account, in the testcase this is needed to ensure that
RefChecks does not emit an error about
`def foo[T <: Serializable](x: T): Unit` being an invalid override of
`def foo[T <: Cloneable](x: T): Unit`.

Note that our treatment of polymorphic methods differs from Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see scala#8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.
@smarter
Copy link
Member Author

smarter commented Aug 12, 2020

Closing now that #9139 is merged:

Note that our treatment of polymorphic methods differs from Scala 2
which seems to consider that two polymorphic methods with the same
number of type and term parameters always match and cannot be
overloads (see #8929), but it's closer to what Java does and allows us
to override some Java methods which scalac can't.

@smarter smarter closed this as completed Aug 12, 2020
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

1 participant