Rename `Liftable` to `ToExpr` and `Unliftable` to `FromExpr` #10618

nicolasstucki · 2020-12-03T09:16:19Z

Rename Liftable to ToExpr and name its method apply

- trait Liftable[T]:
-   def toExpr(x: T): Quotes ?=> Expr[T]
+ trait ToExpr[T]:
+   def apply(x: T)(using Quotes): Expr[T]

Rename Unliftable to FromExpr and name its method unapply

- trait Unliftable[T]:
-   def fromExpr(x: Expr[T]): Quotes ?=> Option[T]
+ trait FromExpr[T]:
+   def unapply(x: Expr[T])(using Quotes): Option[T]

Move Unlifted.unapply(Expr[T]) to Expr.unapply
Move Unlifted.unapply(Seq[Expr[T]]) to Exprs.unapply
Rename (Expr).unlift to (Expr).value
Rename (Expr).unliftOrError to (Expr).valueOrError

nicolasstucki · 2020-12-03T09:21:04Z

@liufengyun I updated all the names and methods related to Liftable/Unliftable that were discussed during the meeting. I updated the docs, though there is potential to make them more precise which I plan to do in the future. I will update the community build once the PR is approved to avoid conflicts.

liufengyun

LGTM

liufengyun · 2020-12-03T12:52:31Z

library/src-bootstrapped/scala/quoted/Expr.scala

+   *  To directly get the value of an expression `expr: Expr[T]` consider using `expr.value`/`expr.valueOrError` insead.
+   */
+  def unapply[T](x: Expr[T])(using FromExpr[T])(using Quotes): Option[T] =
+    scala.Predef.summon[FromExpr[T]].unapply(x)


If we want to keep this general extractor, a more informative name like Value might be better.

I thought about that one. The issue is that it would either fragment concepts into Expr.apply/Value.unapply or we would need to duplicate some logic between Expr and Value which might lead to confusion.

liufengyun · 2020-12-03T12:55:08Z

library/src-bootstrapped/scala/quoted/Expr.scala

@@ -43,7 +58,7 @@ object Expr {
  def ofSeq[T](xs: Seq[Expr[T]])(using Type[T])(using Quotes): Expr[Seq[T]] =
    Varargs(xs)


Now we have @targetName feature, may be we can rename Expr.ofSeq to Expr.apply? Just to open the discussion, honestly I find Expr.ofSeq is more informative and easy to find.

That won't work, the name of the method is not to distinguish the arguments but the target type. Expr(seq) should this return a Expr[Seq[T]] or a Expr[List[T]]? Also, Exprs might be a better place for it.

Let's keep this for a follow-up PR.

Maybe we can only have Value.apply and Value.unapply. This would imply that Expr(v) will not work anymore.

This part of the API clearly needs a deeper analysis. I will try it in a separate PR and we can keep this one only to remove the Liftable/Unliftable.

Values seems to be a much better name as it captures much better the semantics of the operation in the names. The only downside is that it would affect 99% of the macros.

Maybe we can only have Value.apply and Value.unapply. This would imply that Expr(v) will not work anymore.

It seems to be Expr(v) is better than Value.apply, as we are constructing a value of Expr[T]. Could you please elaborate?

To avoid confusions we need to have Expr.apply/Expr.unapply or Value.apply/Value.unapply.

Would you understand if you relate the Some concept if your code would be like the following?

Some(2) match case Just(n) => // or Just(2) match case Some(n) =>

I would find it extremely confusing and would probably assume they are different concepts. Even though logically they are the dual of each other.

There is a difference here: for Trees, we expect there are different cases correspond to different syntactic forms, but for Option we don't have the expectation.

That would be true for trees but here we are on the Expr abstraction which hides those details. In the expression world you don't distinguish '{1}, '{{1}}, '{{{1}}}, ... . Those are only differences that can be seen on Trees.

library/src-bootstrapped/scala/quoted/FromExpr.scala

library/src-bootstrapped/scala/quoted/ToExpr.scala

liufengyun · 2020-12-03T13:19:39Z

library/src/scala/quoted/Exprs.scala

+        case _ => return None
+    Some(builder.result())
+
+end Exprs


What about renaming it to Values?

It is a nice name but the is the fragmentation or duplication concern.

library/src/scala/quoted/Quotes.scala

* Rename `Liftable` to `ToExpr` and name its method `apply` ```diff - trait Liftable[T]: - def toExpr(x: T): Quotes ?=> Expr[T] + trait ToExpr[T]: + def apply(x: T)(using Quotes): Expr[T] ``` * Rename `Unliftable` to `FromExpr` and name its method `unapply` ```diff - trait Unliftable[T]: - def fromExpr(x: Expr[T]): Quotes ?=> Option[T] + trait FromExpr[T]: + def unapply(x: Expr[T])(using Quotes): Option[T] ``` * Move `Unlifted.unapply(Expr[T])` to `Expr.unapply` * Move `Unlifted.unapply(Seq[Expr[T]])` to `Exprs.unapply` * Rename `(Expr).unlift` to `(Expr).value` * Rename `(Expr).unliftOrError` to `(Expr).valueOrError`

nicolasstucki requested a review from liufengyun December 3, 2020 09:21

nicolasstucki assigned liufengyun Dec 3, 2020

This comment has been minimized.

Sign in to view

nicolasstucki force-pushed the rename-liftable-and-unliftable branch from 65662ba to 588930c Compare December 3, 2020 09:55

nicolasstucki marked this pull request as ready for review December 3, 2020 12:12

nicolasstucki force-pushed the rename-liftable-and-unliftable branch from 588930c to e04ae8d Compare December 3, 2020 12:42

liufengyun approved these changes Dec 3, 2020

View reviewed changes

nicolasstucki force-pushed the rename-liftable-and-unliftable branch 2 times, most recently from aa4543d to 0e97698 Compare December 3, 2020 15:37

nicolasstucki added the release-notes Should be mentioned in the release notes label Dec 3, 2020

nicolasstucki force-pushed the rename-liftable-and-unliftable branch from 0e97698 to 6c5cd2e Compare December 4, 2020 08:29

nicolasstucki added 2 commits December 4, 2020 11:40

Update community-build

664f7a4

nicolasstucki force-pushed the rename-liftable-and-unliftable branch from bc6a97f to 664f7a4 Compare December 4, 2020 10:43

nicolasstucki merged commit a853d9b into scala:master Dec 4, 2020

nicolasstucki deleted the rename-liftable-and-unliftable branch December 4, 2020 12:48

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

		@@ -43,7 +58,7 @@ object Expr {
		def ofSeq[T](xs: Seq[Expr[T]])(using Type[T])(using Quotes): Expr[Seq[T]] =
		Varargs(xs)

Rename Liftable to ToExpr and Unliftable to FromExpr #10618

Rename Liftable to ToExpr and Unliftable to FromExpr #10618

Uh oh!

Conversation

nicolasstucki commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Dec 3, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Dec 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Dec 4, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rename `Liftable` to `ToExpr` and `Unliftable` to `FromExpr` #10618

Rename `Liftable` to `ToExpr` and `Unliftable` to `FromExpr` #10618

nicolasstucki commented Dec 3, 2020 •

edited

Loading

nicolasstucki Dec 3, 2020 •

edited

Loading

nicolasstucki Dec 4, 2020 •

edited

Loading