Skip to content

Unintended recursive implicit conversion #13542

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
prolativ opened this issue Sep 16, 2021 · 2 comments · Fixed by #13589
Closed

Unintended recursive implicit conversion #13542

prolativ opened this issue Sep 16, 2021 · 2 comments · Fixed by #13589
Assignees
Milestone

Comments

@prolativ
Copy link
Contributor

Compiler version

3.0.2

Minimized code

import scala.language.implicitConversions
implicit def strToInt(s: String): Int = s.toInt
val x: Int = "1"

Output (at runtime)

java.lang.StackOverflowError
  at repl$.rs$line$1$.strToInt(rs$line$1:1)
  at repl$.rs$line$1$.strToInt(rs$line$1:1)
  at repl$.rs$line$1$.strToInt(rs$line$1:1)
  ...

Less minimal but more self-contained code

import scala.language.implicitConversions

case class Foo(i: Int) extends AnyVal:
  def toFoo = this

case class Bar(i: Int) extends AnyVal

class BarOps(bar: Bar):
  def toFoo = Foo(bar.i)

implicit def augmentBar(bar: Bar): BarOps = BarOps(bar)

val x =
  implicit def barToFoo(bar: Bar): Foo = bar.toFoo
  val foo: Foo = Bar(1)

Output (at runtime)

java.lang.StackOverflowError
  at repl$.rs$line$1$.barToFoo$1(rs$line$1:14)
  at repl$.rs$line$1$.barToFoo$1(rs$line$1:14)
  at repl$.rs$line$1$.barToFoo$1(rs$line$1:14)
  ...

Expectation

The first snippet was a real puzzler to me and I spent quite a lot of time trying to figure out what was happening there until I found the reason shown in the second snippet (in the first one it's about toInt being defined on Int and at the same time added to String from StringOps by augmentString from Predef).

Not sure whether this would be easy to achieve with the current scheme of resolving implicits but it would be good to avoid the cyclic call of barToFoo by choosing the method added by the old style extension. Note that there's no cycle if we use a new style extension method instead.

import scala.language.implicitConversions

case class Foo(i: Int) extends AnyVal:
  def toFoo = this

case class Bar(i: Int) extends AnyVal

extension (bar: Bar)
  def toFoo = Foo(bar.i)
  
val x =
  implicit def barToFoo(bar: Bar): Foo = bar.toFoo
  val foo: Foo = Bar(1)
@som-snytt
Copy link
Contributor

duplicates #10947

Extensions are not immune, but it depends on whether they come from implicit scope:

import scala.language.implicitConversions

case class Foo(i: Int) extends AnyVal:
  def toFoo = this

case class Bar(i: Int) extends AnyVal

object Bar:
  extension (bar: Bar) def toFoo = Foo(bar.i)

implicit def barToFoo(bar: Bar): Foo = bar.toFoo  // implicit def barToFoo(bar: Bar): Foo = barToFoo(bar).toFoo

@main def test() = println {
  val foo: Foo = Bar(1)
  foo
}

The 2-step similar to Scala 2 is at the bottom of https://docs.scala-lang.org/scala3/reference/contextual/extension-methods.html where it explains: "This second rewriting is attempted at the time where the compiler also tries an implicit conversion..."

This is a classic puzzler; I hit it every time I code a quick example because converting string to int is the first thing I reach for, like reaching for my eyeglasses from the nightstand when my sleep is disturbed, and always knocking over the water glass that is also handy.

@prolativ
Copy link
Contributor Author

Maybe we could remove the method providing the implicit conversion from the implicit scope just inside this method? I guess that should be relatively easy and not influence any other rules of resolving implicits.

olsdavis pushed a commit to olsdavis/dotty that referenced this issue Apr 4, 2022
Broadens Martin's fix for scala#13542

Fixes scala#9880

Also, exempt Boolean's && and || methods, which aren't set up as having
by-name parameters.

Co-authored-by: Dale Wijnand <[email protected]>
@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants