Skip to content
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

Assertion error during macro generation for an enum #20349

Open
MateuszKubuszok opened this issue May 7, 2024 · 9 comments · May be fixed by #22240
Open

Assertion error during macro generation for an enum #20349

MateuszKubuszok opened this issue May 7, 2024 · 9 comments · May be fixed by #22240
Assignees
Labels
area:metaprogramming:quotes Issues related to quotes and splices area:metaprogramming:reflection Issues related to the quotes reflection API area:transform itype:question

Comments

@MateuszKubuszok
Copy link

MateuszKubuszok commented May 7, 2024

Compiler version

3.3.3

Minimized code

repro.scala

//> using scala 3.3.3

import scala.quoted.*

object Macros {


  def valuesImpl[A: Type](using quotes: Quotes): Expr[List[A]] = {
    import quotes.*, quotes.reflect.*

    extension (sym: Symbol)
     def isPublic: Boolean = !sym.isNoSymbol &&
          !(sym.flags.is(Flags.Private) || sym.flags.is(Flags.PrivateLocal) || sym.flags.is(Flags.Protected) ||
            sym.privateWithin.isDefined || sym.protectedWithin.isDefined)

    def isSealed[A: Type]: Boolean =
      TypeRepr.of[A].typeSymbol.flags.is(Flags.Sealed)

    def extractSealedSubtypes[A: Type]: List[Type[?]] = {
      def extractRecursively(sym: Symbol): List[Symbol] =
        if sym.flags.is(Flags.Sealed) then sym.children.flatMap(extractRecursively)
        else if sym.flags.is(Flags.Enum) then List(sym.typeRef.typeSymbol)
        else if sym.flags.is(Flags.Module) then List(sym.typeRef.typeSymbol.moduleClass)
        else List(sym)

      extractRecursively(TypeRepr.of[A].typeSymbol).distinct.map(typeSymbol =>
        typeSymbol.typeRef.asType
      )
    }

    if isSealed[A] then {
      val refs = extractSealedSubtypes[A].flatMap { tpe =>
        val sym = TypeRepr.of(using tpe).typeSymbol
        val isCaseVal = sym.isPublic && sym.flags
          .is(Flags.Case | Flags.Enum) && (sym.flags.is(Flags.JavaStatic) || sym.flags.is(Flags.StableRealizable))

        if (isCaseVal) then List(Ref(sym).asExprOf[A])
        else Nil
      }
      Expr.ofList(refs)
    } else '{ Nil }
  }

  inline def values[A]: List[A] = ${ valuesImpl[A] }
}

repro.test.sc

//> using dep org.scalameta::munit:1.0.0-RC1

object domain:
  enum PaymentMethod:
    case PayPal(email: String)
    case Card(digits: Long, name: String)
    case Cash

println(Macros.values[domain.PaymentMethod])
scala-cli test .

Output

Compiling project (Scala 3.3.3, JVM (17))
Error compiling project (Scala 3.3.3, JVM (17))
Error: Unexpected error when compiling eldupa7_d832c8dfee: java.lang.AssertionError: assertion failed: missing outer accessor in class repro$u002Etest$_

Expectation

repro.test.sc should compile successfully (and print something like List(Cash)).

@MateuszKubuszok MateuszKubuszok added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels May 7, 2024
@Gedochao Gedochao added area:transform area:metaprogramming:reflection Issues related to the quotes reflection API 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 May 9, 2024
@jchyb
Copy link
Contributor

jchyb commented May 9, 2024

Let's rewrite repro.test.sc into repr.scala kind of like scala-cli would to make things a bit clearer:

class Test {
  object domain {
    enum PaymentMethod:
      case PayPal(email: String)
      case Card(digits: Long, name: String)
      case Cash
  }
  println(Macros.values[domain.PaymentMethod])
}
object Test {
  lazy val script = new Test()
  def main(args: Array[String]): Unit =
    val _ = script.hashCode()
}

Running this with -Xcheck-macros brings us a more clear problem with the macro method than shown in the compiler crash:

-- Error: /Users/jchyb/workspace/scalabug/2/repro1.scala:8:23 ------------------
 8 |  println(Macros.values[domain.PaymentMethod])
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |Malformed tree was found while expanding macro with -Xcheck-macros.
   |               |The tree does not conform to the compiler's tree invariants.
   |               |
   |               |Macro was:
   |               |scala.quoted.runtime.Expr.splice[scala.collection.immutable.List[Test.this.domain.PaymentMethod]](((evidence$1: scala.quoted.Quotes) ?=> Macros.valuesImpl[Test.this.domain.PaymentMethod](scala.quoted.Type.of[Test.this.domain.PaymentMethod](evidence$1), evidence$1)))
   |               |
   |               |The macro returned:
   |               |scala.List.apply[Test.this.domain.PaymentMethod](PaymentMethod.this.Cash)
   |               |
   |               |Error:
   |               |assertion failed: error while typing PaymentMethod.this, value <local Test> is not contained in object PaymentMethod
   |               |
   |stacktrace available when compiling with `-Ydebug`
   |               |
   |----------------------------------------------------------------------------
   |Inline stack trace
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from repro.scala:50
50 |  inline def values[A]: List[A] = ${ valuesImpl[A] }
   |                                  ^^^^^^^^^^^^^^^^^^
    ----------------------------------------------------------------------------

Evidently, the macro generates us code with PaymentMethod.this.Cash, that it expects to be in context of PaymentMethod (which is why we have this in there). However when we inline that code into the println in Test, we do not have direct access to the PaymentMethod, but instead we have to go through domain.PaymentMethod.

The issue in the macro method lies with the use of typeSymbol.typeRef.asType. typeRef returns us a TypeRepr that is usable only in scope of its owner (https://dotty.epfl.ch/api/scala/quoted/Quotes$reflectModule$SymbolMethods.html#typeRef-d26). Replacing that with TypeRepr.of[A].memberType(typeSymbol) fixes the crash.

@MateuszKubuszok
Copy link
Author

I did some experimentation. I found that:

class OuterClass {
  type InnerType
}

would indeed confuse macros - make them throw AssertionError - when typeSymbol.typeRef is used and would be solved by TypeRepr.of[A].memberType(typeSymbol)... but only for Scala 3 enums:

  • sealed trait with case objects and case classes defined in the companion works fine
  • Java enums defined in a separate .java file (obviously) also works fine

However, for case like:

class Snippet { // simluating what .sc file creates

  enum Foo:
    case Bar(a: Int) // with parameter (separate class)
    case Baz // parameterless (val with valueName.type)
}

for TypeRepr.of[Foo].children.map(c => TypeRepr.of[Foo].memberType(c)) would produce List(Bar, Foo) - parameterless values are upcasted which makes them impossible to handle properly. If TypeRepr.of[A].memberType(subtype) is used to Java enums, they stop working as well.

So while use TypeRepr.of[A].memberType(typeSymbol) instead might the correct answer, it's an incomplete answer, it solves AssertionError but removed the ability to handle some cases.

Perhaps we can consider this bug ticket solved, but I believe that some full canonical answer should be posted here, in case someone else has the same issue.

@OndrejSpanel
Copy link
Member

This reminds me of #19825 - it is impossible to get the correct inner type using typeMember method when dealing with path dependent types.

@jchyb
Copy link
Contributor

jchyb commented Nov 29, 2024

Apologies for the late reply.
TL:DR Using typeMember was a bad suggestion, select (like TypeRepr.of[A].select(c)) for the use case here is a much better choice that fixes issues above. I apologize for the error! I explain everything that’s going on, in detail, below:

.typeRef

First of, the reason why we should be careful with the .typeRef method is because we are calling it on Symbol, which, by design, might not have all of the information we need here. Symbol can reference any part of the code that defines a name and its attributes. E.g.:

trait Foo[T]: // <- symbols for Foo and T defined here
  val foo: T = ??? // <- symbol for foo defined here 
class Bar extends Foo[Int] // <- symbol for Bar defined here
class Baz extends Foo[String] // <- symbol for Baz defined here

Those are precisely all of the symbols defined here. So if we were to get the Symbol for Bar, then ask the compiler for the foo member symbol (TypeRepr.of[Bar].typeSymbol.fieldMember(„foo”)), we would have as much information about it, as if started from Baz or Foo
(TypeRepr.of[Bar].typeSymbol.fieldMember(„foo”) == TypeRepr.of[Baz].typeSymbol.fieldMember(„foo”)).

This means that this might not work well (or at all) with path dependent types. E.g.:

  • calling TypeRepr.of[Bar].typeSymbol.fieldMember(„foo”).typeRef results with TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),trait Foo)),val foo), or Foo.this.foo, which is usable only in the scope of a Foo class

Of course, for simpler code, typeRef will work, as we do not always need that prefix type information:

object Foo:
  val foo: Int = 0 // calling TypeRepr.of[Foo].typeSymbol.fieldMember(„foo”).typeRef just returns Int, we can even get a reference to foo via Ref(TypeRepr.of[Foo].typeSymbol.fieldMember(„foo”)) and use it in a macro

TypeRepr methods

There are 2 ways of using those symbols with a more precise type information (via a TypeRepr) to get more information about those fields in those contexts.

For

val parentTypeRepr: TypeRepr = TypeRepr.of[Bar]
val symbol: Symbol = TypeRepr.of[Foo].typeSymbol.fieldMember(„foo”)
  • parentTypeRepr.memberType(symbol) will return the TypeRepr of foo, as seen from Bar
  • parentTypeRepr.select(symbol) will return a TermRef as seen from parentTypeRepr (so a type signifying a symbol with a precise type information). Depending on what kind of symbol we pass to the select, it might also return a TypeRef.

enums

(This part I am not an expert of, so certain subtleties might be wrong, but the general logic should still apply)

Scala compiler, when reading java enums (let's say we have enum Parent{ Child }), instead of defining multiple new types, defines a Parent type and a val of Child of the Parent type. Scala 3 enums are similar where possible, so parameterless enum cases are also desugared into something akin to a val, and cases with parameters are desugared into classes with a companion val (where we access the apply for the parameters). This is where the difference between using a .memberType and .select come in. Classes have types, so memberType on an enum case with parameters will return a TypeRef to that class, but for vals, it will point to the type of that val, so to the general enum type. Same happens for java enums. Using .select we just get a TermRef to the val (TermRef(typePrefix, val symbol)), which I assume is what we want here.

@MateuszKubuszok
Copy link
Author

MateuszKubuszok commented Dec 4, 2024

@jchyb So, if I understand correctly, when I obtain from .children a sym: Symbol of a subtype, I should:

  • check whether it's a parameterless enum case/Java enum - sym.flags.is(Flag.Enum) is ok for that?
  • if it is, then find its parent module(?)
  • then use parentTypeModule.select(symbol) - should I be using the same sym that I obtained from .children or rather I should query module for a member with the same name as sym?

I am asking because that quickly becomes rather hard to do it right. I am starting to think that perhaps it would be easier to Expr.summon[Mirror.SumOf[A]] and extract its MirroredElemLabels, which would impose an overhead, but wouldn't make Chimney deal with errors that Ducktape simply avoided.

@jchyb
Copy link
Contributor

jchyb commented Dec 4, 2024

I started writing a detailed response, but while trying something out I happened upon a legitimate crash, not reported by -Xcheck-macros... I am right now trying to better diagnose it.

@jchyb
Copy link
Contributor

jchyb commented Dec 17, 2024

While writing my previous explanations here I focused on the fact that that a lot of typeRefs are used, which only work in local contexts... and I did not notice that in our minimisation:

class Test {
  object domain {
    enum PaymentMethod:
      case PayPal(email: String)
      case Card(digits: Long, name: String)
      case Cash
  }
  println(Macros.values[domain.PaymentMethod])
}

we are indeed expanding the macro in a local context of Test.this, so the symbol.typeRef calls should work (and they do!). The immediate problem here lies with Ref(symbol) method, and it's not only to limited to enum symbols, but for all val references e.g.:

class Cls{
  object a {
    object domain {
      val value = ""
    }
  }
  println(Macros.values[a.domain.type])
}

object Test {
  lazy val script = new Cls()
  def main(args: Array[String]): Unit =
    val _ = script.hashCode()
    ???
}
import scala.quoted.*

object Macros {
  def valuesImpl[A: Type](using Quotes): Expr[Any] = {
    import quotes.*, quotes.reflect.*
    val symbol = TypeRepr.of[A].typeSymbol.fieldMember("value")
    Ref(symbol).asExprOf[Any]
  }

  transparent inline def values[A]: Any = ${ valuesImpl[A] }
}

our constructed Ref(symbol) returns Select(This(Ident(domain$)),value), which requires us be in the context of domain (which is unnecessary, as that is an object). This doesn't happen when Cls is an object, as in that case we receive an Ident(value). I am currently working on an improvement so that the Ref calls will result in trees usable in larger scope (assuming those changes are accepted). Current workaround is to construct trees manually (which I understand might be difficult), or use Ref.term() (which uses TermRefs which might be even more difficult to construct, in part due to the lack of checks, as reported here: #22151)

@MateuszKubuszok
Copy link
Author

Great thanks for the explanation!

Should we assume that the improvement will be backported to 3.3 line or for LTS should I construct the trees manually?

@jchyb
Copy link
Contributor

jchyb commented Dec 17, 2024

Unsure about LTS, I'd personally prefer if it was backported, and I don't see anybody relying on that generated incorrect This() in the macro code, but if something would break in the open community build, it would be reverted from there. But it should probably be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metaprogramming:quotes Issues related to quotes and splices area:metaprogramming:reflection Issues related to the quotes reflection API area:transform itype:question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants