Skip to content

Deprecate old Expr value extractors #10684

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Deprecate old Expr value extractors #10684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions library/src-bootstrapped/scala/quoted/FromExpr.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ object FromExpr {
*/
given StringContextFromExpr: FromExpr[StringContext] with {
def unapply(x: Expr[StringContext])(using Quotes) = x match {
case '{ new StringContext(${Varargs(Consts(args))}: _*) } => Some(StringContext(args: _*))
case '{ StringContext(${Varargs(Consts(args))}: _*) } => Some(StringContext(args: _*))
case '{ new StringContext(${Varargs(Exprs(args))}: _*) } => Some(StringContext(args: _*))
case '{ StringContext(${Varargs(Exprs(args))}: _*) } => Some(StringContext(args: _*))
case _ => None
}
}
Expand Down
2 changes: 2 additions & 0 deletions library/src/scala/quoted/Const.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package scala.quoted

/** Literal constant values */
@deprecated("Use `scala.quoted.Expr` instead. This will be removed in 3.0.0-RC1", "3.0.0-M3")
object Const {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember in the meeting we agree to keep Const as an extractor. Or I misunderstood?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep it in scala.quoted.util as it duplicates logic in scala.quoted. This is the only responsable action as we could add it back in scala.quoted but we would not be able to remove it.

It is already in scala.quoted.util.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be a pity that when a meta-programmer wants to pattern macher on a constant (which is a common need as we know in meta-programming), he has to add another library as a dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is that they can do it using .value or Expr. Both cover all the cases that Const covers without the performance penalty and with better warnings in the pattern matching. It would be a pity to give them a version that is just tempting to use due to its name, but is always a worse option.

We have already seen that all uses cases in our test and in the community-build where better with .value/Expr. We do not have any remaining places were we would use Const.

Explaining this extractor to new users is also unnecessarily hard and confusing. After introducing .value/Expr we need to tell them that they can optionally use this other variant that does less and is less performant. Why would a user want to use it then? Then the next step is to tell them that the Const extractor does not match Constant, the reason being that Const matches what Expr matches but only for primitive types. This amount of confusion is too much. I know what is the difference between those extractors, but even you, having worked with them for a long time, have show confusion about the differences.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have already seen that all uses cases in our test and in the community-build where better with .value/Expr. We do not have any remaining places were we would use Const.

I basically agree with this. However, we cannot exclude some use cases where the usage of Const(v) in pattern matching is more convenient. If I were correct, the typical textbook example power is one such usage.

On the other hand, something is missing at the conceptual level if there is no capability to directly pattern match on literals, while it is possible to do so on other syntactic forms. If we remove this capability, it would be good to remove pattern matching on quotes completely, and only keep tree pattern matching in reflect, which IMHO is a reasonable choice.

Explaining this extractor to new users is also unnecessarily hard and confusing.

It seems to me in meta-programming, inspecting literals is very natural, and having a dedicated extractor for that will not cause problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the following power?

def power(xExpr: Expr[Double], nExpr: Expr[Int])(using Quotes): Expr[Double] =
  (xExpr.value, nExpr.value) match 
     case (Some(x), Some(n)) => Expr(x.pow(n))
     case (Some(x), _) => '{...}
     case (_, Some(n)) => unrollPower(xExpr, n)
     case (_, _) => '{ $xExpr.pow($nExpr) }

How would that benefit from using Const or Expr? We could do the following but it only has drawbacks. Calling Const.unapply is more expensive than .value and we need to call it twice instead of once.

-  (xExpr, nExpr) match 
-    case (Const(x), Const(n)) => Expr(x.pow(n))
-     case (Const(x), _) => '{...}
-     case (_, Const(n)) => unrollPower(xExpr, n)
-     case (_, _) => '{ $xExpr.pow($nExpr) }

It seems to me in meta-programming, inspecting literals is very natural, and having a dedicated extractor for that will not cause problems.

We have a dedicated concept for it which is called FromExpr and used by Expr.unapply. Calling it Const or Literal only leads to confusion as those do not match Constants or Literals. They match what Expr matches. It does not make any sense overload concepts and bloat the API with duplicated logic. It is simpler to explain Expr and be done with it rather than giving them another option that does less but requires the understanding the Expr first.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the following power?

def power(xExpr: Expr[Double], nExpr: Expr[Int])(using Quotes): Expr[Double] =
  (xExpr.value, nExpr.value) match 
     case (Some(x), Some(n)) => Expr(x.pow(n))
     case (Some(x), _) => '{...}
     case (_, Some(n)) => unrollPower(xExpr, n)
     case (_, _) => '{ $xExpr.pow($nExpr) }

How would that benefit from using Const or Expr? We could do the following but it only has drawbacks. Calling Const.unapply is more expensive than .value and we need to call it twice instead of once.

-  (xExpr, nExpr) match 
-    case (Const(x), Const(n)) => Expr(x.pow(n))
-     case (Const(x), _) => '{...}
-     case (_, Const(n)) => unrollPower(xExpr, n)
-     case (_, _) => '{ $xExpr.pow($nExpr) }

Thanks for correcting my memory. I have in mind use cases like case '{ power(${const(x)}. $y) } => ..., where the usage of extractors are more convenient than .value.

It seems to me in meta-programming, inspecting literals is very natural, and having a dedicated extractor for that will not cause problems.

We have a dedicated concept for it which is called FromExpr and used by Expr.unapply. Calling it Const or Literal only leads to confusion as those do not match Constants or Literals. They match what Expr matches. It does not make any sense overload concepts and bloat the API with duplicated logic. It is simpler to explain Expr and be done with it rather than giving them another option that does less but requires the understanding the Expr first.

It seems to be the exact reason why it cannot be renamed --- because Expr.unapply is not dedicated for pattern matching on literals. Missing a dedicated extractor for a category of syntactic forms seems to be a flaw at the coneptual level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be renamed as shown in #10673. But there was some backlash against that. Either way, we cannot add every of the extractor in the library. This is why scala.quoted.util was created. This way we can provide different more specialized extractors such as Const, Value, StringContextExpr, ... . All those extractors that are not required or not necessarily practical to all users.

I cannot spend more days fixing this issue. We should just merge it. Users will be able to use trivially the scala.quoted.util.Const instead or other variants that we might come up with that is more inline with their goals. If in the future we see actual use cases, then we can consider moving it to scala.quoted.

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be arguments for the design I'm not aware of. Please do what makes sense.


/** Matches expressions containing literal constant values and extracts the value.
Expand All @@ -18,6 +19,7 @@ object Const {
*
* To directly unlift an expression `expr: Expr[T]` consider using `expr.value`/`expr.valueOrError` insead.
*/
@deprecated("Use `scala.quoted.Expr.unapply` instead. This will be removed in 3.0.0-RC1", "3.0.0-M3")
def unapply[T](expr: Expr[T])(using Quotes): Option[T] = {
import quotes.reflect._
def rec(tree: Term): Option[T] = tree match {
Expand Down
2 changes: 2 additions & 0 deletions library/src/scala/quoted/Consts.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package scala.quoted

/** Literal constant values */
@deprecated("Use `scala.quoted.Exprs` instead. This will be removed in 3.0.0-RC1", "3.0.0-M3")
object Consts {

/** Matches literal sequence of literal constant value expressions and return a sequence of values.
Expand All @@ -17,6 +18,7 @@ object Consts {
*
* To directly unlift all expressions in a sequence `exprs: Seq[Expr[T]]` consider using `exprs.map(_.value)`/`exprs.map(_.valueOrError)` insead.
*/
@deprecated("Use `scala.quoted.Exprs.unapply` instead. This will be removed in 3.0.0-RC1", "3.0.0-M3")
def unapply[T](exprs: Seq[Expr[T]])(using Quotes): Option[Seq[T]] =
exprs.foldRight(Option(List.empty[T])) { (elem, acc) =>
(elem, acc) match {
Expand Down