Make ArrayOverBuffer behave like an Array/Array.sort no longer mutate…

…s the Array (#4022) Most of the problems with accessing `ArrayOverBuffer` have been resolved by using `CoerceArrayNode` (#3817). In `Array.sort` we still however specialized on Array which wasn't compatible with `ArrayOverBuffer`. Similarly sorting JS or Python arrays wouldn't work. Added a specialization to `Array.sort` to deal with that case. A generic specialization (with `hasArrayElements`) not only handles `ArrayOverBuffer` but also polyglot arrays coming from JS or Python. We could have an additional specialization for `ArrayOverBuffer` only (removed in the last commit) that returns `ArrayOverBuffer` rather than `Array` although that adds additional complexity which so far is unnecessary. Also fixed an example in `Array.enso` by providing a default argument.
enso-org · Jan 9, 2023 · ae0889e · ae0889e
1 parent 41b2aac
commit ae0889e
Show file tree

Hide file tree

Showing 11 changed files with 190 additions and 79 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -498,6 +498,8 @@
 - [IGV can jump to JMH sources & more][4008]
 - [Basic support of VSCode integration][4014]
 - [Sync language server with file system after VCS restore][4020]
+- [`ArrayOverBuffer` behaves like an `Array` and `Array.sort` no longer sorts in
+  place][4022]
 - [Introducing Meta.atom_with_hole][4023]
 - [Report failures in name resolution in type signatures][4030]
 
@@ -581,6 +583,7 @@
 [4008]: https://github.com/enso-org/enso/pull/4008
 [4014]: https://github.com/enso-org/enso/pull/4014
 [4020]: https://github.com/enso-org/enso/pull/4020
+[4022]: https://github.com/enso-org/enso/pull/4022
 [4023]: https://github.com/enso-org/enso/pull/4023
 [4030]: https://github.com/enso-org/enso/pull/4030
 

diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso
@@ -134,19 +134,20 @@ type Array
     length : Integer
     length self = @Builtin_Method "Array.length"
 
-    ## Sorts the this array in place.
+    ## Sorts the Array.
 
-       Arguments:
-       - comparator: A comparison function that takes two elements and returns
-         an Ordering that describes how the first element is ordered with
-         respect to the second.
+      Arguments:
+      - comparator: A comparison function that takes two elements and returns
+        an Ordering that describes how the first element is ordered with
+        respect to the second.
 
       > Example
-        Sorting an array of numbers.
+        Getting a sorted array of numbers.
 
-            [1,2,3].to_array.sort
-    sort : (Any -> Any -> Ordering) -> Nothing
-    sort self comparator = @Builtin_Method "Array.sort"
+            [3,2,1].to_array.sort
+    sort : (Any -> Any -> Ordering) -> Array
+    sort self comparator=(_.compare_to _) =
+        self.sort_builtin comparator
 
     ## Identity.
 

diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso
@@ -871,12 +871,6 @@ type Vector a
              [Pair 1 2, Pair -1 8].sort (_.first) (order = Sort_Direction.Descending)
     sort : (Any -> Any) -> (Any -> Any -> Ordering) -> Sort_Direction -> Vector Any ! Incomparable_Values
     sort self (on = x -> x) (by = (_.compare_to _)) (order = Sort_Direction.Ascending) =
-        ## Prepare the destination array that will underlie the vector. We do
-           not want to sort in place on the original vector, as `sort` is not
-           intended to be mutable.
-        new_vec_arr = Array.new self.length
-        Array.copy self.to_array 0 new_vec_arr 0 self.length
-
         ## As we want to account for both custom projections and custom
            comparisons we need to construct a comparator for internal use that
            does both.
@@ -886,7 +880,7 @@ type Vector a
             comp_descending
 
         Incomparable_Values.handle_errors <|
-            new_vec_arr.sort compare
+            new_vec_arr = self.to_array.sort compare
             Vector.from_polyglot_array new_vec_arr
 
     ## UNSTABLE

diff --git a/.../runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SortNode.java b/.../runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SortNode.java
@@ -2,24 +2,32 @@
 
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Fallback;
 import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.InvalidArrayIndexException;
+import com.oracle.truffle.api.interop.UnsupportedMessageException;
+import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.LoopNode;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.profiles.BranchProfile;
+
 import java.util.Arrays;
 import java.util.Comparator;
+
 import org.enso.interpreter.dsl.BuiltinMethod;
 import org.enso.interpreter.node.callable.dispatch.CallOptimiserNode;
 import org.enso.interpreter.node.callable.dispatch.SimpleCallOptimiserNode;
+import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode;
 import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.callable.atom.Atom;
 import org.enso.interpreter.runtime.callable.function.Function;
 import org.enso.interpreter.runtime.data.Array;
 import org.enso.interpreter.runtime.error.PanicException;
 import org.enso.interpreter.runtime.state.State;
 
-@BuiltinMethod(type = "Array", name = "sort", description = "Sorts a mutable array in place.")
+@BuiltinMethod(type = "Array", name = "sort_builtin", description = "Returns a sorted array.")
 public abstract class SortNode extends Node {
   private @Child CallOptimiserNode callOptimiserNode = SimpleCallOptimiserNode.build();
   private @Child InvalidComparisonNode invalidComparisonNode = InvalidComparisonNode.build();
@@ -32,15 +40,32 @@ static SortNode build() {
   }
 
   @Specialization
-  Object doSortFunction(State state, Array self, Function comparator) {
+  Object doArray(State state, Array self, Function comparator) {
     EnsoContext context = EnsoContext.get(this);
-    Comparator<Object> compare = getComparator(comparator, context, state);
-    return runSort(compare, self, context);
+    int size = self.getItems().length;
+    Object[] newArr = new Object[size];
+    System.arraycopy(self.getItems(), 0, newArr, 0, size);
+
+    return getComparatorAndSort(state, newArr, comparator, context);
   }
 
-  @Specialization
-  Object doAtomThis(State state, Atom self, Object that) {
-    return EnsoContext.get(this).getBuiltins().nothing();
+  @Specialization(guards = "arrays.hasArrayElements(self)")
+  Object doPolyglotArray(
+      State state,
+      Object self,
+      Function comparator,
+      @CachedLibrary(limit = "3") InteropLibrary arrays,
+      @Cached HostValueToEnsoNode hostValueToEnsoNode) {
+    try {
+      long size = arrays.getArraySize(self);
+      Object[] newArray = new Object[(int) size];
+      for (int i = 0; i < size; i++) {
+        newArray[i] = hostValueToEnsoNode.execute(arrays.readArrayElement(self, i));
+      }
+      return getComparatorAndSort(state, newArray, comparator, EnsoContext.get(this));
+    } catch (UnsupportedMessageException | InvalidArrayIndexException e) {
+      throw new IllegalStateException(e);
+    }
   }
 
   @Fallback
@@ -52,21 +77,24 @@ Object doOther(State state, Object self, Object comparator) {
         this);
   }
 
-  Object runSort(Comparator<Object> compare, Array self, EnsoContext context) {
-    doSort(self.getItems(), compare);
-    LoopNode.reportLoopCount(this, (int) self.length());
-    return context.getBuiltins().nothing();
+  private Object getComparatorAndSort(
+      State state, Object[] rawItems, Function comparator, EnsoContext context) {
+    Comparator<Object> compare = new SortComparator(comparator, context, this, state);
+    runSort(compare, rawItems);
+    return new Array(rawItems);
+  }
+
+  Object[] runSort(Comparator<Object> compare, Object[] self) {
+    doSort(self, compare);
+    LoopNode.reportLoopCount(this, self.length);
+    return self;
   }
 
   @TruffleBoundary
   void doSort(Object[] items, Comparator<Object> compare) {
     Arrays.sort(items, compare);
   }
 
-  private SortComparator getComparator(Function comp, EnsoContext context, State state) {
-    return new SortComparator(comp, context, this, state);
-  }
-
   private class SortComparator implements Comparator<Object> {
     private final Function compFn;
     private final EnsoContext context;

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArrayOverBuffer.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArrayOverBuffer.java
@@ -1,6 +1,8 @@
 package org.enso.interpreter.runtime.data;
 
+import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.InvalidArrayIndexException;
 import com.oracle.truffle.api.interop.TruffleObject;
 import com.oracle.truffle.api.library.ExportLibrary;
 import com.oracle.truffle.api.library.ExportMessage;
@@ -16,8 +18,12 @@ private ArrayOverBuffer(ByteBuffer buffer) {
   }
 
   @ExportMessage
-  Object readArrayElement(long index) {
-    return (long) buffer.get(buffer.position() + Math.toIntExact(index));
+  Object readArrayElement(long index) throws InvalidArrayIndexException {
+    try {
+      return (long) buffer.get(buffer.position() + Math.toIntExact(index));
+    } catch (IndexOutOfBoundsException e) {
+      throw InvalidArrayIndexException.create(index);
+    }
   }
 
   @ExportMessage
@@ -38,4 +44,10 @@ long getArraySize() {
   public static ArrayOverBuffer wrapBuffer(ByteBuffer buffer) {
     return new ArrayOverBuffer(buffer);
   }
+
+  @ExportMessage
+  String toDisplayString(boolean allowSideEffects) {
+    final InteropLibrary iop = InteropLibrary.getUncached();
+    return DisplayArrayUtils.toDisplayString(this, allowSideEffects, iop);
+  }
 }
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/DisplayArrayUtils.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/DisplayArrayUtils.java
@@ -0,0 +1,52 @@
+package org.enso.interpreter.runtime.data;
+
+import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.InvalidArrayIndexException;
+import com.oracle.truffle.api.interop.UnsupportedMessageException;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+
+public class DisplayArrayUtils {
+
+  @CompilerDirectives.TruffleBoundary
+  public static String toDisplayString(
+      Object arrayLike, boolean allowSideEffects, InteropLibrary iop) {
+    StringBuilder sb = new StringBuilder();
+    try {
+      sb.append('[');
+      String sep = "";
+      long len = iop.getArraySize(arrayLike);
+      for (long i = 0; i < len; i++) {
+        sb.append(sep);
+
+        Object at = iop.readArrayElement(arrayLike, i);
+        Object str = showObject(iop, allowSideEffects, at);
+        if (iop.isString(str)) {
+          sb.append(iop.asString(str));
+        } else {
+          sb.append("_");
+        }
+        sep = ", ";
+      }
+      sb.append(']');
+    } catch (InvalidArrayIndexException | UnsupportedMessageException ex) {
+      StringWriter w = new StringWriter();
+      ex.printStackTrace(new PrintWriter(w));
+      sb.append("...\n").append(w);
+    }
+    return sb.toString();
+  }
+
+  private static Object showObject(InteropLibrary iop, boolean allowSideEffects, Object child)
+      throws UnsupportedMessageException {
+    if (child == null) {
+      return "null";
+    } else if (child instanceof Boolean) {
+      return (boolean) child ? "True" : "False";
+    } else {
+      return iop.toDisplayString(child, allowSideEffects);
+    }
+  }
+}
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java
@@ -11,8 +11,6 @@
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.library.ExportLibrary;
 import com.oracle.truffle.api.library.ExportMessage;
-import java.io.PrintWriter;
-import java.io.StringWriter;
 
 import com.oracle.truffle.api.nodes.Node;
 import org.enso.interpreter.dsl.Builtin;
@@ -188,32 +186,7 @@ final void removeArrayElement(long index) throws UnsupportedMessageException {
   @CompilerDirectives.TruffleBoundary
   String toDisplayString(boolean allowSideEffects) {
     final InteropLibrary iop = InteropLibrary.getUncached();
-    StringBuilder sb = new StringBuilder();
-    try {
-      sb.append('[');
-      String sep = "";
-      long len = length(iop);
-      for (long i = 0; i < len; i++) {
-        sb.append(sep);
-
-        Object at =
-            readArrayElement(
-                i, iop, WarningsLibrary.getUncached(), HostValueToEnsoNode.getUncached());
-        Object str = showObject(iop, allowSideEffects, at);
-        if (iop.isString(str)) {
-          sb.append(iop.asString(str));
-        } else {
-          sb.append("_");
-        }
-        sep = ", ";
-      }
-      sb.append(']');
-    } catch (InvalidArrayIndexException | UnsupportedMessageException ex) {
-      StringWriter w = new StringWriter();
-      ex.printStackTrace(new PrintWriter(w));
-      sb.append("...\n").append(w.toString());
-    }
-    return sb.toString();
+    return DisplayArrayUtils.toDisplayString(this, allowSideEffects, iop);
   }
 
   @ExportMessage
@@ -271,16 +244,4 @@ public String toString() {
   private static <E extends Exception> E raise(Class<E> clazz, Throwable t) throws E {
     throw (E) t;
   }
-
-  @CompilerDirectives.TruffleBoundary
-  private Object showObject(InteropLibrary iop, boolean allowSideEffects, Object child)
-      throws UnsupportedMessageException {
-    if (child == null) {
-      return "null";
-    } else if (child instanceof Boolean) {
-      return (boolean) child ? "True" : "False";
-    } else {
-      return iop.toDisplayString(child, allowSideEffects);
-    }
-  }
 }
diff --git a/test/Tests/src/Data/Array_Spec.enso b/test/Tests/src/Data/Array_Spec.enso
@@ -43,4 +43,28 @@ spec =
             res = Array.new err
             res . should_fail_with Illegal_State.Error
 
+        Test.specify "should not sort in place" <|
+            arr = make_enso_array [3, 1, 2]
+            new_arr = arr.sort
+            arr . should_equal [3, 1, 2]
+            new_arr . should_equal [1, 2, 3]
+
+    Test.group "ArrayOverBuffer" <|
+        location_pending = case Platform.os of
+            Platform.OS.Windows -> "This test is disabled on Windows."
+            _ -> Nothing
+
+        Test.specify "should behave like an Array" pending=location_pending <|
+            array_over_buffer = (File.new (enso_project.data / "sample.txt") . read_last_bytes 10).to_array
+
+            case array_over_buffer of
+                _ : Array -> Nothing
+                _ -> Test.fail "Expected ArrayOverBuffer to match on Array type"
+
+            array_over_buffer.to_text . should_equal "[32, 106, 117, 106, 117, 98, 101, 115, 46, 10]"
+            sorted = array_over_buffer.sort
+            array_over_buffer.to_text . should_equal "[32, 106, 117, 106, 117, 98, 101, 115, 46, 10]"
+            sorted.to_text . should_equal "[10, 32, 46, 98, 101, 106, 106, 115, 117, 117]"
+
+
 main = Test_Suite.run_main spec
diff --git a/test/Tests/src/Semantic/Js_Interop_Spec.enso b/test/Tests/src/Semantic/Js_Interop_Spec.enso
@@ -47,7 +47,7 @@ foreign js make_array = """
     return [{ x: 10}, {x: 20}, {x: 30}];
 
 foreign js make_simple_array = """
-    return [10, 20, 30];
+    return [30, 10, 20];
 
 foreign js make_str str = """
     return "foo " + str + " bar"
@@ -100,7 +100,16 @@ spec = Test.group "Polyglot JS" <|
         vec = Vector.from_polyglot_array make_array
         vec.map .x . should_equal [10, 20, 30]
         vec2 = Vector.from_polyglot_array make_simple_array
-        vec2.to_array.at 0 . should_equal 10
+        vec2.to_array.at 0 . should_equal 30
+        arr = make_simple_array
+        new_arr = arr.sort
+        arr . should_equal [30, 10, 20]
+        new_arr . should_equal [10, 20, 30]
+
+        arr_2 = make_simple_array
+        sorted_2 = arr_2.sort
+        arr_2 . should_equal [30, 10, 20]
+        sorted_2 . should_equal [10, 20, 30]
 
     Test.specify "should correctly marshall strings" <|
         str = make_str "x" + " baz"