Multi value `Complex` test and robustness refactoring #11525

JaroslavTulach · 2024-11-11T10:32:45Z

Pull Request Description

While working on #11482 and enhancing the tests suite with more tests based on type Complex a getRootNode() did not terminate in 100000 iterations problem was discovered. Detailed investigation revealed that the existing ReadArgumentCheckNode infrastructure was able to create a cycle of parent pointers in the Truffle AST.

The problem was in intricate manipulation of the AST while rewriting internals in ReadArgumentCheckNode. This PR avoids such manipulation by refactoring the type checking code. ReadArgumentNode knows nothing about types anymore. When a type check is needed, IrToTruffle adds additional TypeCheckValueNode.wrap around the ReadArgumentNode - that breaks the vicious circle.

All the type checks nodes are moved to its own package. All but one of the classes are made package private. The external API for doing type checking is concentrated into TypeCheckValueNode.

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.
Benchmarks remain fine:
- engine
- stdlibs

JaroslavTulach · 2024-11-11T12:06:12Z

New 4387ccf tests are running and are green:

JaroslavTulach · 2024-11-12T04:43:17Z

Interesting failure in all engine tests:

java.lang.AssertionError: getRootNode() did not terminate in 100000 iterations.
 at <java> org.graalvm.truffle/com.oracle.truffle.api.nodes.Node.getRootNodeImpl(Node.java:604)
 at <java> org.graalvm.truffle/com.oracle.truffle.api.nodes.Node.getRootNode(Node.java:583)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.resolveLayer(PolyglotFastThreadLocals.java:433)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.resolveEngine(PolyglotFastThreadLocals.java:441)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.findLanguage(PolyglotFastThreadLocals.java:450)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.findContextClass(PolyglotFastThreadLocals.java:467)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals$ContextReferenceImpl.get(PolyglotFastThreadLocals.java:513)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoContext.get(EnsoContext.java:249)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.library.dispatch.TypeOfNode.doLong(TypeOfNode.java:55)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.library.dispatch.TypeOfNodeGen.execute(TypeOfNodeGen.java:161)
 at <java> org.enso.runtime/org.enso.interpreter.node.callable.argument.ReadArgumentCheckNode$TypeCheckNode.findType(ReadArgumentCheckNode.java:439)
 at <java> org.enso.runtime/org.enso.interpreter.node.callable.argument.ReadArgumentCheckNodeFactory$TypeCheckNodeGen.executeCheckOrConversion(ReadArgumentCheckNodeFactory.java:130)

JaroslavTulach · 2024-11-14T12:01:31Z

Interesting failure in all engine tests:

java.lang.AssertionError: getRootNode() did not terminate in 100000 iterations.
 at <java> org.graalvm.truffle/com.oracle.truffle.api.nodes.Node.getRootNodeImpl(Node.java:604)
 at <java> org.graalvm.truffle/com.oracle.truffle.api.nodes.Node.getRootNode(Node.java:583)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.resolveLayer(PolyglotFastThreadLocals.java:433)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.resolveEngine(PolyglotFastThreadLocals.java:441)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.findLanguage(PolyglotFastThreadLocals.java:450)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals.findContextClass(PolyglotFastThreadLocals.java:467)
 at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotFastThreadLocals$ContextReferenceImpl.get(PolyglotFastThreadLocals.java:513)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoContext.get(EnsoContext.java:249)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.library.dispatch.TypeOfNode.doLong(TypeOfNode.java:55)
 at <java> org.enso.runtime/org.enso.interpreter.runtime.library.dispatch.TypeOfNodeGen.execute(TypeOfNodeGen.java:161)
 at <java> org.enso.runtime/org.enso.interpreter.node.callable.argument.ReadArgumentCheckNode$TypeCheckNode.findType(ReadArgumentCheckNode.java:439)
 at <java> org.enso.runtime/org.enso.interpreter.node.callable.argument.ReadArgumentCheckNodeFactory$TypeCheckNodeGen.executeCheckOrConversion(ReadArgumentCheckNodeFactory.java:130)

The failure is caused by creating cycle in the Truffle AST! I guess it is time to rewrite ReadArgumentCheckNode and turn it into proper ExpressionNode.

JaroslavTulach · 2024-11-15T05:52:19Z

With 04d3835 we can finally try to address #11482 and specify the return type of the Complex.new method:

diff --git test/Base_Tests/src/Data/Complex.enso test/Base_Tests/src/Data/Complex.enso
index 9a0c98590b..41dd0c5567 100644
--- test/Base_Tests/src/Data/Complex.enso
+++ test/Base_Tests/src/Data/Complex.enso
@@ -10,7 +10,7 @@ import project.Data.Complex_Helpers.Complex_Comparator
 type Complex
     private Value re:Float im:Float
 
-    new re=0:Float im=0:Float =
+    new re=0:Float im=0:Float -> Complex =
         c = Complex.Value re im
         if im != 0 then c:Complex else
             c.as_complex_and_float

with this change the test/Base_Tests fail "the right way" - only then we can address the needs of #11482. However I leave such a work for another PR. Let's dedicate this PR to fixing the getRootNode() did not terminate in 100000 iterations problem.

…alueCasts11482

JaroslavTulach · 2024-11-15T13:56:26Z

test/Base_Tests/src/Data/Complex_Helpers.enso

+
+## explicit "conversion" of `Complex` to `Float` in a private module
+   used in `as_complex_and_float`
+Float.from (that:Complex) =


Info for library guys (@jdunkerley, @radeusgd, @GregoryTravis, etc.). You can implement values with mutiple roles even right now. Steps:

define a type Complex in one file

create another private helper module with explicit conversion

define extension method on Complex that creates the mixed in value - e.g. as_complex_and_float

when you want to "mix" multiple types into Complex - call such extension method

Your review is welcomed.

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/TypeCheckValueNode.java

Akirathan

I am not entirelly sure what is going on in the code. But it is definitely better than adding all type checking under ReadArgumentNode.Before integrating, please ensure that all benchmarks seems OK.

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

test/Base_Tests/src/Semantic/Multi_Value_Spec.enso

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/TypeCheckValueNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/LazyCheckRootNode.java

Akirathan · 2024-11-15T16:44:37Z

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/LazyCheckRootNode.java

+import org.enso.interpreter.runtime.callable.function.Function;
+import org.enso.interpreter.runtime.callable.function.FunctionSchema;
+
+final class LazyCheckRootNode extends RootNode {


Why this class does not extend EnsoRootNode? By extending it, we would have functionality like source section and associated scopes.

Probably because it represents internal function only:

Function wrapThunk(Function thunk) { return new Function(getCallTarget(), thunk.getScope(), SCHEMA, new Object[] {thunk}, null); }

In any case, I am not aware of any missing source sections or scopes.

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/AbstractTypeCheckNode.java

jdunkerley

Good to correct the typo @Akirathan spotted.

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/AbstractTypeCheckNode.java

radeusgd

Looks good

While working on #11482 and enhancing the tests suite with more tests based on `type Complex` a [getRootNode() did not terminate in 100000 iterations](#11525 (comment)) problem was discovered. Detailed investigation revealed that the existing `ReadArgumentCheckNode` infrastructure was able to create a **cycle** of parent pointers in the Truffle AST. The problem was in intricate manipulation of the AST while rewriting internals in `ReadArgumentCheckNode`. This PR avoids such manipulation by _refactoring the type checking code_. `ReadArgumentNode` knows nothing about types anymore. When a type check is needed, `IrToTruffle` adds additional `TypeCheckValueNode.wrap` around the `ReadArgumentNode` - that breaks the **vicious circle**. All the _type checks_ nodes are moved to its own package. All but one of the classes are made package private. The external API for doing _type checking_ is concentrated into `TypeCheckValueNode`.

JaroslavTulach added 2 commits November 11, 2024 11:16

Separating type Complex definition into its own module

c3852c7

Complex number with imaginary part cannot be converted to Float

4387ccf

JaroslavTulach self-assigned this Nov 11, 2024

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley and marthasharkey as code owners November 11, 2024 10:32

JaroslavTulach marked this pull request as draft November 11, 2024 10:32

Uses explicit conversion to create Complex&Float intersection type

8bd7fe3

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

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

Closed

JaroslavTulach added 5 commits November 14, 2024 15:39

Moving ReadArgumentCheckNode node to its own package

d341e18

Split ReadArgumentCheckNode inner classes into their own files

062d40f

Renaming to type check value node

089f120

Let the conversion always happen in TypeCheckExpressionNode

1999523

Propagate the ExpressionNode down for use in findConversionNode

04d3835

JaroslavTulach changed the title ~~Multi value casts tests and implementation changes~~ Multi value casts tests and robustness changes Nov 15, 2024

JaroslavTulach added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Nov 15, 2024

Internal AbstractTypeCheckNode. External API in TypeCheckValueNode.

33f88b8

JaroslavTulach changed the title ~~Multi value casts tests and robustness changes~~ Multi value Complex test and robustness refactoring Nov 15, 2024

JaroslavTulach marked this pull request as ready for review November 15, 2024 08:55

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners November 15, 2024 08:55

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

11ef168

…alueCasts11482

JaroslavTulach commented Nov 15, 2024

View reviewed changes

4e6 reviewed Nov 15, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/TypeCheckValueNode.java Show resolved Hide resolved

JaroslavTulach requested a review from 4e6 November 15, 2024 15:04

Akirathan approved these changes Nov 15, 2024

View reviewed changes

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

Java Values are displayed differently in the Chrome Devtools debugger #7890

Closed

jdunkerley approved these changes Nov 18, 2024

View reviewed changes

JaroslavTulach added 4 commits November 18, 2024 12:19

Removing unused method

f978643

Correcting a typo

dd43268

Documenting meaning of expr parameter

6539e48

Using var keyword

4bc57ef

radeusgd reviewed Nov 18, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/typecheck/AbstractTypeCheckNode.java Show resolved Hide resolved

radeusgd approved these changes Nov 18, 2024

View reviewed changes

JaroslavTulach added CI: Ready to merge This PR is eligible for automatic merge CI: Keep up to date Automatically update this PR to the latest develop. labels Nov 18, 2024

Merge branch 'develop' into wip/jtulach/MultiValueCasts11482

2635512

JaroslavTulach added CI: Keep up to date Automatically update this PR to the latest develop. and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Nov 19, 2024

mergify bot merged commit 3d8a0e1 into develop Nov 19, 2024
44 checks passed

mergify bot deleted the wip/jtulach/MultiValueCasts11482 branch November 19, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi value `Complex` test and robustness refactoring #11525

Multi value `Complex` test and robustness refactoring #11525

JaroslavTulach commented Nov 11, 2024 •

edited

Loading

JaroslavTulach commented Nov 11, 2024

JaroslavTulach commented Nov 12, 2024

JaroslavTulach commented Nov 14, 2024

JaroslavTulach commented Nov 15, 2024 •

edited

Loading

JaroslavTulach Nov 15, 2024

Akirathan left a comment

Akirathan Nov 15, 2024

JaroslavTulach Nov 18, 2024

jdunkerley left a comment

radeusgd left a comment

Multi value Complex test and robustness refactoring #11525

Multi value Complex test and robustness refactoring #11525

Conversation

JaroslavTulach commented Nov 11, 2024 • edited Loading

Pull Request Description

Checklist

JaroslavTulach commented Nov 11, 2024

JaroslavTulach commented Nov 12, 2024

JaroslavTulach commented Nov 14, 2024

JaroslavTulach commented Nov 15, 2024 • edited Loading

JaroslavTulach Nov 15, 2024

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Akirathan Nov 15, 2024

Choose a reason for hiding this comment

JaroslavTulach Nov 18, 2024

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Multi value `Complex` test and robustness refactoring #11525

Multi value `Complex` test and robustness refactoring #11525

JaroslavTulach commented Nov 11, 2024 •

edited

Loading

JaroslavTulach commented Nov 15, 2024 •

edited

Loading