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

Add scala.quoted.Expr.unapply as dual of Expr.apply #10580

Closed
wants to merge 1 commit into from

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Dec 1, 2020

Expr.unapply replaces Unlifted.unapply using the same signature that uses an Unliftable to perform the unlifting. The overload the works on sequences of expression is moved to Exprs.unapply (similar to Consts.unapply).

Migration

- case '{... ${Unlifted(value)} ... }
+ case '{... ${Expr(value)} ... } =>
- case '{... ${Varargs(Unlifted(values))} ... } =>
+ case '{... ${Varargs(Expr(values))} ... } =>

@nicolasstucki
Copy link
Contributor Author

@liufengyun this improves the issues with the Unlifted name and aligns the Liftable/Unliftable API on Expr.

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.

Looking at the diff below, I find Const make the program more readable and easily understandable:

-      case '{ power($x, ${Const(y)}) } =>
+      case '{ power($x, ${Expr(y)}) } =>

@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Dec 1, 2020

The point of this PR is to improve the usability of Unlifted, not Const.

- case '{ power(${Unlifted(x)}, ${Unlifted(y)}) } => Expr(power(x, y))  
+ case '{ power(${Expr(x)}, ${Expr(y)}) } => Expr(power(x, y))    

@nicolasstucki nicolasstucki force-pushed the add-Expr-unapply branch 2 times, most recently from b55981b to 4d746f5 Compare December 2, 2020 07:55
@nicolasstucki
Copy link
Contributor Author

Looking at the diff below, I find Const make the program more readable and easily understandable:

-      case '{ power($x, ${Const(y)}) } =>
+      case '{ power($x, ${Expr(y)}) } =>

If we look at the full code it is actually much clearer with Expr as we both match and construct it with Expr.

- case '{ power($x, ${Const(y)}) } =>
-   if y == 0 then Expr(1)
-   else '{ times($x, power($x, ${Expr(y-1)})) }
+ case '{ power($x, ${Expr(y)}) } =>
+   if y == 0 then Expr(1)
+   else '{ times($x, power($x, ${Expr(y-1)})) }

Though this is out of scope for this PR

@nicolasstucki
Copy link
Contributor Author

This is clearly the most intuitive version we had so far due to the symmetry between Expr.apply and Expr.unapply. This works in simple patterns as well as in nested patterns in quote patterns.

Expr(4) match 
  case Expr(n) => Expr(n)
'{ println(5) } match
  case '{ println(5) } =>
    '{ println(5) }
'{ println(${Expr(5)}) } match
  case '{ println(${Expr(n)}) } =>
    '{ println(${Expr(n)}) }

@nicolasstucki nicolasstucki added this to the 3.0.0-M3 milestone Dec 2, 2020
@nicolasstucki nicolasstucki added prio:blocker release-notes Should be mentioned in the release notes labels Dec 2, 2020
`Expr.unapply` replaces `Unlifted.unapply` using the same signature that uses an `Unliftable` to perform the unlifting. The overload the works on sequences of expression is moved to `Exprs.unapply` (similar to `Consts.unapply`).
@abgruszecki
Copy link
Contributor

abgruszecki commented Dec 2, 2020

Having an Expr "deconstructor" that matches only a tiny subset of values of type Expr feels really weird to me. I think that the "liftable" concept corresponds to constant values, so names which look much more natural to me are Const for what Expr means in the current PR (both the constructor and the deconstructor). To avoid confusion, we can rename Constant to PrimitiveConst, which I think matches the intent nicely.

@nicolasstucki
Copy link
Contributor Author

Expr extractor can match every value created with the Expr constructor. You may be confusing the Expr construction with quote '{ .. } construction.

@abgruszecki
Copy link
Contributor

Right, by the Expr constructor, but that's by far not the only way to construct an Expr. That's the part that looks confusing at first glance - usually if a pattern is named the same as a type, it will always match values of that type. Imagine if there was a String.apply(i: Int): String method. Would a pattern String(i) that only matches strings corresponding to integers look strange/unexpected? This is exactly how the Expr pattern from this PR looks to me at first glance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:blocker release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants