Make HOAS Quote pattern match with def method capture #17567

zeptometer · 2023-05-24T07:37:11Z

This PR will fix #17105 by extracting symbols from eta-expanded identifiers.

This fix enables the use of patterns such as

case '{ def f(...): T = ...; $g(f): U } =>

where g will match any expression that may contain references to f.

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

nicolasstucki · 2023-05-24T08:30:53Z

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

- val capturedArgs = args.map(_.symbol)
- val captureEnv = env.filter((k, v) => !capturedArgs.contains(v))
+ val capturedIds = args.map(getCapturedIdent)
+ val capturedSymbols = capturedIds.map(_.symbol)


getCapturedIdent could return the Symbol directly. This way, we can avoid this extra map.

Here I intended to use capturedIds as a parameter to matchedOpen and we cannot omit it (we get compiler errors if we use args for matchedOpen instead).

According to your other comment on i17105.check, it's likely I need to change this part anyway.

nicolasstucki · 2023-05-24T08:40:39Z

tests/run-macros/i17105.check

These outputs show that we need to adapt the type of g in the lambdas. For example g: (y: scala.Int)scala.Int should be g: scala.Int => scala.Int. We probably need to addapt the type here
https://github.com/lampepfl/dotty/blob/main/compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala#LL468C13-L468C13. We need to convert MethodTypes into funtion types. The defn.FunctionOf method might be useful here (defined in Definitions.scala).

I would expect this output to look like

((g: scala.Int => scala.Int, n: scala.Int) => g(n)) ((g: (scala.Int, scala.Int) => scala.Int) => g(1, 2)) ((g2: (scala.math.Ordered[scala.Int]) => scala.Boolean, ord: scala.math.Ordered[scala.Int]) => (g2(ord).||(2.<(3)): scala.Boolean))

Thank you, I'll try to fix this issue (and add appropriate test cases)

Int => Int could also be scala.Function1[scala.Int, scala.Int]. From Function0 up to Function22 we have concrete FnunctionN classes that implement the lambda type. T => R is equivalent to Function1[T, R] in the Scala type system. This only applies to normal function parameters, not if the argument is implicit as in def f(using T): R = ... or T ?=> R (similar for functions that contain erased parameters).

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

zeptometer · 2023-06-03T15:27:05Z

Hi @nicolasstucki, I'm trying to fix the issue that types of parameters in higher-order term holes being method types with the commit 209d763.

However, the current hotfix fails even with the simplest test case (I used this test case). I got the following error

-- Error: /home/yuito/code/dotty-workspace/issue-17105/test_2.scala:7:43 ------- 7 | testExpr { def f(x: Int) = "hello" * x; f(0) + "bye" } | ^^^^ | parameter g does not take parameters

I found this message is caused at a method in QuoteMatcher.scala:l524, which generates a method body from a quote that matched against higher-order term pattern.

 def bodyFn(lambdaArgss: List[List[Tree]]): Tree = { val argsMap = args.view.map(_.symbol).zip(lambdaArgss.head).toMap val body = new TreeMap { override def transform(tree: Tree)(using Context): Tree = tree match case tree: Ident => env.get(tree.symbol).flatMap(argsMap.get).getOrElse(tree) case tree => super.transform(tree) }.transform(tree) TreeOps(body).changeNonLocalOwners(meth) }

In the test case, the matched body is f(0) + ("bye") and the current bodyFn turns it into g(0) + ("bye") (given f maps to g). However, here g has a function type Int => String whereas f has a method type (y: Int) => String: hence the method call g(0) fails (because only methods are allowed to apply at this stage... as far as I understand).

To avoid this issue, we want to refine bodyFn to convert f(0) to g.apply(0). I guess it will be something like this

 tree match case Apply(fnId: Ident, args) => { val fnId2 = env.get(tree.symbol).flatMap(argsMap.get).getOrElse(tree) ??? // want to return '{$fnId2.apply($args)} Not sure what is desirable way to construct Tree here... } case tree => super.transform(tree)

That's something I'd like your feedback. Does it sound valid to you? If so, what can I fill for ??? part?

zeptometer · 2023-06-04T05:48:08Z

I managed to fix a part of issue via c7a88fa and added a test case in 8d73b97👍 I'll try to generalize the fix to accommodate other patterns.

zeptometer · 2023-06-04T14:17:51Z

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

+ // TODO issue-17105: Is this pattern appropriate for methods?
+ case tp: MethodOrPoly => cpy.Apply(tree)(tfun, targs)
+ // TODO issue-17105: Appropriate pattern for function (appliable?) types?
+ case _ => ctx.typer.typed(


Is this the right way to construct a typed tree?

zeptometer · 2023-06-04T14:18:24Z

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

+ val targs = transform(args)
+ tfun.tpe match
+ // TODO issue-17105: Is this pattern appropriate for methods?
+ case tp: MethodOrPoly => cpy.Apply(tree)(tfun, targs)


Is this the right pattern to match against methods?

zeptometer · 2023-06-04T14:18:49Z

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

+ case Apply(methId: Ident, args) =>
+ env.get(tree.symbol).flatMap(argsMap.get)
+ .map(fnId => ctx.typer.typed(
+ untpd.Apply(


Is this the right pattern to construct typed tree?

Typing an untyped tree is dangerous. Depending on the context where this is typed, we could end up with different result. It is also a bit inefficient.

The correct way to do this is to create the typed tree directly. In this case you can use the select and appliedToArgs helper methods to construct this tree. The code should be fnId.select(nme.apply).appliedToArgs(args)).

tests/run-macros/i17105/Macro_1.scala

nicolasstucki · 2023-06-05T07:32:27Z

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

+ case Apply(methId: Ident, args) =>
+ env.get(tree.symbol).flatMap(argsMap.get)
+ .map(fnId => ctx.typer.typed(
+ untpd.Apply(


Typing an untyped tree is dangerous. Depending on the context where this is typed, we could end up with different result. It is also a bit inefficient.

The correct way to do this is to create the typed tree directly. In this case you can use the select and appliedToArgs helper methods to construct this tree. The code should be fnId.select(nme.apply).appliedToArgs(args)).

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

zeptometer · 2023-06-05T02:32:03Z

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

+ case Apply(fun, args) =>
+ val tfun = transform(fun)
+ val targs = transform(args)
+ (fun.tpe, tfun.tpe) match


Do we want to use fun.tpe.widenTermRefExpr in this case?

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

zeptometer · 2023-06-15T11:04:46Z

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

+ val (pEnv, pmatch) = matchParamss(paramss1, paramss2)
+ val defEnv = pEnv + (scrutinee.symbol -> pattern.symbol)
+
+ ematch


ematch is always Seq.empty and pmatch should also return Seq.empty on match.
Should I remove the part ematch &&& pmatch &&& to simplify logic?

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

zeptometer · 2023-06-18T01:40:16Z

The latest test report says that the test case quote-nested-6.scala fails, but it passes in my environment https://github.com/lampepfl/dotty/actions/runs/5298658706/jobs/9591140406?pr=17567

Output from 'tests/run-staging/quote-nested-6.scala' did not match check file. Actual output: { type T[X] = [A >: scala.Nothing <: scala.Any] =>> scala.collection.immutable.List[A][X] val x: java.lang.String = "foo" val z: [X >: scala.Nothing <: scala.Any] =>> scala.collection.immutable.List[X][java.lang.String] = scala.List.apply[java.lang.String](x) (x: java.lang.String) } Test output dumped in: tests/run-staging/quote-nested-6.check.out See diff of the checkfile (`brew install icdiff` for colored diff) > diff tests/run-staging/quote-nested-6.check tests/run-staging/quote-nested-6.check.out Replace checkfile with current output > mv tests/run-staging/quote-nested-6.check.out tests/run-staging/quote-nested-6.check foo

The issue is that I cannot reproduce this failure in my environment. It might be due to different JDK (I'm using OpenJDK 11). I'll try switching OpenJDK 16.

[update] Hmm, tests keep passing with OpenJDK 16.

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

nicolasstucki · 2023-06-19T08:01:46Z

The failure of quote-nested-6.scala was unrelated to this PR. This broke on that main branch due to a conflict between to other PRs, see #17993.

nicolasstucki

Otherwise LGTM

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

nicolasstucki · 2023-06-20T06:53:00Z

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

+ case OpenTree(tree: Tree, patternTpe: Type, argIds: List[Tree], argTypes: List[Type], env: Env)
+
+ /** The Definitions object */
+ def defn(using Context): Definitions = ctx.definitions


defn should have been imported by import dotty.tools.dotc.core.Symbols.*. This definition should be removed.

Thank you, I've followed your advice.

closes scala#17105

nicolasstucki self-requested a review May 24, 2023 07:39

nicolasstucki self-assigned this May 24, 2023

nicolasstucki requested review from nicolasstucki and removed request for nicolasstucki May 24, 2023 07:39

nicolasstucki reviewed May 24, 2023

View reviewed changes

zeptometer commented May 24, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

zeptometer commented May 25, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

zeptometer force-pushed the issue-17105 branch from f36948c to c38b378 Compare June 4, 2023 14:07

zeptometer commented Jun 4, 2023

View reviewed changes

nicolasstucki reviewed Jun 5, 2023

View reviewed changes

zeptometer commented Jun 5, 2023

View reviewed changes

nicolasstucki reviewed Jun 7, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

zeptometer commented Jun 15, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Show resolved Hide resolved

zeptometer commented Jun 15, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

zeptometer commented Jun 19, 2023

View reviewed changes

compiler/src/scala/quoted/runtime/impl/QuoteMatcher.scala Outdated Show resolved Hide resolved

zeptometer force-pushed the issue-17105 branch from 3652eca to 96ab2ea Compare June 19, 2023 13:43

zeptometer marked this pull request as ready for review June 19, 2023 13:46

zeptometer changed the title ~~WIP: Issue 17105~~ Make HOAS Quote pattern match with def method capture Jun 19, 2023

zeptometer force-pushed the issue-17105 branch from 96ab2ea to 3af515d Compare June 20, 2023 00:35

nicolasstucki reviewed Jun 20, 2023

View reviewed changes

Make HOAS Quote pattern match with def method capture

d819d9f

closes scala#17105

zeptometer force-pushed the issue-17105 branch from 3af515d to d819d9f Compare June 20, 2023 23:48

nicolasstucki approved these changes Jun 21, 2023

View reviewed changes

nicolasstucki merged commit 0d8bdd6 into scala:main Jun 21, 2023

zeptometer mentioned this pull request Jun 26, 2023

Inhibit typer to insert contextual arguments when it is inside arguments of HOAS patterns #18040

Merged

Make HOAS Quote pattern match with def method capture #17567

Make HOAS Quote pattern match with def method capture #17567

Uh oh!

Conversation

zeptometer commented May 24, 2023 • edited by nicolasstucki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

zeptometer May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

zeptometer commented Jun 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zeptometer commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeptometer Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeptometer commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Jun 19, 2023

nicolasstucki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

2 participants

zeptometer commented May 24, 2023 •

edited by nicolasstucki

Loading

zeptometer May 24, 2023 •

edited

Loading

zeptometer commented Jun 3, 2023 •

edited

Loading

zeptometer commented Jun 4, 2023 •

edited

Loading

zeptometer Jun 15, 2023 •

edited

Loading

zeptometer commented Jun 18, 2023 •

edited

Loading