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 1 commit
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
11 changes: 10 additions & 1 deletion compiler/src/dotty/tools/dotc/quoted/ToolboxImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,17 @@ 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.ToolboxAlreadyRunning()
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.

}

}
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

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 ToolboxAlreadyRunning extends Exception("Cannot call `scala.quoted.run(...)` within another `run(...)`")

}
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
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 ToolboxAlreadyRunning
z.toExpr
}
}
def main(args: Array[String]): Unit = {
try {
run(ret).apply(10)
throw new Exception
} catch {
case ex: scala.quoted.Toolbox.ToolboxAlreadyRunning =>
// 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.ToolboxAlreadyRunning])
}
}
}