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

extension method hides overridden method via Conversion + a conflicting message #22267

Open
sjbiaga opened this issue Dec 28, 2024 · 7 comments
Labels
area:extension-methods area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement

Comments

@sjbiaga
Copy link

sjbiaga commented Dec 28, 2024

Compiler version

3.6.3-RC1

Minimized code

case class M[T](var value: T)
given [T]: Conversion[M[T], T] = _.value
class C { def m(n: Double): Unit = println(0->n) }
extension (self: C) def m(n: Int): Unit = println(1->n)

REPL session

scala> val c = new M(new C)
val c: M[C] = M(rs$line$3$C@4e26c308)
scala> c.value.m(1)
(0,1.0)
scala> c.value.m(1.0)
(0,1.0)
scala> c.m(1.0)
-- [E007] Type Mismatch Error: -------------------------------------------------
1 |c.m(1.0)
  |    ^^^
  |    Found:    (1.0d : Double)
  |    Required: Int
  |
  | longer explanation available when compiling with `-explain`
there was 1 feature warning; re-run with -feature for details
1 warning found
1 error found
                                                                                                                               
scala> c.m(1)
(1,1)
there was 1 feature warning; re-run with -feature for details
1 warning found

Now, if I add an extension with n: Double, there is even a weird message, because what happens is not what it says.

scala> extension (self: C) def m(n: Double): Unit = println(2->n)
1 warning found
-- [E194] Potential Issue Warning: ---------------------------------------------
1 |extension (self: C) def m(n: Double): Unit = println(2->n)
  |                        ^
  |Extension method m will never be selected
  |because rs$line$3$C already has a member with the same name and compatible parameter types.
  |
  | longer explanation available when compiling with `-explain`
def m(self: C)(n: Double): Unit
                                                                                                                               
scala> c.m(1.0)
(2,1.0)

Expectation

scala> c.m(1.0)
(0,1.0)
@sjbiaga sjbiaga added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Dec 28, 2024
@som-snytt
Copy link
Contributor

This is per spec (or per reference)

https://docs.scala-lang.org/scala3/reference/contextual/extension-methods.html#translation-of-calls-to-extension-methods-1

where the "precise rules" give two steps.

In the first step, c.m(3.14) is rewritten m(c)(3.14) which typechecks (using the conversion).

Conversions to C (the expectation) are considered in the second step: "This second rewriting is attempted at the time where the compiler also tries an implicit conversion from T to a type containing m."

The warning could be improved by adding: will never be selected from type C.

The explain message could add which interface contributes that member to C.

@som-snytt som-snytt added better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement area:reporting Error reporting including formatting, implicit suggestions, etc and removed itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Dec 28, 2024
@som-snytt som-snytt changed the title extension method hides overriden method via Conversion + a conflicting message extension method hides overridden method via Conversion + a conflicting message Dec 28, 2024
@sjbiaga
Copy link
Author

sjbiaga commented Dec 29, 2024

The reference is a bit TL;DR for me (I should read the whole of it, not just the pointed article), but I get the idea - by using -Xprint:typer:

scala> c.m(1.0)
-- [E007] Type Mismatch Error: -------------------------------------------------
1 |c.m(1.0)
  |    ^^^
  |    Found:    (1.0d : Double)
  |    Required: Int
  |
  | longer explanation available when compiling with `-explain`
[[syntax trees at end of                     typer]] // rs$line$6
package <empty> {
  final lazy module val rs$line$6: rs$line$6 = new rs$line$6()
  final module class rs$line$6() extends Object() { this: rs$line$6.type =>
    val res0: <error unspecified error> =
      m(given_Conversion_M_T[C].apply(c))(1.0d)
  }
}

there was 1 feature warning; re-run with -feature for details
1 warning found
1 error found

However, in the absence of the extension method, the same is:

scala> c.m(1.0)
(0,1.0)
there was 1 feature warning; re-run with -feature for details
1 warning found
[[syntax trees at end of                     typer]] // rs$line$5
package <empty> {
  final lazy module val rs$line$5: rs$line$5 = new rs$line$5()
  final module class rs$line$5() extends Object() { this: rs$line$5.type =>
    val res0: Unit = given_Conversion_M_T[C].apply(c).m(1.0d)
  }
}

My intuition strongly suggests that the non-extension method should be found!
So, why not ;) an amend to the specification that in the case the translation m(c)(3.14) fails, then - in another attempt to type check -, a first step would be to find a Conversion?
I tried similar code with Scala 2 version 2.13.15, but because Conversion exists only in Scala 3, I think it is impossible to get what I want. But not in Scala 3, especially because of the existence of Conversion, not an implicit Ops class.

@som-snytt
Copy link
Contributor

-Vprint:typer is the modern spelling.

I sympathize with not wanting to do a lot of reading just to get something to work that should "just work" -- I just had to re-read how to run a test, for instance -- but the order in which adaptations are tried helps reduce confusion over "feature interactions".

That also constrains the "solution space", just for reasons of efficiency and predictability.

This is a good example of why conversions are under a "feature" flag. There is probably no shared intuition about how to apply the conversion. However, maybe you'd like to open a github Discussion or a forum topic on the competition between conversions and extensions. You're suggesting try extensions with conversions turned off, then try conversions, then try extensions again with conversions turned on. They would probably want a compelling use case.

I may submit a PR for the message tweak here, since I suggested it, if they deem it useful.

@som-snytt
Copy link
Contributor

I neglected to post the correct way to get the desired behavior:

import language.implicitConversions

case class M[T](value: T)
given [T]: Conversion[M[T], T] = _.value
class C:
  def m(n: Double): Unit = println(0->n)
object C:
  given Ops = Ops()
class Ops:
  extension (self: C) def m(n: Int): Unit = println(1->n)
  extension (self: C) def m(n: Double): Unit = println(2->n) // warn
  extension (self: C) def m(s: String): Unit = println(3->s)

@main def test() =
  val c = M(C())
  def i = 42
  def pi = 3.14
  c.value.m(i)
  c.value.m(pi)
  c.m(i) // conversion
  c.m(pi) // conversion
  c.m("hello, world") // extension
  //m(c)(pi)
  val c0 = C()
  c0.m(pi)
  c0.m("hello, world")

contrast with extensions in lexical scope, as in the OP

import language.implicitConversions

case class M[T](value: T)
given [T]: Conversion[M[T], T] = _.value
class C:
  def m(n: Double): Unit = println(0->n)
class Ops:
  extension (self: C) def m(n: Int): Unit = println(1->n)
  extension (self: C) def m(n: Double): Unit = println(2->n) // warn
  extension (self: C) def m(s: String): Unit = println(3->s)

@main def test() =
  val c = M(C())
  val ops = Ops()
  import ops.*
  def i = 42
  def pi = 3.14
  c.value.m(i)
  c.value.m(pi)
  c.m(i) // extension
  c.m(pi) // extension
  c.m("hello, world") // extension
  //m(c)(pi)
  val c0 = C()
  c0.m(pi)
  c0.m("hello, world")

@sjbiaga
Copy link
Author

sjbiaga commented Dec 30, 2024

... and the code that does not type check is:

case class M[T](value: T)
given [T]: Conversion[M[T], T] = _.value
class C:
  def m(n: Double): Unit = println(0->n)
object Ops:
  extension (self: C) def m(n: Int): Unit = println(1->n)

@main def test() =
  val c = M(C())
  import Ops.*
  def i = 42
  def pi = 3.14
  c.value.m(i)
  c.value.m(pi)
  c.m(i) // extension
  c.m(pi) // non-extension

@sjbiaga
Copy link
Author

sjbiaga commented Dec 30, 2024

What I had in mind was much simpler: perhaps there is in the codebase

  • an else branch of an if that could be implemented
  • a queue of "attempts" that could be added to
  • a state machine (sealed trait and case classes) that could be enhanced
  • a try...catch with a lacking case

rather than turning things on and off - i.e., when it comes to the "translation".

@som-snytt
Copy link
Contributor

som-snytt commented Dec 30, 2024

@sjbiaga I'd suggest opening a Discussion to lobby your point. That is how "relaxed imports" to permit "overloaded extensions" was added. (Someone asked for it.) Alternatively, I can remove the "reporting" label and you can pursue the enhancement here.

Currently, c.m(i) is tried as m(c), which typechecks via conversion. You're asking that if m(cv(c))(i) does not typecheck, toss out that result and move on to step 2, where c.m will be tried again with conversion so cv(c).m(i) works.

Worth adding that if the second attempt also fails, you want the error from the first attempt.

Edit: the ticket allows multiple area labels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:extension-methods area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement
Projects
None yet
Development

No branches or pull requests

2 participants