Graaljs: [interop] removeArrayElement not resizing arrays

[fniephaus]

If I understand removeArrayElement correctly, it should remove the element and resize the array. In GraalPython, it does the right thing. In Graal.js, however, the array is not resized. Also, the removed slot is no longer readable.

GraalSqueak repro:

((Polyglot eval: 'python' string: '[1, 3, 4]') primitiveRemoveArrayElement: 2) "ForeignObject[arraySize=2,memberSize=50]"

((Polyglot eval: 'js' string: '[1, 3, 4]') primitiveRemoveArrayElement: 2) "ForeignObject[arraySize=3]"

((Polyglot eval: 'js' string: '[1, 3, 4]') primitiveRemoveArrayElement: 2) at: 2. "FAILS (not readable)"

Tested with GraalVM CE 19.2.1 on macOS.

/cc @chumer

[wirthi]

Hi @fniephaus

We are probably just doing what JavaScript itself would do:

> var a = [0,1,2,3]
> a
(4)[0, 1, 2, 3]
> delete a[3]
true
> a
(4)[0, 1, 2, empty]
> a.length
4

The element is deleted, but the length is not changed.

The JavaDoc of removeArrayElement is not very explizit on that: Remove an array element from the receiver object - from a JS perspective, we have fulfilled that (the property with that index is gone). Also This method does not have not observable side-effects other than the removed array element (sic!) hints to not manipulating the length. But I know other languages would disagree.

@chumer do you think this should be unified? At least, we should mention what "removing" the object means (i.e., whether that strictly needs to make the array one element shorter).

-- Christian

[fniephaus]

In the current implementation, a deleted slot is no longer readable, which violates the array interop concept (must be readable from 0 to getArraySize if I understand correctly, otherwise they cannot be iterated correctly. Maybe @chumer can confirm this).

In Javascript, the following works fine:

> a[3]
undefined

This is also true for a[4] etc. So I'd say isMemberReadable might always return true in Javascript, but may return undefined (an interop null value) in the case the element was deleted or if the index is out of bounds.

The following example demonstrates how the current behavior affects interop with Python:

$ polyglot --shell --jvm
GraalVM MultiLanguage Shell 19.2.1
Copyright (c) 2013-2019, Oracle and/or its affiliates
  JavaScript version 19.2.1
  Python version 3.7.3
js> let x = [1, 2, 3]
js> Polyglot.export('x', x)
(3)[1, 2, 3]
js> delete x[1]
true
js> x
(3)[1, empty, 3]
js> python>
python> import polyglot
python> x = polyglot.import_value('x')
python> x
[1] # may be `[1, None, 3]`, certainly not just a list with `len(x) = 1`
python> len(x)
3 # This is correct!
python> x[2]
3 # works!
python> x[1]
IndexError: invalid index 1 # may return `None`
python> for i in x:
      +   print(i)
      +
1 # `None` and `3` are missing, lists are no longer iterated correctly
python>

[shoaniki]

This doesn't just seem to affect removeArrayElement - it also carries over if you use Value#as to cast the array to something like java.util.List.

There's no ambiguity in this case: the remove method on the List interface is documented as shifting subsequent elements to the left rather than leaving a hole, or if an implementation cannot do this, the documented alternative is to throw an UnsupportedOperationException. The current graal.js behavior violates this contract in a rather unexpected way.

[wirthi]

Hi,

@woess is currently working on this, a fix will be merged shortly. We will ensure that all delete operations on foreign objects actually shift the higher indices, instead of leaving a hole. This will also be clarified in the Javadoc for removeArrayElement.

Best,
Christian

[woess]

This issue should be fixed in master (will be released in GraalVM 20.1.0).
removeArrayElement now removes the element and shrinks the array, and holes in arrays are now readable and return undefined.

[fniephaus]

Thanks, https://github.com/graalvm/graaljs/commit/e45a671be53fe0210b1014bbb683c09ed5a2b48c seems to be the corresponding change. I will double check when the next dev build is out and close this if everything WAI.

[fniephaus]

I can confirm this is working now (tested with 20.1.0-dev-20200226_0309). Thanks!