Skip to content

Remove scala.quoted.autolift implicit conversion #9304

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

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

nicolasstucki
Copy link
Contributor

It has been discouraged to use this implicit conversion for a while as it interacts poorly
with type inference. To avoid further issues we will simply remove it from the library.

Note: Anyone could implement it in their project with a single line of code

@nicolasstucki nicolasstucki self-assigned this Jul 6, 2020
@nicolasstucki nicolasstucki requested a review from liufengyun July 6, 2020 16:41
@nicolasstucki nicolasstucki changed the title Remove scala.quoted.autolift Remove scala.quoted.autolift implicit conversion Jul 6, 2020
It has been discouraged to use this implicit conversion for a while as it interacts poorly
with type inference. To avoid further issues we will simply remove it from the library.
@nicolasstucki nicolasstucki force-pushed the remove-quoted-autolift branch from 58ec69b to acc1612 Compare July 7, 2020 08:15
@nicolasstucki nicolasstucki marked this pull request as ready for review July 7, 2020 11:05
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

@@ -33,25 +32,25 @@ object Macro {
private[this] var oldReported = false
def partError(message : String, index : Int, offset : Int) : Unit = {
reported = true
errors += '{ Tuple5(true, 0, $index, $offset, $message) }
errors += '{ Tuple5(true, 0, ${Expr(index)}, ${Expr(offset)}, ${Expr(message)}) }
Copy link
Contributor

@liufengyun liufengyun Jul 7, 2020

Choose a reason for hiding this comment

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

For such use cases, it seems we lose a little usability here.

However, from my experience with Scala 3 macros, it is always good to be explicit, as it is so easy to make mistakes and write difficult to understand code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the verbosity in only there because I made sure to not change the semantics in the tests.
Seen this one would immediately try to make it shorter and the natural way would be to write:

errors += Expr(Tuple5(true, 0, index, offset, message))

@nicolasstucki nicolasstucki merged commit 25a48dd into scala:master Jul 7, 2020
@nicolasstucki nicolasstucki deleted the remove-quoted-autolift branch July 7, 2020 13:07
@szeiger
Copy link
Contributor

szeiger commented Aug 3, 2020

Wouldn't it make more sense to do auto-lifting of terms the same way it already works for types? Types can be moved from level 0 to level 1 without any boilerplate as long as an implicit Type is available. So why not auto-lift values when an implicit Liftable is in scope? The syntax is even simpler than the old auto-lifting and the types are correct.

@nicolasstucki
Copy link
Contributor Author

The problem is that it only works properly when all types are statically known. But when the lifted type is part of some type inference or another implicit search the and it fails, the error messages can be quite unhelpful.

Also, If you really want it, you just need to add the following line in your project.

given autolift[T](using Liftable[T], QuoteContext) as Conversion[T, Expr[T]] = Expr(_)

@szeiger
Copy link
Contributor

szeiger commented Aug 3, 2020

What I am suggesting is to not use Expr types and splicing at all for auto-lifting.

Manual lifting:

case class Foo[T](a: Int, b: T)
...
case Foo[T](a, b) => '{ Foo[T](${Expr(a)}, ${Expr(b)}) }

Old auto-lifting:

case Foo[T](a, b) => '{ Foo[T]($a, $b) }

Simplified auto-lifting proposal:

case Foo[T](a, b) => '{ Foo[T](a, b) }

This is already correctly typed, no implicit conversions required. The compiler knows that all of T, a and b have to lifted from level 0 to level 1. It already does that automatically for T if a given Type[T] is available. It could do the same for a and b if given Liftable[Int] and Liftable[T] are available.

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.

3 participants