Refactor pickled quotes logic

[nicolasstucki]

Introduce a PickledExpr, PickledType and PickledSplice abstractions.
This will allow the underlying representation of the pickled quote or quote arguments to change in a future version if needed.

[liufengyun]

Conceptually, it seems to be part of the compiler interface. Will it be better to put it there?

[nicolasstucki]

Not so sure. This represents a quote that has not been contextualized yet. But maybe the PickledSplices abstraction can be hidden with the PickledExpr and PickledType.

[nicolasstucki]

This seems to be the cleanest option

[liufengyun]

It seems I miss some context here. Could you elaborate a little bit about what kind of compiler changes that this PR aims to defend against and how this PR achieves that goal?

[nicolasstucki]

The idea is to be able to change the code that is generated to fill the holes of the pickled quote. Right now we use a sequence of lambdas to keep these and we cannot easily change it in future versions because this leaks into the API. One possible improvement is described here.

With this API we could then evolve the implementation of this code without breaking the old encodings.

// Code generated by compiller
PickledSplices.make(Seq(
  (args: Seq[Any]) => (qctx: QuoteContext) => ..., // Some Expr
  (args: Seq[Any]) => (qctx: QuoteContext) => ..., // Another Expr
))
// could be replaced without breaking API compatibility by a more performant version
new PickledSplices {
  def exprSplice(idx: Int)(args: Seq[Any])(qctx: QuoteContext): Expr[Any] = 
    idx match 
      case 0 => ... // Some Expr
      case 1 => ... // Another Expr
}

In this case, we would be avoiding allocations of the sequence and the lambdas. But other unforeseen improvements could arise in the future.

On the other side, PickledExpr and PickledType aim to be able to change the binary encoding of the pickled quote. Now we convert the Array[Byte] into a string using the Base64 encoder, but we should be able to change this encoding without breaking existing code.

[liufengyun]

Thanks for the explanation 👍

[liufengyun]

Minor: another possibility is to just define a single trait to hold all the protocols instead of defining 3 different traits.

[nicolasstucki]

I added an extra commit where I refactored it in a single interface.

[liufengyun]

LGTM

[liufengyun]

Thanks for the explanation 👍

[liufengyun]

In the original design, the interface defines abstract methods such as unpickle(). Will it be more flexible to keep the original design without committing to a concrete representation in the interface?

[nicolasstucki]

We have the same flexibility with both designs. In this one we just need to add can add a PickledQuote.unpickleV2 if we need to change the unpickling logic without breaking the old one. Though make is the one that will most likely evolve as it contains the byte representation logic and the hole logic.

[nicolasstucki]

The old PickledExpr interface was guided by an attempt to have a user-facing interface for pickled quotes. But we could provide the same abstraction and internally use PickledQuote to achieve the same.

[liufengyun]

As we put the protocol methods in a single trait. What will happen if we put the methods in CompilerInterface similar to the original design before this PR? Will that have the same benefit?

trait CompilerInterface {
  type PickledQuote
  type PickledArgs
  def unpickleExpr(repr: PickledQuote, args: PickledArgs): Term
  def unpickleType(repr: PickledQuote, args: PickledArgs): TypeTree
}

[nicolasstucki]

How would one create a PickledQuote in that design? We need to create it with some concrete class that exists in the library, but that design seems to indicate that it is not known in the library.

[liufengyun]

Just repeat the message in the email, it seems I miss something about the following point:

And we still need an implementation for each one of those on the library side as is the current case.

I think qctx.reflect.pickTasty(...) will remove the need to put concrete implementation in stdlib.

[nicolasstucki]

Indeed, it could all be implemented on the compiler. But that means that each improvement will be painfully complex in the compiler (i.e. the reason why we have not been able to improve it so far) and would have to add a new method on the interface on each change. I don't see how that would scale. The version I propose has the option of adding new static helper method if it is simpler or lets the code be generated without the CompilerInterface which is more flexible.

If we do it your way we also need to add the static helper methods as we do not have access to pickTasty method directly because we don't have access to the qctx where the transformation happens.

[liufengyun]

If we do it your way we also need to add the static helper methods as we do not have access to pickTasty method directly because we don't have access to the qctx where the transformation happens.

This seems to be an implementation detail, as a QuoteContext must be available in scope. Currently, in the implementation, it seems each quoted tree is associated with a QuoteContext, thus it's possible to get that in later compiler phases. It seems to me that polluting the API design with accidental implementation details is not a good idea.

But there might be other aspects that I'm not aware of in the design. Therefore, feel free to choose whatever you think makes more sense technically.

[nicolasstucki]

This design also generated reduces the code size of each quote by putting common code in those static methods. We do not want to generate the following code (or equivalent) at each place a quote is load.

val qctx = quoteContextWithCompilerInterface(summon[QuoteContext])
val tree = qctx.reflect.unpickleTerm(pickledQuote)
new scala.internal.quoted.Expr(tree, qctx.hashCode).asInstanceOf[Expr[T]]

[nicolasstucki]

I would use this design as I can see some clear benefits for the optimization I have in mind. We can always evolve the API the way you said later if we need to do a major change.