-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 */ | ||
|
@@ -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 | ||
} | ||
} | ||
} | ||
|
||
type ScopeId = Int | ||
|
||
private[dotty] def checkScopeId(id: ScopeId) given Context: Unit = { | ||
if (id != scopeId) | ||
throw new Toolbox.RunScopeException | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 /cc: @anatoliykmetyuk There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
That sounds like a simple enough protocol to be encoded. Otherwise it warrants a discussion on how to make There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that I'm wondering if it is possible to just remove the method so that it will not be misused by macro authors. /cc : @biboudis There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
@@ -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"} | ||
} | ||
} | ||
} |
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}) | ||
} | ||
} |
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 | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1 @@ | ||
foo | ||
bar |
There was a problem hiding this comment.
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 nestedrun
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.
There was a problem hiding this comment.
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.