Inline expr incorrectly translates by-value references to by-name

[japgolly]

Compiler version

3.1.2

Minimized code

BUG.scala:

import scala.quoted.*

object BUG {

  class ApiWithMacros[A] {
    def remember(a: A): ApiWithMacros[A] =
      this

    final inline def test: Unit =
      ${ macroDefinition('this) }
  }

  def macroDefinition[A](self: Expr[ApiWithMacros[A]])(using Quotes): Expr[Unit] = {
    import quotes.reflect.*
    val callSite = self.asTerm.underlying
    println("=" * 80)
    println(callSite.show)
    println()
    println(callSite)
    println()
    '{ () }
  }
}

demo.scala:

object Demo {

  // These 3 lines work correctly
  val api = new BUG.ApiWithMacros[AnyRef]
  val n = new AnyRef
  api.remember(n).remember(n).test

  val block = {
    // These 3 lines are the same as above, just enclosed in a block
    val api = new BUG.ApiWithMacros[AnyRef]
    val n = new AnyRef
    api.remember(n).remember(n).test
  }

  def method(): Unit = {
    // These 3 lines are the same as above, just enclosed in a method
    val api = new BUG.ApiWithMacros[AnyRef]
    val n = new AnyRef
    api.remember(n).remember(n).test
  }
}

Output

Each 3-line block above prints the following at compile-time:

================================================================================
Demo.api.remember(Demo.n).remember(Demo.n)

Apply(Select(Apply(Select(Ident(api),remember),List(Ident(n))),remember),List(Ident(n)))

================================================================================
new BUG.ApiWithMacros[scala.AnyRef]().remember(new scala.AnyRef()).remember(new scala.AnyRef())

Apply(Select(Apply(Select(Apply(TypeApply(Select(New(AppliedTypeTree(Select(Ident(BUG),ApiWithMacros),List(Ident(AnyRef)))),<init>),List(TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),type AnyRef)])),List()),remember),List(Apply(Select(New(Ident(AnyRef)),<init>),List()))),remember),List(Apply(Select(New(Ident(AnyRef)),<init>),List())))

================================================================================
new BUG.ApiWithMacros[scala.AnyRef]().remember(new scala.AnyRef()).remember(new scala.AnyRef())

Apply(Select(Apply(Select(Apply(TypeApply(Select(New(AppliedTypeTree(Select(Ident(BUG),ApiWithMacros),List(Ident(AnyRef)))),<init>),List(TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),type AnyRef)])),List()),remember),List(Apply(Select(New(Ident(AnyRef)),<init>),List()))),remember),List(Apply(Select(New(Ident(AnyRef)),<init>),List())))

Expectation

The 1st result is correct; we see two Ident(n) clauses which is correct because n is being referenced by-value.

The 2nd and 3rd results are incorrect. You can see that n has been inlined to be effectively by-name, resulting in two separate new AnyRef calls. Imagine that n is something obviously-mutable like List.newBuilder and you can see how disastrous changing the semantics to by-name is.

[japgolly]

Oh and FYI, inspecting c.macroApplication in a Scala 2, I correctly see Ident(n) in all 3 cases.

[odersky]

There's too much macro operation for me to see whether something is wrong here. What should have been the expected code in the 2nd and 3rd case? And what would be the expectation why and where that code would be generated?

[japgolly]

I'll try to clarify

There's too much macro operation for me to see whether something is wrong here.

The macro only does this following one line, and then prints out the output.

val callSite = self.asTerm.underlying

What should have been the expected code in the 2nd and 3rd case?

Instead of

new BUG.ApiWithMacros[scala.AnyRef]().remember(new scala.AnyRef()).remember(new scala.AnyRef())

which contains

new scala.AnyRef()

twice, it should be like the first example, like this:

new BUG.ApiWithMacros[scala.AnyRef]().remember(n).remember(n)

I've replaced new scala.AnyRef() with n.

And what would be the expectation why and where that code would be generated?

Sorry I don't understand this question.

[odersky]

I guess it's the underlying that goes from n to new AnyRef(). But I think that's as specced?

In any case you are dealing here with a low-level API for term plumbing. It seems that any expectations of call-by-name vs call-by-value are up to the macro. They are not guaranteed by the API.

@nicolasstucki What is your opinion here?

[japgolly]

Ah that's interesting. For reference, using .underlying is the only way I could figure out how to get close to macroApplication from Scala 2. If there's something else I should be using please let me know and I'll play around with it and see if it's still a problem

[nicolasstucki]

From a quick read though the issue it seems that underlying is doing what it is supposed to do.

As I understand, the code should recover the api.remember(n).remember(n) prefix of the test method. If that is the case, the test method should be an extension method with an inline prefix.

extension [A](inline self: ApiWithMacros[A]) inline def test: Unit =
      ${ macroDefinition('self) }

[japgolly]

I tried that out like this:

-    final inline def test: Unit =
-      ${ macroDefinition('this) }
   }
 
+  extension [A](inline self: ApiWithMacros[A]) inline def test: Unit =
+    ${ macroDefinition('self) }
+
   def macroDefinition[A](self: Expr[ApiWithMacros[A]])(using Quotes): Expr[Unit] = {
     import quotes.reflect.*
-    val callSite = self.asTerm.underlying
+    val callSite = self.asTerm

and now it works as expected

================================================================================
Demo.api.remember(Demo.n).remember(Demo.n)

Inlined(EmptyTree,List(),Apply(Select(Apply(Select(Ident(api),remember),List(Ident(n))),remember),List(Ident(n))))

================================================================================
api.remember(n).remember(n)

Inlined(EmptyTree,List(),Apply(Select(Apply(Select(Ident(api),remember),List(Ident(n))),remember),List(Ident(n))))

================================================================================
api.remember(n).remember(n)

Inlined(EmptyTree,List(),Apply(Select(Apply(Select(Ident(api),remember),List(Ident(n))),remember),List(Ident(n))))

so thank you! 🎉 It's a great workaround. Now the question is should the use of an extension method be required? Shouldn't there be away to accomplish the same thing directly from the method in the trait? WDYT?

[nicolasstucki]

The extension method solution is by design and not a workaround.

The limitation we have is that we do not have a way to state that the this is inline for a particular method. Maybe we could have something like

class A:
  // mimic extension method syntax to annotate  the `this`
  inline (inline this) def foo: Int = ...

or

class A:
  @inlineThis inline def foo: Int = ...

[odersky]

I think steering people towards extension methods in this case is fine.

[odersky]

Should we move this to feature requests?

[nicolasstucki]

Yes, we should move it to feature requests

[japgolly]

Understood. Thank you both for the help