8000 Improve `s.u.Using` suppression order by NthPortal · Pull Request #11000 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Improve s.u.Using suppression order #11000

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 3, 2025
Merged

Improve s.u.Using suppression order #11000

merged 1 commit into from
Apr 3, 2025

Conversation

NthPortal
Copy link
Contributor

Change scala.util.Using to have
scala.util.control.ControlThrowable be suppressed by non-fatal
exceptions (scala.util.control.NonFatal).

Fixes scala/bug#13088

@NthPortal NthPortal added the library:base Changes to the base library, i.e. Function Tuples Try label Feb 18, 2025
@scala-jenkins scala-jenkins added this to the 2.13.17 milestone Feb 18, 2025
@som-snytt
Copy link
Contributor

Probably you didn't anticipate such far-reaching effects!

java.io.IOException: No space left on device
	at java.base/java.io.FileOutputStream.writeBytes(Native Method)
***
	at java.base/java.lang.Thread.run(Thread.java:1583)
ERROR: Maximum checkout retry attempts reached, aborting

@lrytz
Copy link
Member
lrytz commented Feb 18, 2025

I'll fix Jenkins. GHA says

Test run scala.util.UsingTest finished: 4 failed

@lrytz
Copy link
Member
lrytz commented Feb 18, 2025

Here's an example that changes semantics. Is that a good change?

Before:

scala> def h(s: String): Unit = throw new RuntimeException
scala> def t: Int = util.Using.resource("hai")(s => return 42)(h)
scala> t
val res0: Int = 42

After:

...
scala> t
java.lang.RuntimeException
  at h(<console>:1)
  at $anonfun$t$2(<console>:1)
  at scala.util.Using$.resource(Using.scala:304)
  at t(<console>:1)
  ... 30 elided
  Suppressed: scala.runtime.NonLocalReturnControl$mcI$sp

@som-snytt I don't follow your mUsings around Odersky's Rule and the Titanic, I'm afraid..

@NthPortal
Copy link
Contributor Author
NthPortal commented Feb 18, 2025 •
edited
Loading

Is that a good change?

I would argue that not only is it a good change, it is a correct change. before the change, where did the RuntimeException go? did we not close the resource? was it eaten by a grue? we've somehow returned normally without handling the exception. after this change, the exception when closing the resource propagates as it should (and would if Using was language syntax like try-with-resources is and not just a regular method).

I'll have a look at the failing tests Soon™. I admittedly did not run them locally, and just kinda assumed I adjusted them correctly

@som-snytt
Copy link
Contributor

In the example, we want the happy path.

If I want to "handle all the possibilities", then I have to catch all the possible aborts.

The answer for Future is use UEH so exceptions don't "disappear".

@lrytz
Copy link
Member
lrytz commented Feb 18, 2025

Java:

jshell> public int f() throws Exception { try (Closeable c = () -> { throw new RuntimeException(); }) { return 42; } }
|  modified method f()

jshell> f()
|  Exception java.lang.RuntimeException
|        at lambda$f$0 (#3:1)
|        at f (#3:1)
|        at (#4:1)

@som-snytt
Copy link
Contributor

The Odersky Rule is promulgated at
scala/scala3#17737 (comment)

@som-snytt
Copy link
Contributor 
scala> :pa -java
// Entering paste mode (ctrl-D to finish)

package j;
import java.io.Closeable;
public class C {
public int f() throws Exception { try (Closeable c = () -> { throw new RuntimeException(); }) { return 42; } }
}

// Exiting paste mode... now compiling with javac.

scala> new j.C().f()
java.lang.RuntimeException
  at j.C.lambda$f$0(C.java:4)
  at j.C.f(C.java:4)
  ... 30 elided

scala>

Ergonomics in repl could be improved.

@Ichoran
Copy link
Contributor
Ichoran commented Feb 18, 2025

The new behavior is correct, I think: if you throw an exception while closing the resource, you did not successfully use the resource.

However, non-local return is deprecated, and boundary throws a RuntimeException which is eaten by Try so to be consistently forgiving of people who let control flow escape their threads, Using needs to (and does) catch that one too, which makes any change to ControlThrowable-handling a matter of backwards compatibility only: there's no real purpose to ControlThrowable any longer, and code would run faster if we deprecate and eliminate it.

Therefore, I am not convinced that there's much benefit to changing Using in this way. The existing way is arguably wrong, but there shouldn't be much future for it, and someone may have relied upon returns working even if closing mucked up in ordinary ways. On the other hand, the danger of changing behavior in unpleasantly also seems modest.

@lrytz
Copy link
Member
lrytz commented Feb 19, 2025

... so we cannot fix Using for boundary / break?

@NthPortal
10000
Copy link
Contributor Author

so we cannot fix Using for boundary / break?

personally, I think this PR is a bug fix. the previous/current behaviour is wrong I believe because I (and everyone else) didn't think about it particularly hard past "fatal should have priority", not because we explicitly decided that ControlThrowable specifically ought to have priority over non-fatal exceptions.

also, sorry for taking so long to fix the tests. life happened at me

@Ichoran
Copy link
Contributor
Ichoran commented Apr 2, 2025 •
edited
Loading

... so we cannot fix Using for boundary / break?

We could (by intercepting the type of exception that they throw and handling them specially), but since boundary/break control flow can't get through Try by design (we had long arguments about that, and the side I favored lost)--then Using should eat it also for consistency. That way you just remember that standard library exception-handling stuff eats control flow.

And then if you want exception handling that allows control flow to cross, you use a library for that.

@NthPortal
Copy link
Contributor Author

I don't think this PR changes anything meaningful wrt boundary/break given that it throws a RuntimeException

@lrytz
Copy link
Member
lrytz commented Apr 3, 2025

since boundary/break control flow can't get through Try by design (we had long arguments about that, and the side I favored lost)--then Using should eat it also for consistency. That way you just remember that standard library exception-handling stuff eats control flow.

Thanks, that makes sense :-/

@NthPortal @lrytz
Improve s.u.Using suppression order
8091336 
Change `scala.util.Using` to have
`scala.util.control.ControlThrowable` be suppressed by non-fatal
exceptions (`scala.util.control.NonFatal`).
@NthPortal
Copy link
Contributor Author

thanks for squashing for me Lukas!

@lrytz lrytz merged commit 483b4ee into scala:2.13.x Apr 3, 2025
3 checks passed
@lrytz
Copy link
Member
lrytz commented Apr 3, 2025

Thank you for the work!

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Apr 3, 2025
@NthPortal NthPortal deleted the using-pref/PR branch April 4, 2025 00:47
@SethTisue SethTisue mentioned this pull request Apr 14, 2025
Open
hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this pull request May 2, 2025
@lrytz
Merge pull request scala/scala#11000 from NthPortal/using-pref/PR
262e595 
Improve `s.u.Using` suppression order
hamzaremmal pushed a commit to scala/scala3 that referenced this pull request May 7, 2025
@lrytz
Merge pull request scala/scala#11000 from NthPortal/using-pref/PR
62d069e 
Improve `s.u.Using` suppression order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz approved these changes

Assignees
No one assigned
Labels
library:base Changes to the base library, i.e. Function Tuples Try release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.17
Development

Successfully merging this pull request may close these issues.

Incorrect/unreasonable suppression behavior of s.u.Using
6 participants
@NthPortal @som-snytt @lrytz @Ichoran @SethTisue @scala-jenkins
0