[GR-70214] Bytecode DSL: fix bug in BinarySubscript, initialize empty lists to empty storage.

PullRequest: graalpython/4036

Author: steve-s

graalpython/com.oracle.graal.python.test/src/tests/test_sequence_strategies.py

@@ -0,0 +1,109 @@
+# Copyright (c) 2020, 2025, Oracle and/or its affiliates. All rights reserved.
+# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+#
+# The Universal Permissive License (UPL), Version 1.0
+#
+# Subject to the condition set forth below, permission is hereby granted to any
+# person obtaining a copy of this software, associated documentation and/or
+# data (collectively the "Software"), free of charge and under any and all
+# copyright rights in the Software, and any and all patent rights owned or
+# freely licensable by each licensor hereunder covering either (i) the
+# unmodified Software as contributed to or provided by such licensor, or (ii)
+# the Larger Works (as defined below), to deal in both
+#
+# (a) the Software, and
+#
+# (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
+# one is included with the Software each a "Larger Work" to which the Software
+# is contributed by such licensors),
+#
+# without restriction, including without limitation the rights to copy, create
+# derivative works of, display, perform, and distribute the Software and make,
+# use, sell, offer for sale, import, export, have made, and have sold the
+# Software and the Larger Work(s), and to sublicense the foregoing rights on
+# either these or other terms.
+#
+# This license is subject to the following condition:
+#
+# The above copyright notice and either this complete permission notice or at a
+# minimum a reference to the UPL must be included in all copies or substantial
+# portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+import sys
+
+
+if sys.implementation.name == "graalpy":
+    if not hasattr(__graalpython__, 'get_storage_strategy'):
+        raise SystemError("This test must run with --python.EnableDebuggingBuiltins=true")
+
+    def check_strategy(container, expected):
+        actual = __graalpython__.get_storage_strategy(container)
+        assert actual == expected, f"Container '{container}', wrong storage strategy : {actual=} != {expected=}"
+
+    LITERAL_INT_STORAGE = "NativeIntSequenceStorage" if __graalpython__.using_native_primitive_storage_strategy else "IntSequenceStorage"
+else:
+    # For CPython, just to verify other test results
+    def check_strategy(container, expected):
+        pass
+
+    LITERAL_INT_STORAGE = "dummy"
+
+
+def test_appending_ints():
+    l = list()
+    for i in range(10):
+        l.append(i)
+    assert l[5] == 5
+    check_strategy(l, "IntSequenceStorage")
+
+
+def test_appending_doubles():
+    l = list()
+    for i in range(10):
+        l.append(i * 0.1)
+    assert l[5] == 0.5
+    check_strategy(l, "DoubleSequenceStorage")
+
+
+def test_generator_int():
+    l = [x for x in range(10)]
+    assert l[5] == 5
+    check_strategy(l, "IntSequenceStorage")
+
+
+def test_generator_double():
+    l = [x * 0.1 for x in range(10)]
+    assert l[5] == 0.5
+    check_strategy(l, "DoubleSequenceStorage")
+
+
+# GR-70364
+# def test_generator_bool():
+#     l = [x >= 5 for x in range(10)]
+#     assert l[5] == True
+#     check_strategy(l, "BoolSequenceStorage")
+
+
+def test_literal_int():
+    l = [1,2,3,4,5]
+    assert l[4] == 5
+    check_strategy(l, LITERAL_INT_STORAGE)
+
+
+def test_literal_double():
+    l = [1.1,2.2,3.3,4.4,0.5]
+    assert l[4] == 0.5
+    check_strategy(l, "DoubleSequenceStorage")
+
+
+def test_literal_mixed():
+    l = [1,2,3,4,0.5]
+    assert l[4] == 0.5
+    check_strategy(l, "ObjectSequenceStorage")

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/GraalPythonModuleBuiltins.java

@@ -270,6 +270,7 @@ public void postInitialize(Python3Core core) {
             mod.setAttribute(tsLiteral("dump_truffle_ast"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("tdebug"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("set_storage_strategy"), PNone.NO_VALUE);
+            mod.setAttribute(tsLiteral("get_storage_strategy"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("storage_to_native"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("dump_heap"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("is_native_object"), PNone.NO_VALUE);
@@ -278,6 +279,8 @@ public void postInitialize(Python3Core core) {
             mod.setAttribute(tsLiteral("clear_interop_type_registry"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("foreign_number_list"), PNone.NO_VALUE);
             mod.setAttribute(tsLiteral("foreign_wrapper"), PNone.NO_VALUE);
+        } else {
+            addBuiltinConstant("using_native_primitive_storage_strategy", context.getLanguage().getEngineOption(PythonOptions.UseNativePrimitiveStorageStrategy));
         }
         if (PythonImageBuildOptions.WITHOUT_PLATFORM_ACCESS || !context.getOption(PythonOptions.RunViaLauncher)) {
             mod.setAttribute(tsLiteral("list_files"), PNone.NO_VALUE);
@@ -693,7 +696,7 @@ static long doIt(@SuppressWarnings("unused") Object dummy) {
         }
     }
 
-// Internal builtin used for testing: changes strategy of newly allocated set or map
+    // Internal builtin used for testing: changes strategy of newly allocated set or map
     @Builtin(name = "set_storage_strategy", minNumOfPositionalArgs = 2)
     @GenerateNodeFactory
     public abstract static class SetStorageStrategyNode extends PythonBinaryBuiltinNode {
@@ -734,6 +737,16 @@ private void validate(HashingStorage dictStorage) {
         }
     }
 
+    @Builtin(name = "get_storage_strategy", minNumOfPositionalArgs = 1)
+    @GenerateNodeFactory
+    public abstract static class GetStorageStrategyNode extends PythonUnaryBuiltinNode {
+        @Specialization
+        @TruffleBoundary
+        TruffleString doSet(PSequence seq) {
+            return PythonUtils.toTruffleStringUncached(seq.getSequenceStorage().getClass().getSimpleName());
+        }
+    }
+
     @Builtin(name = "storage_to_native", minNumOfPositionalArgs = 1)
     @GenerateNodeFactory
     abstract static class StorageToNative extends PythonUnaryBuiltinNode {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/IndexNodes.java

@@ -44,8 +44,10 @@
 import com.oracle.graal.python.builtins.objects.common.IndexNodesFactory.NormalizeIndexWithBoundsCheckNodeGen;
 import com.oracle.graal.python.builtins.objects.common.IndexNodesFactory.NormalizeIndexWithoutBoundsCheckNodeGen;
 import com.oracle.graal.python.builtins.objects.ints.PInt;
+import com.oracle.graal.python.builtins.objects.list.PList;
 import com.oracle.graal.python.nodes.ErrorMessages;
 import com.oracle.graal.python.nodes.PRaiseNode;
+import com.oracle.graal.python.runtime.sequence.PTupleListBase;
 import com.oracle.graal.python.util.OverflowException;
 import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
 import com.oracle.truffle.api.dsl.Bind;
@@ -159,6 +161,16 @@ public static NormalizeIndexWithBoundsCheckNode getUncached() {
             return NormalizeIndexWithBoundsCheckNodeGen.getUncached();
         }
 
+        public static int normalizeIntIndex(Node inliningTarget, int index, int length, PTupleListBase container,
+                        InlinedConditionProfile negativeIndexProfile, PRaiseNode raiseNode) {
+            int normalizedIndex = index;
+            if (negativeIndexProfile.profile(inliningTarget, normalizedIndex < 0)) {
+                normalizedIndex += length;
+            }
+            checkBounds(inliningTarget, raiseNode, container, normalizedIndex, length);
+            return normalizedIndex;
+        }
+
         @Specialization
         static int doInt(int index, int length, TruffleString errorMessage,
                         @Bind Node inliningTarget,
@@ -322,6 +334,12 @@ static long doLongLong(long index, long length, @SuppressWarnings("unused") Truf
         }
     }
 
+    public static void checkBounds(Node inliningTarget, PRaiseNode raiseNode, PTupleListBase container, int idx, int length) {
+        if (idx < 0 || idx >= length) {
+            raiseIndexError(inliningTarget, container, raiseNode);
+        }
+    }
+
     public static void checkBounds(Node inliningTarget, PRaiseNode raiseNode, TruffleString errorMessage, int idx, int length) {
         if (idx < 0 || idx >= length) {
             raiseIndexError(inliningTarget, errorMessage, raiseNode);
@@ -340,6 +358,12 @@ public static void checkBounds(Node inliningTarget, PRaiseNode raiseNode, Truffl
         }
     }
 
+    @InliningCutoff
+    private static void raiseIndexError(Node inliningTarget, PTupleListBase container, PRaiseNode raiseNode) {
+        TruffleString errorMessage = container instanceof PList ? ErrorMessages.LIST_INDEX_OUT_OF_RANGE : ErrorMessages.TUPLE_OUT_OF_BOUNDS;
+        raiseIndexError(inliningTarget, errorMessage, raiseNode);
+    }
+
     @InliningCutoff
     private static void raiseIndexError(Node inliningTarget, TruffleString errorMessage, PRaiseNode raiseNode) {
         throw raiseNode.raise(inliningTarget, PythonBuiltinClassType.IndexError, errorMessage);

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/list/PList.java

@@ -34,7 +34,7 @@
 import com.oracle.graal.python.nodes.interop.PForeignToPTypeNode;
 import com.oracle.graal.python.runtime.GilNode;
 import com.oracle.graal.python.runtime.exception.PException;
-import com.oracle.graal.python.runtime.sequence.PSequenceWithStorage;
+import com.oracle.graal.python.runtime.sequence.PTupleListBase;
 import com.oracle.graal.python.runtime.sequence.storage.ArrayBasedSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
 import com.oracle.graal.python.util.OverflowException;
@@ -57,7 +57,7 @@
 
 @SuppressWarnings("truffle-abstract-export")
 @ExportLibrary(InteropLibrary.class)
-public final class PList extends PSequenceWithStorage {
+public final class PList extends PTupleListBase {
     private final ListOrigin origin;
 
     public PList(Object cls, Shape instanceShape, SequenceStorage store) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/tuple/PTuple.java

@@ -27,7 +27,7 @@
 
 import static com.oracle.graal.python.util.PythonUtils.builtinClassToType;
 
-import com.oracle.graal.python.runtime.sequence.PSequenceWithStorage;
+import com.oracle.graal.python.runtime.sequence.PTupleListBase;
 import com.oracle.graal.python.runtime.sequence.storage.ObjectSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
 import com.oracle.truffle.api.CompilerAsserts;
@@ -40,7 +40,7 @@
 
 @SuppressWarnings("truffle-abstract-export")
 @ExportLibrary(InteropLibrary.class)
-public final class PTuple extends PSequenceWithStorage {
+public final class PTuple extends PTupleListBase {
 
     private long hash = -1;
 

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java

@@ -217,9 +217,10 @@
 import com.oracle.graal.python.runtime.exception.PythonErrorType;
 import com.oracle.graal.python.runtime.object.PFactory;
 import com.oracle.graal.python.runtime.sequence.PSequence;
-import com.oracle.graal.python.runtime.sequence.PSequenceWithStorage;
+import com.oracle.graal.python.runtime.sequence.PTupleListBase;
 import com.oracle.graal.python.runtime.sequence.storage.BoolSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.DoubleSequenceStorage;
+import com.oracle.graal.python.runtime.sequence.storage.EmptySequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.IntSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.LongSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.ObjectSequenceStorage;
@@ -1636,7 +1637,16 @@ public static Object perform(VirtualFrame frame,
 
     @Operation(storeBytecodeIndex = false)
     public static final class MakeList {
-        @Specialization
+        @Specialization(guards = "elements.length == 0")
+        public static PList doEmpty(@Variadic Object[] elements,
+                        @Bind PBytecodeDSLRootNode rootNode) {
+            // Common pattern is to create an empty list and then add items.
+            // We need to start from empty storage, so that we can specialize to, say, int storage
+            // if only ints are appended to this list
+            return PFactory.createList(rootNode.getLanguage(), EmptySequenceStorage.INSTANCE);
+        }
+
+        @Specialization(guards = "elements.length > 0")
         public static PList perform(@Variadic Object[] elements,
                         @Bind PBytecodeDSLRootNode rootNode) {
             return PFactory.createList(rootNode.getLanguage(), elements);
@@ -3209,45 +3219,51 @@ public static boolean doObject(long typeFlags, Object value,
     @Operation(storeBytecodeIndex = true)
     @ImportStatic(PGuards.class)
     public static final class BinarySubscript {
-        static boolean isBuiltinListOrTuple(PSequenceWithStorage s) {
-            return PGuards.isBuiltinTuple(s) && PGuards.isBuiltinList(s);
+        static boolean isBuiltinListOrTuple(PTupleListBase s) {
+            return PGuards.isBuiltinTuple(s) || PGuards.isBuiltinList(s);
         }
 
-        public static boolean isIntBuiltinListOrTuple(PSequenceWithStorage s) {
+        public static boolean isIntBuiltinListOrTuple(PTupleListBase s) {
             return isBuiltinListOrTuple(s) && s.getSequenceStorage() instanceof IntSequenceStorage;
         }
 
         @Specialization(guards = "isIntBuiltinListOrTuple(list)")
-        public static int doIntStorage(PSequenceWithStorage list, int index,
-                        @Shared @Cached NormalizeIndexWithBoundsCheckNode normalizeIndexNode) {
+        public static int doIntStorage(PTupleListBase list, int index,
+                        @Bind Node inliningTarget,
+                        @Shared @Cached InlinedConditionProfile negativeIndexProfile,
+                        @Shared @Cached PRaiseNode raiseNode) {
             IntSequenceStorage storage = (IntSequenceStorage) list.getSequenceStorage();
-            int normalizedIndex = normalizeIndexNode.execute(index, storage.length(), ErrorMessages.LIST_INDEX_OUT_OF_RANGE);
+            int normalizedIndex = NormalizeIndexWithBoundsCheckNode.normalizeIntIndex(inliningTarget, index, storage.length(), list, negativeIndexProfile, raiseNode);
             return storage.getIntItemNormalized(normalizedIndex);
         }
 
-        public static boolean isDoubleBuiltinListOrTuple(PSequenceWithStorage s) {
+        public static boolean isDoubleBuiltinListOrTuple(PTupleListBase s) {
             return isBuiltinListOrTuple(s) && s.getSequenceStorage() instanceof DoubleSequenceStorage;
         }
 
         @Specialization(guards = "isDoubleBuiltinListOrTuple(list)")
-        public static double doDoubleStorage(PSequenceWithStorage list, int index,
-                        @Shared @Cached NormalizeIndexWithBoundsCheckNode normalizeIndexNode) {
+        public static double doDoubleStorage(PTupleListBase list, int index,
+                        @Bind Node inliningTarget,
+                        @Shared @Cached InlinedConditionProfile negativeIndexProfile,
+                        @Shared @Cached PRaiseNode raiseNode) {
             DoubleSequenceStorage storage = (DoubleSequenceStorage) list.getSequenceStorage();
-            int normalizedIndex = normalizeIndexNode.execute(index, storage.length(), ErrorMessages.LIST_INDEX_OUT_OF_RANGE);
+            int normalizedIndex = NormalizeIndexWithBoundsCheckNode.normalizeIntIndex(inliningTarget, index, storage.length(), list, negativeIndexProfile, raiseNode);
             return storage.getDoubleItemNormalized(normalizedIndex);
         }
 
         @Specialization(replaces = {"doIntStorage", "doDoubleStorage"}, guards = "isBuiltinListOrTuple(list)")
-        public static Object doObjectStorage(PSequenceWithStorage list, int index,
+        public static Object doGenericStorage(PTupleListBase list, int index,
                         @Bind Node inliningTarget,
-                        @Shared @Cached NormalizeIndexWithBoundsCheckNode normalizeIndexNode,
+                        @Exclusive @Cached InlinedConditionProfile negativeIndexProfile,
+                        @Exclusive @Cached PRaiseNode raiseNode,
                         @Cached GetItemScalarNode getItemScalarNode) {
             SequenceStorage storage = list.getSequenceStorage();
-            int normalizedIndex = normalizeIndexNode.execute(index, storage.length(), ErrorMessages.LIST_INDEX_OUT_OF_RANGE);
+            int normalizedIndex = NormalizeIndexWithBoundsCheckNode.normalizeIntIndex(inliningTarget, index, storage.length(), list, negativeIndexProfile, raiseNode);
             return getItemScalarNode.execute(inliningTarget, storage, normalizedIndex);
         }
 
         @Fallback
+        @InliningCutoff
         public static Object doOther(VirtualFrame frame, Object receiver, Object key,
                         @Bind Node inliningTarget,
                         @Cached GetObjectSlotsNode getSlotsNode,

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/sequence/PSequenceWithStorage.java renamed to graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/sequence/PTupleListBase.java

@@ -44,10 +44,10 @@
 import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
 import com.oracle.truffle.api.object.Shape;
 
-public abstract class PSequenceWithStorage extends PSequence {
+public abstract class PTupleListBase extends PSequence {
     protected SequenceStorage store;
 
-    public PSequenceWithStorage(Object cls, Shape instanceShape) {
+    public PTupleListBase(Object cls, Shape instanceShape) {
         super(cls, instanceShape);
     }