Defer keyword argument deduplication until entire group is evaluated

Author: DSouzaM

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

@@ -378,6 +378,15 @@ def test_doesnt_modify_passed_dict():
     assert d1 == {'a': 1}
     assert d2 == {'b': 2}
 
+def test_keyword_validation_order_of_evaluation():
+    # The division by zero is evaluated before the duplicate argument is detected.
+    assert_call_raises(ZeroDivisionError, "f26(**{'a':1}, a=1/0)")
+    assert_call_raises(ZeroDivisionError, "f26(**{'p1':0}, p1=1, p2=2, p3=3, p4=4, p5=5, p6=6, p7=7, p8=8, p9=9, p10=10, p11=11, p12=12, p13=13, p14=14, p15=15/0)")
+    assert_call_raises(ZeroDivisionError, "f26(**{'p1':0}, p1=1, p2=2, p3=3, p4=4, p5=5, p6=6, p7=7, p8=8, p9=9, p10=10, p11=11, p12=12, p13=13, p14=14, p15=15, p16=16/0)")
+
+    # The duplicate argument is detected before the division by zero is evaluated.
+    assert_call_raises(TypeError, "f26(**{'a':1}, **{'a':2}, b=1/0)")
+
 fooo = f26
 def test_multiple_values_with_callable_name():
     def assert_get_message(err, fn, *args, **kwargs):

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/Compiler.java

@@ -696,7 +696,7 @@ public void flushStackIfNecessary() {
             }
         }
 
-        protected void doFlushStack() {
+        private void doFlushStack() {
             assert stackItems <= CollectionBits.KIND_MASK;
             if (collectionOnStack) {
                 addOp(COLLECTION_ADD_STACK, typeBits | stackItems);
@@ -729,6 +729,27 @@ public boolean isEmpty() {
     }
 
     private class KwargsMergingDictCollector extends Collector {
+        /*
+         * Keyword arguments get evaluated in "groups", where a group is a contiguous sequence of
+         * named keyword arguments or a keyword-splat. For example, in
+         *
+         * f(a=2, b=3, **kws, c=4, d=5, **more_kws)
+         *
+         * there are 4 groups. Each group is evaluated and only then merged with the existing
+         * keyword arguments. The merge step is where duplicate keyword arguments are checked.
+         *
+         * A call can have an arbitrarily long sequence of named keyword arguments, so we flush them
+         * from the stack and collect them into an intermediate dict. We cannot eagerly merge them
+         * with the existing keyword arguments since that could trigger a duplication check before
+         * all of the arguments in the group have been evaluated. For example, in
+         *
+         * f(**{'a': 2}, a=3, b=4, ..., z=function_with_side_effects())
+         *
+         * we cannot merge "a=3" with the existing keywords dict (and consequently raise a
+         * TypeError) until we compute "z".
+         */
+        private boolean namedKeywordDictOnStack = false;
+
         public KwargsMergingDictCollector(OpCodes callOp) {
             super(CollectionBits.KIND_DICT);
             /*
@@ -740,25 +761,66 @@ public KwargsMergingDictCollector(OpCodes callOp) {
         }
 
         @Override
-        protected void doFlushStack() {
-            addOp(COLLECTION_FROM_STACK, typeBits | stackItems);
+        public void appendItem() {
+            stackItems += stackItemsPerItem;
+            if (stackItems + stackItemsPerItem > CollectionBits.KIND_MASK) {
+                collectIntoNamedKeywordDict();
+            }
+        }
+
+        @Override
+        public void flushStackIfNecessary() {
+            if (stackItems == 0 && !namedKeywordDictOnStack) {
+                // Nothing pending to be merged.
+                return;
+            }
+
+            if (stackItems > 0) {
+                collectIntoNamedKeywordDict();
+            }
             if (collectionOnStack) {
+                // If there's already a collection on the stack, merge it with the dict we just
+                // finished creating.
                 addOp(KWARGS_DICT_MERGE);
             }
             collectionOnStack = true;
+            namedKeywordDictOnStack = false;
+        }
+
+        protected void collectIntoNamedKeywordDict() {
+            if (namedKeywordDictOnStack) {
+                // Just add the keywords to the existing dict. Since we statically check for
+                // duplicate named keywords, we don't need to worry about duplicates in this dict.
+                addOp(COLLECTION_ADD_STACK, typeBits | stackItems);
+            } else {
+                // Create a new dict.
+                addOp(COLLECTION_FROM_STACK, typeBits | stackItems);
+                namedKeywordDictOnStack = true;
+            }
             stackItems = 0;
         }
 
         @Override
         public void appendCollection() {
             assert stackItems == 0;
+            assert !namedKeywordDictOnStack;
             if (collectionOnStack) {
                 addOp(KWARGS_DICT_MERGE);
             } else {
                 addOp(COLLECTION_FROM_COLLECTION, typeBits);
             }
             collectionOnStack = true;
         }
+
+        @Override
+        public void finishCollection() {
+            flushStackIfNecessary();
+        }
+
+        @Override
+        public boolean isEmpty() {
+            return stackItems == 0 || !namedKeywordDictOnStack || !collectionOnStack;
+        }
     }
 
     private void collectIntoArray(ExprTy[] nodes, int bits, int alreadyOnStack) {