Binary operator resolution based on `that` value #8779

JaroslavTulach · 2024-01-16T16:03:53Z

Pull Request Description

Fixes #8346 by making sure binary operations like +, -,/, *, % accept any type as second argument under the assumption that the left Number is convertible to the type of second that argument via a from conversion.

Checklist

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

The documentation has been updated in 234417a - read the paragraph here
All code follows the
Scala,
Java,
style guides
All code has been tested:
- Unit tests have been written where possible.
- Test ThatConversionNode when it runs out of specialization limit - done in 4f23530
- Support multi value conversions - failing test
- Check benchmark results, 2nd run.

JaroslavTulach · 2024-01-16T19:25:24Z

We have three failing tests:

That outlines what kind of Nodes we have to enhance to allow implementation of BigDecimal as a regular type.

…imalAndCo_8346

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java

…changes

JaroslavTulach · 2024-01-26T04:23:28Z

Need to solve f5 d failure. It is a recent regression. 0542f48 should do the trick.

…imalAndCo_8346

JaroslavTulach · 2024-01-26T10:35:48Z

234417a adds new paragraph to operators docs. The paragraph follows the section that states Enso only supports binary operators (enforced by #8789). The next sections talk about operator priorities - that's very likely untrue/outdated, but it is for another PR to fix that.

radeusgd

Looks great, I have a few questions / suggestions.

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

docs/syntax/functions.md

...ruments/src/main/java/org/enso/interpreter/test/instruments/tck/EnsoTckLanguageProvider.java

engine/runtime/src/main/java/org/enso/interpreter/node/BinaryOperatorNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/MethodRootNode.java

radeusgd · 2024-01-26T17:04:26Z

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

+                val operators = ".!$%&*+-/<>?^~\\"
+                def isOperator(n: Name): Boolean = {
+                  n.name
+                    .chars()
+                    .allMatch(operators.indexOf(_) >= 0)
+                }


Later I will need to be able to check isOperator also in the compiler at the type inference pass, so this should probably be somewhere reachable from both places.

But I'm happy to move it when the time comes, it seems OK to be here for now

I wasn't sure what is a right place for such method. But yeah, right now I'd rather integrate then seek for the right method name.

Another incarnation of this isOperator check is here CCing @kazcw and @4e6.

engine/runtime/src/test/java/org/enso/interpreter/test/ValuesGenerator.java

engine/runtime/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala

test/Base_Tests/src/Data/Numbers_Spec.enso

JaroslavTulach · 2024-01-26T18:47:34Z

Benchmarks ( benchmark results, 2nd run) seem to be fine in general. If there is a regression, let's solve it individually after integrating.

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

JaroslavTulach · 2024-01-27T06:35:13Z

Run #4 failed in LibrariesTest: https://github.com/enso-org/enso/actions/runs/7672566748/job/20924679342?pr=8779#step:10:15522 - e.g. #8806

GitHub
Binary operator resolution based on `that` value · enso-org/enso@0e01b29
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

JaroslavTulach · 2024-01-27T07:06:05Z

Run #5 failed on #8806

JaroslavTulach · 2024-01-27T07:38:32Z

Finally!

JaroslavTulach added 2 commits January 16, 2024 16:58

assert the returned value is Ordering or Nothing

3c4112f

Specs definition how Complex and Number shall interoperate

7407518

JaroslavTulach self-assigned this Jan 16, 2024

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, 4e6, hubertp and Akirathan as code owners January 16, 2024 16:03

GregoryTravis approved these changes Jan 16, 2024

View reviewed changes

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

Rich atoms don't cache suspended fields #8710

Closed

JaroslavTulach added 9 commits January 17, 2024 04:41

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

83d3ecb

…imalAndCo_8346

verifyOperationWithConvertibleObject test

80a28b8

AddNode passes the verifyOperationWithConvertibleObject test

bc66409

Pass VirtualFrame around

3ad5e40

Let all the Integer numeric binary operators delegate to Integer.doOther

a96ed7d

Handle node manipulations in interpreter mode

8e7d36a

Ordering with conversions

59d67a7

Wrap.Int self with Integer that

7b53460

More ordering and equality tests

aeaf9ea

JaroslavTulach mentioned this pull request Jan 17, 2024

Binary operator must have two arguments #8789

Merged

2 tasks

JaroslavTulach added 6 commits January 17, 2024 17:55

Making package private

19a29f4

Simplify by using InteropConversionCallNode

855d5b6

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

6fad8ef

…imalAndCo_8346

Avoid wrapping internal exceptions by PolyglotException

4447663

Testing UnresolvedConversion equality

5d89ef0

Module, ModuleScope are only equal when they are equal

56d0062

JaroslavTulach commented Jan 18, 2024

View reviewed changes

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java Show resolved Hide resolved

JaroslavTulach added 3 commits January 25, 2024 16:02

Testing EnsoMultiValue and conversions of operators

48116c9

Binary dispatch test with multivalue fixed by ReadaArgumentCheckNode …

8f66cb5

…changes

@specialization handling EnsoMultiValue

efbe6a0

JaroslavTulach added 6 commits January 26, 2024 06:18

Apply the check even for null Type[]

0542f48

Verifying doThatConversionUncached handles many different types

4f23530

Faster bench.compileOnly=true

1e9ef59

Import Numbers in the benchmarks

a131449

Documenting binary operator resolution based on that

234417a

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

080c977

…imalAndCo_8346

JaroslavTulach force-pushed the wip/jtulach/BigDecimalAndCo_8346 branch from ae09192 to 080c977 Compare January 26, 2024 10:28

JaroslavTulach added 2 commits January 26, 2024 11:36

javafmtAll

25af881

Merging with develop branch

a93e6d6

radeusgd approved these changes Jan 26, 2024

View reviewed changes

JaroslavTulach and others added 5 commits January 26, 2024 19:59

Type first, default value then

3dcf02a

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

Just return operatorNode

ffe265b

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

Internal Javadoc update

71e203b

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

Formatting after applying suggestions

c01085a

Don't swallow ArityException when -ea

0e01b29

JaroslavTulach merged commit 9a37357 into develop Jan 27, 2024
25 of 27 checks passed

JaroslavTulach deleted the wip/jtulach/BigDecimalAndCo_8346 branch January 27, 2024 07:38

Akirathan mentioned this pull request Mar 5, 2024

3x slower IfVsCaseBenchmarks_ifBench6In #9165

Open

JaroslavTulach mentioned this pull request Mar 12, 2024

Decimal type: constuctors, comparisons, and arithmetic #9272

Merged

5 tasks

JaroslavTulach mentioned this pull request Apr 18, 2024

Run ydoc-server with GraalVM #9528

Merged

5 tasks

JaroslavTulach mentioned this pull request Sep 18, 2024

SO when attempting to subtract from a Day value #11119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary operator resolution based on `that` value #8779

Binary operator resolution based on `that` value #8779

JaroslavTulach commented Jan 16, 2024 •

edited

Loading

JaroslavTulach commented Jan 16, 2024

JaroslavTulach commented Jan 26, 2024 •

edited

Loading

JaroslavTulach commented Jan 26, 2024

radeusgd left a comment

radeusgd Jan 26, 2024

JaroslavTulach Jan 26, 2024 •

edited

Loading

JaroslavTulach Apr 18, 2024 •

edited

Loading

JaroslavTulach commented Jan 26, 2024

JaroslavTulach commented Jan 27, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach commented Jan 27, 2024

JaroslavTulach commented Jan 27, 2024

Binary operator resolution based on that value #8779

Binary operator resolution based on that value #8779

Conversation

JaroslavTulach commented Jan 16, 2024 • edited Loading

Pull Request Description

Checklist

JaroslavTulach commented Jan 16, 2024

JaroslavTulach commented Jan 26, 2024 • edited Loading

JaroslavTulach commented Jan 26, 2024

radeusgd left a comment

Choose a reason for hiding this comment

radeusgd Jan 26, 2024

Choose a reason for hiding this comment

JaroslavTulach Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach commented Jan 26, 2024

JaroslavTulach commented Jan 27, 2024 • edited by unfurl-links bot Loading

JaroslavTulach commented Jan 27, 2024

JaroslavTulach commented Jan 27, 2024

Binary operator resolution based on `that` value #8779

Binary operator resolution based on `that` value #8779

JaroslavTulach commented Jan 16, 2024 •

edited

Loading

JaroslavTulach commented Jan 26, 2024 •

edited

Loading

JaroslavTulach Jan 26, 2024 •

edited

Loading

JaroslavTulach Apr 18, 2024 •

edited

Loading

JaroslavTulach commented Jan 27, 2024 •

edited by unfurl-links bot

Loading