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

Place staged type captures in Quote AST #17424

Merged
merged 1 commit into from
May 11, 2023

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented May 5, 2023

This change makes it easier to handle instances Type[T] that are inserted into a level 0 quote. These used to be encoded in the staging phase as type aliases @TypeSplice type A = t.Underlying. This makes it hard to debug and check invariants. Instead, we can collect the list of type tags needed and store them in the quote. At the same time, we can convert references to those types into t.Underlying. We can generate the @TypeSplice type A = t.Underlying just before pickling and replace all references to t.Underlying with A at that point.

This is only an internal representation change while transforming the quote. It will not affect TASTy.

@nicolasstucki nicolasstucki marked this pull request as ready for review May 5, 2023 16:46
@nicolasstucki nicolasstucki requested a review from smarter May 5, 2023 16:47
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can generate the @TypeSplice type A = t.Underlying just before pickling and replace all references to t.Underlying with A at that point.

Would it make sense to keep the tags-based representation in Tasty in the future?

// `quoted.Type.of[<body>](<quotes>)` --> `'[<body1>].apply(<quotes>)`
val quotes = transform(tree.args.head)
val TypeApply(fun, _) = tree.fun: @unchecked
if level != 0 then cpy.Apply(tree)(cpy.TypeApply(tree.fun)(fun, body1 :: Nil), quotes :: Nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused when I first read this code, because tags isn't used in this branch, I had to go read transformQuoteBody to realize that when level != 0 then tags is always Nil. Maybe, adding a comment mentioning this would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inlined transformQuoteBody and refactored the code to only make this transformation at level == 0. Other levels are transformed using super.transform.

case Nil => transformedBody
case tags => tpd.Block(tags, transformedBody).withSpan(body.span)
}
private def transformQuoteBody(body: Tree, span: Span)(using Context): (List[Tree], Tree) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter span isn't used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inlined transformQuoteBody.

}

/** Remove references to local types that will not be defined in this quote */
private def getTypeHoleType(using Context) = new TypeMap() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this code already existed before but FWIW, I think TypeOps.AvoidMap would be better suited here (or its superclass ApproximatingTypeMap if more flexibility is needed), just using TypeMap doesn't take variance and bounds into account.

@nicolasstucki
Copy link
Contributor Author

Would it make sense to keep the tags-based representation in Tasty in the future?

It would not make sense to pickle the tags as they are represented in Quotes. We only pickle these when we pickle the contents of the quote. Therefore the tag reference would be out of scope. In general we need to replace the tags with a local definition that is contained in the pickle quote. We could find a more efficient encoding, maybe in a custom AST.

mapOver(tp)
private def getTermHoleType(using Context) = new AvoidMap {
def toAvoid(tp: NamedType): Boolean =
tp.prefix == NoPrefix // reference to terms and type defined in outer quote
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about references defined in the quote itself, e.g.:

'{ class A; val x: A = new A; ... }

they have NoPrefix as a prefix, but they're not defined in an outer quote.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will probably need to move this into the Splicing phase where we know the set of definitions that are in the outer quote. I will open a separate PR. Should I revert that change in this PR? As far as I can see we are already avoiding those references.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R. Should I revert that change in this PR?

Sure!

@nicolasstucki nicolasstucki enabled auto-merge May 11, 2023 13:06
@nicolasstucki nicolasstucki merged commit c720e5a into scala:main May 11, 2023
@nicolasstucki nicolasstucki deleted the quote-tags branch May 11, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants