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

Allow to beta reduce curried function applications in quotes reflect #18121

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Jul 3, 2023

Closes #17506
Previously, the curried functions with multiple applications were not able to be beta-reduced in any way, which was unexpected. Now, we allow reducing any number of top-level function applications for a top-level curried function. This was also made clearer in the documentation for the affected (Expr.betaReduce and Term.betaReduce) methods.

I've extended the test provided by the minimisation from the above issue with multiple additional tests for beta reduction with type parameters and more-than-two curried function applications.

@jchyb jchyb self-assigned this Jul 4, 2023
@jchyb jchyb marked this pull request as draft July 4, 2023 09:16
@jchyb jchyb removed their assignment Jul 4, 2023
@jchyb jchyb marked this pull request as ready for review July 4, 2023 11:00
@jchyb jchyb requested a review from nicolasstucki July 4, 2023 11:00
@nicolasstucki nicolasstucki self-assigned this Jul 4, 2023
tests/pos-macros/i17506/Test.scala Outdated Show resolved Hide resolved
tests/pos-macros/i17506/Macro.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala Show resolved Hide resolved
tpd.Apply(tpd.Select(expr1, nme), args)
).withSpan(tree.span)
}
case tpd.Apply(tpd.TypeApply(tpd.Select(expr: Apply, nme), tpts), args) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is becoming a bit ad-hoc. I feel that my original design was not the best.

I wonder if we could simplify this using a TreeMap. I have something like this in mind. Though I have not tested it.

val tree1 = new TreeMap {
  def transform(tree: Tree)(using Context): Tree = tree match {
    case tpd.Block(Nil, _) | tpd.Inlined(_, Nil, _) => super.transform(tree)
    case _: tpd.Apply | _: tpd.TypeApply  =>
      val tree1 = super.transform(tree)
      if tree.tpe.isInstanceOf[MethodicType] then tree1
      else dotc.transform.BetaReduce(tree1).withSpan(tree.span)
    case _ => tree
  }
}.transform(tree)
if tree1 eq tree then None else Some(tree1)

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 replaced this with a TreeMap now but I was unable to simplify the overall logic there by much (but at the very least the Block and Inlined cases are simpler)

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala Outdated Show resolved Hide resolved
@nicolasstucki nicolasstucki assigned jchyb and unassigned nicolasstucki Jul 5, 2023
@jchyb jchyb force-pushed the fix-i17506 branch 2 times, most recently from 2f630c5 to f3dda82 Compare July 28, 2023 11:49
@jchyb jchyb requested a review from nicolasstucki July 31, 2023 08:36
@jchyb jchyb assigned nicolasstucki and unassigned jchyb Jul 31, 2023
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

* To retain semantics the argument `ei` is bound as `val yi = ei` and by-name arguments to `def yi = ei`.
* Some bindings may be elided as an early optimization.
*
* Example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I still belive this is to complex for a first-time user. I would call this the general rule.

Suggested change
* Example:
* Generally:

I would add a trivial beta reduction example before this.

Example:
```scala sc:nocompile
((a: Int, b: Int) => a + b).apply(x, y)
```
will be reduced to
```scala sc:nocompile
val a = x
val b = y
a + b
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see that you did that in the other file. We should have the same example in both documentations. I believe that my new example will be better for new users that are less experienced.

Previously, the curried functions with multiple applications
were not able to be beta-reduced in any way, which was unexpected.
Now we allow reducing any number of top-level function applications
for a curried function. This was also made clearer in the documentation
for the affected (Expr.betaReduce and Term.betaReduce) methods.
@jchyb
Copy link
Contributor Author

jchyb commented Mar 26, 2024

@nicolasstucki Sorry for the (very) delayed update on this, today I have finally updated the documentation as suggested

@jchyb jchyb requested a review from nicolasstucki March 26, 2024 12:01
@jchyb jchyb assigned nicolasstucki and unassigned jchyb Mar 26, 2024
@jchyb jchyb merged commit 447cdf4 into scala:main Apr 8, 2024
19 checks passed
@jchyb jchyb deleted the fix-i17506 branch April 8, 2024 08:59
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
WojciechMazur added a commit that referenced this pull request Jul 5, 2024
…s reflect" to LTS (#21040)

Backports #18121 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
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.

Term.betaReduce not working for curried context functions
3 participants