Mixed Decimal/Float operations throw error or attach warning #10725

GregoryTravis · 2024-07-31T14:26:41Z

Arithmetic operations throw; comparisons attach warnings.

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,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

radeusgd · 2024-08-01T13:13:58Z

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

-    + self (that : Decimal) -> Decimal ! Arithmetic_Error = self.add that
+    + self (that : Decimal) = self.add that


Why are these return type check being removed? It seems suspicious.

If I leave the checked annotations in, I get this:

Reason: An unexpected panic was thrown: Type error: expected the result of `+` to be Decimal, but got <ERR>.

I'll be submitting a bug for this.

Oh, weird. Okay then, I assume we should re-add once the bug is fixed.

Added a TODO.

I can reproduce the failure as

sbt:enso> runEngineDistribution --run test/Base_Tests Arithmetic.with.decimal.and.implicitly.converted.float.should.fail

correct? Then the problem is that there is a WithWarning instance over DataflowError instance. Do we really want to support attaching warnings to errors? It doesn't make sense to re-attach warnings to DataflowError. Possible fix:

diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java index e1ff24d35c..f750e377d6 100644 --- engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java +++ engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java @@ -73,7 +73,7 @@ public final class Warning implements EnsoObject { description = "Attaches the given warning to the value.", autoRegister = false) @Builtin.Specialize - public static WithWarnings attach( + public static EnsoObject attach( EnsoContext ctx, WithWarnings value, Object warning, Object origin) { return value.append(ctx, new Warning(warning, origin, ctx.nextSequenceId())); } @@ -83,7 +83,7 @@ public final class Warning implements EnsoObject { description = "Attaches the given warning to the value.", autoRegister = false) @Builtin.Specialize(fallback = true) - public static WithWarnings attach(EnsoContext ctx, Object value, Object warning, Object origin) { + public static EnsoObject attach(EnsoContext ctx, Object value, Object warning, Object origin) { return WithWarnings.wrap(ctx, value, new Warning(warning, origin, ctx.nextSequenceId())); } diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java index 2968eaf419..0bb3191157 100644 --- engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java +++ engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java @@ -120,9 +120,11 @@ public final class WithWarnings implements EnsoObject { return goodValue; } - public static WithWarnings wrap(EnsoContext ctx, Object value, Warning... warnings) { + public static EnsoObject wrap(EnsoContext ctx, Object value, Warning... warnings) { if (value instanceof WithWarnings with) { return with.append(ctx, warnings); + } else if (value instanceof DataflowError err) { + throw err; } else { return new WithWarnings(value, ctx.getWarningsLimit(), warnings); }

CCing @Akirathan.

This warning is attached by propagation, so perhaps this is a case we do need to handle. I can reproduce it like this:

err_warn -> Integer ! Illegal_Argument = v = Warning.attach (Out_Of_Range.Error "qewr") 12 case v of _ : Integer -> Error.throw (Illegal_Argument.Error "asdf")

In this case the warning and error are in the same code. But in general, if a value with a warning results in a throw, the warning will propagate to the thrown error.

For this PR I can remove the warning as a workaround, but in general this seems like something that could happen naturally. I'm surprised it hasn't happened already.

This patch is resulting in this:

Execution finished with an error: Wrapped_Dataflow_Error.Error at <enso> Panic.throw(Internal) at <enso> Panic.type.rethrow_wrapped_if_error<arg-1>(Panic.enso:223:32-85) at <enso> Panic.type.rethrow_wrapped_if_error(Panic.enso:223:9-96) at <enso> Builder.append_vector_range(Vector.enso:1409-1415) at <enso> case_branch(Suite.enso:99:25-68) at <enso> Suite.run_with_filter.junit_sb_builder(Suite.enso:95-101) at <enso> Array_Like_Helpers.each.Array_Like_Helpers.each(Array_Like_Helpers.enso:301:9-24) at <enso> Range.each.go<arg-1>(Range.enso:262:36-51) at <enso> Range.each.go<arg-2>(Range.enso:262:21-52) at <enso> Range.each.go(Range.enso:261-263) at <enso> Test_Reporter.wrap_junit_testsuites(Test_Reporter.enso:26:14-19) at <enso> Suite.run_with_filter(Suite.enso:91-101) at <enso> Decimal_Spec.main(Decimal_Spec.enso:1068:5-32)

That's what the patch results in. I had no time to investigate where the problem is. I assumed it is in libs....

...case the warning and error are in the same code...

Great example! I turning it into a failing test and I'll play with it.

JaroslavTulach

instanceof in a specialization is not correct
Any.to is a "final" method and shouldn't be overriden
what are the benchmarks results?

JaroslavTulach · 2024-08-02T06:59:14Z

.../runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsAtomNode.java

@@ -100,7 +101,8 @@ boolean equalsAtomsWithCustomComparator(
    var args = new Object[] {cachedComparator, self, other};
    var result = invokeNode.execute(compareFn, null, State.create(ctx), args);
    assert orderingOrNullOrError(this, ctx, result, compareFn);
-    return ctx.getBuiltins().ordering().newEqual() == result;
+    var resultWithWarningRemoved = result instanceof WithWarnings ww ? ww.getValue() : result;


I don't think we should be using instanceof. We should use WarningsLibrary.

At minimum add a new @CachedLibrary WarningsLibrary argument to the specialization method and use it to extract the original value via removeWarnings.

What is the impact on benchmarks? I am afraid just adding this check may slow things down. Maybe we will need a separate specialization...

JaroslavTulach · 2024-08-02T07:01:59Z

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

@@ -442,6 +443,64 @@ type Number
     floating point numbers.
 @Builtin_Type
 type Float
+    ## GROUP Conversions
+       ICON convert
+       Generic conversion of an arbitrary Enso value to requested type.


arbitrary Enso value!? I guess the whole documentation has just been copied. The documentation doesn't make sense here.

This is removed.

JaroslavTulach · 2024-08-02T07:05:12Z

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

+             yields_5 = Complex.Value 3 4 . to Number ignore_im=False
+             default5 = Complex.Value 3 4 . to Number
+    to : Any -> Any ! No_Such_Conversion
+    to self target_type = case target_type of


I don't understand why there should be a special to method on Float? The generic Any.to method should be good enough.

If there was a way to define a final method (like in Java), then Any.to should be final. If one wants to hook into behavior of Any.to method, one should do it with from conversions. Otherwise one cannot guarantee consistency between from and to.

I'm afraid the point of this PR is to add an exception to the from/to inconsistency.

This is removed.

JaroslavTulach · 2024-08-02T07:06:42Z

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

@@ -89,6 +89,9 @@ type Decimal
    ## PRIVATE
    Value (big_decimal : BigDecimal)


Use private keyword.

I see private keyword is gone. Why? 1381d29 doesn't explain why that has happened.

When I put the private keyword back, I get this:

Private access error: The project-private method 'null.big_decimal' in unknown project cannot be accessed from project 'Standard.Base'.

This is happening in a method of Decimal, which should have access. I don't know why it calls it an "unknown project".

Full test failure:

- [FAILED] should be able to construct a Decimal with dec [43ms] Reason: An unexpected panic was thrown: (Private_Access.Error 'Standard.Base' Nothing 'null.big_decimal') at <enso> Decimal.unscaled_value<arg-0>(/Users/gmt/dev/enso/enso/built-distribution/enso-engine-0.0.0-dev-macos-aarch64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso:1169:38-53) at <enso> Decimal.unscaled_value(/Users/gmt/dev/enso/enso/built-distribution/enso-engine-0.0.0-dev-macos-aarch64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso:1169:38-67) at <enso> Decimal.internal_representation(/Users/gmt/dev/enso/enso/built-distribution/enso-engine-0.0.0-dev-macos-aarch64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Decimal.enso:1173:55-73) at <enso> Decimal.should_have_rep<arg-0>(/Users/gmt/dev/enso/enso/test/Base_Tests/src/Data/Decimal_Spec.enso:14:36-63) at <enso> Decimal.should_have_rep(/Users/gmt/dev/enso/enso/test/Base_Tests/src/Data/Decimal_Spec.enso:14:36-82)

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

radeusgd · 2024-08-02T12:44:31Z

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

+       ? Creating `Decimal`s and Converting to `Decimal`
+
+         When creating a `Decimal` from a literal floating-point value, the
+         preferred method is to express the literal as a string and use
+         `Decimal.from_text`, since this will give a `Decimal` that matches the
+         value precisely.
+
+         To convert a `Float` or `Integer` to a `Decimal`, use its `.to_decimal`
+         method. You can also use `Float.to Decimal` to convert a `Float` to a
+         `Decimal`.


It feels like this is still missing information about when I should expect a warning and when not.

This is removed. The same documentation exists in Decimal.enso but there it is followed by error/warning documentation.

JaroslavTulach

Glad to see 9ce1a0a we no longer override to method. Rather than removing -> Decimal, consider not attaching warnings to errors as the provided patch does. If the benchmarks are OK, go on and merge.

JaroslavTulach · 2024-08-05T03:37:23Z

.../runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsAtomNode.java

+      var result = invokeNode.execute(compareFn, null, State.create(ctx), args);
+      assert orderingOrNullOrError(this, ctx, result, compareFn);
+      if (warnings.hasWarnings(result)) {
+        result = warnings.removeWarnings(result);


Yes, this is better than instanceof. The effect on benchmark results remain to be seen. Possibly one could also use BranchProfile to lower the number of generated IR nodes in case without warnings.

JaroslavTulach · 2024-08-05T04:00:18Z

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

-    + self (that : Decimal) -> Decimal ! Arithmetic_Error = self.add that
+    + self (that : Decimal) = self.add that


I can reproduce the failure as

sbt:enso> runEngineDistribution --run test/Base_Tests Arithmetic.with.decimal.and.implicitly.converted.float.should.fail

correct? Then the problem is that there is a WithWarning instance over DataflowError instance. Do we really want to support attaching warnings to errors? It doesn't make sense to re-attach warnings to DataflowError. Possible fix:

diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java index e1ff24d35c..f750e377d6 100644 --- engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java +++ engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java @@ -73,7 +73,7 @@ public final class Warning implements EnsoObject { description = "Attaches the given warning to the value.", autoRegister = false) @Builtin.Specialize - public static WithWarnings attach( + public static EnsoObject attach( EnsoContext ctx, WithWarnings value, Object warning, Object origin) { return value.append(ctx, new Warning(warning, origin, ctx.nextSequenceId())); } @@ -83,7 +83,7 @@ public final class Warning implements EnsoObject { description = "Attaches the given warning to the value.", autoRegister = false) @Builtin.Specialize(fallback = true) - public static WithWarnings attach(EnsoContext ctx, Object value, Object warning, Object origin) { + public static EnsoObject attach(EnsoContext ctx, Object value, Object warning, Object origin) { return WithWarnings.wrap(ctx, value, new Warning(warning, origin, ctx.nextSequenceId())); } diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java index 2968eaf419..0bb3191157 100644 --- engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java +++ engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java @@ -120,9 +120,11 @@ public final class WithWarnings implements EnsoObject { return goodValue; } - public static WithWarnings wrap(EnsoContext ctx, Object value, Warning... warnings) { + public static EnsoObject wrap(EnsoContext ctx, Object value, Warning... warnings) { if (value instanceof WithWarnings with) { return with.append(ctx, warnings); + } else if (value instanceof DataflowError err) { + throw err; } else { return new WithWarnings(value, ctx.getWarningsLimit(), warnings); }

CCing @Akirathan.

JaroslavTulach · 2024-08-09T08:44:17Z

Attaching a warning to error is no op #10765 has been integrated. Attaching warnings to errors shall no longer crash the engine

jdunkerley and others added 20 commits July 16, 2024 16:56

WIP idea.

e6cdaf6

typo

5ecd4bb

merge

23411ee

Merge branch 'jd-tmp' into gmt-tmp

9b3556a

tests, keep original float

cdb246b

Merge branch 'develop' into wip/gmt/10631-mixed-err

6df2066

mark == warning test pending

c0fd213

Merge branch 'develop' into wip/gmt/10631-mixed-err

819d589

Merge branch 'develop' into wip/gmt/10631-mixed-err

a7ad2f9

fix test

50d1a2b

add IA to add ret err

a542114

Merge branch 'develop' into wip/gmt/10631-mixed-err

80337ed

fix for warning-attached Equals

b82a81a

other operators, remove return types

d2834bc

Merge branch 'develop' into wip/gmt/10631-mixed-err

3e88b37

fix tests

c4ae795

fix tests

2bf1ed0

cleanup

43a6c25

Merge branch 'develop' into wip/gmt/10631-mixed-err

fd88c52

fix tests

6508485

GregoryTravis requested review from jdunkerley, radeusgd, AdRiley, marthasharkey, 4e6, JaroslavTulach, hubertp and Akirathan as code owners July 31, 2024 14:26

changelog

67d577b

radeusgd reviewed Aug 1, 2024

View reviewed changes

GregoryTravis requested review from radeusgd and jdunkerley August 1, 2024 19:19

JaroslavTulach requested changes Aug 2, 2024

View reviewed changes

radeusgd reviewed Aug 2, 2024

View reviewed changes

GregoryTravis added 3 commits August 2, 2024 15:57

warnings lib

ff92f45

remove modified Float.to

9ce1a0a

private

da6a3d5

GregoryTravis requested review from radeusgd and JaroslavTulach August 2, 2024 20:10

fmt

ab8e863

enso-bot bot mentioned this pull request Aug 3, 2024

Encapsulation of polyglot/java Dependencies #7082

Open

JaroslavTulach approved these changes Aug 5, 2024

View reviewed changes

radeusgd approved these changes Aug 5, 2024

View reviewed changes

Merge branch 'develop' into wip/gmt/10631-mixed-err

3775568

JaroslavTulach mentioned this pull request Aug 6, 2024

WithWarnings uses EnsoHashMap to speed things up #10555

Merged

1 task

merge, changelog

4c960a6

GregoryTravis added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Aug 7, 2024

GregoryTravis added 2 commits August 8, 2024 11:19

Merge branch 'develop' into wip/gmt/10631-mixed-err

2cd4d63

remove private from Decimal ctor, merge

1381d29

GregoryTravis added 3 commits August 12, 2024 10:37

Merge branch 'develop' into wip/gmt/10631-mixed-err

cd6c243

fmt

7f02ed1

move constructor test to Base_Internal_Tests

ed356d5

GregoryTravis mentioned this pull request Aug 12, 2024

Make Decimal constructors private #10799

Open

GregoryTravis added 3 commits August 12, 2024 12:29

put return types back

6eac98d

Merge branch 'develop' into wip/gmt/10631-mixed-err

ba8550c

missing import

3110355

mergify bot merged commit e836373 into develop Aug 14, 2024
41 checks passed

mergify bot deleted the wip/gmt/10631-mixed-err branch August 14, 2024 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mixed Decimal/Float operations throw error or attach warning #10725

Mixed Decimal/Float operations throw error or attach warning #10725

GregoryTravis commented Jul 31, 2024 •

edited

Loading

radeusgd Aug 1, 2024

GregoryTravis Aug 1, 2024 •

edited

Loading

radeusgd Aug 1, 2024

GregoryTravis Aug 1, 2024

JaroslavTulach Aug 5, 2024

GregoryTravis Aug 5, 2024

GregoryTravis Aug 5, 2024

JaroslavTulach Aug 6, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Aug 2, 2024

JaroslavTulach Aug 2, 2024

GregoryTravis Aug 2, 2024

JaroslavTulach Aug 2, 2024

radeusgd Aug 2, 2024

GregoryTravis Aug 2, 2024

JaroslavTulach Aug 2, 2024

GregoryTravis Aug 2, 2024

JaroslavTulach Aug 9, 2024

GregoryTravis Aug 12, 2024

GregoryTravis Aug 12, 2024

radeusgd Aug 2, 2024

GregoryTravis Aug 2, 2024

JaroslavTulach left a comment

JaroslavTulach Aug 5, 2024

JaroslavTulach Aug 5, 2024

JaroslavTulach commented Aug 9, 2024

		+ self (that : Decimal) -> Decimal ! Arithmetic_Error = self.add that
		+ self (that : Decimal) = self.add that

		@@ -89,6 +89,9 @@ type Decimal
		## PRIVATE
		Value (big_decimal : BigDecimal)

Mixed Decimal/Float operations throw error or attach warning #10725

Mixed Decimal/Float operations throw error or attach warning #10725

Conversation

GregoryTravis commented Jul 31, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

GregoryTravis Aug 1, 2024 • edited Loading

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 Aug 6, 2024 • edited Loading

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

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

JaroslavTulach commented Aug 9, 2024

GregoryTravis commented Jul 31, 2024 •

edited

Loading

GregoryTravis Aug 1, 2024 •

edited

Loading

JaroslavTulach Aug 6, 2024 •

edited

Loading