Skip to content

Should we allow polymorphic overloads with the same number of type parameters? #9109

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 Jun 4, 2020 · 6 comments
Closed

Comments

@smarter
Copy link
Member

smarter commented Jun 4, 2020

These two overloads are considered valid by Dotty, because their signatures are different:

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

... but trying to override one of them fails, because refchecks uses Types#matches to find overrides which does not take signatures into account (it calls TypeComparer#matchesType which in turns does matchesType(tp1.resultType, tp2.resultType.subst(tp2, tp1)), therefore comparing (x: T): Unit to (x: T): Unit):

class B extends A {
  override def foo[T <: Serializable](x: T): Unit = {}
}
-- Error: try/B.scala:21:15 ----------------------------------------------------
21 |  override def foo[T <: Serializable](x: T): Unit = {}
   |               ^
   |  error overriding method foo in class A of type [T <: Cloneable](x: T): Unit;
   |    method foo of type [T <: Serializable](x: T): Unit has incompatible type

So the question is: should we disallow these overloads, or should we strive to support them? The answer is not clear cut to me because Scala 2 never supported them:

try/B.scala:16: error: method foo is defined twice;
  the conflicting method foo was defined at line 15:7
  def foo[S <: Cloneable](x: S): Unit = {}
      ^

... but Java does support them, so if we don't support them we're still in trouble when trying to override them (and in fact scala 2 cannot override them):

public class A {
  public <T extends java.io.Serializable> void foo(T x) {}
  public <T extends Cloneable> void foo(T x) {}
}

I would be in favor of supporting them for better interoperability and because it doesn't seem too hard (require matching param signatures for Types#matches to bring it in line with Denotation#matches)

WDYT @odersky ?

@odersky
Copy link
Contributor

odersky commented Jun 8, 2020

I agree, it seems to be more useful and also more consistent to support them.

@odersky odersky closed this as completed Jun 8, 2020
@bishabosha
Copy link
Member

should a new issue for this be made in feature requests or here?

@smarter
Copy link
Member Author

smarter commented Jun 8, 2020

We should keep this open since the current situation is inconsistent (adding an overload can break overriding)

@smarter smarter reopened this Jun 8, 2020
@smarter
Copy link
Member Author

smarter commented Jun 8, 2020

Thinking about this more, I'm not so sure anymore if we should allow this or not. I think it comes down to what we want to do with #8929:

class A {
  def foo[T <: Serializable](x: T): Unit = {}
}
class B extends A {
  override def foo[T](x: T): Unit = {}
}

These are valid overrides for scalac but not for dotty, that is: scalac allows not only covariant result type overriding but contravariant type parameter bounds overriding, dotty refuses them because their signatures are different. If we were to support this, then it would make sense to ban overloads with the same number of type parameters, because overriding relationships would become very confusing to figure out, both for the compiler and for users, who cannot rely on just counting the number of type parameters to see what overrides what.

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.
@odersky odersky closed this as completed in 7964d17 Jun 9, 2020
odersky added a commit that referenced this issue Jun 9, 2020
@soronpo
Copy link
Contributor

soronpo commented Jun 9, 2020

What happens if we mix @alpha into this?

@smarter
Copy link
Member Author

smarter commented Jun 9, 2020

It doesn't change anything, why would it? overrides must have matching uses of @alpha

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

4 participants