From 107fd5d3008b1f9d89970b15d10e66250bfc93bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Wa=C5=9Bko?= Date: Fri, 21 Jan 2022 16:52:14 +0100 Subject: [PATCH] Fix filter and Array.at and set_at to correctly handle errors --- .../Base/0.2.32-SNAPSHOT/src/Data/Vector.enso | 17 +++++++++++------ .../expression/builtin/mutable/GetAtNode.java | 10 +++++++++- .../expression/builtin/mutable/SetAtNode.java | 12 ++++++++++-- engine/runtime/src/main/resources/Builtins.enso | 8 ++++++-- test/Tests/src/Data/Array_Spec.enso | 16 ++++++++++++++++ test/Tests/src/Data/Vector_Spec.enso | 1 + 6 files changed, 53 insertions(+), 11 deletions(-) diff --git a/distribution/lib/Standard/Base/0.2.32-SNAPSHOT/src/Data/Vector.enso b/distribution/lib/Standard/Base/0.2.32-SNAPSHOT/src/Data/Vector.enso index 493976ce1ee57..f0fe610dd2bb0 100644 --- a/distribution/lib/Standard/Base/0.2.32-SNAPSHOT/src/Data/Vector.enso +++ b/distribution/lib/Standard/Base/0.2.32-SNAPSHOT/src/Data/Vector.enso @@ -140,7 +140,7 @@ type Vector Get the last element of a vector. [1, 2, 3].at -1 == 3 - at : Number -> Any ! Index_Out_Of_Bounds_Error + at : Integer -> Any ! Index_Out_Of_Bounds_Error at index = actual_index = if index < 0 then this.length + index else index if actual_index>=0 && actual_index Any + unsafe_at : Integer -> Any unsafe_at index = this.to_array.at index @@ -325,9 +325,14 @@ type Vector filter : (Any -> Boolean) -> Vector Any filter predicate = builder = here.new_builder - this.each elem-> - if predicate elem then builder.append elem - builder.to_vector + acc = this.fold Nothing acc-> elem-> + ## The accumulator is threaded through to preserve dataflow + dependencies which are cut by using the stateful builder. This to + ensure correct dataflow error propagation. + case acc of + _ -> if predicate elem then builder.append elem + case acc of + _ -> builder.to_vector ## Applies a function to each element of the vector, returning the vector of results. @@ -526,7 +531,7 @@ type Vector Join the elements of the vector together as a string. ["foo", "bar", "baz"].join ", " - join : String -> Text + join : Text -> Text join separator = if this.length == 0 then "" else if this.length == 1 then this.unsafe_at 0 else diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/GetAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/GetAtNode.java index 99553c6265c7d..8bf3df9c8af09 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/GetAtNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/GetAtNode.java @@ -1,8 +1,11 @@ package org.enso.interpreter.node.expression.builtin.mutable; import com.oracle.truffle.api.nodes.Node; +import org.enso.interpreter.Language; import org.enso.interpreter.dsl.BuiltinMethod; +import org.enso.interpreter.runtime.builtin.Builtins; import org.enso.interpreter.runtime.data.Array; +import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod( type = "Array", @@ -11,6 +14,11 @@ public class GetAtNode extends Node { Object execute(Array _this, long index) { - return _this.getItems()[(int) index]; + try { + return _this.getItems()[(int) index]; + } catch (IndexOutOfBoundsException exception) { + Builtins builtins = lookupContextReference(Language.class).get().getBuiltins(); + throw new PanicException(builtins.error().makeInvalidArrayIndexError(_this, index), this); + } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetAtNode.java index 6ce7682671153..10aeeaaefb266 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetAtNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetAtNode.java @@ -1,9 +1,12 @@ package org.enso.interpreter.node.expression.builtin.mutable; import com.oracle.truffle.api.nodes.Node; +import org.enso.interpreter.Language; import org.enso.interpreter.dsl.AcceptsError; import org.enso.interpreter.dsl.BuiltinMethod; +import org.enso.interpreter.runtime.builtin.Builtins; import org.enso.interpreter.runtime.data.Array; +import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod( type = "Array", @@ -12,7 +15,12 @@ public class SetAtNode extends Node { Object execute(Array _this, long index, @AcceptsError Object value) { - _this.getItems()[(int) index] = value; - return _this; + try { + _this.getItems()[(int) index] = value; + return _this; + } catch (IndexOutOfBoundsException exception) { + Builtins builtins = lookupContextReference(Language.class).get().getBuiltins(); + throw new PanicException(builtins.error().makeInvalidArrayIndexError(_this, index), this); + } } } diff --git a/engine/runtime/src/main/resources/Builtins.enso b/engine/runtime/src/main/resources/Builtins.enso index a6c491c77ffde..2503e9e9c7f78 100644 --- a/engine/runtime/src/main/resources/Builtins.enso +++ b/engine/runtime/src/main/resources/Builtins.enso @@ -874,6 +874,10 @@ type Array Arguments: - index: The index to get the element from. + ? Safety + If index < 0 or index >= this.length, then this operation will result + in an Invalid_Array_Index_Error exception. + > Example Get the element at index 1. @@ -892,8 +896,8 @@ type Array programming style in Enso. ? Safety - If index >= this.length, then this operation will result in an - Invalid_Array_Index_Error exception. + If index < 0 or index >= this.length, then this operation will result + in an Invalid_Array_Index_Error exception. set_at : Integer -> Any -> Array set_at index value = @Builtin_Method "Array.set_at" diff --git a/test/Tests/src/Data/Array_Spec.enso b/test/Tests/src/Data/Array_Spec.enso index 5c468d3684878..f288fa1016e5d 100644 --- a/test/Tests/src/Data/Array_Spec.enso +++ b/test/Tests/src/Data/Array_Spec.enso @@ -10,3 +10,19 @@ spec = Test.group "Arrays" <| as_vec = json.into (Vector.Vector Number) as_vec.should_equal <| Vector.fill 100 0 + Test.specify "should allow accessing elements" + arr = [1, 2, 3] . to_array + arr.at 0 . should_equal 1 + arr.at 2 . should_equal 3 + + Test.specify "should allow setting elements" + arr = [1, 2, 3] . to_array + arr.set_at 1 10 + arr.at 1 . should_equal 10 + Vector.from_array arr . should_equal [1, 10, 3] + + Test.specify "should panic on out of bounds access" + arr = [1, 2, 3] . to_array + Test.expect_panic_with (arr.at -1) Invalid_Array_Index_Error + Test.expect_panic_with (arr.at 3) Invalid_Array_Index_Error + Test.expect_panic_with (arr.at 3 100) Invalid_Array_Index_Error diff --git a/test/Tests/src/Data/Vector_Spec.enso b/test/Tests/src/Data/Vector_Spec.enso index c724f70de221f..416b7f75c0600 100644 --- a/test/Tests/src/Data/Vector_Spec.enso +++ b/test/Tests/src/Data/Vector_Spec.enso @@ -90,6 +90,7 @@ spec = Test.group "Vectors" <| vec.filter (ix -> ix > 3) . should_equal [4, 5] vec.filter (ix -> ix == 1) . should_equal [1] vec.filter (ix -> ix < 0) . should_equal [] + vec.filter (ix -> if ix == 2 then Error.throw <| My_Error "foo" else True) . should_fail_with My_Error Test.specify "should allow mapping an operation, returning a new vector" <| vec = [1, 2, 3, 4]