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

Replace AsFunction implicit class with Expr.reduce #7299

Merged

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki self-assigned this Sep 24, 2019
@nicolasstucki nicolasstucki force-pushed the remove-asFunction-implicit-class branch 2 times, most recently from b44e0ff to 30915f0 Compare September 24, 2019 13:06
@nicolasstucki nicolasstucki marked this pull request as ready for review September 24, 2019 13:50
Copy link
Contributor

@liufengyun liufengyun 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

```scala
AsFunction(_).apply: Expr[S => T] => (Expr[S] => Expr[T])
Expr.reduce(_).apply: Expr[(T1, ..., Tn) => R] => ((Expr[T1], ..., Expr[Tn]) => Expr[R])
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call it distribute? From wikipedia:

K, Distribution Axiom: □(p → q) → (□p → □q).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

distribite is not precise, it only captures a generic property in the types. It would not describe what the method actually does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also reduce itself is not a distribution, reduce(_) is the a distribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @liufengyun. Reduce is counter intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have another suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

The following sentence does not read well, given the literal meaning of beta-reduction:

The Expr companion object contains a betaReduce conversion that turns a tree
describing a function into a function mapping trees to trees.

library/src/scala/quoted/Expr.scala Show resolved Hide resolved
import qctx.tasty._
tg.untupled(args => qctx.tasty.internal.betaReduce(f.unseal, args.toArray.toList.map(_.asInstanceOf[QuoteContext => Expr[_]](qctx).unseal)).seal.asInstanceOf[Expr[R]])
}
/** `Expr.reduceGiven(f)(x1, ..., xn)` is functionally the same as `'{($f)(given $x1, ..., $xn)}`, however it optimizes this call
Copy link
Contributor

Choose a reason for hiding this comment

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

A function with a given parameter list is still considered contextual so keeping the terminology Contextual is still relevant. WDYT about reduceGiven to distributeContextual?

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 considered Contextual but it is a bit long (though it could be acceptable)
  • The operation does not do a distribution

Copy link
Contributor

@biboudis biboudis left a comment

Choose a reason for hiding this comment

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

In partial evaluation literature reduce is usually a function for symbolic reduction expressions that returns either a value (a number or an S expression) or a reduced residual expression) according to whether the passed e is static or dynamic (Section 5.4.2 in Partial Evaluation and Automatic Code Generation).

What about callStatic or something else?

@@ -1,6 +1,6 @@
import scala.quoted._

def testImpl(f: Expr[(Int, Int) => Int])(given QuoteContext): Expr[Int] = f('{1}, '{2})
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, by reading this reduce confuses a lot.

@nicolasstucki
Copy link
Contributor Author

In partial evaluation literature reduce is usually a function for symbolic reduction expressions that returns either a value (a number or an S expression) or a reduced residual expression) according to whether the passed e is static or dynamic (Section 5.4.2 in Partial Evaluation and Automatic Code Generation).

This function does exactly one step of that reduction depending if the f is a static or dynamic function.

@biboudis
Copy link
Contributor

biboudis commented Sep 25, 2019

It is a nitpick because reduce reminds folding: but my proposals are betaReduce as is the internal name or just callStatic otherwise LGTM.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

@liufengyun liufengyun merged commit 0e1c29d into scala:master Sep 25, 2019
@liufengyun liufengyun deleted the remove-asFunction-implicit-class branch September 25, 2019 11:59
@anatoliykmetyuk anatoliykmetyuk added this to the 0.20 Tech Preview milestone Oct 31, 2019
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.

None yet

4 participants