Enforce conversion method return type & redefine Comparable

[JaroslavTulach]

Pull Request Description

Fixes #10355 by enforcing return type of Xyz.from conversion methods. Most frequent violations are caused by ordering support - hence 280b7a7 defines Comparable.new to address the problem:

• use Ordering.compare to compare two values to be equal, less or greater or uncomparable
• use Ordering.hash to compute hash code for any object in Enso
• use Comparable.new while implementing Comparable.from conversion to control compare and hash for an object

Those are the most important API changes. Other types, like Default_Comparator were moved outside of the API into private module.

Checklist

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

• The documentation has been updated, if necessary - done in ecfcece
• All code follows the
  Scala,
  Java,
  style guides.
• Unit tests have been written where possible.
• Benchmarks continue to be fast

[JaroslavTulach]

With 280b7a7 we seem to get following results:

sbt:enso> runEngineDistribution --run test/Base_Tests
2450 tests succeeded.
401 tests failed.
38 tests skipped.
9 groups skipped.

Fixing:

• down to 178 tests failed with e908a32
• down to 134 tests failed after 5ed87b2
• at 112 failing tests after 4443559

[jdunkerley]

Can we still return errors from
conversions as we use that?

[JaroslavTulach]

Can we still return errors from conversions as we use that?

Yes. Error.throw - e.g. DataflowErrors are special "all fit" values and they pass thru without any check.

[JaroslavTulach]

at 112 failing tests after 4443559

At 33 failing tests with 18dde9b as this report confirms. Time to execute some benchmarks:

• engine - good!
• stdlibs - looking good as well

[JaroslavTulach]

The problem for today is that following program works:

from Standard.Base import Vector, Comparable

type T
    Value a b

type T_Comparator
    compare t1 t2 = Comparable.from t1.a . compare t2.a
    hash t = Comparable.from t.a . hash

Comparable.from (that:T) = Comparable.new that T_Comparator

main = [T.Value 1 2, T.Value 1 2] . distinct

but when one changes the first line to

from Standard.Base import all

then it doesn't and yields:

Execution finished with an error: Assertion Error: 'Expecting Ordering or Nothing, but got: Error:Error (By 1 Decimal_Comparator) (By 1 Default_Comparator) with type Error calling T_Comparator.compare[v.enso:7:5-55] self=_ t1=_ t2=_'
        at <enso> Map.get(Map.enso:220:40-70)

Update: workarounded by a639364

[JaroslavTulach]

Time to re-run benchmarks:

• engine
• stdlib with b652893 fix

Engine benchmarks are looking good. StdLibs benchmarks look fine, except:

which demonstrate something has been changed.

[JaroslavTulach]

There is only a single remaining failure:

 ️ [FAILED] [PostgreSQL]  Interactions Between various operations: [9/10, 924ms]
 ️ 
 ️     - [FAILED] aggregates and distinct [63ms]
 ️         Reason: An unexpected dataflow error ((Incomparable_Values.Error Nothing Nothing)) has been matched (at test/Table_Tests/src/Common_Table_Operations/Integration_Tests.enso:70:13-72).
 ️         at <enso> Incomparable_Values.type.handle_errors.handle.case handle(built-distribution/enso-engine-2024.3.1-dev-linux-amd64/enso-2024.3.1-dev/lib/Standard/Base/2024.3.1-dev/src/Errors/Common.enso:278:47-101)
 ️         at <enso> Panic.catch(Internal)
 ️         at <enso> Incomparable_Values.type.handle_errors<arg-1>(built-distribution/enso-engine-2024.3.1-dev-linux-amd64/enso-2024.3.1-dev/lib/Standard/Base/2024.3.1-dev/src/Errors/Common.enso:281-282)
 ️         at <enso> Panic.catch(Internal)
 ️         at <enso> Incomparable_Values.type.handle_errors(built-distribution/enso-engine-2024.3.1-dev-linux-amd64/enso-2024.3.1-dev/lib/Standard/Base/2024.3.1-dev/src/Errors/Common.enso:281-282)
 ️         at <enso> Table.sort<arg-1>(built-distribution/enso-engine-2024.3.1-dev-linux-amd64/enso-2024.3.1-dev/lib/Standard/Table/2024.3.1-dev/src/Table.enso:916-917)

how do I run the PostgreSQL tests? Usage of docker is described here. I can start the docker and then as #10501 suggests:

enso$ ENSO_POSTGRES_DATABASE=postgres \
  ENSO_POSTGRES_PASSWORD=pwd \
  ENSO_POSTGRES_USER=postgres \
  ./built-distribution/enso-engine-*/enso-*/bin/enso --run test/Table_Tests/ aggregates.and.distinct
[FAILED] [PostgreSQL]  Interactions Between various operations: [0/1, 327ms]

    - [FAILED] aggregates and distinct [380ms]
        Reason: An unexpected dataflow error ((Incomparable_Values.Error Nothing Nothing)) has been matched (at /home/devel/NetBeansProjects/enso/enso/test/Table_Tests/src/Common_Table_Operations/Integration_Tests.enso:70:13-72).
        at <enso> Incomparable_Values.type.handle_errors.handle.case handle(/home/devel/NetBeansProjects/enso/enso/built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso:278:47-101)

I can see the failure locally.

[JaroslavTulach]

There is only a single remaining failure:

This failure has nothing to do with Nothing - it is just wrong error message - the culprit exception is from ObjectComparator:

          (v, u) -> {
            var result = compare_callback.execute(null, v, u);
            if (result.isNull()) {
              throw new CompareException(u, v);
            } else {

when comparing BigInteger(13) and BigInteger(5). The following program now returns [Nothing] while previously it was returning [-1]:

polyglot java import java.math.BigInteger

from Standard.Base import all
import Standard.Base.Internal.Ordering_Helpers.Default_Comparator

main =
  f = BigInteger.new "5"
  t = BigInteger.new "13"

  r = Default_Comparator.compare_callback f t
  [ r ]

Solved by a2c4def

[JaroslavTulach]

CI seems green, time for yet another round of benchmarks:

• engine
• stdlib

[Akirathan]

It is still not entirely clear to me why a custom comparator's compare method can throw Incomparable_Values. But I don't see any big problems with that. Looks good overall to me.

[JaroslavTulach]

It is still not entirely clear to me why a custom comparator's compare method can throw Incomparable_Values.

There are many places where comparator's compare delegates to Ordering.compare. Ordering.compare doesn't return Nothing, but yields an error. So either we relax the rules, or all the delegations will be complicated by recovering from the error.

I don't see any big problems with that.

Yes, relaxing the rules costs us nothing and the delegation is then very simple.

[JaroslavTulach]

• engine
• stdlib

Engine benchmarks are fine and my favorite stdlib benchmark looks way too optimistic:

One of the sorting benchmarks got speed up as well

Let's merge and see.