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

Regression for frugalmechanic/fm-serializer in quotes-reflect + -Xcheck-macros #19842

Closed
WojciechMazur opened this issue Mar 1, 2024 · 7 comments · Fixed by #19870
Closed
Assignees
Labels
area:metaprogramming:quotes Issues related to quotes and splices area:metaprogramming:reflection Issues related to the quotes reflection API itype:invalid regression This worked in a previous version but doesn't anymore
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Based on OpenCB failure of frugalmechanic/fm-serializer - build logs
Under -Xcheck-macros flag we get an error about different sets of parents - the parents of tree don't include class Object which is present in parents of symbol being created. Removing the class Object from list of parents leads to assertion error in the compiler, becouse it requires the first param to be a class, not a trait.

Compiler version

3.4.nightly
Last good release: 3.4.1-RC1-bin-20240210-6efcdba-NIGHTLY
First bad release: 3.4.1-RC1-bin-20240212-c529a48-NIGHTLY
Bisect points to aa5492f

Minimized code

// main.scala
trait Serializer[@specialized T]// extends RawSerializer[T] with NestedSerializer[T] with FieldSerializer[T]
object Serializer{
    implicit inline def implicitMakeSerializer[T]: Serializer[T] = ${ Macros.makeSerializer[T] }
}
case class ValidationCls(string: String)

@main def Test = summon[Serializer[ValidationCls]]
// macros.scala
//> using options -Xcheck-macros
import scala.annotation.{experimental, targetName}
import scala.quoted.*
import scala.util.Try

object Macros {
  def makeSerializer[T: Type](using Quotes): Expr[Serializer[T]] = {
    import quotes.reflect.*

    val tpe: TypeRepr = TypeRepr.of[T]
    val name: String = Symbol.freshName("objectSerializer")

    val modSym: Symbol = Symbol.newModule(
      Symbol.spliceOwner,
      name,
      Flags.Implicit,
      Flags.EmptyFlags,
      // Without TypeRep.of[Object] it would fail with java.lang.AssertionError: assertion failed: First parent must be a class
      List(TypeRepr.of[Object], TypeRepr.of[Serializer[T]]),
      _ => Nil,
      Symbol.noSymbol
    )

    val (modValDef: ValDef, modClassDef: ClassDef) =
      ClassDef.module(modSym, List(TypeTree.of[Serializer[T]]), Nil)

    Block(List(modValDef, modClassDef), Ref(modSym)).asExprOf[Serializer[T]]
  }
}

Output

-- Error: /Users/wmazur/projects/dotty/bisect/main.scala:7:50 ------------------
7 |@main def Test = summon[Serializer[ValidationCls]]
  |                                                  ^
  |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[Serializer[ValidationCls]](((contextual$2: scala.quoted.Quotes) ?=> Macros.makeSerializer[ValidationCls](scala.quoted.Type.of[ValidationCls](contextual$2), contextual$2)))
  |
  |The macro returned:
  |{
  |  object objectSerializer$macro$1 extends Serializer[ValidationCls] { this: objectSerializer$macro$1.type =>
  |    
  |  }
  |  objectSerializer$macro$1
  |}
  |
  |Error:
  |assertion failed: Parents of class symbol differs from the parents in the tree for object objectSerializer$macro$1
  |
  |Parents in symbol: [class Object, trait Serializer]
  |Parents in tree: [trait Serializer]
  |
  |
  |stacktrace available when compiling with `-Ydebug`
  |-----------------------------------------------------------------------------
  |Inline stack trace
  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
  |This location contains code that was inlined from main.scala:3
3 |    implicit inline def implicitMakeSerializer[T]: Serializer[T] = ${ Macros.makeSerializer[T] }
  |                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   -----------------------------------------------------------------------------
1 error found
Compilation failed

Expectation

Probably should not couse comilation error.

@WojciechMazur WojciechMazur added itype:bug regression This worked in a previous version but doesn't anymore area:metaprogramming:reflection Issues related to the quotes reflection API area:metaprogramming:quotes Issues related to quotes and splices labels Mar 1, 2024
@hamzaremmal
Copy link
Member

hamzaremmal commented Mar 1, 2024

The purpose of the check added in #18935 was to add a guarantee that the parents in the symbol are consistent with the parents in the tree.

The following change would make the code compile again.

- val (modValDef: ValDef, modClassDef: ClassDef) =
-     ClassDef.module(modSym, List(TypeTree.of[Serializer[T]]), Nil)
+ val (modValDef: ValDef, modClassDef: ClassDef) =
+     ClassDef.module(modSym, List(TypeTree.of[Object], TypeTree.of[Serializer[T]]), Nil)

We can maybe loose a bit the test and assume that if the symbol has Object as a first parent then the tree might skip Object in the parents.

cc @nicolasstucki for advice on how to proceed.

@nicolasstucki
Copy link
Contributor

All other trees at this point have the Object as first parents if they extend only traits. We should require macros to follow this format.

This is a bug in the implementation of the macro that was caught by the new check. The bug should be fixed (not hidden).

@nicolasstucki
Copy link
Contributor

It also seems that all uses of newModule in that code base already use Object before Serailizer[T]

https://github.com/frugalmechanic/fm-serializer/blob/e4ae0efc9efe80b15ea1935822a2bcaff6764e6a/src/main/scala-3/fm/serializer/MacroHelpers.scala#L837

@nicolasstucki
Copy link
Contributor

I opened an issue in their repo: tpunder/fm-serializer#14

@hamzaremmal
Copy link
Member

I opened an issue in their repo: frugalmechanic/fm-serializer#14

I've forked the repository to fix it and submit a PR, but opening an issue might be sufficient.

@tpunder
Copy link

tpunder commented Mar 6, 2024

Here is a PR: tpunder/fm-serializer#15

@tpunder
Copy link

tpunder commented Mar 6, 2024

PR has been merged. Thanks for identifying the issue and providing the fix!

@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
[Cherry-picked 96e1d4e]
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 itype:invalid regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants