Skip to content

Fix #4730,#6992: Detect scope extrusions in quoted.run and fail-fast #7007

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 2 commits into from
Aug 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 11 additions & 4 deletions compiler/src/dotty/tools/dotc/core/quoted/PickledQuotes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import dotty.tools.dotc.core.Types._
import dotty.tools.dotc.core.tasty.TreePickler.Hole
import dotty.tools.dotc.core.tasty.{PositionPickler, TastyPickler, TastyPrinter, TastyString}
import dotty.tools.dotc.core.tasty.TreeUnpickler.UnpickleMode
import dotty.tools.dotc.quoted.ToolboxImpl
import dotty.tools.dotc.tastyreflect.ReflectionImpl

import scala.internal.quoted._
Expand All @@ -35,12 +36,18 @@ object PickledQuotes {
}

/** Transform the expression into its fully spliced Tree */
def quotedExprToTree[T](expr: quoted.Expr[T])(implicit ctx: Context): Tree =
healOwner(expr.asInstanceOf[TastyTreeExpr[Tree]].tree)
def quotedExprToTree[T](expr: quoted.Expr[T])(implicit ctx: Context): Tree = {
val expr1 = expr.asInstanceOf[TastyTreeExpr[Tree]]
ToolboxImpl.checkScopeId(expr1.scopeId)
healOwner(expr1.tree)
}

/** Transform the expression into its fully spliced TypeTree */
def quotedTypeToTree(expr: quoted.Type[_])(implicit ctx: Context): Tree =
healOwner(expr.asInstanceOf[TreeType[Tree]].typeTree)
def quotedTypeToTree(tpe: quoted.Type[_])(implicit ctx: Context): Tree = {
val tpe1 = tpe.asInstanceOf[TreeType[Tree]]
ToolboxImpl.checkScopeId(tpe1.scopeId)
healOwner(tpe1.typeTree)
}

private def dealiasTypeTags(tp: Type)(implicit ctx: Context): Type = new TypeMap() {
override def apply(tp: Type): Type = {
Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import scala.collection.mutable.ListBuffer
import scala.collection.mutable
import config.Printers.pickling
import core.quoted.PickledQuotes
import dotty.tools.dotc.quoted.ToolboxImpl

import scala.quoted
import scala.internal.quoted.{TastyTreeExpr, TreeType}
Expand Down Expand Up @@ -1273,8 +1274,8 @@ class TreeUnpickler(reader: TastyReader,
val args = until(end)(readTerm())
val splice = splices(idx)
def wrap(arg: Tree) =
if (arg.isTerm) given (qctx: scala.quoted.QuoteContext) => new TastyTreeExpr(arg)
else new TreeType(arg)
if (arg.isTerm) given (qctx: scala.quoted.QuoteContext) => new TastyTreeExpr(arg, ToolboxImpl.scopeId)
else new TreeType(arg, ToolboxImpl.scopeId)
val reifiedArgs = args.map(wrap)
val filled = if (isType) {
val quotedType = splice.asInstanceOf[Seq[Any] => quoted.Type[_]](reifiedArgs)
Expand Down
24 changes: 23 additions & 1 deletion compiler/src/dotty/tools/dotc/quoted/ToolboxImpl.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package dotty.tools.dotc.quoted

import dotty.tools.dotc.core.Contexts.Context

import scala.quoted._

/** Default runners for quoted expressions */
Expand All @@ -15,10 +17,30 @@ object ToolboxImpl {

private[this] val driver: QuoteDriver = new QuoteDriver(appClassloader)

private[this] var running = false

def run[T](exprBuilder: QuoteContext => Expr[T]): T = synchronized {
driver.run(exprBuilder, settings)
try {
if (running) // detected nested run
throw new scala.quoted.Toolbox.RunScopeException()
running = true
driver.run(exprBuilder, settings)
} finally {
running = false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation on the method run already says nested run is not supported, while this fix seems to suggest that nesting is fine if different toolboxes are used.

As the usage of nesting violates the protocol, I think an improvement of the documentation might be enough.

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 is the other fix that catches issues of nesting with different toolboxes.

}
}

type ScopeId = Int

private[dotty] def checkScopeId(id: ScopeId) given Context: Unit = {
if (id != scopeId)
throw new Toolbox.RunScopeException
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The approach to check scope extrusion seems to be too ad-hoc, and it only addresses the tip of a hard iceberg problem. I'm not sure if it's a win given the complication of the code.

Can we just make it clear that never use run in macros in the documentation for run?

/cc: @anatoliykmetyuk

Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation is too weak for such things. If a user does a wrong thing, the compiler should tell them what exactly they did wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's something that can be effectively checked, then the compiler should perform the check. However, in programming there are so many properties and contracts that cannot be effectively checked by the compiler and have to be defined via documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some prominent protocols not enforced by the compiler I worked with were the actor model, the http model, machine learning models, purely functional style and OOP patterns. Others were Scala 2 macros, SBT and Slick SQL library. The former group are the "laws of physics" you live by and the latter are the building blocks with which you try to build a building under the given laws of physics.

Weird physics can be fun, but if each building block is different and blows up under its own conditions and in its own way, it gets painful. Often we don't choose the rules of the game, but I believe that our responsibility is to manufacture reliable tools with which the given game can be played most efficiently. As a minimum it implies tools with no surprises (such as an obscure exception if you do something that is not in the free-form docs).

Can we just make it clear that never use run in macros

That sounds like a simple enough protocol to be encoded. Otherwise it warrants a discussion on how to make run reliable (possibly trading power for reliability), not a guide on how to live with its surprises.

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 way the check is implemented is quite ad-hoc but it will catch any attempt of using and Expr is a different instance of the compiler. Those crash the compiler in various and unexpected ways.

Copy link
Contributor

@liufengyun liufengyun Aug 9, 2019

Choose a reason for hiding this comment

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

It seems that run is useless in macros --- I'm not sure how useful it's in staged programming. I remember there is also theoretical problems with run.

I'm wondering if it is possible to just remove the method so that it will not be misused by macro authors.

/cc : @biboudis

Copy link
Contributor

Choose a reason for hiding this comment

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

run is useful for running literals. I use it in uTest for that purpose. I believe at least for the literals it should remain there, it works fine for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NO, do not use run to evaluate literals. Use https://github.com/lampepfl/dotty/blob/master/library/src/scala/quoted/ValueOfExpr.scala or the Const extractor directly.

This miss use show that we really need to fail as soon as the user mixes the two concepts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it is not possible to remove the run when executing code in a macro. Though it might be possible to statically partially check for scope extrusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those check would not be complete either and therefore we still need a runtime check


// TODO Explore more fine grained scope ids.
// This id can only differentiate scope extrusion from one compiler instance to another.
private[dotty] def scopeId given Context: ScopeId =
the[Context].outersIterator.toList.last.hashCode()

}
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
//

def unpickleExpr(repr: Unpickler.PickledQuote, args: Unpickler.PickledExprArgs): scala.quoted.Expr[_] =
new scala.internal.quoted.TastyTreeExpr(PickledQuotes.unpickleExpr(repr, args))
new scala.internal.quoted.TastyTreeExpr(PickledQuotes.unpickleExpr(repr, args), compilerId)

def unpickleType(repr: Unpickler.PickledQuote, args: Unpickler.PickledTypeArgs): scala.quoted.Type[_] =
new scala.internal.quoted.TreeType(PickledQuotes.unpickleType(repr, args))
new scala.internal.quoted.TreeType(PickledQuotes.unpickleType(repr, args), compilerId)

//
// CONTEXT
Expand Down Expand Up @@ -1752,7 +1752,7 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
tpd.Closure(closureMethod, tss => etaExpand(new tpd.TreeOps(term).appliedToArgs(tss.head)))
case _ => term
}
new scala.internal.quoted.TastyTreeExpr(etaExpand(self))
new scala.internal.quoted.TastyTreeExpr(etaExpand(self), compilerId)
}

/** Checked cast to a `quoted.Expr[U]` */
Expand All @@ -1773,7 +1773,7 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
/** Convert `Type` to an `quoted.Type[_]` */
def QuotedType_seal(self: Type) given (ctx: Context): scala.quoted.Type[_] = {
val dummySpan = ctx.owner.span // FIXME
new scala.internal.quoted.TreeType(tpd.TypeTree(self).withSpan(dummySpan))
new scala.internal.quoted.TreeType(tpd.TypeTree(self).withSpan(dummySpan), compilerId)
}

//
Expand Down Expand Up @@ -1934,4 +1934,6 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
private def withDefaultPos[T <: Tree](fn: given Context => T) given (ctx: Context): T =
(fn given ctx.withSource(rootPosition.source)).withSpan(rootPosition.span)

private def compilerId: Int = rootContext.outersIterator.toList.last.hashCode()

}
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/Splicer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import dotty.tools.repl.AbstractFileClassLoader

import scala.reflect.ClassTag

import dotty.tools.dotc.quoted.QuoteContext
import dotty.tools.dotc.quoted.{QuoteContext, ToolboxImpl}

/** Utility class to splice quoted expressions */
object Splicer {
Expand Down Expand Up @@ -251,10 +251,10 @@ object Splicer {
}

private def interpretQuote(tree: Tree)(implicit env: Env): Object =
new scala.internal.quoted.TastyTreeExpr(Inlined(EmptyTree, Nil, tree).withSpan(tree.span))
new scala.internal.quoted.TastyTreeExpr(Inlined(EmptyTree, Nil, tree).withSpan(tree.span), ToolboxImpl.scopeId)

private def interpretTypeQuote(tree: Tree)(implicit env: Env): Object =
new scala.internal.quoted.TreeType(tree)
new scala.internal.quoted.TreeType(tree, ToolboxImpl.scopeId)

private def interpretLiteral(value: Any)(implicit env: Env): Object =
value.asInstanceOf[Object]
Expand Down
4 changes: 2 additions & 2 deletions library/src/scala/quoted/Expr.scala
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ package quoted {
package internal {
package quoted {

import scala.quoted.{Expr, QuoteContext}
import scala.quoted.Expr

/** An Expr backed by a tree. Only the current compiler trees are allowed.
*
Expand All @@ -88,7 +88,7 @@ package internal {
*
* May contain references to code defined outside this TastyTreeExpr instance.
*/
final class TastyTreeExpr[Tree](val tree: Tree) extends Expr[Any] {
final class TastyTreeExpr[Tree](val tree: Tree, val scopeId: Int) extends Expr[Any] {
override def toString: String = s"Expr(<tasty tree>)"
}

Expand Down
3 changes: 3 additions & 0 deletions library/src/scala/quoted/Toolbox.scala
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,7 @@ object Toolbox {
}

class ToolboxNotFoundException(msg: String, cause: ClassNotFoundException) extends Exception(msg, cause)

class RunScopeException extends Exception("Cannot call `scala.quoted.run(...)` within a macro or another `run(...)`")

}
2 changes: 1 addition & 1 deletion library/src/scala/quoted/Type.scala
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ package internal {
package quoted {

/** An Type backed by a tree */
final class TreeType[Tree](val typeTree: Tree) extends scala.quoted.Type[Any] {
final class TreeType[Tree](val typeTree: Tree, val scopeId: Int) extends scala.quoted.Type[Any] {
override def toString: String = s"Type(<tasty tree>)"
}

Expand Down
2 changes: 0 additions & 2 deletions library/src/scala/quoted/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ package object quoted {
*
* This method should not be called in a context where there is already has a `QuoteContext`
* such as within a `run` or a `withQuoteContext`.
*
* May throw a FreeVariableError on expressions that came from a macro.
*/
def run[T](expr: given QuoteContext => Expr[T]) given (toolbox: Toolbox): T = toolbox.run(expr given _)

Expand Down
23 changes: 23 additions & 0 deletions tests/run-macros/i6992/Macro_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

import scala.quoted._, scala.quoted.matching._
import delegate scala.quoted._

delegate for Toolbox = Toolbox.make(getClass.getClassLoader)

object macros {
inline def mcr(x: => Any): Any = ${mcrImpl('x)}

class Foo { val x = 10 }

def mcrImpl(body: Expr[Any]) given (ctx: QuoteContext): Expr[Any] = {
import ctx.tasty._
try {
body match {
case '{$x: Foo} => run(x).x.toExpr
}
} catch {
case _: scala.quoted.Toolbox.RunScopeException =>
'{"OK"}
}
}
}
9 changes: 9 additions & 0 deletions tests/run-macros/i6992/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import macros._

object Test {
val foo = new Foo

def main(args: Array[String]) = {
println(mcr {foo})
}
}
20 changes: 20 additions & 0 deletions tests/run-with-compiler/i4730.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import scala.quoted._

object Test {
implicit val toolbox: scala.quoted.Toolbox = scala.quoted.Toolbox.make(getClass.getClassLoader)
def ret given QuoteContext: Expr[Int => Int] = '{ (x: Int) =>
${
val z = run('{x + 1}) // throws a RunScopeException
z.toExpr
}
}
def main(args: Array[String]): Unit = {
try {
run(ret).apply(10)
throw new Exception
} catch {
case ex: scala.quoted.Toolbox.RunScopeException =>
// ok
}
}
}
1 change: 0 additions & 1 deletion tests/run-with-compiler/i6754.check
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
foo
bar
8 changes: 7 additions & 1 deletion tests/run-with-compiler/i6754.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ object Test {
println("foo")
run(x)
}
run(y)
try {
run(y)
throw new Exception
} catch {
case ex: java.lang.reflect.InvocationTargetException =>
assert(ex.getTargetException.isInstanceOf[scala.quoted.Toolbox.RunScopeException])
}
}
}