-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Introduce boundary/break
control abstraction.
#16612
Conversation
42cffa3
to
2848427
Compare
It would be good to discuss naming and semantics of this PR while it is in flight. |
c44ad23
to
964bccf
Compare
catch case ex: Break[T] @unchecked => | ||
if ex.label eq local then ex.value | ||
else throw ex |
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.
This @unchecked
is unsound. At the point of the case
, you don't know yet that ex
has the same T
as the Label
. So you're introduce unsound relationships here. This is the kind of things that led us to make a distinction between user cast and compiler-inserted casts. Let's not burn more of that in the standard library.
Consider:
catch case ex: Break[T] @unchecked => | |
if ex.label eq local then ex.value | |
else throw ex | |
catch case ex: Break[?] => | |
if ex.label eq local then ex.value.asInstanceOf[T] | |
else throw ex |
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 asInstanceOf[T]
is always a no-op since it erases to asInstanceOf[Object]
which is then dropped. So it's only complicating the extractors. The problem we had previously was with a post-hoc isInstanceOf
.
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.
Note that, given that this method is inlined, when the T
is statically known then asInstanceOf[T]
will be checked.
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.
This follows #16550 (comment)
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.
Yes, but the check will always succeed. And the cast will anyway inserted. I am leaving out the cast since it makes the boundary recognition more fragile. If we accept (and drop) a cast, how do we know it's not a user inserted cast that can fail?
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.
That would be a non-issue if we don't inline boundary.apply
.
tests/run/rescue-boundary.scala
Outdated
case ex: ControlException => throw ex | ||
case NonFatal(_) => fallback |
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.
This is a perfect example of why ControlThrowable
s are not caught by NonFatal
. By making this new ControlException
by-passing that mechanism, we introduce more work for user-space code like this to do the right thing.
* { val local: Label[...] = ...; <LabelTry(local, body)> } | ||
*/ | ||
def unapply(tree: Tree)(using Context): Option[(Symbol, Tree)] = stripTyped(tree) match | ||
case Block((vd @ ValDef(nme.local, _, _)) :: Nil, LabelTry(caughtAndRhs)) |
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.
Can't the ValDef
be renamed under inlining? It might not always be called nme.local
.
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.
Currently I don't see how it would be renamed. Testing for the name means we narrow down the check must faster, so we can execute LabelTry
only for very likely positives.
* Instances of `ControlException` should not normally have a cause. | ||
* Legacy subclasses may set a cause using `initCause`. | ||
*/ | ||
abstract class ControlException(message: String | Null) extends RuntimeException( |
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.
I thought that we wanted to make Break
extend Throwable
to avoid catching it in a catch that expects an Exception
. Why did this change? What is the use case?
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.
We had a long discussion on contributors about that.
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.
See #16612 (comment)
@@ -0,0 +1,72 @@ | |||
import scala.util.* | |||
|
|||
object breakOpt: |
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.
With run
tests we only check the semantics of the boundary
/break
. We also need to test that the optimization in happening. I propose using bytecode tests similar to https://github.com/lampepfl/dotty/blob/main/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala#L1519-L1558. There we will be able to check that the gotos and returns are generated as expected.
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.
I could add these tests.
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.
Yes, please go ahead!
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.
Can cherrypick 8715766
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.
These will have to be adapted to latest codegen. In fact, in the interest of robustness, it's probably best to just test that a test contains a labeled block or a try or both. That's all we need to know, and that leaves open later changes in code generation.
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.
Updated. Now it can be cherry-picked from 5c7a000.
I am still skeptical about the new |
@sjrd That test was an adaptation of I think With |
The discussion on Contributors illustrates both pros and cons to
It is equally valid to say: |
I think result the discussion in contributors was that we will not deprecate The reason for |
But what are "all the problems" with I can see an argument for why Basically: I see no good argument why new code should suddenly use a different concept that doesn't work like the perfectly valid status quo. |
My problem with Sure, sometimes we need to make an exception for To illustrate the current insanity, look at this code:
Conclusion? non-local returns are fatal! Overall it's just too much hair splitting going on. |
Focusing on non-local-returns to make |
c1a173b
to
337f9b5
Compare
The abstractions are intended to replace the `scala.util.control.Breaks` and `scala.uitl.control.NonLocalReturns`. They are simpler, safer, and more performant, since there is a new MiniPhase `DropBreaks` that rewrites local breaks to labeled returns, i.e. jumps. The abstractions are not experimental. This break from usual procedure is because we need to roll them out fast. Non local returns were just deprecated in 3.2, and we proposed `NonLocalReturns.{returning,throwReturn}` as an alternative. But these APIs were a mistake and should be deprecated. So rolling out boundary/break now counts as a bugfix.
Drop the `transparent` in order to curcumvent scala#16609
Change the recommendation in the warning about non-local returns accordingly. Still to do: A PR against scala/scala that deprecates scala.util.control.Breaks.
`DropBreaks` now detects the calls to `break` instead of their inline expansion. Also: Make `Break`'s constructor private.
This is needed to avoid verify errors
# Conflicts: # tests/run/errorhandling/break.scala
Move the detected `break` methods into the `boundary` object and keep inline methods in object `scala.util.break` as facades.
The case for keeping import scala.util.{boundary, break} I think that's acceptable as a replacement for non-local returns. But requiring two imports import scala.util.boundary
import scala.util.boundary.break or requiring the user to write |
An alternative design that would satisfy all our concerns
would be the following import scala.utils.boundaries.*
boundary {
while ... do
...
if .. then break()
} package scala.util
object boundaries:
final class Break[T] private[boundaries](val label: Label[T], val value: T)
extends RuntimeException(
/*message*/ null, /*cause*/ null, /*enableSuppression=*/ false, /*writableStackTrace*/ false)
final class Label[-T]
def break[T](value: T)(using label: Label[T]): Nothing =
throw Break(label, value)
def break()(using label: Label[Unit]): Nothing =
throw Break(label, ())
inline def boundary[T](inline body: Label[T] ?=> T): T =
val local = Label[T]()
try body(using local)
catch case ex: Break[T] @unchecked =>
if ex.label eq local then ex.value
else throw ex
end boundaries |
What does |
7ad1e84
to
90f54de
Compare
I had to try this to be sure it works. Never saw that one in the wild 😜 |
90f54de
to
761c2d0
Compare
We can't use `String.lines()` since that only exists on Java 11 and the CI still runs on Java 8. Use `linesIterator` in `StringOps` instead.
761c2d0
to
a17a6df
Compare
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.
Otherwise LGTM
@@ -357,6 +357,7 @@ object StdNames { | |||
val Flag : N = "Flag" | |||
val Ident: N = "Ident" | |||
val Import: N = "Import" | |||
val Label_this: N = "Label_this" |
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.
val Label_this: N = "Label_this" |
* | ||
* where `target` is the `goto` return label associated with `local`. | ||
* Adjust associated ref counts accordingly. The local refcount is increased | ||
* and the non-local refcount is decreased, since `local` the `Label_this` |
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.
We don't have the Label_this
anymore
// we want by-name parameters to be converted to closures | ||
|
||
/** The number of boundary nodes enclosing the currently analized tree. */ | ||
var enclosingBoundaries: Int = 0 |
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.
private
?
Ha! |
That's not the only style of code Scala targets.
Because the Java/C |
Syntax design should follow literal meaning. The specific logic of the implementations should be handled by the compiler. |
def nextPowerOf2(i: Int) = Hop.int{
Iterator.iterate(1)(_ * 2)
.takeWhile(_ > 0)
.foreach(j => if j > i then hop(j))
i
} So, being able to discard this part of the library and just being able to def nextPowerOf2(i: Int): Int = boundary {
Iterator.iterate(1)(_ * 2)
.takeWhile(_ > 0)
.foreach(j => if j > i then break(j))
i
} seems nice! However, I'm concerned about the composability of def nextDangerousThing(i: Int) = Hop.int{
Iterator.iterate(i)(_ * 2)
.takeWhile(_ > 0)
.foreach(j => Try{ hop(dangerously(j)) })
i
} then because def nextDangerousThing(i: Int) = boundary {
Iterator.iterate(i)(_ * 2)
.takeWhile(_ > 0)
.foreach( j => Try{ break(dangerously(j)) } )
i
} does not give the desired result because the Try silently intercepts the control flow. My understanding is that this is supposed to be desirable, but I don't see how this results in composable code. I wouldn't care if there were other options for going at full speed where throwing stackless exceptions wasn't necessary, but if this is the only way to do it, I'm kind of worried. Now, of course, I can always avoid using object Attempt {
def apply[A](a: => A) =
try Correct(a)
catch
case b: boundary.Label[_] => throw b
case NonFatal(e) => Wrong(e)
} which is basically just But then people will write things like boundary {
Future(Attempt(if nice then foo() else break(bar)))
} and the exception will leak through and clobber the thread again. So we have all this churn and accomplish almost nothing, don't we? Aside from pushing off the exact same problem for a while--trading it for a temporary composability problem--until people recover composable code patterns? I like the boundary/break semantics. I think it's a very clear way to express the problem--as you can tell because I have already been using the same thing! And if the compiler can rewrite the boundary/break into a local return / jump when appropriate, that's fantastic--the JIT compiler sometimes manages with locally-caught stackless exceptions, but it's not something one can really count on. But I don't see why we don't just reuse the existing The real issue is that some things need to be unbreakable, isn't it? object Future {
def apply[A](f: NotGiven[boundary.Label[_]] ?=> A): Future[A] = ???
} Except that isn't quite a strong enough guarantee: that would be true if you were careless with propagating the breakability context. You probably want something that takes active work to propagate, not just a object Future {
def apply[A](f: Unbreakable ?=> A): Future[A] = ???
}
object boundary {
def apply[A](f: Label[A] ?=> A)(given NotGiven[Unbreakable]): A = ???
} Then you might finally have enough information in the type system to start allowing the compiler to peel apart the conflict between composability in breakable contexts with unbreakability. Finally, extension [L, R](e: Either[L, R])
inline def ?(using boundary.Label[Either[L, R]]): R = e match
case l: Left[L] => break(l)
case Right(r) => r Now whenever you're in breakable context you can shortcut-exit with the error-case with But this kind of empowering feature just raises the pressure yet more for having code that is composable. So, in conclusion: I think this was a great addition, but I think (The precise kind of composability I mean is that wherever you have foo => bar(foo) and you replace it with foo => bar(nice(foo)) where Final thought: I am personally not at all troubled by dropping |
I figured out a way to get around the limitations of opaque types for the loop example: import scala.util.boundary, boundary.break
object Loops:
type ExitToken = Unit { type Exit }
type ContinueToken = Unit { type Continue }
type Exit = boundary.Label[ExitToken]
type Continue = boundary.Label[ContinueToken]
object loop:
inline def apply(inline op: (Exit, Continue) ?=> Unit): Unit =
boundary[ExitToken]:
while true do
boundary[ContinueToken]:
op
inline def exit()(using Exit) = break(().asInstanceOf[ExitToken])
inline def continue()(using Continue) = break(().asInstanceOf[ContinueToken]) This manages to optimise to labelled jumps and does not box Unit! e.g. import Loops.*
@main def oddsUpToLimit(limit: Int) =
var i = 0
loop:
i += 1
if i == limit then loop.exit()
if i % 2 == 0 then loop.continue()
println(i) with CFR output it decompiles to public void oddsUpToLimit(int limit) {
int i = 0;
while (true && ++i != limit) {
if (i % 2 == 0) continue;
Predef$.MODULE$.println((Object)BoxesRunTime.boxToInteger((int)i));
}
} |
For reference: I have rewritten all my control abstractions that used to use nonlocal return (and macros, usually) to use I have also thrown away every use of The only thing that could make things better is a witness that closures, lazy values, threads, and the like, cannot escape the context where early returns are defined. Any of these are broken regardless of how far the exception propagates and should be caught at compile-time when possible. Still, I'm used to having to do this manually anyway, and the advantage of direct-style error handling, for instance, is far bigger than the downside of accidental leakage of control flow into non-consecutive execution context. Anyway, my code is extra-pretty now! I just write things like def load(path: Path): DataSet Or Err = Or.Ret:
val data = nice{ Files.readAllBytes(findDataFile(path).?) }.?
val reliable = easy(data)
computeOn(data, reliable).?.dataSet where The vanilla equivalent would be something like def load(path: Path): Either[Err, DataSet] =
Try{ findDataFile(path) } match
case f: Failure => Left(Err from f)
case Success(Left(e)) => Left(e)
case Success(Right(file)) =>
Try{ Files.readAllBytes(file) } match
case f: Failure => Left(Err from f)
case Success(data) =>
val reliable = easy(data)
computeOn(data, reliable).map(_.dataSet) Anyway, I continue to think that re-using |
That's exactly what our ongoing work on capture checking is about. Hopefully this will mature so that it becomes more widely usable. An important next step that is necessary for widespread usage is to capture-annotate the standard library. |
The abstractions are intended to replace the
scala.util.control.Breaks
andscala.util.control.NonLocalReturns
. They are simpler, safer, and more performant,since there is a new MiniPhase
DropBreaks
that rewrites local breaks to labeledreturns, i.e. jumps.
The abstractions are not experimental. This break from usual procedure is because
we need to roll them out fast. Non local returns were just deprecated in 3.2, and
we proposed
NonLocalReturns.{returning,throwReturn}
as an alternative. But theseAPIs were a mistake and should be deprecated themselves. So rolling out boundary/break now counts
as a bugfix.