bpo-39320: Handle unpacking of **values in compiler (GH-18141)

* Add DICT_UPDATE and DICT_MERGE bytecodes. Use them for ** unpacking.

* Remove BUILD_MAP_UNPACK and BUILD_MAP_UNPACK_WITH_CALL, as they are now unused.

* Update magic number for ** unpacking opcodes.

* Update dis.rst to incorporate new bytecodes.

* Add blurb entry.

Doc/library/dis.rst

@@ -873,32 +873,25 @@ All of the following opcodes use their arguments.
    .. versionadded:: 3.9
 
 
-.. opcode:: SET_UPDATE
+.. opcode:: SET_UPDATE (i)
 
    Calls ``set.update(TOS1[-i], TOS)``.  Used to build sets.
 
    .. versionadded:: 3.9
 
 
-.. opcode:: BUILD_MAP_UNPACK (count)
+.. opcode:: DICT_UPDATE (i)
 
-   Pops *count* mappings from the stack, merges them into a single dictionary,
-   and pushes the result.  Implements dictionary unpacking in dictionary
-   displays ``{**x, **y, **z}``.
+   Calls ``dict.update(TOS1[-i], TOS)``.  Used to build dicts.
 
-   .. versionadded:: 3.5
+   .. versionadded:: 3.9
 
 
-.. opcode:: BUILD_MAP_UNPACK_WITH_CALL (count)
+.. opcode:: DICT_MERGE
 
-   This is similar to :opcode:`BUILD_MAP_UNPACK`,
-   but is used for ``f(**x, **y, **z)`` call syntax.  The stack item at
-   position ``count + 2`` should be the corresponding callable ``f``.
+    Like :opcode:`DICT_UPDATE` but raises an exception for duplicate keys.
 
-   .. versionadded:: 3.5
-   .. versionchanged:: 3.6
-      The position of the callable is determined by adding 2 to the opcode
-      argument instead of encoding it in the second byte of the argument.
+   .. versionadded:: 3.9
 
 
 .. opcode:: LOAD_ATTR (namei)

Include/opcode.h

@@ -117,8 +117,6 @@ extern "C" {
 #define SET_ADD                 146
 #define MAP_ADD                 147
 #define LOAD_CLASSDEREF         148
-#define BUILD_MAP_UNPACK        150
-#define BUILD_MAP_UNPACK_WITH_CALL 151
 #define SETUP_ASYNC_WITH        154
 #define FORMAT_VALUE            155
 #define BUILD_CONST_KEY_MAP     156
@@ -127,6 +125,8 @@ extern "C" {
 #define CALL_METHOD             161
 #define LIST_EXTEND             162
 #define SET_UPDATE              163
+#define DICT_MERGE              164
+#define DICT_UPDATE             165
 
 /* EXCEPT_HANDLER is a special, implicit block type which is created when
    entering an except handler. It is not an opcode but we define it here

Lib/importlib/_bootstrap_external.py

@@ -276,6 +276,7 @@ _code_type = type(_write_atomic.__code__)
 #     Python 3.9a0  3422 (remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY, POP_FINALLY bytecodes #33387)
 #     Python 3.9a2  3423 (add IS_OP, CONTAINS_OP and JUMP_IF_NOT_EXC_MATCH bytecodes #39156)
 #     Python 3.9a2  3424 (simplify bytecodes for *value unpacking)
+#     Python 3.9a2  3425 (simplify bytecodes for **value unpacking)
 
 #
 # MAGIC must change whenever the bytecode emitted by the compiler may no
@@ -285,7 +286,7 @@ _code_type = type(_write_atomic.__code__)
 # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
 # in PC/launcher.c must also be updated.
 
-MAGIC_NUMBER = (3424).to_bytes(2, 'little') + b'\r\n'
+MAGIC_NUMBER = (3425).to_bytes(2, 'little') + b'\r\n'
 _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little')  # For import.c
 
 _PYCACHE = '__pycache__'

Lib/opcode.py

@@ -200,9 +200,6 @@ hasfree.append(148)
 def_op('EXTENDED_ARG', 144)
 EXTENDED_ARG = 144
 
-def_op('BUILD_MAP_UNPACK', 150)
-def_op('BUILD_MAP_UNPACK_WITH_CALL', 151)
-
 jrel_op('SETUP_ASYNC_WITH', 154)
 
 def_op('FORMAT_VALUE', 155)
@@ -214,5 +211,7 @@ def_op('CALL_METHOD', 161)
 
 def_op('LIST_EXTEND', 162)
 def_op('SET_UPDATE', 163)
+def_op('DICT_MERGE', 164)
+def_op('DICT_UPDATE', 165)
 
 del def_op, name_op, jrel_op, jabs_op

Misc/NEWS.d/next/Core and Builtins/2020-01-15-15-50-22.bpo-39320.oWARyk.rst

@@ -0,0 +1,4 @@
+
+Replace two complex bytecodes for building dicts with two simpler ones.
+The new bytecodes ``DICT_MERGE`` and ``DICT_UPDATE`` have been added
+The old bytecodes ``BUILD_MAP_UNPACK`` and ``BUILD_MAP_UNPACK_WITH_CALL`` have been removed.

Python/ceval.c

@@ -2801,49 +2801,32 @@ main_loop:
             DISPATCH();
         }
 
-        case TARGET(BUILD_MAP_UNPACK): {
-            Py_ssize_t i;
-            PyObject *sum = PyDict_New();
-            if (sum == NULL)
-                goto error;
-
-            for (i = oparg; i > 0; i--) {
-                PyObject *arg = PEEK(i);
-                if (PyDict_Update(sum, arg) < 0) {
-                    if (_PyErr_ExceptionMatches(tstate, PyExc_AttributeError)) {
-                        _PyErr_Format(tstate, PyExc_TypeError,
-                                      "'%.200s' object is not a mapping",
-                                      arg->ob_type->tp_name);
-                    }
-                    Py_DECREF(sum);
-                    goto error;
+        case TARGET(DICT_UPDATE): {
+            PyObject *update = POP();
+            PyObject *dict = PEEK(oparg);
+            if (PyDict_Update(dict, update) < 0) {
+                if (_PyErr_ExceptionMatches(tstate, PyExc_AttributeError)) {
+                    _PyErr_Format(tstate, PyExc_TypeError,
+                                    "'%.200s' object is not a mapping",
+                                    update->ob_type->tp_name);
                 }
+                Py_DECREF(update);
+                goto error;
             }
-
-            while (oparg--)
-                Py_DECREF(POP());
-            PUSH(sum);
+            Py_DECREF(update);
             DISPATCH();
         }
 
-        case TARGET(BUILD_MAP_UNPACK_WITH_CALL): {
-            Py_ssize_t i;
-            PyObject *sum = PyDict_New();
-            if (sum == NULL)
-                goto error;
+        case TARGET(DICT_MERGE): {
+            PyObject *update = POP();
+            PyObject *dict = PEEK(oparg);
 
-            for (i = oparg; i > 0; i--) {
-                PyObject *arg = PEEK(i);
-                if (_PyDict_MergeEx(sum, arg, 2) < 0) {
-                    Py_DECREF(sum);
-                    format_kwargs_error(tstate, PEEK(2 + oparg), arg);
-                    goto error;
-                }
+            if (_PyDict_MergeEx(dict, update, 2) < 0) {
+                format_kwargs_error(tstate, PEEK(2 + oparg), update);
+                Py_DECREF(update);
+                goto error;
             }
-
-            while (oparg--)
-                Py_DECREF(POP());
-            PUSH(sum);
+            Py_DECREF(update);
             PREDICT(CALL_FUNCTION_EX);
             DISPATCH();
         }

Python/compile.c

@@ -1007,9 +1007,6 @@ stack_effect(int opcode, int oparg, int jump)
         case BUILD_SET:
         case BUILD_STRING:
             return 1-oparg;
-        case BUILD_MAP_UNPACK:
-        case BUILD_MAP_UNPACK_WITH_CALL:
-            return 1 - oparg;
         case BUILD_MAP:
             return 1 - 2*oparg;
         case BUILD_CONST_KEY_MAP:
@@ -1125,6 +1122,8 @@ stack_effect(int opcode, int oparg, int jump)
             return 0;
         case LIST_EXTEND:
         case SET_UPDATE:
+        case DICT_MERGE:
+        case DICT_UPDATE:
             return -1;
         default:
             return PY_INVALID_STACK_EFFECT;
@@ -3868,37 +3867,58 @@ static int
 compiler_dict(struct compiler *c, expr_ty e)
 {
     Py_ssize_t i, n, elements;
-    int containers;
+    int have_dict;
     int is_unpacking = 0;
     n = asdl_seq_LEN(e->v.Dict.values);
-    containers = 0;
+    have_dict = 0;
     elements = 0;
     for (i = 0; i < n; i++) {
         is_unpacking = (expr_ty)asdl_seq_GET(e->v.Dict.keys, i) == NULL;
-        if (elements == 0xFFFF || (elements && is_unpacking)) {
-            if (!compiler_subdict(c, e, i - elements, i))
-                return 0;
-            containers++;
-            elements = 0;
-        }
         if (is_unpacking) {
+            if (elements) {
+                if (!compiler_subdict(c, e, i - elements, i)) {
+                    return 0;
+                }
+                if (have_dict) {
+                    ADDOP_I(c, DICT_UPDATE, 1);
+                }
+                have_dict = 1;
+                elements = 0;
+            }
+            if (have_dict == 0) {
+                ADDOP_I(c, BUILD_MAP, 0);
+                have_dict = 1;
+            }
             VISIT(c, expr, (expr_ty)asdl_seq_GET(e->v.Dict.values, i));
-            containers++;
+            ADDOP_I(c, DICT_UPDATE, 1);
         }
         else {
-            elements++;
+            if (elements == 0xFFFF) {
+                if (!compiler_subdict(c, e, i - elements, i)) {
+                    return 0;
+                }
+                if (have_dict) {
+                    ADDOP_I(c, DICT_UPDATE, 1);
+                }
+                have_dict = 1;
+                elements = 0;
+            }
+            else {
+                elements++;
+            }
         }
     }
-    if (elements || containers == 0) {
-        if (!compiler_subdict(c, e, n - elements, n))
+    if (elements) {
+        if (!compiler_subdict(c, e, n - elements, n)) {
             return 0;
-        containers++;
+        }
+        if (have_dict) {
+            ADDOP_I(c, DICT_UPDATE, 1);
+        }
+        have_dict = 1;
     }
-    /* If there is more than one dict, they need to be merged into a new
-     * dict.  If there is one dict and it's an unpacking, then it needs
-     * to be copied into a new dict." */
-    if (containers > 1 || is_unpacking) {
-        ADDOP_I(c, BUILD_MAP_UNPACK, containers);
+    if (!have_dict) {
+        ADDOP_I(c, BUILD_MAP, 0);
     }
     return 1;
 }
@@ -4263,8 +4283,8 @@ ex_call:
     }
     /* Then keyword arguments */
     if (nkwelts) {
-        /* the number of dictionaries on the stack */
-        Py_ssize_t nsubkwargs = 0;
+        /* Has a new dict been pushed */
+        int have_dict = 0;
 
         nseen = 0;  /* the number of keyword arguments on the stack following */
         for (i = 0; i < nkwelts; i++) {
@@ -4272,13 +4292,18 @@ ex_call:
             if (kw->arg == NULL) {
                 /* A keyword argument unpacking. */
                 if (nseen) {
-                    if (!compiler_subkwargs(c, keywords, i - nseen, i))
+                    if (!compiler_subkwargs(c, keywords, i - nseen, i)) {
                         return 0;
-                    nsubkwargs++;
+                    }
+                    have_dict = 1;
                     nseen = 0;
                 }
+                if (!have_dict) {
+                    ADDOP_I(c, BUILD_MAP, 0);
+                    have_dict = 1;
+                }
                 VISIT(c, expr, kw->value);
-                nsubkwargs++;
+                ADDOP_I(c, DICT_MERGE, 1);
             }
             else {
                 nseen++;
@@ -4286,14 +4311,15 @@ ex_call:
         }
         if (nseen) {
             /* Pack up any trailing keyword arguments. */
-            if (!compiler_subkwargs(c, keywords, nkwelts - nseen, nkwelts))
+            if (!compiler_subkwargs(c, keywords, nkwelts - nseen, nkwelts)) {
                 return 0;
-            nsubkwargs++;
-        }
-        if (nsubkwargs > 1) {
-            /* Pack it all up */
-            ADDOP_I(c, BUILD_MAP_UNPACK_WITH_CALL, nsubkwargs);
+            }
+            if (have_dict) {
+                ADDOP_I(c, DICT_MERGE, 1);
+            }
+            have_dict = 1;
         }
+        assert(have_dict);
     }
     ADDOP_I(c, CALL_FUNCTION_EX, nkwelts > 0);
     return 1;

Python/opcode_targets.h

@@ -149,8 +149,8 @@ static void *opcode_targets[256] = {
     &&TARGET_MAP_ADD,
     &&TARGET_LOAD_CLASSDEREF,
     &&_unknown_opcode,
-    &&TARGET_BUILD_MAP_UNPACK,
-    &&TARGET_BUILD_MAP_UNPACK_WITH_CALL,
+    &&_unknown_opcode,
+    &&_unknown_opcode,
     &&_unknown_opcode,
     &&_unknown_opcode,
     &&TARGET_SETUP_ASYNC_WITH,
@@ -163,8 +163,8 @@ static void *opcode_targets[256] = {
     &&TARGET_CALL_METHOD,
     &&TARGET_LIST_EXTEND,
     &&TARGET_SET_UPDATE,
-    &&_unknown_opcode,
-    &&_unknown_opcode,
+    &&TARGET_DICT_MERGE,
+    &&TARGET_DICT_UPDATE,
     &&_unknown_opcode,
     &&_unknown_opcode,
     &&_unknown_opcode,