Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays #3628

hubertp · 2022-07-29T18:50:56Z

Pull Request Description

Use Proxy_Polyglot_Array as a proxy for polyglot arrays, thus unifying
the way the underlying array is accessed in Vector.

Used the opportunity to cleanup builtin lookup, which now actually
respects what is defined in the body of @Builtin_Method annotation.

Also discovered that polyglot null values (in JS, Python and R) were leaking to Enso.
Fixed that by doing explicit translation to Nothing.

https://www.pivotaltracker.com/story/show/181123986

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide dist and ./run ide watch.

jdunkerley

This is along the lines needed.

If we move the to_array to be a method the new storage type this allows us to make Slice a no-op :).

The bit missing is to review the current Vector.Vector calls and map them into the new polyglot functions.

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot/Proxy_Polyglot_Array.enso

JaroslavTulach

I'd like to see Array in Enso to be done properly. If this "solution" somehow helps libraries, feel free to go on, but I find this wrong from Engine perspective. Especially knowing that we don't handle nested arrays [[1,2], [3,4,5]].

CHANGELOG.md

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

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot/Proxy_Polyglot_Array.enso

hubertp · 2022-08-05T17:26:42Z

@jdunkerley I think I addressed all your concerns. This also revealed some bugs that I fixed on the way.
@JaroslavTulach As discussed, this only addresses concerns and use-cases mentioned in the ticket. Arrays, in general, will need some ❤️, as we discussed offline, and I will try to address that in a separate PR.

Also, while working on this ticket, I discovered that polyglot null values were leaking into Enso rather than being translated to Nothing. I added the necessary coercion for all polyglot languages and null check. I would appreciate your feedback if that is the right way to do it (see b145485) @JaroslavTulach @kustosz

JaroslavTulach

I am completely OK with the null & Nothing changes.

JaroslavTulach · 2022-08-05T19:55:41Z

...ne/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java

@@ -64,6 +64,11 @@ long doInteger(Object integer, @CachedLibrary(limit = "5") InteropLibrary number
    }
  }

+  @Specialization(guards = "interop.isNull(value)")
+  Object doNull(Object value, @CachedLibrary(limit = "5") InteropLibrary interop) {
+    return null;


Are we using Java null to represent Enso Nothing? Truffle languages in general try to avoid usage of null as it requires additional checks in the partial evaluation mode and the whole interpreter code doesn't optimize so nicely. Following the traditional approach, we should have Nothing.NULL truffle object. CCing @kustosz

Yes, null is not ideal but I had to use something that Epb and Enso know.

Decided to get rid of this "to null" and "from null" conversion and added a separate conversion node for the results of foreign calls.

.../java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java

JaroslavTulach · 2022-08-05T20:01:32Z

.../java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java

+      Object elem = coercion.execute(library.readArrayElement(array, index));
+      if (elem == null) {
+        Builtins builtins = Context.get(this).getBuiltins();
+        return builtins.nothing().newInstance();


I hope newInstances returns a singleton!

Yes, see cachedInstance field

JaroslavTulach · 2022-08-05T20:03:08Z

test/Tests/src/Data/Vector_Spec.enso

@@ -49,7 +49,7 @@ spec = Test.group "Vectors" <|
        built_from_js = Vector.from_polyglot_array generate_js_array
        built_from_js . should_equal [1, 2, 3, 4, 5]
        built_from_py = Vector.from_polyglot_array generate_py_array
-        built_from_py . should_equal [1, 2, 3, 4, 5]
+        built_from_py . should_equal [1, 2, 3, 4, Nothing]


Yup, I believe this is desirable. Foreign nulls are equal to Enso Nothing. Btw. this is not what Graal.js does. If you:

var value = foreignCall(); if (value === null) print('==='); if (value == null) print("==");

then only the second condition may be true! In Graal.js foreign nulls are not === with JavaScript null, but are only == with it. I've been burned by this feature so many times - I suggest in Enso Nothing is equal with any foreign Nothing/null.

radeusgd · 2022-08-10T15:30:33Z

test/Tests/src/Data/Vector_Spec.enso

+foreign python generate_py_array = """
+    return [1, 2, 3, 4, None]
+
+foreign python generate_nested_py_array = """
+    return [[1, 2, 3], [4, 5]]


I think I'd also like to see a test where these two situations are joined - a nested array containing a null - so that we can correctly verify that not only surface level nulls are converted but also ones deep into the nesting.

That's unlikely to work atm - see the pending test I added. It also demonstrates that it wouldn't work even before this PR.
In order to support it, we need a bigger rewrite of polyglot arrays which is not part of this PR.

Oh, I missed that it is pending. That is concerning - is the fix for that planned for the near future?

Well, I'd suggest adding this mixed test as pending too - to ensure that, once we do the rewrite, stuff is handled correctly.

That is concerning

Yes, I also find this concerning.

radeusgd · 2022-08-11T12:46:19Z

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot/Proxy_Polyglot_Array.enso

+    to_array self =
+        len = self.length
+        a = Array.new len
+        0.up_to len . each ix-> a.set_at ix (self.at ix)


This seems very inefficient.

Any reason why you cannot replace it with Array.copy self.arr 0 a 0 len?

Also I'm a bit worried about the fact that we would always copy here. Cannot we somehow wrap the polyglot array while avoiding copying? At least for cases where we can guarantee immutability as we were discussing.

It is pretty much as inefficient as it was before, really. I would defer optimizations until we actually decide to rewrite Array, and we seem to be coming to that point very soon :)

What are you comparing to? Before we didn't have a way to convert a polyglot array to Enso, to_array would just return it - which was a zero-cost operation.

to (now gone) from_array

Ah, ok, right.

I was thinking of the Vector.Vector polyglot_array approach which we used most of the time.

radeusgd · 2022-08-11T12:57:27Z

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot/Proxy_Polyglot_Array.enso

+    at : Number -> Any
+    at self index =
+        Polyglot.read_array_element self.arr index
+
+    to_array : Array Any
+    to_array self =
+        len = self.length
+        a = Array.new len
+        0.up_to len . each ix-> a.set_at ix (self.at ix)


Also one observation regarding that - we do the copy only upon a to_array call - so this gives us the right type, but this does not solve the mutability issue - if the underlying polyglot array is mutable, the calls to wrapped_array.at will not be referentially transparent as the underlying array may change in-between them.

I'm not saying that is an issue, because I assume that this PR is not aiming to solve the mutability issue, but I just wanted to ensure that we are on the same page there.

Yeap, we are on the same page.

.../java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java

JaroslavTulach

If there is an overall consensus this PR shall be integrated before work on Interop for Array & Vector starts, then let's do it.

My minority opinion continues to suggest to focus on Interop for Array & Vector which solves these array interop issues properly. But I don't want to stay in the way.

Engine approval is needed to get this PR off the table. Approving.

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

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

.../java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java

hubertp · 2022-08-22T08:48:50Z

If there is an overall consensus this PR shall be integrated before work on Interop for Array & Vector starts, then let's do it.

My minority opinion continues to suggest to focus on Interop for Array & Vector which solves these array interop issues properly. But I don't want to stay in the way.

Yeap, improved Array/Vector is next in line

Use Proxy_Polyglot_Array as a proxy for polyglot arrays, thus unifying the way the underlying array is accessed in Vector. Used the opportunity to cleanup builtin lookup, which now actually respects what is defined in the body of @Builtin_Method annotation.

Vector now has a `storage` field, which will delegate to Proxy_Polyglot_Array. Identified what appears to be all invalid `Vector.Vector` constructs when the underlying storage was a polyglot array.

Fixed a bug in our handling of polyglot values by 1) doing coercion of primitive values on all languages 2) explicitly checking for null values 3) whenever foreign calls return null we translate that to Nothing

This way we avoid explicit null checking which is admittedly not the nicest way of dealing with Nothing conversion.

One last request to avoid copying Arrays when the underlying storage is already an Array.

radeusgd · 2022-08-24T12:02:22Z

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

+        arr = self.storage.to_array
+        case arr of
+            Array ->
+                arr
+            _     ->
+                len = self.storage.length
+                a = Array.new len
+                Array.copy arr 0 a 0 len
+                a


Did you test this code?

My experience tells me that polyglot arrays also used to match the Array case in pattern matching, so I'm not really sure if it will do what you want - it will most likely always enter the Array branch. Any array-object that is compatible with Array.copy will also match the Array branch, so the second branch is essentially dead code, unless I'm missing something.

See the following code:

from Standard.Base import all polyglot java import java.util.List main = listarr = List.of 1 2 3 IO.println listarr m = Meta.meta listarr IO.println m IO.println m.get_language IO.println (Meta.get_simple_type_name listarr) IO.println (Meta.get_qualified_type_name listarr) case listarr of Array -> IO.println "Matches Array!" _ -> IO.println "nope"

it actually prints

[1, 2, 3] (Polyglot [1, 2, 3]) Java java.util.ImmutableCollections$ListN null Matches Array!

Also, I think the to_array method should have some doc comment, if at least ## PRIVATE.

(Sorry for commenting after merge, but I did not notice these earlier)

This was affecting my Array.set_at removal tests, so I fixed it in 29c4abc (part of #3634).

hubertp changed the title ~~Use Vector.from_polyglot_array, drop from_array~~ Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays Jul 29, 2022

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from 50b2521 to a5a3a2c Compare August 1, 2022 09:27

hubertp marked this pull request as ready for review August 1, 2022 09:27

hubertp requested review from 4e6, jdunkerley, radeusgd and JaroslavTulach as code owners August 1, 2022 09:27

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from a5a3a2c to ec0c08f Compare August 1, 2022 09:29

hubertp marked this pull request as draft August 1, 2022 10:26

hubertp marked this pull request as ready for review August 1, 2022 13:08

jdunkerley requested changes Aug 1, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot/Proxy_Polyglot_Array.enso Show resolved Hide resolved

JaroslavTulach reviewed Aug 2, 2022

View reviewed changes

JaroslavTulach requested a review from kustosz August 2, 2022 17:15

kustosz approved these changes Aug 3, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch 2 times, most recently from e593166 to b145485 Compare August 5, 2022 17:21

hubertp requested a review from jdunkerley August 5, 2022 17:22

hubertp requested a review from JaroslavTulach August 5, 2022 17:30

JaroslavTulach reviewed Aug 5, 2022

View reviewed changes

radeusgd reviewed Aug 10, 2022

View reviewed changes

radeusgd reviewed Aug 11, 2022

View reviewed changes

radeusgd mentioned this pull request Aug 11, 2022

Remove Array.set_at #3634

Merged

4 tasks

radeusgd reviewed Aug 11, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from 0711f55 to 6c956cf Compare August 12, 2022 10:43

jdunkerley reviewed Aug 15, 2022

View reviewed changes

.../java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java Outdated Show resolved Hide resolved

jdunkerley approved these changes Aug 15, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from 6c956cf to 0fd1217 Compare August 19, 2022 15:54

JaroslavTulach approved these changes Aug 20, 2022

View reviewed changes

hubertp added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Aug 22, 2022

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from 23d1194 to 7e97b19 Compare August 23, 2022 07:49

hubertp added 11 commits August 23, 2022 15:27

Formatting, make use of the new builtins feature

11490d0

changelog

2c53308

Use Polyglot read_array_element for at, docs

bcc2492

Address PR review, handle Vector.Vector for polyglot

81b3540

Vector now has a `storage` field, which will delegate to Proxy_Polyglot_Array. Identified what appears to be all invalid `Vector.Vector` constructs when the underlying storage was a polyglot array.

Don't leak polyglot null values to Enso

be14f6b

Fixed a bug in our handling of polyglot values by 1) doing coercion of primitive values on all languages 2) explicitly checking for null values 3) whenever foreign calls return null we translate that to Nothing

Use a separate node for null coercion

e444e04

This way we avoid explicit null checking which is admittedly not the nicest way of dealing with Nothing conversion.

PR review

c7b0a3f

Avoid copying Arrays

b811fda

One last request to avoid copying Arrays when the underlying storage is already an Array.

Make python test in Vector_Spec optional

e881324

Missed two spots where we use polyglot arrays

b22edb1

hubertp force-pushed the wip/hubert/polyglot-arrays-181123986 branch from 5e34d39 to b22edb1 Compare August 23, 2022 13:27

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Aug 23, 2022

mergify bot and others added 3 commits August 23, 2022 13:57

Merge branch 'develop' into wip/hubert/polyglot-arrays-181123986

018f8ec

More missing conversions discovered by new changes

fa41b70

I think that's the last one

43a5dae

mergify bot merged commit 4b9c916 into develop Aug 23, 2022

mergify bot deleted the wip/hubert/polyglot-arrays-181123986 branch August 23, 2022 21:13

radeusgd reviewed Aug 24, 2022

View reviewed changes

wdanilo mentioned this pull request Feb 6, 2023

Truffle Interop Types: Array & Vectors #4542

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays #3628

Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays #3628

hubertp commented Jul 29, 2022 •

edited

Loading

jdunkerley left a comment

JaroslavTulach left a comment

hubertp commented Aug 5, 2022 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Aug 5, 2022

hubertp Aug 8, 2022

hubertp Aug 8, 2022

JaroslavTulach Aug 5, 2022

hubertp Aug 8, 2022

JaroslavTulach Aug 5, 2022

radeusgd Aug 10, 2022

hubertp Aug 10, 2022

radeusgd Aug 10, 2022

JaroslavTulach Aug 11, 2022

radeusgd Aug 11, 2022

radeusgd Aug 11, 2022

hubertp Aug 12, 2022

radeusgd Aug 12, 2022

hubertp Aug 12, 2022

radeusgd Aug 12, 2022

radeusgd Aug 11, 2022

hubertp Aug 12, 2022

JaroslavTulach left a comment

hubertp commented Aug 22, 2022

radeusgd Aug 24, 2022

radeusgd Aug 24, 2022

radeusgd Aug 24, 2022

radeusgd Aug 24, 2022

Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays #3628

Use Vector.from_polyglot_array to make Vectors backed by polyglot arrays #3628

Conversation

hubertp commented Jul 29, 2022 • edited Loading

Pull Request Description

Checklist

jdunkerley left a comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

hubertp commented Aug 5, 2022 • edited Loading

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

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

hubertp commented Aug 22, 2022

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

hubertp commented Jul 29, 2022 •

edited

Loading

hubertp commented Aug 5, 2022 •

edited

Loading