Replace quoted type variables in signature of HOAS pattern result #16951

nicolasstucki · 2023-02-17T15:19:56Z

To be able to construct the lambda returned by the HOAS pattern we need:
first resolve the type variables and then use the result to construct the
signature of the lambdas.

To simplify this transformation, QuoteMatcher returns a Seq[MatchResult]
instead of an untyped Tuple containing Expr[?]. The tuple is created
once we have accumulated and processed all extracted values.

Fixes #15165

smarter

Otherwise LGTM.

smarter · 2023-03-01T15:49:28Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

-            }
-            Closure(meth, bodyFn)
-          }
+          val env = summon[Env]


Looks like the summon[Env] two lines below could become env with this addition.

smarter · 2023-03-01T15:55:01Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

  /** Result of matching a part of an expression */
-  private type Matching = Option[Tuple]


The documentation comment for =?= still mention that we return a Some(tup: Tuple). That should be updated, but I also think that the meaning of None vs Some(Seq()) vs Some(...) should be explained in a documentation comment for type Matching itself.

smarter · 2023-03-01T15:57:09Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+
+    QuoteMatcher.treeMatch(scrutinee, pat1)(using ctx1).map { matchings =>
+      import QuoteMatcher.MatchResult.*
+      lazy val spliceScope = SpliceScope.getCurrent


spliceScope appears to be unused?

I intended to use pass it to toExpr. I will change that.

smarter · 2023-03-01T16:01:49Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+      val matchedExprs = matchings.map(_.toExpr(typeHoleMap))
+      val matchedTypes = typeHoleApproximations.map(reflect.TypeReprMethods.asType)
+      val results = matchedTypes ++ matchedExprs
+      Tuple.fromIArray(results.toArray.asInstanceOf[IArray[Object]])


Could this be:

Suggested change

Tuple.fromIArray(results.toArray.asInstanceOf[IArray[Object]])

Tuple.fromIArray(IArray.unsafeFromArray(results.toArray))

smarter · 2023-03-01T16:15:29Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

@@ -452,20 +433,52 @@ object QuoteMatcher {
      accumulator.apply(Set.empty, term)
  }

+  enum MatchResult:
+    case ClosedTree(tree: Tree)
+    case OpenTree(tree: Tree, patternTpe: Type, args: List[Tree], env: Env)


Woudl be nice to document the expectation on the valid values of the arguments here.

smarter · 2023-03-01T16:16:24Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+      case MatchResult.ClosedTree(tree) =>
+        new ExprImpl(tree, SpliceScope.getCurrent)
+      case MatchResult.OpenTree(tree, patternTpe, args, env) =>
+        def hoasClosure = {


Why a def if we then immediately call it?

Accident of refactoring.

smarter · 2023-03-01T16:17:21Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+    case ClosedTree(tree: Tree)
+    case OpenTree(tree: Tree, patternTpe: Type, args: List[Tree], env: Env)
+
+    def toExpr(mapTypeHoles: TypeMap)(using Context): Expr[Any] = this match


Documenting what this method generates would also be nice.

smarter · 2023-03-01T16:19:08Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+          val methTpe = MethodType(names)(_ => paramTypes, _ => mapTypeHoles(patternTpe))
+          val meth = newAnonFun(ctx.owner, methTpe)
+          def bodyFn(lambdaArgss: List[List[Tree]]): Tree = {
+            val argsMap = args.map(_.symbol).zip(lambdaArgss.head).toMap


Could use a view to avoid intermediate allocations:

Suggested change

val argsMap = args.map(_.symbol).zip(lambdaArgss.head).toMap

val argsMap = args.view.map(_.symbol).zip(lambdaArgss.head).toMap

nicolasstucki

I took care of the comments but also rebased on #16953 to take care of the conflicts before they happen.

nicolasstucki · 2023-03-02T10:08:36Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+
+    QuoteMatcher.treeMatch(scrutinee, pat1)(using ctx1).map { matchings =>
+      import QuoteMatcher.MatchResult.*
+      lazy val spliceScope = SpliceScope.getCurrent


I intended to use pass it to toExpr. I will change that.

nicolasstucki · 2023-03-02T10:16:11Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+      case MatchResult.ClosedTree(tree) =>
+        new ExprImpl(tree, SpliceScope.getCurrent)
+      case MatchResult.OpenTree(tree, patternTpe, args, env) =>
+        def hoasClosure = {


Accident of refactoring.

smarter · 2023-03-02T19:23:05Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+
+    /** Return the expression that was extracted from a hole.
+     *
+     *  If the it was a closed expression it return that expression. Otherwise,


Suggested change

* If the it was a closed expression it return that expression. Otherwise,

* If it was a closed expression it returns that expression. Otherwise,

smarter · 2023-03-02T19:24:03Z

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala

+     */
+    def toExpr(mapTypeHoles: TypeMap, spliceScope: Scope)(using Context): Expr[Any] = this match
+      case MatchResult.ClosedTree(tree) =>
+        new ExprImpl(tree, SpliceScope.getCurrent)


Why isn't spliceScope used here?

I missed it. Now it uses spliceScope.

To be able to construct the lambda returned by the HOAS pattern we need: first resolve the type variables and then use the result to construct the signature of the lambdas. To simplify this transformation, `QuoteMatcher` returns a `Seq[MatchResult]` instead of an untyped `Tuple` containing `Expr[?]`. The tuple is created once we have accumulated and processed all extracted values. Fixes scala#15165

nicolasstucki force-pushed the fix-15165 branch from ea2f748 to afc2b56 Compare February 17, 2023 15:21

nicolasstucki marked this pull request as ready for review February 18, 2023 13:28

nicolasstucki marked this pull request as draft February 18, 2023 13:29

nicolasstucki marked this pull request as ready for review February 20, 2023 14:13

nicolasstucki requested a review from smarter February 20, 2023 14:13

nicolasstucki assigned smarter Feb 20, 2023

nicolasstucki force-pushed the fix-15165 branch from afc2b56 to dfae680 Compare February 21, 2023 07:57

Simplify QuoteMatcher using optional

dbdca17

smarter requested changes Mar 1, 2023

View reviewed changes

smarter assigned nicolasstucki and unassigned smarter Mar 1, 2023

nicolasstucki force-pushed the fix-15165 branch from dfae680 to 6ed7e25 Compare March 2, 2023 10:31

nicolasstucki commented Mar 2, 2023

View reviewed changes

nicolasstucki requested a review from smarter March 2, 2023 19:09

nicolasstucki assigned smarter and unassigned nicolasstucki Mar 2, 2023

smarter approved these changes Mar 2, 2023

View reviewed changes

smarter assigned nicolasstucki and unassigned smarter Mar 2, 2023

nicolasstucki force-pushed the fix-15165 branch from 6ed7e25 to 20174d7 Compare March 3, 2023 06:58

nicolasstucki enabled auto-merge March 3, 2023 06:59

nicolasstucki mentioned this pull request Mar 3, 2023

Simplify QuoteMatcher.&&& using optional #16953

Closed

nicolasstucki merged commit 6ea3ea6 into scala:main Mar 3, 2023

nicolasstucki deleted the fix-15165 branch March 3, 2023 08:12

Kordyjan added this to the 3.3.1 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace quoted type variables in signature of HOAS pattern result #16951

Replace quoted type variables in signature of HOAS pattern result #16951

nicolasstucki commented Feb 17, 2023 •

edited

Loading

smarter left a comment

smarter Mar 1, 2023

smarter Mar 1, 2023

smarter Mar 1, 2023

nicolasstucki Mar 2, 2023

smarter Mar 1, 2023

smarter Mar 1, 2023

smarter Mar 1, 2023

nicolasstucki Mar 2, 2023

smarter Mar 1, 2023

smarter Mar 1, 2023

nicolasstucki left a comment

nicolasstucki Mar 2, 2023

nicolasstucki Mar 2, 2023

smarter Mar 2, 2023

smarter Mar 2, 2023

nicolasstucki Mar 3, 2023

		/** Result of matching a part of an expression */
		private type Matching = Option[Tuple]

	Tuple.fromIArray(results.toArray.asInstanceOf[IArray[Object]])
	Tuple.fromIArray(IArray.unsafeFromArray(results.toArray))

	val argsMap = args.map(_.symbol).zip(lambdaArgss.head).toMap
	val argsMap = args.view.map(_.symbol).zip(lambdaArgss.head).toMap

	* If the it was a closed expression it return that expression. Otherwise,
	* If it was a closed expression it returns that expression. Otherwise,

Replace quoted type variables in signature of HOAS pattern result #16951

Replace quoted type variables in signature of HOAS pattern result #16951

Conversation

nicolasstucki commented Feb 17, 2023 • edited Loading

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Feb 17, 2023 •

edited

Loading