Array methods #2137

rbotafogo · 2020-11-05T14:24:59Z

Working on issue #1844 and trying to make foreign arrays look like Ruby arrays. The following methods were implemented:

size;
at: returns the element at the given index. Should dynamically grow the array if the index is larger than max_size. Not doing this yet and raising an IndexError;
fetch: does exactly the same as at
first
last

graalvmbot · 2020-11-05T14:43:06Z

Hello Rodrigo Botafogo, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address rodrigo -(dot)- a -(dot)- botafogo -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

…, but this is printed on the terminal and not caught by minitest. Needs fixing.

eregon · 2020-11-06T13:21:18Z

Note that you can use jt format to automatically format most of the code.

eregon · 2020-11-06T13:44:05Z

Could you add tests in spec/truffle/interop/special_forms_spec.rb?
That in turn will automatically generate documentation at https://github.com/oracle/truffleruby/blob/master/doc/contributor/interop_implicit_api.md

rbotafogo · 2020-11-06T15:55:03Z

Note that you can use jt format to automatically format most of the code.

jt format fails with the following results. Is there a way to get more information on why it's failling?

$ /home/rbotafogo/desenv/truffleruby-ws/mx/mx --java-home /home/rbotafogo/.mx/cache/truffleruby/openjdk1.8.0_272-jvmci-20.3-b04 eclipseformat --no-backup --primary
we have: 2 batches
Processing batch 1 (36 files)...
Processing batch 2 (964 files)...
1 files were modified
 - src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java
Changes:
--- a/src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java
+++ b/src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java
@@ -116,9 +116,9 @@
             limit = "1")
     protected Object at(Object receiver, String name, Object[] args,
             @Cached(value = "name", allowUncached = true) @Shared("name") String cachedName,
-            @CachedLibrary("receiver") InteropLibrary interop,                        
+            @CachedLibrary("receiver") InteropLibrary interop,
             @Cached TranslateInteropExceptionNode translateInteropException,
-            @CachedContext(RubyLanguage.class) RubyContext context,                        
+            @CachedContext(RubyLanguage.class) RubyContext context,
             @Cached InteropNodes.ReadArrayElementNode atNode) {
         try {
             long size = interop.getArraySize(receiver);
@@ -137,7 +137,8 @@
         }
     }
 
-    @Specialization(guards = {
+    @Specialization(
+            guards = {
                     "name == cachedName",
                     "cachedName.equals(FETCH)",
                     "args.length == 1",
@@ -145,9 +146,9 @@
             limit = "1")
     protected Object fetch(Object receiver, String name, Object[] args,
             @Cached(value = "name", allowUncached = true) @Shared("name") String cachedName,
-            @CachedLibrary("receiver") InteropLibrary interop,                        
+            @CachedLibrary("receiver") InteropLibrary interop,
             @Cached TranslateInteropExceptionNode translateInteropException,
-            @CachedContext(RubyLanguage.class) RubyContext context,                        
+            @CachedContext(RubyLanguage.class) RubyContext context,
             @Cached InteropNodes.ReadArrayElementNode fetchNode) {
         try {
             long size = interop.getArraySize(receiver);
@@ -166,7 +167,8 @@
         }
     }
 
-    @Specialization(guards = {
+    @Specialization(
+            guards = {
                     "name == cachedName",
                     "cachedName.equals(FIRST)",
                     "args.length == 0" },
@@ -174,21 +176,22 @@
     protected Object first(Object receiver, String name, Object[] args,
             @Cached(value = "name", allowUncached = true) @Shared("name") String cachedName,
             @Cached InteropNodes.ReadArrayElementNode firstNode) {
-                return firstNode.execute(receiver, 0);
-    }
-
-    @Specialization(guards = {
+        return firstNode.execute(receiver, 0);
+    }
+
+    @Specialization(
+            guards = {
                     "name == cachedName",
                     "cachedName.equals(LAST)",
                     "args.length == 0" },
             limit = "1")
     protected Object last(Object receiver, String name, Object[] args,
             @Cached(value = "name", allowUncached = true) @Shared("name") String cachedName,
-            @CachedLibrary("receiver") InteropLibrary interop,                        
+            @CachedLibrary("receiver") InteropLibrary interop,
             @Cached TranslateInteropExceptionNode translateInteropException,
             @Cached InteropNodes.ReadArrayElementNode lastNode) {
         try {
-                return lastNode.execute(receiver, interop.getArraySize(receiver) - 1);
+            return lastNode.execute(receiver, interop.getArraySize(receiver) - 1);
         } catch (UnsupportedMessageException e) {
             throw translateInteropException.execute(e);
         }


FAILED (pid 2152 exit 1): /home/rbotafogo/desenv/truffleruby-ws/mx/mx --java-home /home/rbotafogo/.mx/cache/truffleruby/openjdk1.8.0_272-jvmci-20.3-b04 eclipseformat --no-backup --primary

eregon · 2020-11-06T18:14:20Z

@rbotafogo When it does, it will have modified the files as the diff it shows, so just committing these changes should be enough.
You can run jt format a second time to make sure.

rbotafogo · 2020-11-06T19:59:51Z

@eregon although foreign_array_spec specifies:

foreign.fetch(-5).should raise_error(IndexError)

and the IndexError is beign raised and printed when the test is executed, it is not actually caught, so the test actually fails.
Do you know what is going on and how to fix it?

Thanks

eregon · 2020-11-07T13:58:45Z

You need to wrap in a lambda for the raise_error matcher, see https://github.com/ruby/spec/blob/master/CONTRIBUTING.md#exception-matchers

rbotafogo · 2020-11-09T03:11:57Z

#eregon How do I return 'nil' from an interop call?

eregon · 2020-11-09T11:22:25Z

You can use Nil.INSTANCE, or if under a RubyBaseNode just nil.

rbotafogo · 2020-11-10T16:49:49Z

@eregon, ruby arrays can be indexed by a range or a starting index and size. In this case, the return value is another array. When indexing a foreign array with a range, should we return a RubyArray or another foreign array? My first instinct is to return a RubyArray, but I guess it might make more sense to stay in the foreign land. What do you think?

eregon · 2020-11-10T17:07:07Z

@rbotafogo I wouldn't handle indexing with a Range in this PR, that's a lot of extra complicated logic.
I think we should move most of the logic to Ruby code if we want to handle more fancy arguments and more methods, but that's for another time.

rbotafogo · 2020-11-10T17:14:01Z

@eregon OK, will move on to other methods. Still, other methods such as 'take' also return an array, so should this be a foreign array or a ruby array?

eregon · 2020-11-10T17:19:45Z

@rbotafogo I would keep it to just the methods in this PR, to not make it too big.
Could you sign the OCA? (#2137 (comment))

Many Array methods are also available through Enumerable, including take, so there is also no need to reimplement them.
But for that, we would first need to give a proper class for foreign arrays like ForeignArray < ForeinObject < Object and ForeignArray would include Enumerable. Then we could easily define the logic in Ruby and add the remaining methods, handle Ranges, etc.

Since we don't really know how to build a foreign Array, I think it would be a Ruby Array. It seems in Ruby 3 Array and String methods might stop returning subclasses and just always return a plain Array/String.

rbotafogo · 2020-11-10T18:43:51Z

@eregon OCA signed and mailed.

OK, understand you idea. But where should ForeignArray be created? Can you elaborate a bit? There is a ForeignObject class in TruffleDebugNodes.java, but this does not seem like what we want.

eregon · 2020-11-10T19:22:10Z

I wrote more about it in #2149

graalvmbot · 2020-11-13T17:43:06Z

Rodrigo Botafogo has signed the Oracle Contributor Agreement (based on email address rodrigo -(dot)- a -(dot)- botafogo -(at)- gmail -(dot)- com) so can contribute to this repository.

eregon · 2020-11-17T12:42:26Z

.gitignore

@@ -3,6 +3,9 @@

 # Editors
 *~
+*.#
+*/**/#*
+*/**/.#*


Could you undo changes here? They seem too broad.
You can use local excludes for this kind of editor-specific ignores: https://stackoverflow.com/questions/1753070/how-do-i-configure-git-to-ignore-some-files-locally

eregon · 2020-11-17T12:43:34Z

spec/truffle/interop/foreign_array_spec.rb

  it "can be printed with #print" do
-    foreign = Truffle::Interop.to_java_array([1, 2, 3])
+    @foreign


Dead code, and more cases below

eregon · 2020-11-17T12:44:44Z

spec/truffle/interop/foreign_array_spec.rb

+    @foreign.length.should == 3
+  end
+
+  it "can access elements by indexing #[]" do


by indexing +with+ (same below)

eregon · 2020-11-17T12:45:58Z

spec/truffle/interop/foreign_array_spec.rb

+    # foreign = Truffle::Interop.to_java_array([])
+    @foreign


Can be removed

eregon · 2020-11-17T12:47:28Z

spec/truffle/interop/special_forms_spec.rb

@@ -70,6 +70,34 @@
    pa.log.should include([:polyglot_read_array_element, 0])
  end

+  it description['.at(index)', :at, (:index)] do


The second argument should be the primary interop message that is used, in this case that's readArrayElement.
The third argument should be an Array like above.
Same below.

eregon · 2020-11-17T12:48:48Z

spec/truffle/interop/special_forms_spec.rb

+    l.log.should include(['getArraySize'])
+    pa.log.should include([:polyglot_array_size])


These should check for readArrayElement since that's the primary interop message used.
Same below of course.

eregon · 2020-11-17T12:51:47Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+            @Cached InteropNodes.ReadArrayElementNode readNode) {
+        try {
+            long size = interop.getArraySize(receiver);
+            long bound = (size != 0) ? size : 0;


This seems to be the same as bound = size, and so the bound variable seems redundant.

eregon · 2020-11-17T12:55:15Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+        try {
+            long size = interop.getArraySize(receiver);
+            long bound = (size != 0) ? size : 0;
+            if (((int) args[0]) < -bound || (int) args[0] >= bound) {


This cast will crash if args[0] is a long, a short or a byte.
Use LongCastNode to properly cast it to a long no matter what type it is.

eregon · 2020-11-17T12:57:21Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+                throw new RaiseException(
+                        context,
+                        context.getCoreExceptions().indexError("Index " + (int) args[0] +
+                                " outside of array bounds: -" + bound + "..." + bound, this));


Actually, InteropNodes.ReadArrayElementNode already raises an IndexError if the index is out of bounds, so I think we don't need different logic for fetch here.

But the exception message send by InteropNodes.ReadArrayElementNode is not the same as send by MRI in this case. Maybe we need to change the ReadArraElementNode exception message.

I think the exception message doesn't matter too much, I think having the same exception class is enough for most cases.

Just realized that the message has a bug, the same as in JRuby: if the array has 3 elements and we do
fetch(-5) the exception message is: "invalid array index -2".

eregon · 2020-11-17T12:58:37Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

@@ -296,6 +389,7 @@ protected static int expectedArity(String name) {
            case TO_A:
            case TO_ARY:
            case SIZE:
+            case LENGTH:


This and below need to include the other new methods too, for a proper error when given an incorrect number of arguments.

…n arrays.

eregon · 2020-11-20T11:18:11Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+            @CachedContext(RubyLanguage.class) RubyContext context,
+            @Cached InteropNodes.ReadArrayElementNode readNode) {
+        try {
+            long args0 = (LongCastNodeGen.create()).executeCastLong(args[0]);


The node should be created with @Cached, not on every call.

eregon · 2020-11-20T11:18:53Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+            @CachedContext(RubyLanguage.class) RubyContext context,
+            @Cached InteropNodes.ReadArrayElementNode readNode) {
+        try {
+            long args0 = (LongCastNodeGen.create()).executeCastLong(args[0]);


Can you give it a better name, like index?

eregon · 2020-11-20T11:22:35Z

src/main/java/org/truffleruby/interop/OutgoingForeignCallNode.java

+            }
+            return (args0 < 0)
+                    ? readNode.execute(receiver, size + args0)
+                    : readNode.execute(receiver, args0);


Take a look at how index normalization is done in org.truffleruby.core.array.ArrayNodes.IndexNode.
Both conditions should use a ConditionProfile, and if < 0; += size should be done first to simplify the logic.
Also, we should not have two calls to readNode.execute() but a single one.

eregon · 2020-11-20T11:24:42Z

spec/truffle/interop/special_forms_spec.rb

+    pa.log.should include([:polyglot_array_size])
+    pa.log.should include([:polyglot_has_array_elements?])
+    pa.log.should include([:polyglot_array_element_readable?, 0])
+    pa.log.should include([:polyglot_read_array_element, 0])


hasArrayElements/:polyglot_has_array_elements? is only used if assertions are enabled, we should not check for it here.
Same for polyglot_array_element_readable?.
The main messages here are readArrayElement & getArraySize, we should not check for other messages.

Same for other new methods.

eregon · 2020-11-20T11:25:25Z

spec/truffle/interop/special_forms_spec.rb

@@ -65,11 +65,57 @@

  it description['[index]', :readArrayElement, [:index]] do
    pfo, pa, l  = proxy[TruffleInteropSpecs::PolyglotArray.new]
-    -> { pfo[0] }.should raise_error(IndexError)
+    pfo[0] = 1
+    pfo[0]


pfo[0].should == 1 to make sure it's correct.

…_forms_spec

eregon · 2021-08-16T13:26:02Z

I'll close this in favor of the more general #2153

rbotafogo added 3 commits November 3, 2020 15:57

Add method length to foreign arrays

820b502

Add methods #at and #fetch to foreign arrays

b819b5d

Add methods #first and #last

b9e9225

eregon self-requested a review November 5, 2020 14:38

rbotafogo added 3 commits November 5, 2020 12:02

Skip minitest on must_raise issue. Method #fetch raises an IndexError…

f481711

…, but this is printed on the terminal and not caught by minitest. Needs fixing.

remove trailing white spaces flagged by lint

cdf15c1

Fix style after running jt checkstyle

e645bd2

rbotafogo added 3 commits November 6, 2020 15:38

Add test to special_forms_spec and foreign_array_spec

d36990c

Ignore all files with '.#' and '#*' that are automatic backups for emacs

df76aa4

remove trailling white spaces flaged by lint

58dec3a

rbotafogo added 2 commits November 9, 2020 21:08

Refactoring of methods 'at', 'fetch' and their specs

99a0a54

Refactor '[]' and add specs

d7d459d

graalvmbot added the oca-signed label Nov 13, 2020

eregon requested changes Nov 17, 2020

View reviewed changes

rbotafogo added 2 commits November 18, 2020 12:59

Include methods 'at', 'fetch', 'first' and 'last' for polyglot foreig…

a29b08b

…n arrays.

Remove unused import

9623a41

eregon reviewed Nov 20, 2020

View reviewed changes

rbotafogo added 4 commits November 23, 2020 14:10

add @cached longCastNode

90f53d8

Refactor OutgoingForeignCallNode and remove expectations from special…

c7eb077

…_forms_spec

Fix formatting

a5c57af

Replace @cached("create()") with @cached

f4cc6dd

rbotafogo requested a review from eregon November 24, 2020 20:39

eregon closed this Aug 16, 2021

		l.log.should include(['getArraySize'])
		pa.log.should include([:polyglot_array_size])

Array methods #2137

Array methods #2137

Conversation

rbotafogo commented Nov 5, 2020

graalvmbot commented Nov 5, 2020

eregon commented Nov 6, 2020

eregon commented Nov 6, 2020

rbotafogo commented Nov 6, 2020 • edited by eregon Loading

eregon commented Nov 6, 2020

rbotafogo commented Nov 6, 2020

eregon commented Nov 7, 2020

rbotafogo commented Nov 9, 2020

eregon commented Nov 9, 2020

rbotafogo commented Nov 10, 2020

eregon commented Nov 10, 2020

rbotafogo commented Nov 10, 2020

eregon commented Nov 10, 2020

rbotafogo commented Nov 10, 2020

eregon commented Nov 10, 2020

graalvmbot commented Nov 13, 2020

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

eregon commented Aug 16, 2021

rbotafogo commented Nov 6, 2020 •

edited by eregon

Loading