Add QuoteContext #6700

nicolasstucki · 2019-06-17T13:52:32Z

The main idea is to correctly keep track of the current compiler. This is paramount to support correctly show and quote patterns. This PR only contains the addition of the QuoteContext to the run but not the top level $ (which will come in a second step).

Add QuoteContext
Define a single Expr.show entry point in QuoteContext(for expr.show)
Remove coloring options from the Reflection.Context and Toolbox settings (show never has colors unless a coloring format is given)
Replace TreeCleaner with @quoteTypeTag and logic in the printers
Fix ArrayIndexOutOfBoundsException while unpickling quote #5161
Regression local definitions are not renamed in quotes (postponed for future PR). See tests/run-with-compiler/quote-run-2.check.

Followed by #6723

liufengyun

Otherwise, LGTM

The concern I have is the following, which we can discuss during the meeting:

stability of macro interface (it's planned to be changed Move reflection inside QuoteContext #6723 )
usability: macro authors have to deal with QuoteContext, which they may not care

compiler/src/dotty/tools/dotc/quoted/QuoteCompiler.scala

compiler/src/dotty/tools/dotc/quoted/ExprCompilationUnit.scala

liufengyun · 2019-06-24T09:35:49Z

library/src/scala/quoted/Toolbox.scala

-  def run[T](expr: Expr[T]): T
-  def show[T](expr: Expr[T]): String
-  def show[T](tpe: Type[T]): String
+  def run[T](expr: QuoteContext => Expr[T]): T
 }


Is it possible to merge the concept Toolbox with QuoteContext?

Not really. The toolbox represents an instance of the compiler (including caches) while the QuoteContext corresponds to a single compilation run.

liufengyun · 2019-06-24T09:38:10Z

library/src/scala/quoted/show/package.scala

+
+  def apply[T](expr: given QuoteContext => Expr[T]) given (toolbox: Toolbox): String = run(expr.show.toExpr)
+
+}


Why not just expr.show instead of run(expr.show.toExpr)?

Because the prefix is of type Expr[T] but we need a given QuoteContext => Expr[T] which we cannot encode as an extension method

Aslo, this show intanciated a new compiler run which we should never do inside a run or a macro. Better to not get the two method confused

It would be clearer for the users if we remove this variant of show and use only the Expr.show. The latter one can only be called within the a run { ... } or a macro ${ ... }. In case they write tests and only care about showing the result of computing some closed expression they could write run(expr.show.toExpr) themselves.

liufengyun · 2019-06-24T09:39:52Z

library/src/scala/tasty/Reflection.scala

+  // For backward compat with macros
+  implicit def reflectionToQuoteContext(implicit reflect: Reflection): scala.quoted.QuoteContext =
+    new scala.quoted.QuoteContext(reflect)
+


As discussed, in macros it will improve usability if we can reduce the number of concepts.

This one will be simplified the next time we update the reference compiler. The de facto implicit will be QuoteContext.

liufengyun · 2019-06-24T09:43:00Z

tests/run-with-compiler/i3823-b.scala

@@ -5,6 +5,6 @@ object Test {
      val z: $t = $x
    }
    implicit val toolbox: scala.quoted.Toolbox = scala.quoted.Toolbox.make(getClass.getClassLoader)
-    println(f('{2})(Type.IntTag).show)
+    println(show(f('{2})(Type.IntTag)))


It seems to me for staging, having Expr[T].show will make debugging way easier.

Yes, that one is supported as well. In this case I used the variant of show to minimize the diff.

The test could also have been rewritten as

implicit val toolbox: scala.quoted.Toolbox = scala.quoted.Toolbox.make(getClass.getClassLoader) run { println(f('{2})(Type.IntTag).show) '{} }

or

implicit val toolbox: scala.quoted.Toolbox = scala.quoted.Toolbox.make(getClass.getClassLoader) println(run { f('{2})(Type.IntTag).show.toExpr })

It seems a little unnatural that we need to run in order to show expression.

We need to be inside the macro ${ ... } or the run { ... } that generated the expression in the first place. The only usecase of using show without a run of a $ splice is in tests.

liufengyun · 2019-06-25T08:06:22Z

compiler/src/dotty/tools/dotc/quoted/QuoteDriver.scala

@@ -34,7 +34,7 @@ class QuoteDriver(appClassloader: ClassLoader) extends Driver {
    val ctx = setToolboxSettings(ctx0.fresh.setSetting(ctx0.settings.outputDir, outDir), settings)

    val driver = new QuoteCompiler
-    driver.newRun(ctx).compileExpr(expr)
+    driver.newRun(ctx).compileExpr(exprBuilder)


It seems after driver.newRun(ctx), we already be able to create an instance of QuoteContext to call exprBuilder(), such that we can keep ExprCompilationUnit and related API unchanged as before?

We do have a context but it is not the correct one. In compileExpr we call compileUnits which setup the correct context . I would not dare duplicating this logic here.

liufengyun · 2019-06-25T08:15:25Z

library/src-3.x/scala/quoted/Expr.scala

  sealed abstract class Expr[+T] {

    /** Evaluate the contents of this expression and return the result.
     *
     *  May throw a FreeVariableError on expressions that came from a macro.
     */
    @deprecated("Use scala.quoted.run", "")
-    final def run(implicit toolbox: Toolbox): T = toolbox.run(this)
+    final def run(implicit toolbox: Toolbox): T = toolbox.run(_ => this)


Thanks to the explanation by @nicolasstucki, now I understand run is double-meaning for a compiled language: it means to execute the expression, but also mean inside a specific compiler run --- Expr[T] may only be created for a specific compiler run.

If that is the case, it seems make sense to use path-dependent types to make Expr[T] depend on compiler run.

liufengyun · 2019-06-25T10:43:53Z

After a thorough discussion with @nicolasstucki , I have the following feedback:

The overall design of QuoteContext is a good direction to unify quoted macros and staging.
The refactoring of the macro interface is based on the assumption that most macros will be quoted macros, only advanced macros will be reflection macros.
@nicolasstucki mentioned the constraint that Expr[T] unpicked from a compiler run context may only compile in that compiler run. It would be good to document this compiler invariant.
If the constraint generalizes to Expr[T] always dependent on a compiler run, then a logical conclusion would be make Expr[T] path-dependent on QuoteContext --- which will have a big impact on the usability of Expr[T]. It would be good to document this.
For staging, current PR only exposes run that gives a QuoteContext, which does not make sense if a meta-programmer only wants to generate C code from staging or just show an Expr (no compile nor run).
It would be good to evaluate how does the same-run constraint play with the scenario above and how does it impact the API design.

Points 5 and 6 may be addressed later, as it's not a priority. And I propose wait 2 release cycles (12 weeks) before we remove the support for the old macro interface, so that we may get feedback if people are already using reflection macros.

/cc: @odersky @biboudis you may want to have a look too.

library/src-3.x/scala/quoted/Expr.scala

biboudis

Update the staging doc and the description of the PR with the motivation and its ok with me.

For motivation I propose this #5161 (comment) (at least that helped me understand)

edit: after discussions a new withFreshContext { show ... } will be used instead of run(show ...) for improved understandability

liufengyun · 2019-06-27T08:47:51Z

library/src-3.x/scala/quoted/package.scala

+   *
+   *  This method shoul not be called in a context where there is already has a QuoteContext.
+   */
+  def withNewQuoteContext[T](thunk: given QuoteContext => T) given (toolbox: Toolbox): T = {


Nitpick: withQuoteContext seems to be a better name, as the provided QuoteContext is assumed to be scoped by the thunk --- there is no old context there, so new does not convey any meaning.

Another thing for documentation is: what if programmer nest run or withQuoteContext --- it's forbidden or allowed.

The main idea is to correctly keep track of the current compiler. This is paramount to support correctly `show` and quote patterns. This PR only contains the addition of the `QuoteContext` to the `run` but not the top level `$` (which will come in a second step). * Add `QuoteContext` * Define a single `Expr.show` entry point in `QuoteContext`(for `expr.show`) * Define `show(expr)` which is an alias of `run(expr.show.toExpr)` that provides a `QuoteContext` * Remove coloring options from the `Reflection.Context` and `Toolbox` settings (`show` never has colors unless a coloring format is given) * Replace `TreeCleaner` with `@quoteTypeTag` and logic in the printers * Fix scala#5161 * Regression local definitions are not renamed in quotes (postponed for future PR). See tests/run-with-compiler/quote-run-2.check.

`scala.quoted.show.apply` was only intorduced to reduce the diff in the tests. Instead we use the equivalent `run(expr.show.toExpr)` notation explicitly. This has the added advantage of showing explicitly where new compiler runs are located.

This provides a way to materialize a QuoteContext without calling `run` and hence performing the full compilation. It is useful if some user only wants to show or inspect an expression without evaluating it. Added a general optimization where if the expression passed to `run` is a known literal, the literal is returned directly without compiling the code.

nicolasstucki · 2019-06-27T15:17:09Z

Rebased

liufengyun

LGTM

nicolasstucki self-assigned this Jun 17, 2019

nicolasstucki mentioned this pull request Jun 17, 2019

Meta-programming TODOs #6688

Closed

16 tasks

nicolasstucki force-pushed the add-quote-context branch 9 times, most recently from cbac1cc to f72acbd Compare June 20, 2019 15:15

nicolasstucki requested a review from liufengyun June 20, 2019 15:35

nicolasstucki force-pushed the add-quote-context branch 2 times, most recently from bc8b23d to e4d5bf1 Compare June 20, 2019 20:19

nicolasstucki marked this pull request as ready for review June 20, 2019 20:59

nicolasstucki assigned liufengyun and unassigned nicolasstucki Jun 20, 2019

nicolasstucki force-pushed the add-quote-context branch 3 times, most recently from 53ce0da to c22eb4b Compare June 21, 2019 08:07

nicolasstucki mentioned this pull request Jun 21, 2019

Move reflection inside QuoteContext #6723

Merged

liufengyun approved these changes Jun 24, 2019

View reviewed changes

nicolasstucki force-pushed the add-quote-context branch from 1dbb191 to 10d3078 Compare June 24, 2019 13:17

nicolasstucki requested a review from liufengyun June 24, 2019 15:19

liufengyun reviewed Jun 25, 2019

View reviewed changes

biboudis self-requested a review June 25, 2019 13:33

nicolasstucki mentioned this pull request Jun 25, 2019

Type the quoted patterns as precisely as possible #6742

Merged

biboudis reviewed Jun 26, 2019

View reviewed changes

library/src-3.x/scala/quoted/Expr.scala Show resolved Hide resolved

biboudis approved these changes Jun 26, 2019

View reviewed changes

nicolasstucki force-pushed the add-quote-context branch 2 times, most recently from 6409b8e to 9dc1087 Compare June 26, 2019 15:24

liufengyun reviewed Jun 27, 2019

View reviewed changes

nicolasstucki added 11 commits June 27, 2019 17:12

Move SyntaxHighlight to scala.quoted

1d37294

Remove deadcode

5b5853a

Revert some formatting changes

b5ccc52

Renamings

9531cb3

Update scala.quoted.run docs

5302651

Remove deadcode

37413e0

Rename withNewQuoteContext to withQuoteContext

06d564b

More explicit documentation

f6bfbfa

nicolasstucki force-pushed the add-quote-context branch from 6496692 to 59cc2d3 Compare June 27, 2019 15:16

Update community build scalatest

68387bb

nicolasstucki force-pushed the add-quote-context branch from 59cc2d3 to 68387bb Compare June 27, 2019 15:31

liufengyun approved these changes Jun 27, 2019

View reviewed changes

nicolasstucki merged commit 8212f60 into scala:master Jun 28, 2019

nicolasstucki deleted the add-quote-context branch June 28, 2019 08:41

nicolasstucki changed the title ~~Add quote context~~ Add QuoteContext Jul 1, 2019

nicolasstucki mentioned this pull request Jul 1, 2019

CyclicReference exception when working with toExprOfList in context of Reflection #6765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QuoteContext #6700

Add QuoteContext #6700

nicolasstucki commented Jun 17, 2019 •

edited

Loading

liufengyun left a comment

liufengyun Jun 24, 2019

nicolasstucki Jun 24, 2019

liufengyun Jun 24, 2019

nicolasstucki Jun 24, 2019

nicolasstucki Jun 24, 2019

nicolasstucki Jun 24, 2019

liufengyun Jun 24, 2019

nicolasstucki Jun 24, 2019

liufengyun Jun 24, 2019

nicolasstucki Jun 24, 2019

liufengyun Jun 24, 2019

nicolasstucki Jun 24, 2019

liufengyun Jun 25, 2019

nicolasstucki Jun 25, 2019

liufengyun Jun 25, 2019

liufengyun commented Jun 25, 2019

biboudis left a comment •

edited

Loading

liufengyun Jun 27, 2019

nicolasstucki commented Jun 27, 2019

liufengyun left a comment


		def apply[T](expr: given QuoteContext => Expr[T]) given (toolbox: Toolbox): String = run(expr.show.toExpr)

		}

Add QuoteContext #6700

Add QuoteContext #6700

Conversation

nicolasstucki commented Jun 17, 2019 • edited Loading

liufengyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun commented Jun 25, 2019

biboudis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Jun 27, 2019

liufengyun left a comment

Choose a reason for hiding this comment

nicolasstucki commented Jun 17, 2019 •

edited

Loading

biboudis left a comment •

edited

Loading