Skip to content

Macro extending selectable: very strange select behavior when accessing member value #20100

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
hmf opened this issue Apr 5, 2024 · 7 comments
Assignees
Labels

Comments

@hmf
Copy link

hmf commented Apr 5, 2024

Compiler version

3.4.2-RC1

Minimized code

Macro file:

package examples.instantiate

import scala.annotation.experimental
import scala.quoted.*

@experimental
object MyMacro5Vals:

  trait SelectableBase extends scala.Selectable:
    val test = "test"
    transparent inline def selectDynamic(name: String): Any =
      ${selectDynamicImpl('this, 'name)}



  def selectDynamicImpl[A:Type](inner: Expr[A], name: Expr[String])(using quotes: Quotes): Expr[Any] =
    import quotes.reflect.*
    // Following true if: we do not declare nor override the macro
    // If I use "fieldx" and it is not defined get correct compile error 
    // If I use "field" and it is defined get incorrect compile error saying field does not take parameters 
    // Seems lie issue https://github.com/scala/scala3/issues/20044
    // If I use "test" and it is defined in base get no compile error and correct output!?!? 
    // Select.unique(inner.asTerm, name.valueOrAbort).asExpr
    Select.unique(inner.asTerm, "test").asExpr


  transparent inline def client[I](r: I): Any = ${MyMacro5Vals.clientImpl[I]('r)}

  def clientImpl[I: Type](r: Expr[I])(using Quotes): Expr[Any] = 
    import quotes.reflect.*

    val selectable = TypeTree.of[SelectableBase]


    // First parent must be a class
    val parentsX = List(TypeTree.of[Object], selectable)
    def declsX(cls: Symbol): List[Symbol] =
      List(
        Symbol.newVal(cls, "field", TypeRepr.of[I], Flags.Inline, Symbol.noSymbol ),
        )
    val clsX = Symbol.newClass(Symbol.spliceOwner, "Struct", parents = parentsX.map(_.tpe), declsX, selfType = None)

    // Adding value members
    val fieldSymbol = clsX.declaredField("field")
    val fieldValue = r.asTerm
    val fieldDef2: ValDef = ValDef.apply(fieldSymbol, Some(fieldValue))
    val clsDefX = ClassDef(clsX, parentsX, body = List(fieldDef2))

    // Build constructor
    val yy = TypeTree.ref(clsX.info.typeSymbol)
    val newClsX = Typed(Apply(Select(New(TypeIdent(clsX)), clsX.primaryConstructor), Nil), yy)

    val typ1 = clsX.info
    val tt = yy.tpe
    val ttt = yy.tpe.asType
    val blk = ttt match
          case '[t] => 
            Block(List(clsDefX), newClsX).asExprOf[t]

    println("blk:")
    println(blk.show)
    blk


end MyMacro5Vals

Using the macro in another file:

package examples.instantiate

package test

import scala.annotation.experimental

@experimental
@main
def Instantiate5Vals: Unit =
  val myCommand = MyMacro5Vals.client[Int](3)
  println(myCommand.getClass())
  val x = myCommand.field
  println(s"x = $x")

Output

[39/46] meta.compile 
[info] compiling 1 Scala source to /home/hmf/VSCodeProjects/sploty/out/meta/compile.dest/classes ...
[info] done compiling
[info] compiling 1 Scala source to /home/hmf/VSCodeProjects/sploty/out/meta/compile.dest/classes ...
blk:
{
  class Struct extends java.lang.Object with examples.instantiate.MyMacro5Vals.SelectableBase {
    val field: scala.Int = 3
  }

  (new Struct(): Struct)
}
[info] done compiling
[46/46] meta.runMain 
class examples.instantiate.test.Instantiate5Vals$package$Struct$1
x = 3

Expectation

The selectDynamicImpl used this:

    Select.unique(inner.asTerm, "test").asExpr

so I expect the value "test" of trait SelectableBase. Instead I get 3 that is the value of the newly defined an instantiated Struct.field.

What is more troubling is that if I change the code to:

    Select.unique(inner.asTerm, name.valueOrAbort).asExpr

I get:

hmf@gandalf:~/VSCodeProjects/sploty$ ./mill -i meta.runMain examples.instantiate.test.Instantiate5Vals
[39/46] meta.compile 
[info] compiling 1 Scala source to /home/hmf/VSCodeProjects/sploty/out/meta/compile.dest/classes ...
[info] done compiling
[info] compiling 1 Scala source to /home/hmf/VSCodeProjects/sploty/out/meta/compile.dest/classes ...
blk:
{
  class Struct extends java.lang.Object with examples.instantiate.MyMacro5Vals.SelectableBase {
    val field: scala.Int = 3
  }

  (new Struct(): Struct)
}
[error] -- [E050] Type Error: /home/hmf/VSCodeProjects/sploty/meta/src/examples/instantiate/Instantiate5Vals.scala:43:10 
[error] 43 |  val x = myCommand.field
[error]    |          ^^^^^^^^^^^^^^^
[error]    |          expression does not take parameters
[error]    |----------------------------------------------------------------------------
[error]    |Inline stack trace
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    |This location contains code that was inlined from Instantiate5Vals.scala:11
[error] 11 |* override the necessary method. 
[error]    |              ^
[error] 12 |* 
[error] 13 |* https://github.com/com-lihaoyi/mill/discussions/2663
[error]     ----------------------------------------------------------------------------
[error]    |----------------------------------------------------------------------------
[error]    | Explanation (enabled by `-explain`)
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    | You have specified more parameter lists than defined in the method definition(s).
[error]     ----------------------------------------------------------------------------
[error] one error found
1 targets failed
meta.compile Compilation failed

Using any member that does not exist in the base class or the new Struct produces the expected compilation error:

info] compiling 2 Scala sources to /home/hmf/VSCodeProjects/sploty/out/meta/compile.dest/classes ...
blk:
{
  class Struct extends java.lang.Object with examples.instantiate.MyMacro5Vals.SelectableBase {
    val field: scala.Int = 3
  }

  (new Struct(): Struct)
}
[error] -- [E008] Not Found Error: /home/hmf/VSCodeProjects/sploty/meta/src/examples/instantiate/Instantiate5Vals.scala:43:20 
[error] 43 |  val x = myCommand.fieldx
[error]    |          ^^^^^^^^^^^^^^^^
[error]    |value fieldx is not a member of examples.instantiate.MyMacro5Vals.SelectableBase{val field: (3 : Int)}
@hmf hmf added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 5, 2024
@hmf
Copy link
Author

hmf commented Apr 5, 2024

I would like to add the following: if I add several member values to the extending class Struct, and retain the Select.unique(inner.asTerm, "test").asExpr, I can access any of its members correctly.

@Gedochao Gedochao added area:metaprogramming:quotes Issues related to quotes and splices and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 5, 2024
@jchyb
Copy link
Contributor

jchyb commented Apr 5, 2024

Huh, so if I understand correctly, it looks like after generating and instantiating a Selectable class in a macro method:

  • when calling the selectDynamic which is a macro method, it looks like it's both expanded, and that expansion is subsequently ignored, instead using the static field (which I believe the type checker should not have access to and should not allow to call directly)
  • additionally, when I removed Flags.Inline from Flags used in "field" Symbol, I started getting runtime errors:
Exception in thread "main" java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Integer (java.lang.String and java.lang.Integer are in module java.base of loader 'bootstrap')
        at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:99)
        at Main$.main(main.scala:12)
        at Main.main(main.scala)

@hmf
Copy link
Author

hmf commented Apr 5, 2024

In regards to your first point, it seems like it. Note that I may be doing something wrong in the code above or it may be an interaction of some other kind.

However, note that If I remove the base class member test, and access the existing members (example field and field1), I get:

value test is not a member of examples.instantiate.MyMacro5Vals.SelectableBase{
  val field: (3 : Int); val field1: ("f2" : String)}bloop(8)

As for the second point, honestly I cannot say why I used that flag. Now that you mention it, it does not seem to make sense here (not required). Did you use Flags.EmptyFlags?

EDIT: was able to reproduce the same run-time error.

@hmf
Copy link
Author

hmf commented Apr 7, 2024

I tried to replicate the initial code, but without the macro base class construction. Here is what I came up with:

object Hidden6:
  import scala.quoted.*

  trait SelectableBase extends Selectable:
    val test = "test"
    transparent inline def selectDynamic(name: String): Any =
      ${selectDynamicImpl('this, 'name)}
    def applyDynamic(name: String)(args: Any*): Any = ???

  def selectDynamicImpl[A:Type](inner: Expr[A], name: Expr[String])(using quotes: Quotes): Expr[Any] =
    import quotes.reflect.*
    Select.unique(inner.asTerm, name.valueOrAbort).asExpr
    ???
  
  class RecH() extends SelectableBase:
    // Accessed directly, no select dynamic 
    val name: String = "C" 
    val age: Int = 2
    val sex: Char = 'M'


  def makeRecH() =
    new RecH()

If I use it as follows:

  val person12 = Hidden6.makeRecH()
  println(person12.test)
  println(person12.name)
  println(person12.sex)

It compiles and runs. So the direct access is correct (even for the base test member). The dynamic selection is invoked only for those members of the class that directly inherits from Selectable via an anonymous class. This makes sense, because it limits the use of these calls to a known set of members.

So the the issue here is why Select.unique(inner.asTerm, name.valueOrAbort).asExpr cannot be used as in this equivalent example (removing ???).

@Gedochao Gedochao added the itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException) label Apr 8, 2024
@jchyb
Copy link
Contributor

jchyb commented May 16, 2024

Ah, so it looks like my analysis before was incorrect - I did not know about how the structural type was automatically created for anonymous classes. I see that selectDynamic is going to be only called for the refinement members (member1 and member2 in type TypeSeenFromLocalScope {val member1: t1; val member2: t2 ...}) So with that in mind:

  • the class cast runtime exception when returning a different type from a macro selectDynamic makes sense - the compiler expects to use a default selectDynamic implementation here (which just returns the val), but we replace it with one where we return an incorrect type. Even though the macro is transparent, type checker only takes the refinement type into consideration here (because it expects to use the default implementation). Perhaps we could add a new check to -Xcheck-macros, that would confirm if the typing is correct in situations like this (but this seems like a low priority, as in non-macro selectDynamic the correct cast is a responsibility of the user, so I am not sure it should be different here).
  • In the latest example as you mentioned the selectDynamic is not executed, because the type has no refined members. We can force it by adding a refinement to makeRecH, like def makeRecH() = new RecH{val test2: Int = 1}. Then we can run person12.test2. However, returning Select.unique(inner.asTerm, name.valueOrAbort).asExpr will not work here, as there we are trying to select val test2 from this: SelectableBase, which does not have that val (other macros can success, as long as it returns an Int of course, otherwise we get the runtime class cast exception).

So this means the only things to fix here is the macro unnecessarily running for inline vals in anonymous classes/refinements, like in the original example, or in the last one if we do something like def makeRecH() = new RecH() {inline val test2 = 1}, and call person12.test2 - macro is run and subsequently ignored.

@jchyb jchyb added area:inline and removed itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException) labels May 16, 2024
@hmf
Copy link
Author

hmf commented May 16, 2024

Seems to make sense what you say. I am going with the first option - remove the selectDynamic macro and use a standard method.

From my side, this issue can be closed.

@jchyb
Copy link
Contributor

jchyb commented Jun 30, 2024

After some consideration, I decided not to make any changes in the compiler relating to this. Let me explain (this is mostly for the compiler mainteiners and anyone who will choose to revisit this in the future). Let’s start with a minimization:
Test.scala

object Test:
  def main(args: Array[String]): Unit = 
    val rec = makeSelectable()
    rec.number

Macro.scala

import scala.quoted.*

trait SelectableBase extends Selectable:
  val test: String = "a"
  transparent inline def selectDynamic(name: String): Any =
    ${selectDynamicImpl('this, 'name)}

def selectDynamicImpl[A:Type](inner: Expr[A], name: Expr[String])(using quotes: Quotes): Expr[Any] =
  import quotes.reflect.*
  println("MACRO CALLED")
  Select.unique(inner.asTerm, "test").asExpr

def makeSelectable() = // : SelectableBase{val age: 0} 
  new SelectableBase(){inline val number = 0}

The problem here is the fact that calling rec.number will call the selectDynamic macro, however the value returned by that macro will be replaced by the literal type later in a compilation, which can feel a little unexpected.
This is actually because calls to selectDynamic are wrapped into a cast to a refinement type (here a literal type), somewhat like this:

rec.selectDynamic("number”).asInstanceOf[0]

Later in the FirstTransform phase, casts to literal types are optimized into the contents of the type, so here it would change into:

0

So even though the macro was executed in an earlier phase, the results were discarded. I found this a bit confusing so I tried to „fix” that, thinking of 3 ways to do so:

  • Throwing an error if the contents of the macro are not the same as the literal type
  • throwing a warning telling users about the cast
  • In the cases where the results of the macro would be optimized out, not executing the macro
    None were satisfactory:
    Errors would be impossible to properly execute, as we might not know the returned value at compiletime (the users may expect the macro to return a runtime call), so a message about an incorrect type may be confusing, and would artificially force users to if out the cases with literal types to achieve the same effect as before.
    The same could be said about warnings.
    Not executing the macro could also be confusing, since in that case we would suddenly have a behavior that effectively inlines the value, without using the macro, but still requiring the macro to exist to trigger (since we would only do this when inlining selectDynamic calls).

All of the above behaviors are also confusing, if not even more…

This connected with the fact that the users can write:

0.asInstanceOf[1]

and expect to obtain 1 after compilation without any cast exceptions (due to the aforementioned FirstTransform) makes me think that there truly is nothing to fix here, as the current behavior might be the least confusing alternative…

@jchyb jchyb closed this as completed Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants