Skip to content

Regression: inline implicit conversion get called when unnecessary #19862

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
Iltotore opened this issue Mar 4, 2024 · 8 comments · Fixed by #19877
Closed

Regression: inline implicit conversion get called when unnecessary #19862

Iltotore opened this issue Mar 4, 2024 · 8 comments · Fixed by #19877

Comments

@Iltotore
Copy link
Contributor

Iltotore commented Mar 4, 2024

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:

  • uhOh is not called in 3.3.1
  • It only happens when summoning a Function1. summon[Foo[Int, Int]] works as expected ("no given instance of Foo...").
  • The generic parameter A is important. Replacing it by Any does not produce the error.

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.

@Iltotore Iltotore added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 4, 2024
@nicolasstucki
Copy link
Contributor

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
Copy link
Contributor Author

Iltotore commented Mar 4, 2024

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
Copy link
Contributor

nicolasstucki commented Mar 4, 2024

This behavior changed in a7bae7a.

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

@nicolasstucki nicolasstucki added stat:needs spec area:inline area:implicits related to implicits and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 4, 2024
@smarter
Copy link
Member

smarter commented Mar 4, 2024

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
Copy link
Member

smarter commented Mar 4, 2024

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.

smarter added a commit to dotty-staging/dotty that referenced this issue Mar 4, 2024
`inline implicit def` is not really a supported feature since it combines Scala
3's `inline` with Scala 2's `implicit` where the latter should eventually be
deprecated. This however didn't prevent at least one project from using this
combination in a way that was broken by scala#18249, see scala#19862 for the details.

The issue is that when definining:

    implicit def foo(x: A): B = ...

Then `foo` is a valid implicit search candidate when looking up an implicit
`Function1[A, B]`. However, before scala#18249 if instead we wrote:

    inline implicit def foo(x: A): B = ...

Then `foo` would be considered as an implicit search candidate but discarded
because eta-expansion was disabled.

There is no particular reason for `inline implicit def` to behave differently
from `implicit def` here, but since `implicit def` is a legacy feature and since
Scala 3.3 is an LTS release, we choose to restore the pre-scala#18249 behavior for
compatibility reasons.

Fixes scala#19862.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 5, 2024
`inline implicit def` is not really a supported feature since it combines Scala
3's `inline` with Scala 2's `implicit` where the latter should eventually be
deprecated. This however didn't prevent at least one project from using this
combination in a way that was broken by scala#18249, see scala#19862 for the details.

The issue is that when definining:

    implicit def foo(x: A): B = ...

Then `foo` is a valid implicit search candidate when looking up an implicit
`Function1[A, B]`. However, before scala#18249 if instead we wrote:

    inline implicit def foo(x: A): B = ...

Then `foo` would be considered as an implicit search candidate but discarded
because eta-expansion was disabled.

There is no particular reason for `inline implicit def` to behave differently
from `implicit def` here, but since `implicit def` is a legacy feature and since
Scala 3.3 is an LTS release, we choose to restore the pre-scala#18249 behavior for
compatibility reasons.

Fixes scala#19862.
@soronpo
Copy link
Contributor

soronpo commented Mar 5, 2024

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/

nicolasstucki added a commit that referenced this issue Mar 5, 2024
`inline implicit def` is not really a supported feature since it
combines Scala 3's `inline` with Scala 2's `implicit` where the latter
should eventually be deprecated. This however didn't prevent at least
one project from using this combination in a way that was broken by
#18249, see #19862 for the details.

The issue is that when definining:

    implicit def foo(x: A): B = ...

Then `foo` is a valid implicit search candidate when looking up an
implicit `Function1[A, B]`. However, before #18249 if instead we wrote:

    inline implicit def foo(x: A): B = ...

Then `foo` would be considered as an implicit search candidate but
discarded because eta-expansion was disabled.

There is no particular reason for `inline implicit def` to behave
differently from `implicit def` here, but since `implicit def` is a
legacy feature and since Scala 3.3 is an LTS release, we choose to
restore the pre-#18249 behavior for compatibility reasons.

Fixes #19862.
@Iltotore
Copy link
Contributor Author

Iltotore commented Mar 5, 2024

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
Copy link
Contributor Author

Iltotore commented Mar 7, 2024

@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
@Kordyjan Kordyjan modified the milestones: 3.4.2, 3.5.0 May 10, 2024
WojciechMazur pushed a commit that referenced this issue Jul 2, 2024
`inline implicit def` is not really a supported feature since it combines Scala
3's `inline` with Scala 2's `implicit` where the latter should eventually be
deprecated. This however didn't prevent at least one project from using this
combination in a way that was broken by #18249, see #19862 for the details.

The issue is that when definining:

    implicit def foo(x: A): B = ...

Then `foo` is a valid implicit search candidate when looking up an implicit
`Function1[A, B]`. However, before #18249 if instead we wrote:

    inline implicit def foo(x: A): B = ...

Then `foo` would be considered as an implicit search candidate but discarded
because eta-expansion was disabled.

There is no particular reason for `inline implicit def` to behave differently
from `implicit def` here, but since `implicit def` is a legacy feature and since
Scala 3.3 is an LTS release, we choose to restore the pre-#18249 behavior for
compatibility reasons.

Fixes #19862.

[Cherry-picked af69895]
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.

5 participants