The library developer should be able to handle specific types of Panics while passing through others #3344

radeusgd · 2022-03-15T15:45:21Z

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/181569176

Also ensures that Dataflow Errors have proper stack traces (earlier they did not point at the right location).

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoSourceSection.java

kustosz

I like this, but I don't like some of this 😁

kustosz · 2022-03-17T11:38:55Z

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Extensions.enso

+Panic.get_attached_stack_trace error =
+    throwable = case error of
+        Caught_Panic _ internal_original_exception -> internal_original_exception
+        throwable -> throwable


Why does this exist? If I'm reading your code correctly, there's no way to interact with panics other than through CaughtPanic?

to be able to get the stacktrace of a raw Java exception, if you get it from somewhere.

Because it is exactly used as an internal helper to make Caught_Panic able to have the stack_trace method.

from where tho...

in this case, mark it private and state that this is what it's used for

possibly from caught_panic.payload.cause (I know there we simply have caught_panic.stack_trace but the payload may be passed further down) or from catch_java_exception unless we decide to remove this function (but it's not clear yet). I just feel like that function may have its uses somewhere.

OK, I'll believe you

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Extensions.enso

kustosz · 2022-03-17T11:40:33Z

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Extensions.enso

+             IO.println "Caught an `Illegal_Argument_Error`: "+error.payload.message
+             "fallback value"
+Panic.catch : Any -> Any -> (Caught_Panic -> Any) -> Any
+Panic.catch typ ~action handler =


Change it to exception_type or sth. Why scare IDE users.

Changed to panic_type as I guess more consistent with our nomenclature.

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Extensions.enso

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/CatchPanicNode.java

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java

kustosz · 2022-03-17T11:53:47Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java

-public class ThrowPanicNode extends Node {
-  Stateful execute(Object _this, Object payload) {
-    throw new PanicException(payload, this);
+public abstract class ThrowPanicNode extends Node {


This is an annoying mix of specializations and (unprofiled) inline typechecks. What I want this to be is:

A specialization that only triggers for Caught_Panic.

A specialization that only triggers for isException.

A fallback.

Use specialization guards to refactor this nicely.

kustosz

Jest piękny, it's beautiful

kustosz · 2022-03-18T12:45:05Z

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Extensions.enso

+Panic.get_attached_stack_trace error =
+    throwable = case error of
+        Caught_Panic _ internal_original_exception -> internal_original_exception
+        throwable -> throwable


OK, I'll believe you

kustosz · 2022-03-18T12:46:08Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java

+    if (interopLibrary.isException(originalException)) {
+      try {
+        throw interopLibrary.throwException(originalException);
+      } catch (UnsupportedMessageException e) {


please blow up with IllegalStateException – we shouldn't want something that deeply broken to "kinda work" – I'd rather we inform people that this is a bug.

4e6

Very cool stuff!

jdunkerley

LGTM 👍

Fix dataflow errors lacking stacktraces and some other issues

hubertp

nit picking, feel free to ignore

hubertp · 2022-03-18T14:54:57Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java

+        throw interopLibrary.throwException(originalException);
+      } catch (UnsupportedMessageException e) {
+        throw new IllegalStateException(
+            "Impossible, `isException` returned true for `originalException`.");


nit in wording

Suggested change

"Impossible, `isException` returned true for `originalException`.");

"Unexpected true `isException` condition for `originalException`");

If something is impossible then there is no way it happens :)
But I noticed this being used in other places so feel free to ignore.
Also notice that sometimes exceptions end with a dot and sometimes not. My preference is for the latter but as long as things are consistent, idc

Hmmm, fair point. Well I guess the idea is that it is 'impossible under the assumption that the functions we use abide by their contracts'.

Maybe worth considering rephrasing but as you pointed out - it is used for many places, so I'd prefer to be consistent - we may change all of them in some separate PR though if we agree that different phrasing would be better. On the other hand, if the condition indeed is impossible it may not be worth caring about it too much 😅

hubertp · 2022-03-18T14:58:32Z

engine/runtime/src/main/resources/Builtins.enso

-         Handling a panic for a panicking action.
+    ## PRIVATE
+
+       Returns a raw representation of the stack trace attached to the provided


nit

Suggested change

Returns a raw representation of the stack trace attached to the provided

Returns a raw representation of the stacktrace attached to the provided

Either works as long as we are consistent (we don't seem to be)

Fair point! Unfortunately I was off for the day and merge-bot already did its job - but yeah that would probably require going through the codebase and making it consistent - next time we are touching these areas I will try to remember to maybe do this change, as indeed consistency would be good.

radeusgd self-assigned this Mar 15, 2022

kustosz reviewed Mar 16, 2022

View reviewed changes

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/ThrowPanicNode.java Outdated Show resolved Hide resolved

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoSourceSection.java Outdated Show resolved Hide resolved

radeusgd force-pushed the wip/radeusgd/lib-panic-catch-181569176 branch from 3c7fc8d to 3ee235f Compare March 16, 2022 14:21

radeusgd marked this pull request as ready for review March 16, 2022 16:43

radeusgd requested review from 4e6 and jdunkerley as code owners March 16, 2022 16:43

radeusgd requested a review from kustosz March 16, 2022 16:43

radeusgd force-pushed the wip/radeusgd/lib-panic-catch-181569176 branch 2 times, most recently from 9298355 to 94ecc27 Compare March 16, 2022 19:37

kustosz suggested changes Mar 17, 2022

View reviewed changes

radeusgd force-pushed the wip/radeusgd/lib-panic-catch-181569176 branch from cd6a3ae to 095f819 Compare March 17, 2022 20:48

radeusgd requested a review from kustosz March 18, 2022 12:34

kustosz approved these changes Mar 18, 2022

View reviewed changes

4e6 approved these changes Mar 18, 2022

View reviewed changes

jdunkerley approved these changes Mar 18, 2022

View reviewed changes

radeusgd added 15 commits March 18, 2022 15:25

WIP working on Panic.catch

c798440

Catch panics as objects exposing stack trace

70a28d7

Implement rethrowing of caught panics

f42b8f7

fixes

a204bb3

Catching Java exceptions, some tests

6f403ce

cr

db3121b

Make throw not wrap Java exceptions if they are thrown.

43fcf51

Fix dataflow errors lacking stacktraces and some other issues

catch->catch_any, catch matches specific types

37132ee

fix tests

686bd8f

changelog

9ed247d

Reorder args in catch_java_exception for consistency

e1c939a

Make some catch-sites more specific

1f22ee1

Rename Panic.recover to Panic.recover_any

6f146a8

Implement selective recovery of panics

b4d8164

change Json parse recovery

f9c5b73

radeusgd added 12 commits March 18, 2022 15:25

changelog

d7809c1

Fix runtime tests

02d690d

remove recover_any replacing it with recover Any

ca6f3f0

Make catch_any private and catch_primitive, use catch Any instead

7946592

Remove catch_java_exception, merging it into catch

71f885b

code style

5cfb57b

Add branch profiles

9def263

Split into specializations

5b95ee7

fix

66456a1

forgot to rename some

c5853c5

Update changelog

dfe0ce5

fix a test

b4c6bf0

radeusgd force-pushed the wip/radeusgd/lib-panic-catch-181569176 branch from dbdd1f5 to b4c6bf0 Compare March 18, 2022 14:37

radeusgd requested a review from hubertp March 18, 2022 14:38

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Mar 18, 2022

Merge branch 'develop' into wip/radeusgd/lib-panic-catch-181569176

0e09e61

hubertp reviewed Mar 18, 2022

View reviewed changes

mergify bot merged commit cc73338 into develop Mar 18, 2022

mergify bot deleted the wip/radeusgd/lib-panic-catch-181569176 branch March 18, 2022 16:57

JaroslavTulach mentioned this pull request Oct 25, 2023

Try to improve performance of Dataflow Errors #8137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The library developer should be able to handle specific types of Panics while passing through others #3344

The library developer should be able to handle specific types of Panics while passing through others #3344

radeusgd commented Mar 15, 2022 •

edited

Loading

kustosz left a comment

kustosz Mar 17, 2022

radeusgd Mar 17, 2022

kustosz Mar 17, 2022

radeusgd Mar 17, 2022

kustosz Mar 18, 2022

kustosz Mar 17, 2022

radeusgd Mar 18, 2022

kustosz Mar 17, 2022

kustosz left a comment

kustosz Mar 18, 2022

kustosz Mar 18, 2022

4e6 left a comment

jdunkerley left a comment

hubertp left a comment

hubertp Mar 18, 2022

radeusgd Mar 21, 2022

hubertp Mar 18, 2022

radeusgd Mar 21, 2022

	"Impossible, `isException` returned true for `originalException`.");
	"Unexpected true `isException` condition for `originalException`");

	Returns a raw representation of the stack trace attached to the provided
	Returns a raw representation of the stacktrace attached to the provided

The library developer should be able to handle specific types of Panics while passing through others #3344

The library developer should be able to handle specific types of Panics while passing through others #3344

Conversation

radeusgd commented Mar 15, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

kustosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kustosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4e6 left a comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd commented Mar 15, 2022 •

edited

Loading