Skip to content

Override method with implicit methods. #2000

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
nicolasstucki opened this issue Feb 19, 2017 · 13 comments
Closed

Override method with implicit methods. #2000

nicolasstucki opened this issue Feb 19, 2017 · 13 comments

Comments

@nicolasstucki
Copy link
Contributor

Currently we are unable to override a method with an implicit method as the implicit method is not a subtype of the plain method. This is in used in the definitions of the synthetic symbols for ImplicitFunctionN but it does not work properly. I found the following two issues

  • Dotty complains when we try to override a method with its equivalent implicit method.
  • We are forced to implement ImplicitFunctionN.apply without the implicit. Making the function class implementing ImplicitFunctionN behave as a FunctionN.
object Test {
  def main(args: Array[String]): Unit = {
    implicit val x: String = "hello"
    val i0: Int = new Fun0().apply

    val i1: Int = new Fun1().apply
    //                ^^^^^^^^^^^^
    //                missing arguments for method apply in class Fun1
    //                follow this method with `_' if you want to treat it as a partially applied function

    val i2: Int = new Fun2().apply // works as expected
  }
}

class Fun0 extends ImplicitFunction1[String, Int] {
  def apply(implicit x: String): Int = 42
  // overriding method apply in trait Function1 of type (v1: String)Int;
  //   method apply of type (implicit x: String)Int has incompatible type
}

class Fun1 extends ImplicitFunction1[String, Int] {
  def apply(x: String): Int = 43
}

class Fun2 {
  def apply(implicit x: String): Int = 44
}

class A {
  def foo(x: String): Unit = ()
}

class B extends A {
  def foo(implicit x: String): Unit = ()
  // overriding method foo in trait A of type (x: String)Unit;
  //   method foo of type (implicit x: String)Unit has incompatible type
}
@odersky
Copy link
Contributor

odersky commented Feb 19, 2017

That's intentional. Implicit methods should not be able to override normals methods. ImplicitFunctionN is an exception. it should not undergo this check.

@odersky
Copy link
Contributor

odersky commented Feb 19, 2017

And, I am not sure at all we want to make implicit function classes extensible. Make them final instead?

@retronym
Copy link
Member

retronym commented Feb 20, 2017

I believe that treating the implicit modifier as significant in subtyping is incorrect.

Here's an example in scalac that lets you override without the override keyword:

scala> class C[A] { def foo(a: A) = "c" }
class D extends C[String] { def foo(implicit s: String) = "d" }
(new D(): C[String]).foo("")
defined class C
defined class D
res3: String = d

A related bug: SI-3653, was fixed by removing the implicit modifier in the Uncurry info transform.

Wouldn't it be better to treat the method types as equivalent, and report the mismatched implicit modifiers in the overriding pairs analysis in RefChecks?

@retronym
Copy link
Member

retronym commented Feb 20, 2017

Another manifestation:

scala> class C[A] { final def foo(a: A) = "c" }; class D extends C[String] { def foo(implicit s: String) = "d" }; new D
java.lang.VerifyError: class D overrides final method foo.(Ljava/lang/Object;)Ljava/lang/String;
  at java.lang.ClassLoader.defineClass1(Native Method)

@nicolasstucki
Copy link
Contributor Author

Making the methods equivalent would also fix a similar issue with overloading #2002.

I will also try to see if it is possible to decouple ImplicitFunctionN from FunctionN to remove this exceptional override.

@retronym
Copy link
Member

retronym commented Feb 20, 2017

The same hole exists for multiple parameter lists:

scala> class C[A] { final def foo(a: A)(b: A) = "c" }; class D extends C[String] { def foo(s: String, t: String) = "d" }; new D
java.lang.VerifyError: class D overrides final method foo.(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/String;
  at java.lang.ClassLoader.defineClass1(Native Method)

@nicolasstucki
Copy link
Contributor Author

This is another override that is currently valid, but should not.

class A {
  def foo(implicit i: Int): Int = i + i
}

class B extends A {
  override def foo(i: Int): Int = i
}

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 20, 2017
This removes an illegal method override mentioned in scala#2000.
@DarkDimius
Copy link
Contributor

I don't see why last one shouldn't be valid. Both methods have the same signature and thus one overrides another.

I agree with @retronym that it would be better if implicit modifier in argument does not affect overriding relationship. Otherwise we may need to name-mangle all methods with implicit arguments.

@nicolasstucki
Copy link
Contributor Author

The issue is not that it is not a valid override, in fact in this case it should identify it as a valid override. But we would need another check that forces implicit method to be overriden by implicit methods and non implicit method by non implicit methods.

@retronym
Copy link
Member

It is worth noting that we already report modifier mismatches in refchecks, in the same manner as I'm proposing above, for access modifiers:

scala> class C { def foo = 1 }; class D extends C { private def foo = 2 }
<console>:11: error: overriding method foo in class C of type => Int;
 method foo has weaker access privileges; it should not be private
       class C { def foo = 1 }; class D extends C { private def foo = 2 }
                                                                ^

@retronym
Copy link
Member

Trying out a scalac implementation in scala/scala#5722

@odersky
Copy link
Contributor

odersky commented Feb 21, 2017

I agree that we should catch this in rechecks.

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 21, 2017
This removes an illegal method override mentioned in scala#2000.
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 21, 2017
This removes an illegal method override mentioned in scala#2000.
odersky added a commit that referenced this issue Feb 22, 2017
Fix #2000: Make implicit and non-implicit functions incomparable
odersky added a commit to dotty-staging/dotty that referenced this issue Dec 28, 2017
With this change,

    (implicit x: C): D  <:  (x: C): D

but not the other way around. This affects subtyping
of dependent implicit function types. Now:

    (implicit x: C) => D  <:  (x: C) => D

See also scala#2000.

As a second change, prevent crashing on type mismatch errors
involving dependent implicit function types.
odersky added a commit to dotty-staging/dotty that referenced this issue Dec 28, 2017
With this change,

    (implicit x: C): D  <:  (x: C): D

but not the other way around. This affects subtyping
of dependent implicit function types. Now:

    (implicit x: C) => D  <:  (x: C) => D

See also scala#2000.

As a second change, prevent crashing on type mismatch errors
involving dependent implicit function types.
@odersky
Copy link
Contributor

odersky commented Dec 28, 2017

I am still reluctant to allow arbitrary implicit/non-implicit overrides but #3692 requires that an implicit method can match a non-implicit one, since otherwise

implicit (c: C) => D  !<:   (c: C) => D

even tough

implicit (c: C) => D <: C => D

That does not seem to be reasonable. It is changed in #3704.

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