Fix #4730,#6992: Detect scope extrusions in quoted.run and fail-fast
#7007
Conversation
nicolasstucki
force-pushed
the
fix-#4730
branch
from
172f17b to
bb7c869
Compare
nicolasstucki
changed the title
quoted.run and fail-fast
nicolasstucki
force-pushed
the
fix-#4730
branch
from
c27677f to
554c8ae
Compare
nicolasstucki
changed the title
quoted.run and fail-fastquoted.run and fail-fast
This heristic can detects and fails-fast when an Expr or Type is used with a different compiler instance.
nicolasstucki
force-pushed
the
fix-#4730
branch
from
554c8ae to
b35936d
Compare
| driver.run(exprBuilder, settings) | ||
| } finally { | ||
| running = false | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is the other fix that catches issues of nesting with different toolboxes.
|
|
||
| private[dotty] def checkScopeId(id: ScopeId) given Context: Unit = { | ||
| if (id != scopeId) | ||
| throw new Toolbox.RunScopeException | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
Those check would not be complete either and therefore we still need a runtime check
There was a problem hiding this comment.
LGTM
Discussed with @nicolasstucki , we think it's a good experiment to perform stricter checks to avoid mysterious crash in case of misuse of the API.
The changes don't affect API, so it's easy to change when there are better solutions.
No description provided.