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.

Sign up for GitHub

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

Jump to bottom

Properly encode splice hole using PolyFunction #17072

Closed

nicolasstucki wants to merge 5 commits into scala:main from dotty-staging:splice-hole-polyfunction

Contributor

nicolasstucki commented Mar 9, 2023 •

edited

Loading

Background

The splicing phase replaces the spaces with holes that do not refer to any term of the surrounding quote. This is done using a lambda that receives the captured references as Expr[...] and splices them where they are used in the splice.

'{
  val x: Int = ...
  ${ ... '{ ... x ..} ... }: Long
}

becomes

'{
  val x: Int = ...
  {{{ 0 | Long | x | (x2: Expr[Int]) => { ... '{ ... $x2 ..} ... }  }}}
}

then the lambda is extracted from the quote in the pickleQuotes phase. This can be done safely because there are no more references to any local variables.

Limitation

If there is a type defined in the quote, we currently generate the equivalent Type[..]. We end up generating something like

'{
  type T
  ${ ... T ... '{ ... T ..} ... }: Long
}

becomes

'{
  type T
  {{{ 0 | Long | T | (t: Type[T]) => { ... T .. '{ ... t.Underlying..} ... }  }}}
}

This takes care correctly of the run-time aspect of the transformation. But when we extract the lambda from the quote we end up with references to T that are out of scope. This extrusion happens to work but is not ideal. Erasure removes those types making the tree well formed again.

The Solution

Use polymorphic function types to abstract the content of the hole fully.

'{
  type T
-  {{{ 0 | Long | T |         (t: Type[T])  => { ... T  .. '{ ... t.Underlying..} ... }  }}}
+  {{{ 0 | Long | T | [T2] => (t: Type[T2]) => { ... T2 .. '{ ... t.Underlying..} ... }  }}}
}

This PR implements this change.

Other

~~Based on #17070, #17059, #17054~~

nicolasstucki self-assigned this

nicolasstucki force-pushed the splice-hole-polyfunction branch 2 times, most recently from dc8cda5 to 9b2571a Compare

March 14, 2023 15:23

nicolasstucki force-pushed the splice-hole-polyfunction branch 4 times, most recently from f8144ef to 79dcb61 Compare

April 4, 2023 12:38


          Use polyfunction for quote hole contents

92ff00a

This avoids leaking references to types defined within the quotes when the
contents of the holes are extracted from the quote.

nicolasstucki force-pushed the splice-hole-polyfunction branch from 79dcb61 to 92ff00a Compare

April 6, 2023 08:37

nicolasstucki marked this pull request as ready for review

April 6, 2023 11:19

nicolasstucki requested a review from smarter

April 6, 2023 11:19

nicolasstucki assigned smarter and unassigned nicolasstucki

smarter requested changes

View reviewed changes

compiler/src/dotty/tools/dotc/transform/Splicing.scala

		@@ -44,12 +45,13 @@ object Splicing:
		* contains the quotes with references to all cross-quote references. There are some special rules

Member

smarter Apr 11, 2023

In the description a couple of lines above
(<capturedTerms>*) =>
should probably be
[<capturedTypes>*] => (<capturedTerms>*) =>

compiler/src/dotty/tools/dotc/transform/Splicing.scala

@@ @@ -198,10 +200,57 @@ class Splicing extends MacroTransform: @@
                     val newTree = transform(tree)
                     val (refs, bindings) = refBindingMap.values.toList.unzip
                     val bindingsTypes = bindings.map(_.termRef.widenTermRefExpr)
-                    val methType = MethodType(bindingsTypes, newTree.tpe)
+                    val types = bindingsTypes.collect {
+                      case AppliedType(tycon, List(arg: TypeRef)) if tycon.derivesFrom(defn.QuotedTypeClass) => arg

Member

smarter Apr 11, 2023

I think a dealias might be needed in case the user defines an alias such as type TInt = Type[Int]?

Contributor Author

nicolasstucki Apr 11, 2023

These types are created in the SpliceTransformer. They should always have this shape.

Member

smarter Apr 11, 2023

Even if they have the same shape, they might not be referentially equal (they are only referentially equal if they are cached by Uniques.scala, but not all types are cacheable).

compiler/src/dotty/tools/dotc/transform/Splicing.scala Outdated

@@ @@ -198,10 +200,57 @@ class Splicing extends MacroTransform: @@
                     val newTree = transform(tree)
                     val (refs, bindings) = refBindingMap.values.toList.unzip
                     val bindingsTypes = bindings.map(_.termRef.widenTermRefExpr)
-                    val methType = MethodType(bindingsTypes, newTree.tpe)
+                    val types = bindingsTypes.collect {

Member

smarter Apr 11, 2023

There's a lot of types involved in this code :), maybe we can use a more precise name like quotedTypes?

Contributor Author

nicolasstucki Apr 11, 2023

Renamed to capturedTypes.

compiler/src/dotty/tools/dotc/transform/Splicing.scala

+                        spliceOwner,
+                        UniqueName.fresh(tpe.symbol.name.toTypeName),
+                        Param,
+                        TypeBounds.empty

Member

smarter Apr 11, 2023

Shouldn't the type bounds of the type params match the type bounds of the original type for the encoding to always typecheck?

Contributor Author

nicolasstucki Apr 11, 2023

Seems that I am missing something. I will add some test cases and update these bounds.

Member

smarter Apr 11, 2023

Speaking of bounds, if types can refer to terms in their bounds (val x = ...; type U <: x.T; ${ x.foo: U }) then the encoding where types always appear before terms wouldn't work if we need to also store these bounds.

compiler/src/dotty/tools/dotc/transform/Splicing.scala

    
               *         (q: Quotes) ?=> f('{ g(${x1$}, ${x2$}) })

               *     {{{ 0 | T3 | U, x1, x2 |

               *       [U$1] => (U$2: Type[U$1], x1$: Expr[T], x2$: Expr[U$1]) => // body of this lambda does not contain references to U, x1 or x2

               *         (q: Quotes) ?=> f('{ @SplicedType type U$3 = [[[ 0 | U$2 | | U$1 ]]]; g[U$3](${x1$}, ${x2$}) })

Member

smarter Apr 11, 2023

Is the [[[ ... ]]] syntax used here documented somewhere?

Contributor Author

nicolasstucki Apr 11, 2023

Not really. It is the syntax defined in the RefinedPrinter. I plan to update this syntax in #17144.

compiler/src/dotty/tools/dotc/transform/Splicing.scala

@@ @@ -198,10 +200,57 @@ class Splicing extends MacroTransform: @@
                     val newTree = transform(tree)
                     val (refs, bindings) = refBindingMap.values.toList.unzip
                     val bindingsTypes = bindings.map(_.termRef.widenTermRefExpr)
-                    val methType = MethodType(bindingsTypes, newTree.tpe)

Member

smarter Apr 11, 2023

The code example in the doc comment above SpliceTransformer also needs to be updated to use a polymorphic function.

compiler/src/dotty/tools/dotc/transform/Splicing.scala

+                              case tp: TypeRef if tp.typeSymbol.hasAnnotation(defn.QuotedRuntime_SplicedTypeAnnot) => tp
+                              case tp: TypeRef =>
+                                typeIndex.get(tp) match
+                                  case Some(idx) => newTypeParams(idx).typeRef

Member

smarter Apr 11, 2023

Having a Map where the keys are Types might be a little bit fragile since we could have two equivalent Types which are not referentially equal, is this not a problem here?

Contributor Author

nicolasstucki Apr 11, 2023

These should only contain TypeRefs (and TermRefs after some other fix). This worked in the tests I have. As far as I can see, both of those are cached and should have referential equality.

Member

smarter Apr 11, 2023

I think the prefix of a TypeRef could be an uncacheable type, for example if it contains a LazyRef

compiler/src/dotty/tools/dotc/transform/PickleQuotes.scala Outdated

Comment on lines 339 to 343

+                                val sel1 = splice.changeOwner(ddef.symbol.owner, ctx.owner).select(nme.apply)
+                                val appTpe = if typeParamCount == 0 then sel1 else sel1.appliedToTypes(List.fill(typeParamCount)(defn.AnyType))
+                                val app1 = appTpe.appliedToArgs(spliceArgs)
+                                val sel2 = app1.select(nme.apply)
+                                sel2.appliedTo(args(2).asInstance(quotesType))

Member

smarter Apr 11, 2023

A comment with an example showing the shape of the generated tree would make this code easier to follow.

Contributor Author

nicolasstucki Apr 11, 2023

I added an example and refactored a bit to make it more readable.

compiler/src/dotty/tools/dotc/transform/TreeChecker.scala Outdated

@@ @@ -670,6 +670,9 @@ object TreeChecker { @@
                     else assert(tpt.typeOpt =:= pt)
                     // Check that the types of the args conform to the types of the contents of the hole
+                    val typeArgsTypes = args.collect { case arg if arg.isType =>

Member

smarter Apr 11, 2023

Will the new checker still work with tasty files generated by previous compiler versions?

Contributor Author

nicolasstucki Apr 11, 2023

TASTy files will not contain Holes so it will work.

Pickled quotes will not contain content and we should not run this check on them. I will add a guard.

smarter assigned nicolasstucki and unassigned smarter

smarter added the needs-minor-release label

nicolasstucki added 4 commits

April 11, 2023 16:35


          Update documentation

134152a


          Rename types to capturedTypes in Splicing

b931dd2


          Add documentation

7c2d5e9


          Only check type of content if it exists

cf86861

nicolasstucki closed this

nicolasstucki added the stat:revisit label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-minor-release stat:revisit