Merge pull request #41435 from Nadeeshan96/mas-fix-array-41428-41423

Fix incorrect behaviour with array `pop` and `setLength` functions
ballerina-platform · Oct 5, 2023 · 33f2455 · 33f2455
2 parents 3139a96 + da898dd
commit 33f2455
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 13 deletions.
diff --git a/...lerina-runtime/src/main/java/io/ballerina/runtime/internal/values/AbstractArrayValue.java b/...lerina-runtime/src/main/java/io/ballerina/runtime/internal/values/AbstractArrayValue.java
@@ -151,7 +151,7 @@ public void setLength(long length) {
         int newLength = (int) length;
         checkFixedLength(length);
         rangeCheck(length, size);
-        fillerValueCheck(newLength, size);
+        fillerValueCheck(newLength, size, newLength);
         resizeInternalArray(newLength);
         fillValues(newLength);
         size = newLength;
@@ -193,9 +193,9 @@ public Type getIteratorNextReturnType() {
      * helper methods that are visible to the implementation classes.
      */
 
-    protected abstract void fillValues(int newLength);
+    protected abstract void fillValues(int index);
 
-    protected abstract void fillerValueCheck(int newLength, int size2);
+    protected abstract void fillerValueCheck(int index, int size, int expectedLength);
 
     protected abstract void resizeInternalArray(int newLength);
 

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/ArrayValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/ArrayValueImpl.java
@@ -1105,15 +1105,15 @@ protected void rangeCheck(long index, int size) {
     }
 
     @Override
-    protected void fillerValueCheck(int index, int size) {
+    protected void fillerValueCheck(int index, int size, int expectedLength) {
         // if the elementType doesn't have an implicit initial value & if the insertion is not a consecutive append
         // to the array, then an exception will be thrown.
         if (arrayType.hasFillerValue()) {
             return;
         }
         if (index > size) {
             throw ErrorHelper.getRuntimeException(ErrorReasons.ILLEGAL_LIST_INSERTION_ERROR,
-                                                           ErrorCodes.ILLEGAL_ARRAY_INSERTION, size, index + 1);
+                                                           ErrorCodes.ILLEGAL_ARRAY_INSERTION, size, expectedLength);
         }
     }
 
@@ -1172,7 +1172,7 @@ private void prepareForAdd(long index, Object value, Type sourceType, int curren
 
         int intIndex = (int) index;
         rangeCheck(index, size);
-        fillerValueCheck(intIndex, size);
+        fillerValueCheck(intIndex, size, intIndex + 1);
         ensureCapacity(intIndex + 1, currentArraySize);
         fillValues(intIndex);
         resetSize(intIndex);

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TupleValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TupleValueImpl.java
@@ -671,9 +671,9 @@ protected void rangeCheck(long index, int size) {
     }
 
     @Override
-    protected void fillerValueCheck(int index, int size) {
+    protected void fillerValueCheck(int index, int size, int expectedLength) {
         // if there has been values added beyond the current index, that means filler values
-        // has already been checked. Therefore no need to check again.
+        // has already been checked. Therefore, no need to check again.
         if (this.size >= index) {
             return;
         }
@@ -682,7 +682,7 @@ protected void fillerValueCheck(int index, int size) {
         // to the array, then an exception will be thrown.
         if (!TypeChecker.hasFillerValue(this.tupleType.getRestType()) && (index > size)) {
             throw ErrorHelper.getRuntimeException(ErrorReasons.ILLEGAL_LIST_INSERTION_ERROR,
-                    ErrorCodes.ILLEGAL_TUPLE_INSERTION, size, index + 1);
+                    ErrorCodes.ILLEGAL_TUPLE_INSERTION, size, expectedLength);
         }
     }
 
@@ -759,7 +759,7 @@ private void prepareForAdd(long index, Object value, int currentArraySize) {
                                                          TypeChecker.getType(value)));
         }
 
-        fillerValueCheck(intIndex, size);
+        fillerValueCheck(intIndex, size, intIndex + 1);
         ensureCapacity(intIndex + 1, currentArraySize);
         fillValues(intIndex);
         resetSize(intIndex);

diff --git a/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/Pop.java b/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/Pop.java
@@ -18,10 +18,13 @@
 
 package org.ballerinalang.langlib.array;
 
+import io.ballerina.runtime.api.types.ArrayType;
+import io.ballerina.runtime.api.types.Type;
 import io.ballerina.runtime.api.utils.TypeUtils;
 import io.ballerina.runtime.api.values.BArray;
 
 import static org.ballerinalang.langlib.array.utils.ArrayUtils.checkIsArrayOnlyOperation;
+import static org.ballerinalang.langlib.array.utils.ArrayUtils.checkIsClosedArray;
 
 /**
  * Native implementation of lang.array:pop((any|error)[]).
@@ -39,7 +42,11 @@ public class Pop {
     private static final String FUNCTION_SIGNATURE = "pop()";
 
     public static Object pop(BArray arr) {
-        checkIsArrayOnlyOperation(TypeUtils.getImpliedType(arr.getType()), FUNCTION_SIGNATURE);
+        Type type = TypeUtils.getImpliedType(arr.getType());
+        checkIsArrayOnlyOperation(type, FUNCTION_SIGNATURE);
+        checkIsClosedArray((ArrayType) type, FUNCTION_SIGNATURE);
         return arr.shift(arr.size() - 1);
     }
+
+    private Pop() {}
 }
diff --git a/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/SetLength.java b/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/SetLength.java
@@ -35,4 +35,6 @@ public class SetLength {
     public static void setLength(BArray arr, long i) {
         arr.setLength(i);
     }
+
+    private SetLength() {}
 }
diff --git a/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/utils/ArrayUtils.java b/langlib/lang.array/src/main/java/org/ballerinalang/langlib/array/utils/ArrayUtils.java
@@ -86,6 +86,12 @@ public static void checkIsArrayOnlyOperation(Type arrType, String op) {
         }
     }
 
+    public static void checkIsClosedArray(ArrayType arrType, String op) {
+        if (arrType.getState() == ArrayType.ArrayState.CLOSED) {
+            throw createOpNotSupportedError(arrType, op);
+        }
+    }
+
     public static BError createOpNotSupportedError(Type type, String op) {
         return ErrorCreator.createError(getModulePrefixedReason(ARRAY_LANG_LIB,
                                                                 OPERATION_NOT_SUPPORTED_IDENTIFIER),

diff --git a/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibArrayTest.java b/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibArrayTest.java
@@ -600,7 +600,9 @@ public Object[] testFunctions() {
                 "testModificationWithinEvery",
                 "testArrSortWithNamedArgs1",
                 "testArrSortWithNamedArgs2",
-                "testArrSortWithNamedArgs3"
+                "testArrSortWithNamedArgs3",
+                "testArrayPop",
+                "testSetLengthNegative"
         };
     }
 }
diff --git a/langlib/langlib-test/src/test/resources/test-src/arraylib_test.bal b/langlib/langlib-test/src/test/resources/test-src/arraylib_test.bal
@@ -1788,3 +1788,47 @@ function testPushWithErrorConstructorExpr() {
     assertTrue(e is Error);
     assertValueEquality("e2", e.message());
 }
+
+function testArrayPop() {
+    int[] arr1 = [1, 2];
+    assertValueEquality(arr1.pop(), 2);
+    assertValueEquality(arr1, [1]);
+
+    int[2] arr2 = [1, 2];
+    int|error result = trap array:pop(arr2);
+    assertTrue(result is error);
+    if (result is error) {
+        assertValueEquality("{ballerina/lang.array}OperationNotSupported", result.message());
+        assertValueEquality("pop() not supported on type 'int[2]'",
+        <string> checkpanic result.detail()["message"]);
+    }
+
+    int[] arr = arr2;
+    result = trap arr.pop();
+    assertTrue(result is error);
+    if (result is error) {
+        assertValueEquality("{ballerina/lang.array}OperationNotSupported", result.message());
+        assertValueEquality("pop() not supported on type 'int[2]'",
+        <string> checkpanic result.detail()["message"]);
+    }
+}
+
+function testSetLengthNegative() {
+    string:Char[] arr = ["a","b"];
+    error? result = trap arr.setLength(5);
+    assertTrue(result is error);
+    if (result is error) {
+        assertValueEquality("{ballerina/lang.array}IllegalListInsertion", result.message());
+        assertValueEquality("array of length 2 cannot be expanded into array of length 5 without filler values",
+        <string> checkpanic result.detail()["message"]);
+    }
+
+    [string:Char...] tup = ["a","b"];
+    result = trap tup.setLength(10);
+    assertTrue(result is error);
+    if (result is error) {
+        assertValueEquality("{ballerina/lang.array}IllegalListInsertion", result.message());
+        assertValueEquality("tuple of length 2 cannot be expanded into tuple of length 10 without filler values",
+        <string> checkpanic result.detail()["message"]);
+    }
+}
diff --git a/...lerina-unit-test/src/test/java/org/ballerinalang/test/record/RecordAssignabilityTest.java b/...lerina-unit-test/src/test/java/org/ballerinalang/test/record/RecordAssignabilityTest.java
@@ -20,6 +20,8 @@
 import org.ballerinalang.test.BCompileUtil;
 import org.ballerinalang.test.BRunUtil;
 import org.ballerinalang.test.CompileResult;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
@@ -31,7 +33,12 @@
  */
 public class RecordAssignabilityTest {
 
-    private final CompileResult result = BCompileUtil.compile("test-src/record/record_assignability.bal");
+    private CompileResult result;
+
+    @BeforeClass
+    public void setup() {
+        result = BCompileUtil.compile("test-src/record/record_assignability.bal");
+    }
 
     @DataProvider
     public static Object[] recordAssignabilityTestFunctions() {
@@ -95,4 +102,9 @@ public void testRecordAssignabilityNegative() {
                 "found 'record {| readonly int? b; int...; |}'", 70, 33);
         assertEquals(negativeResult.getErrorCount(), i);
     }
+
+    @AfterClass
+    public void tearDown() {
+        result = null;
+    }
 }