Support on_problems=Problem_Behavior.Report_Warning and Map_Error wrapping in Vector.map #8595

GregoryTravis · 2023-12-19T21:44:16Z

Pull Request Description

Implements Warnings.get_all wrap_errors=True which wraps warnings attached to values inside vectors with Map_Error, which includes the position of the value within the vector. See the documentation for more details.

get_all wrap_errors=True does not change the warnings that are attached to values -- it wraps them before returning them to the caller, but does not change the original warnings attached to the values.

Wrapped warnings only appear attached to the vector itself. The values inside the vector do not have their warnings wrapped.

Warning propagation is not changed at all; Warnings.get_all (with default wrap_errors=False) behaves as before. get_all wrap_errors=True is meant to be used primarily by the IDE, although it can be used anywhere this wrapping is desired.

Important Notes

Checklist

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

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

This reverts commit c30d171.

radeusgd · 2024-01-09T17:44:09Z

CHANGELOG.md

@@ -599,6 +599,7 @@
 - [Support for loading big Excel files.][8403]
 - [Added new `Filter_Condition`s - `Equal_Ignore_Case`, `Is_Nan`, `Is_Infinite`
  and `Is_Finite`.][8539]
+- Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]


Suggested change

- Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]

- [Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]

radeusgd · 2024-01-09T17:44:58Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

+   Describes an error attached to a value within a `Vector`. The `index` field
+   contains the index into the `Vector` at which the error occurred.
+
+   If a value is a nested within multiple `Vector`s, its warnings are wrapped


Suggested change

If a value is a nested within multiple `Vector`s, its warnings are wrapped

If a value is nested within multiple `Vector`s, its warnings are wrapped

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

…ke_Helpers.enso Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>

GregoryTravis · 2024-01-02T21:10:34Z

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

@@ -307,8 +307,6 @@ public Object invokeWarnings(

      if (result instanceof DataflowError) {
        return result;
-      } else if (result instanceof WithWarnings withWarnings) {


This case is already handled by WithWarnings.wrap.

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Data_Formatter.enso

docs/semantics/wrapped-errors.md

GregoryTravis · 2024-01-03T15:58:29Z

test/Tests/src/Data/Json_Spec.enso

@@ -164,6 +164,13 @@ spec =
            obj1 . should_not_equal <| JS_Object.from_pairs [["a", 43], ["b", 123]]
            obj1 . should_not_equal <| JS_Object.from_pairs [["a", 42], ["b", JS_Object.from_pairs [["c",1], ["d",3]]]]

+        Test.specify "should be able to set values" <|


This is part of #8630, which will be merged first.

docs/semantics/wrapped-errors.md

GregoryTravis · 2024-01-09T16:50:42Z

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java

@@ -617,7 +617,8 @@ private boolean generateWarningsCheck(
                + " = withWarnings.getValue();");
        out.println(
            "      gatheredWarnings ="
-                + " gatheredWarnings.prepend(withWarnings.getReassignedWarningsAsRope(bodyNode));");


test/Benchmarks/src/Vector/Map_Error_Benchmark.enso

GregoryTravis · 2024-01-09T18:34:06Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

+   Describes an error attached to a value within a `Vector`. The `index` field
+   contains the index into the `Vector` at which the error occurred.
+
+   If a value is a nested within multiple `Vector`s, its warnings are wrapped


GregoryTravis · 2024-01-09T18:34:10Z

CHANGELOG.md

@@ -599,6 +599,7 @@
 - [Support for loading big Excel files.][8403]
 - [Added new `Filter_Condition`s - `Equal_Ignore_Case`, `Is_Nan`, `Is_Infinite`
  and `Is_Finite`.][8539]
+- Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]


JaroslavTulach

I can see that a lot of thought and work has been put into this PR. Let's get it in! I'd update the description of the PR (as that usually gets copied to Git commit description). I'd somehow point readers to docs/semantics/wrapped-errors.md as that contains great explanation of the overall design.

JaroslavTulach · 2024-01-11T05:49:14Z

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

+       not a wrapping error, it is returned unchanged.
+    unwrap : Any -> Any
+    unwrap error_value =
+        Panic.catch No_Such_Conversion handler=_->error_value <|


Interesting use of conversion. Convert to Wrapped_Error and then use inner_error. I can see that the Error.unwrap method is applied recursively to strip off all the wrappers. Consider adding @Tail_Call annotation to turn it into a loop.

JaroslavTulach · 2024-01-11T05:52:10Z

test/Benchmarks/src/Warnings.enso

+
+        builder.group "warnings_get_all_wrapped_"+vector_type options group_builder->
+            group_builder.specify "no_warnings" <|
+                Warning.get_all_wrapped data.no_warnings


Where is the get_all_wrapped function defined? Aren't we supposed to use get_all wrap_errors=True?

Implements Warnings.get_all_wrapped which wraps warnings

If we shall be using get_all wrap_errors=True then I'd like to point out the pull request description is outdated.

JaroslavTulach · 2024-01-11T06:10:00Z

docs/semantics/wrapped-errors.md

+
+## Catching Wrapped Errors
+
+Wrapped errors are "transparent" to `Error.catch`. That is, if you attempt to


JaroslavTulach · 2024-01-11T06:11:17Z

docs/semantics/wrapped-errors.md

+
+## Implementing Error Wrappers
+
+An error wrapper is a regular Enso value that has a conversion to `Wrapped_Error`. For example:


GregoryTravis · 2024-01-12T19:03:41Z

I can see that a lot of thought and work has been put into this PR. Let's get it in! I'd update the description of the PR (as that usually gets copied to Git commit description). I'd somehow point readers to docs/semantics/wrapped-errors.md as that contains great explanation of the overall design.

Done.

This PR is blocked by the error "Invariant contract violation for receiver Nothing" when runtime assertions are turned on. I am unable to reproduce it with a single test, or under the debugger. Discussion here:

https://discord.com/channels/401396655599124480/1194662721958969364/1194662721958969364

Discord
Discord - A New Way to Chat with Friends & Communities
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

Looks like that eventually violates Truffle invariants and was lurking around.

JaroslavTulach · 2024-01-13T06:01:12Z

...untime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/IsSameObjectNode.java

+        "interop.isMetaObject(metaLeft)",
+        "!interop.isNull(metaRight)",
+        "interop.isMetaObject(metaRight)"
+      })


Do we know how to reproduce the problem?

run stdlib test/Tests with assertions on. I haven't been able to reduce it to something small yet.

hubertp · 2024-01-15T15:13:45Z

@GregoryTravis
This failure looks legit?

 INFO ide_ci::program::command: npmℹ️ Checking formatting...
 INFO ide_ci::program::command: npm⚠️ [warn] docs/semantics/README.md
 INFO ide_ci::program::command: npm⚠️ [warn] docs/semantics/wrapped-errors.md
 INFO ide_ci::program::command: npm⚠️ [warn] Code style issues found in 2 files. Run Prettier to fix.

@JaroslavTulach are you OK with merging it with my workaround? @Akirathan will create a PR with a smaller case but I don't think it makes sense to block this work because it is unrelated to Greg's change.

…371-element-direct

Akirathan · 2024-01-15T15:29:42Z

are you OK with merging it with my workaround?

@hubertp yes, go ahead. I have created a separate PR for this specific issue in #8764

GregoryTravis added 30 commits November 8, 2023 15:08

tests

2485d3e

Merge branch 'develop' into wip/gmt/8110-map-error

0cb3675

wip

dd0fdde

wip

ae1fd56

additional warnings

3cb9b4a

wip

506a13b

Merge branch 'develop' into wip/gmt/8110-map-error

f4427df

wip

b5d4325

cleanup

be40d60

nested wrapping

8205cac

multiple nestings

87c246b

wraps_error uses looks_for, test for should_fail_with

40ff235

wip

a66384a

Merge branch 'develop' into wip/gmt/8110-map-error

d1f7013

stack trace line fix

a4d9bbd

use catch_primitive internally

9e3f0f0

fix warning mapping, dtf spec

074ee0b

just one wrapper checker, vector spec

7a3800c

missing ctor, back to non-primitive catch

7516a6b

Merge branch 'develop' into wip/gmt/8110-map-error

f8f0b2f

back to c_p

b15c7a6

put old map back

059559b

wip

8409be6

unnest tests

85e3e2d

Array.map on_problems

b0e811a

wip

c30d171

Revert "wip"

178ccd4

This reverts commit c30d171.

better test names

fd3062c

warning logging

f31bc15

wip

74fa5f7

radeusgd reviewed Jan 9, 2024

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso Outdated Show resolved Hide resolved

GregoryTravis and others added 2 commits January 9, 2024 13:27

Update distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Li…

e9109d7

…ke_Helpers.enso Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>

review

c09b3f5

GregoryTravis requested a review from radeusgd January 9, 2024 18:34

GregoryTravis added CI: Clean build required CI runners will be cleaned before and after this PR is built. and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jan 9, 2024

GregoryTravis commented Jan 10, 2024

View reviewed changes

radeusgd approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'develop' into 8371-element-direct

1745dfb

JaroslavTulach approved these changes Jan 11, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jan 12, 2024

Avoid Enso/Python & co. interop problems by using Truffle TCK tests #5238

Closed

GregoryTravis added 3 commits January 12, 2024 11:23

Merge branch 'develop' into 8371-element-direct

219c38e

update benchmarks, add to Main

421ee96

Error.unwrap tail call

818c1ef

Don't test nulls for meta objects

8b685a1

Looks like that eventually violates Truffle invariants and was lurking around.

JaroslavTulach reviewed Jan 13, 2024

View reviewed changes

GregoryTravis added 3 commits January 15, 2024 10:17

Merge branch 'develop' into 8371-element-direct

b117206

fmt docs

f2cdb7b

Merge branch '8371-element-direct' of github.com:enso-org/enso into 8…

b669303

…371-element-direct

enso-bot bot mentioned this pull request Jan 15, 2024

Arrow language to use industry standard format for columnar data #7755

Closed

GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Jan 15, 2024

mergify bot merged commit f2cb1f0 into develop Jan 16, 2024
29 checks passed

mergify bot deleted the 8371-element-direct branch January 16, 2024 09:36

enso-bot bot mentioned this pull request Jan 16, 2024

Race in application of pending edits leads to stability issues #8770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support on_problems=Problem_Behavior.Report_Warning and Map_Error wrapping in Vector.map #8595

Support on_problems=Problem_Behavior.Report_Warning and Map_Error wrapping in Vector.map #8595

GregoryTravis commented Dec 19, 2023 •

edited

Loading

radeusgd Jan 9, 2024

GregoryTravis Jan 9, 2024

radeusgd Jan 9, 2024

GregoryTravis Jan 9, 2024

GregoryTravis Jan 2, 2024

GregoryTravis Jan 3, 2024

GregoryTravis Jan 9, 2024

GregoryTravis Jan 9, 2024

GregoryTravis Jan 9, 2024

JaroslavTulach left a comment

JaroslavTulach Jan 11, 2024

GregoryTravis Jan 12, 2024

JaroslavTulach Jan 11, 2024

GregoryTravis Jan 12, 2024

JaroslavTulach Jan 11, 2024

JaroslavTulach Jan 11, 2024

GregoryTravis commented Jan 12, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach Jan 13, 2024

hubertp Jan 13, 2024

hubertp commented Jan 15, 2024

Akirathan commented Jan 15, 2024

	- Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]
	- [Support `on_problems=Problem_Behavior.Report_Warning` in `Vector.map`.][8595]

	If a value is a nested within multiple `Vector`s, its warnings are wrapped
	If a value is nested within multiple `Vector`s, its warnings are wrapped


		## Catching Wrapped Errors

		Wrapped errors are "transparent" to `Error.catch`. That is, if you attempt to


		## Implementing Error Wrappers

		An error wrapper is a regular Enso value that has a conversion to `Wrapped_Error`. For example:

Support on_problems=Problem_Behavior.Report_Warning and Map_Error wrapping in Vector.map #8595

Support on_problems=Problem_Behavior.Report_Warning and Map_Error wrapping in Vector.map #8595

Conversation

GregoryTravis commented Dec 19, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

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

JaroslavTulach 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

GregoryTravis commented Jan 12, 2024 • edited by unfurl-links bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Jan 15, 2024

Akirathan commented Jan 15, 2024

GregoryTravis commented Dec 19, 2023 •

edited

Loading

GregoryTravis commented Jan 12, 2024 •

edited by unfurl-links bot

Loading