-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
tpd.Apply(tpd.Select(expr1, nme), args) | ||
).withSpan(tree.span) | ||
} | ||
case tpd.Apply(tpd.TypeApply(tpd.Select(expr: Apply, nme), tpts), args) => |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
2f630c5
to
f3dda82
Compare
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
* 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
```
There was a problem hiding this comment.
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.
@nicolasstucki Sorry for the (very) delayed update on this, today I have finally updated the documentation as suggested |
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
andTerm.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.