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 in arainko/ducktape under -Xcheck-macros when comparing field types #18754

Closed
WojciechMazur opened this issue Oct 24, 2023 · 5 comments
Assignees
Labels
area:metaprogramming:reflection Issues related to the quotes reflection API itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization

Comments

@WojciechMazur
Copy link
Contributor

Open CB failure found in @arainko /ducktape under -Xcheck-macros. Builds logs

The issue is only present when checking types of nested members (contact.phoneNo)

Compiler version

3.4.0-RC1-bin-20231023-44a537b-NIGHTLY
Works in 3.3.1

Minimized code

No minimization yet, reproducer:

//> using option -Xcheck-macros
//> using dep "io.github.arainko::ducktape:0.1.11"
import io.github.arainko.ducktape.*

final case class Primitive(contact: PrimitiveContactInfo)
final case class PrimitiveContactInfo(phoneNo: String)

final case class Complex(contact: ComplexContactInfo)
final case class ComplexContactInfo(phoneNo: PhoneNumber)
final case class PhoneNumber(value: String) extends AnyVal

def Test() = {
  given Transformer[String, PhoneNumber] = str => PhoneNumber(str + "-LOCAL")
  
  val primitive: Primitive = ???
  val _ = primitive.into[Complex].transform()
}

Output

-- Error: /Users/wmazur/projects/sandbox/main.scala:16:43 ----------------------
16 |  val _ = primitive.into[Complex].transform()
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |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[Complex](((evidence$7: scala.quoted.Quotes) ?=> io.github.arainko.ducktape.internal.macros.Transformations.inline$transformConfiguredMacro[Primitive, Complex](scala.quoted.runtime.Expr.quote[Primitive](source$proxy1).apply(using evidence$7), scala.quoted.runtime.Expr.quote[scala.collection.immutable.Seq[io.github.arainko.ducktape.BuilderConfig$package.BuilderConfig[Primitive, Complex]]](: _*).apply(using evidence$7))(scala.quoted.Type.of[Primitive](evidence$7), scala.quoted.Type.of[Complex](evidence$7), evidence$7)))
   |               |
   |               |The macro returned:
   |               |new Complex(contact = {
   |  val LowPriorityTransformerInstances_this: io.github.arainko.ducktape.Transformer.type = io.github.arainko.ducktape.Transformer
   |  new ComplexContactInfo(phoneNo = given_Transformer_String_PhoneNumber.transform(source$proxy1.contact.phoneNo))
   |})
   |               |
   |               |Error:
   |               |assertion failed: Types differ
   |Original type : (source.phoneNo : String)
   |After checking: (source$proxy1.contact.phoneNo : String)
   |Original tree : source$proxy1.contact.phoneNo
   |After checking: source$proxy1.contact.phoneNo
   |Why different :
   |             Subtype trace:
   |  ==> (source$proxy1.contact.phoneNo : String)  <:  (source.phoneNo : String)
   |    ==> (source$proxy1.contact : PrimitiveContactInfo)  <:  (source : PrimitiveContactInfo)
   |      ==> PrimitiveContactInfo  <:  (source : PrimitiveContactInfo) (left is approximated)
   |      <== PrimitiveContactInfo  <:  (source : PrimitiveContactInfo) (left is approximated) = false
   |    <== (source$proxy1.contact : PrimitiveContactInfo)  <:  (source : PrimitiveContactInfo) = false
   |    ==> String  <:  (source.phoneNo : String) (left is approximated)
   |      ==> String  <:  (source.phoneNo : String) (left is approximated)
   |      <== String  <:  (source.phoneNo : String) (left is approximated) = false
   |    <== String  <:  (source.phoneNo : String) (left is approximated) = false
   |  <== (source$proxy1.contact.phoneNo : String)  <:  (source.phoneNo : String) = false
   |               |
   |stacktrace available when compiling with `-Ydebug`
   |               |
   |----------------------------------------------------------------------------
   |Inline stack trace
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from Transformations.scala:69
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from Transformations.scala:69
    ----------------------------------------------------------------------------
1 error found

Expectation

Should not raise an error

@WojciechMazur WojciechMazur added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label stat:needs minimization Needs a self contained minimization regression This worked in a previous version but doesn't anymore area:metaprogramming:reflection Issues related to the quotes reflection API stat:needs bisection Need to use nightly builds and git bisect to find out the commit where this issue was introduced and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 24, 2023
@WojciechMazur
Copy link
Contributor Author

WojciechMazur commented Oct 24, 2023

Project-wise bisect results:
Last good release: 3.4.0-RC1-bin-20230726-97677cc-NIGHTLY
First bad release: 3.4.0-RC1-bin-20230727-ac69c66-NIGHTLY

The commit-based bisect failed due to missing sbt-bridge
Candidate commits:
ac69c66
d903d35
cb73f0b
c04d2db
989c55c [bad commit]
4b67b34 [bad commit] possible regression introducer
182331b [ good commit ]
5625107
f22f9db [sbt-bridge / dotty compilation failure]
101ce3a
f5313ae
96ca0b6

@WojciechMazur WojciechMazur removed the stat:needs bisection Need to use nightly builds and git bisect to find out the commit where this issue was introduced label Oct 24, 2023
@nicolasstucki
Copy link
Contributor

We need a self-contained version of this code to make any progress on this issue. @arainko could you help us with that?

@arainko
Copy link
Contributor

arainko commented Oct 25, 2023

Sure, I'll try to minimize this over the weekend

@arainko
Copy link
Contributor

arainko commented Oct 30, 2023

Ok, so my findings so far (I couldn't get a full minimization yet) - the error happens after a call to LiftTransformation.liftTransformation is made and then when the macro falls into the liftDerivedTransformation(transformer, appliedTo) case.

What this specific piece of code does:

  1. It matches a specific call to a .make method on one of the specialized cases of a Transformer (Transformer.ForProduct in this case) and extracts:
  • The ValDef that corresponds to the SAM lambda argument that creates that Transformer
  • All the definitions from Inlined nodes (the ones responsible for stuff like source$proxy1 etc) that were encountered along the way
  • the method call to a constructor of the destination type
  • the terms that correspond to the arguments of that constructor
    so for something like this
Transformer.ForProduct.make((p: Person) => new Person2(p.int, p.str))

We'll get TransformerLambda.ForProduct( ValDef of 'p', <stuff from Inlined nodes>, 'new Person2', List(p.int, p.str))

  1. After that's done it calls optimizeTransformerInvocation and falls into the TransformerLambda.ForProduct case which will rewrite the tree of the original transformer to elide an instantiation of a new object - so for example, this manually written out Transformer:
  final case class ToplevelPrimitive(value: ReproPrimitive)
  final case class ToplevelComplex(value: ReproComplex)

  final case class ReproPrimitive(name: String)
  final case class ReproComplex(name: Name)

  final case class Name(value: String)

  given Transformer[String, Name] = str => Name(str + "-LOCAL")

  val primitive: ToplevelPrimitive = ???

  inline def manualTransformer = Transformer.ForProduct.make[ToplevelPrimitive, ToplevelComplex](
    (source: ToplevelPrimitive) =>
      new ToplevelComplex(value = 
        new ReproComplex(
          name = given_Transformer_String_Name.transform(source.value.name)
        )  
      )
  ).transform(primitive)

will be rewritten to this:

(new ToplevelComplex(value = new ReproComplex(name = given_Transformer_String_Name.transform(primitive.value.name))): ToplevelComplex)

so it kind of 'lifts' the transformation from the Transformer's body by getting the RHS of the SAM lambda that creates it and replaces all of the references to the lambda parameter with the expression passed into Transformer#transform (so primitive in this case) and it also does this recursively for any invocations of Transformer.ForProduct.make that it encounters in the body of the toplevel transformer.

The error seems to occur when the body of given_Transformer_String_Name.transform(source.value.name) gets replaced with given_Transformer_String_Name.transform(primitive.value.name) aka when replacing arguments passed in to Transformers from implicit scope.

(I'm writing this in hopes that it gives an AHA moment to somebody 😄 )

I'll try to tackle this once again when I've got some free time.

@WojciechMazur
Copy link
Contributor Author

Fixed in both LTS (3.3.3) and 3.nightly. Failing in 3.4.0, compiles in 3.4.1-RC1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metaprogramming:reflection Issues related to the quotes reflection API itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization
Projects
None yet
Development

No branches or pull requests

3 participants