Propagate DataflowError in middle of block instead of ignoring it #11777

JaroslavTulach · 2024-12-05T10:17:35Z

Pull Request Description

Partially fixes #5430 by propagating DataflowErrors found during statement execution out of the method.

Important Notes

This change may affect behavior of existing methods that ignore DataflowError as discussed here.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
Unit tests have been written where possible.
Engine benchmarks run is fine
StdLib benchmarks run seems fine

JaroslavTulach · 2024-12-05T10:28:48Z

...ion-tests/src/test/java/org/enso/interpreter/test/semantic/DataflowErrorPropagationTest.java

+            "OK"
+
+    suppress_error yes:Boolean value =
+        yield_error yes


This 49c628e commit changes the behavior on this line:

previously the Error returned from yield_error was ignored

previously the execution continued to next line and value was returned

now the Error is detected and execution of this method ends immediately with the Error

To suppress such behavior one may simply prefix the call with a dummy _ = yield_error yes assignment. As assignments always return Nothing the block statement will be considered OK as the errorIsAssignedAndThatIsEnoughReturnValue test demonstrates.

Wouldn't we want assignment to also propagate errors in this way? This is another class of common missed errors, I would think. Only catch should allow you to stop propagation, I would think.

Could you elaborate and show some example where you think the behaviour is not what we want?

From my perspective, with the changes from this PR all is good - the dataflow errors flow with the dataflow, determined by dependencies between method calls. The problem was when the value was unassigned because the dataflow error would be lost unhandled. But if it's assigned to something, it should flow further and be propagated. Unless the name is unused - but we have 'unused warning' for that already.

I think that propagating 'more aggressively' upon assignment would make the dataflow error not much (if at all in practice?) different from a Panic. Also it may be problematic for the IDE. The whole point of dataflow errors is that they do not stop execution of other nodes if they are assigned to something, so that even if one node in the GUI errors, all the others can still be executed.

assignment to also propagate errors in this way?
This is another class of common missed errors, I would think.

Can such an error be missed without a

we have 'unused warning' for that already.

warning being printed by the compiler? If not, then it is less critical scenario. Users get compilation warning and that might warn them enough.

Btw. I think there is a contra-example:

x = Error.throw "OK" x . catch Any h->h

the second line wouldn't even be invoked.

@radeusgd I definitely see your point -- assignments like this form the top-level spine of the entire program, so this would prevent execution after a dataflow error. For that case, I agree. For an assignment with a regular method call, one that does not represent a top-level node (from the user's point of view), it's still something that could be lost.

@JaroslavTulach I see your point here as well. However, this case:

x = Error.throw "OK"

Will still mask errors, and hide them from the end user and the library developer. I don't have a suggestion for how to handle this, but it still should be handled. If something is called "error" then it should never be hidden.

mask errors, and hide them from ... the library developer

No, it won't hide them. There will be a warning about unused variable x. E.g. library developer has been notified the code is smelly.

Akirathan · 2024-12-05T10:51:55Z

engine/runtime/src/main/java/org/enso/interpreter/node/callable/function/BlockNode.java

    for (ExpressionNode statement : statements) {
-      statement.executeGeneric(frame);
+      var result = statement.executeGeneric(frame);
+      if (result != nothing) {


Why not get rid of this if statement and use simply just if (result instanceof DataflowError err). If it is an instance of DataflowError, it cannot be nothing.

Indeed this seems redundant, can we remove it, or is there some reason that we don't see here?

The reason is the other part of #5430:

what should happen when the value is not Nothing

and it is not DataflowError (which is immediately returned)

Avoid discarded (error) values in runtime #5430 talks about such values, but

I don't know what to do with them (they are not as simple as DataflowError)

related is Unexpected block behaviour with multiline operators #9944

We don't need to solve it in this PR. Just my 2 Kč explanation of the != Nothing check. In any case, that's for another PR.

JaroslavTulach · 2024-12-05T12:14:27Z

There are two failed tests in standard libraries:

This is the kind of errors we can expect if we enable this DataflowError propagation. Can you fix the tests easily, @GregoryTravis, @radeusgd?

Btw. this change makes the Decimal_Spec error go away:

diff --git test/Base_Tests/src/Data/Decimal_Spec.enso test/Base_Tests/src/Data/Decimal_Spec.enso
index 3465ce9cd8..6e3a463b82 100644
--- test/Base_Tests/src/Data/Decimal_Spec.enso
+++ test/Base_Tests/src/Data/Decimal_Spec.enso
@@ -487,7 +487,7 @@ add_specs suite_builder =
             (Decimal.from_integer 7297927982888383 . multiply  (Decimal.from_integer 828737) (Math_Context.new 6)) . should_equal (Decimal.from_integer 6048060000000000000000 )
             (Decimal.new "893872388.3535345" . multiply  (Decimal.new "72374727737.23434535") (Math_Context.new 14)) . should_equal (Decimal.new "64693770738918000000")
 
-            (Decimal.new "909678645268840" . divide (Decimal.new "28029830") (Math_Context.new 6)) . should_equal (Decimal.new "32453900 ")
+            _ = (Decimal.new "909678645268840" . divide (Decimal.new "28029830") (Math_Context.new 6)) . should_equal (Decimal.new "32453900 ")
             (Decimal.new "384456406.7860325392609633764" . divide (Decimal.new "24556.125563546") (Math_Context.new 7)) . should_equal (Decimal.new "15656.23")
 
             (Decimal.from_integer 3948539458034580838458034803485 . add (Decimal.from_integer 237957498573948579387495837459837) (Math_Context.new 20)) . should_equal (Decimal.from_integer 241906038031983160230000000000000)

e.g. exactly as expected it is enough to add a dummy assignment and the problem goes away. I assume you want to do a better fix @GregoryTravis - please integrate it into my branch, so it gets green.

diff --git test/Base_Tests/src/Data/Time/Date_Range_Spec.enso test/Base_Tests/src/Data/Time/Date_Range_Spec.enso
index b2b469ae1c..22843aa6cb 100644
--- test/Base_Tests/src/Data/Time/Date_Range_Spec.enso
+++ test/Base_Tests/src/Data/Time/Date_Range_Spec.enso
@@ -157,7 +157,7 @@ add_specs suite_builder =
                 r.partition p . should_equal (r.to_vector.partition p)
                 r.all p . should_equal (r.to_vector.all p)
                 r.any p . should_equal (r.to_vector.any p)
-                r.find p . should_equal (r.to_vector.find p)
+                _ = r.find p . should_equal (r.to_vector.find p)
                 r.index_of p . should_equal (r.to_vector.index_of p)
                 r.last_index_of p . should_equal (r.to_vector.last_index_of p)
                 count_mondays acc date =
@@ -170,7 +170,7 @@ add_specs suite_builder =
                 r.partition fc . should_equal (r.to_vector.partition fc)
                 r.all fc . should_equal (r.to_vector.all fc)
                 r.any fc . should_equal (r.to_vector.any fc)
-                r.find fc . should_equal (r.to_vector.find fc)
+                _ = r.find fc . should_equal (r.to_vector.find fc)
                 r.index_of fc . should_equal (r.to_vector.index_of fc)
                 r.last_index_of fc . should_equal (r.to_vector.last_index_of fc)

same for Date_Range_Spec @radeusgd.

…wrongly written test

radeusgd

Santa came early this year! 🎄
I'm so happy to finally see this change - deep down I knew we may be losing something due to the unhandled errors disappearing into the void. And you confirmed my suspicions. Thankfully that isn't that many errors and they look relatively harmless. But I'm happy that from now on I can keep writing code with the peace of mind that errors will go on unnoticed.

Also we can now remove most instances of if_not_error - yay! 🎉 It was basically a workaround for ensuring error propagation. I guess we want to keep the function because it is still useful when we want to control the propagation with 'more precision'. But in numerous cases it should be no longer needed and we can remove a lot of unnecessary indentation from the code.

I submitted commits that fix should_equal silently breaking if expected value provided is dataflow error (in that case should_fail_with is expected). I will add a commit fixing the Date_Range_Spec tests in a moment.

JaroslavTulach · 2024-12-05T13:40:03Z

we want to keep the function because ...

... we already published first customer releases and we are dedicated to backward compatibility, right?

JaroslavTulach · 2024-12-06T03:53:51Z

Engine benchmarks run is fine - looks like nobody writes benchmarks for errors ;-) I also glanced StdLib benchmarks run and found nothing suspicious.

JaroslavTulach · 2024-12-06T03:55:50Z

@radeusgd, @GregoryTravis there are still failures in stdlib tests:

Can you fix them?

jdunkerley

Approving the libs bits.

…ateDataflowError5429

JaroslavTulach · 2024-12-09T13:32:48Z

Except these two errors:

Date_Range

Decimal_Spec

There seem to be additional ones:

GregoryTravis · 2024-12-10T15:21:52Z

Can you fix them?

I will look at these, after the release.

Currently a DataflowError may get lost in a middle of block statements

1c2d007

JaroslavTulach self-assigned this Dec 5, 2024

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners December 5, 2024 10:17

Propagate Error ASAP instead of ignoring it

49c628e

JaroslavTulach changed the title ~~Currently a DataflowError may get lost in a middle of block statements~~ Propagate DataflowError in middle of block instead of ignoring it Dec 5, 2024

JaroslavTulach requested a review from radeusgd December 5, 2024 10:25

JaroslavTulach commented Dec 5, 2024

View reviewed changes

JaroslavTulach mentioned this pull request Dec 5, 2024

Clear cache on reload #11673

Merged

5 tasks

JaroslavTulach requested a review from GregoryTravis December 5, 2024 10:35

Akirathan reviewed Dec 5, 2024

View reviewed changes

Akirathan approved these changes Dec 5, 2024

View reviewed changes

check for dataflow error in should_equal rhs and report it - this is …

91f7497

…wrongly written test

radeusgd requested review from jdunkerley, AdRiley and marthasharkey as code owners December 5, 2024 13:05

add error check in remaining should_equal overrides

1ed8505

radeusgd approved these changes Dec 5, 2024

View reviewed changes

enso-bot bot mentioned this pull request Dec 6, 2024

Allow (multi_value : Table) : Column to succeed #11482

Closed

align behaviour of find across range/vector

7373304

jdunkerley approved these changes Dec 6, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into wip/jtulach/Propag…

00516f5

…ateDataflowError5429

JaroslavTulach mentioned this pull request Dec 11, 2024

Enhance Managed_Resource to allow implementation of in-memory caches #11577

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate DataflowError in middle of block instead of ignoring it #11777

Propagate DataflowError in middle of block instead of ignoring it #11777

JaroslavTulach commented Dec 5, 2024 •

edited

Loading

JaroslavTulach Dec 5, 2024 •

edited

Loading

GregoryTravis Dec 10, 2024

radeusgd Dec 10, 2024 •

edited

Loading

JaroslavTulach Dec 10, 2024

GregoryTravis Dec 10, 2024

JaroslavTulach Dec 11, 2024

Akirathan Dec 5, 2024

radeusgd Dec 5, 2024

JaroslavTulach Dec 5, 2024 •

edited

Loading

JaroslavTulach commented Dec 5, 2024 •

edited

Loading

radeusgd left a comment

JaroslavTulach commented Dec 5, 2024

JaroslavTulach commented Dec 6, 2024

JaroslavTulach commented Dec 6, 2024

jdunkerley left a comment

JaroslavTulach commented Dec 9, 2024

GregoryTravis commented Dec 10, 2024

Propagate DataflowError in middle of block instead of ignoring it #11777

Are you sure you want to change the base?

Propagate DataflowError in middle of block instead of ignoring it #11777

Conversation

JaroslavTulach commented Dec 5, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

GregoryTravis Dec 10, 2024

Choose a reason for hiding this comment

radeusgd Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Dec 10, 2024

Choose a reason for hiding this comment

GregoryTravis Dec 10, 2024

Choose a reason for hiding this comment

JaroslavTulach Dec 11, 2024

Choose a reason for hiding this comment

Akirathan Dec 5, 2024

Choose a reason for hiding this comment

radeusgd Dec 5, 2024

Choose a reason for hiding this comment

JaroslavTulach Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach commented Dec 5, 2024 • edited Loading

radeusgd left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Dec 5, 2024

JaroslavTulach commented Dec 6, 2024

JaroslavTulach commented Dec 6, 2024

jdunkerley left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Dec 9, 2024

GregoryTravis commented Dec 10, 2024

JaroslavTulach commented Dec 5, 2024 •

edited

Loading

JaroslavTulach Dec 5, 2024 •

edited

Loading

radeusgd Dec 10, 2024 •

edited

Loading

JaroslavTulach Dec 5, 2024 •

edited

Loading

JaroslavTulach commented Dec 5, 2024 •

edited

Loading