scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.86k stars 1.06k forks source link

Regression: inline implicit conversion get called when unnecessary #19862

Closed Iltotore closed 7 months ago

Iltotore commented 7 months ago

Compiler version

Last working version: 3.3.1 Not working versions: 3.3.3 and 3.4.0

Did not test nightly versions

Minimized code

import scala.language.implicitConversions

object Main:

  implicit inline def uhOh[A](value: A): A =
    compiletime.error("Should not have been called")

  //Same signature than `Function1` but behaves differently (summoning works as expected)
  @annotation.implicitNotFound(msg = "No implicit view available from ${T1} => ${R}.")
  trait Foo[@specialized(Specializable.Arg) -T1, @specialized(Specializable.Return) +R]

  @main
  def testMain =
    summon[Function1[Int, Int]] //Calls `uhOh` in 3.3.3 but not in 3.3.2

Opaque type variant (also not working)

opaque type Bar[A] <: A = A
import scala.language.implicitConversions

object Main:

  implicit inline def uhOh[A](value: A): Bar[A] =
    compiletime.error("Should not have been called")

  //Same signature than `Function1` but behaves differently (summoning works as expected)
  @annotation.implicitNotFound(msg = "No implicit view available from ${T1} => ${R}.")
  trait Foo[@specialized(Specializable.Arg) -T1, @specialized(Specializable.Return) +R]

  @main
  def testMain =
    summon[Function1[Int, Int]] //Calls `uhOh` in 3.3.3 but not in 3.3.2

Output

[error] 14 |    summon[Function1[Int, Int]] //Calls `uhOh` in 3.3.3 but not in 3.3.2
[error]    |                               ^
[error]    |                               Should not have been called

Notes:

Expectation

I would expect this sample to compile as it did in 3.3.1.

Real world example

uhOh looks meaningless but this issue also breaks actually useful code. For example in a library I have an inline implicit conversion which converts a type A to an opaque type IronType[A, C] <: A impacted by the problem described above, for example in its Scalacheck module.

nicolasstucki commented 7 months ago
summon[Function1[Int, Int]]

is inlined in typer as

val proxy: Int => Int = (value: Int) => uhOh[Int](value)
proxy

as expected.

Then in the inlining phase uhOh is inlined and emits the error. This seems to be the correct behavior for this case.

Iltotore commented 7 months ago

I get what you mean for the first sample. I think I got confused because I used it for implicit conversions (since Conversion does not support inline AFAIK). However, since it breaks code in 3.3.3 that worked in 3.3.1, I am not sure if it should be considered a bug or not.

nicolasstucki commented 7 months ago

This behavior changed in a7bae7a37e6af82aaff1c4c21b8a57e73ab7e402.

@smarter the change in implicit conversion consistent with the eta-expansion change you did in that commit?

smarter commented 7 months ago

It seems like this only worked by chance before. We could revert this change for 3.3.4 to keep compatibility, but I don't see a reason to revert it from 3.4.0. Especially since a future Scala version is supposed to deprecate implicit def.

since Conversion does not support inline AFAIK

You can put an inline def in Conversion, but you have to keep in mind the desugaring:

  given foo: Conversion[Int, String] with
    inline def apply(x: Int): String = x.toString

becomes:

  given foo: foo = new foo()
  class foo extends Conversion[Int, String]:
    inline def apply(x: Int): String = x.toString
    private def apply$retainedBody(x: Int): String = apply(x)

(and when emitting bytecode, the inline def apply is replaced by a plain method with the body of apply$retainedBody)

So your inline def needs to be callable from a non-inline def with the same signature as the inline def.

smarter commented 7 months ago

I think we could tweak implicit search so that implicit inline def foo(a: A): B doesn't show up when looking for an implicit Function1[A, B] to restore the behavior of 3.3 here, but if Conversion doesn't work for your usecase, I'd encourage you to open a thread on https://contributors.scala-lang.org/ to discuss it before implicit def is deprecated.

soronpo commented 7 months ago

if Conversion doesn't work for your usecase, I'd encourage you to open a thread on https://contributors.scala-lang.org/ to discuss it before implicit def is deprecated.

There is already such a thread https://contributors.scala-lang.org/t/before-deprecating-old-style-implicit-conversions-we-need-this/6385/

Iltotore commented 7 months ago

It seems like this only worked by chance before. We could revert this change for 3.3.4 to keep compatibility, but I don't see a reason to revert it from 3.4.0. Especially since a future Scala version is supposed to deprecate implicit def.

since Conversion does not support inline AFAIK

You can put an inline def in Conversion, but you have to keep in mind the desugaring:

  given foo: Conversion[Int, String] with
    inline def apply(x: Int): String = x.toString

becomes:

  given foo: foo = new foo()
  class foo extends Conversion[Int, String]:
    inline def apply(x: Int): String = x.toString
    private def apply$retainedBody(x: Int): String = apply(x)

(and when emitting bytecode, the inline def apply is replaced by a plain method with the body of apply$retainedBody)

So your inline def needs to be callable from a non-inline def with the same signature as the inline def.

I'm afraid this is not compatible with some usages of implicit inline def like getting the value from the Expr of x in a macro which is crucial for my project (and I guess other libraries like Refined).

I will open a new thread on Scala Contributors since I don't think @soronpo's cover the same issue.

Iltotore commented 7 months ago

Link: https://contributors.scala-lang.org/t/equivalent-of-inline-implicit-def/6590