bpo-32226: Implementation of PEP 560 (core components) #4732

ilevkivskyi · 2017-12-05T20:48:26Z

This is a basic implementation of the PEP. Suggestions are welcome.

@gvanrossum @serhiy-storchaka Can one of you (or both?) please review?

cc: @markshannon IIRC you are interested in implementation details of this PEP.

https://bugs.python.org/issue32226

…emoval

ilevkivskyi · 2017-12-07T00:45:57Z

@serhiy-storchaka Thank you very much for review! I will make the changes tomorrow.

…n place

ilevkivskyi · 2017-12-08T01:15:27Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-08T01:15:29Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

ilevkivskyi · 2017-12-10T21:03:35Z

@serhiy-storchaka Following your comment #4731 (comment) I also removed callability checks for special methods here.

If you think it makes sense to customize error messages in future, then I can open a b.p.o. issue for this.

ilevkivskyi · 2017-12-10T21:09:41Z

(I will add documentation for this in a separate PR after the second part of changes -- changes to typing -- will be in.)

serhiy-storchaka · 2017-12-11T18:36:32Z

Python/bltinmodule.c

+            goto error;
+        }
+        if (!replacements) {
+            replacements = PyTuple_New(nargs - 2);


Handle an error.

I suggest to use a list instead of a tuple. You can fill it at the single pass and convert to a tuple at end.

if (!new_bases) { new_bases = PyList_New(i); if (!new_bases) { goto error; } // copy i items to new_bases } if (_PyList_Extend(new_bases, new_base) < 0) { goto error; }

serhiy-storchaka · 2017-12-11T18:58:38Z

Python/bltinmodule.c

+    replacements = NULL;
+    /* We have a separate cycle to calculate replacements with the idea that in
+       most cases we just scroll quickly though it and return original bases */
+    for (i = 2; i < nargs; i++){


I think the code would look simpler if pass nargs-2 and args+2. Or passing just bases may be enough.

Needed a space before {.

serhiy-storchaka · 2017-12-11T19:14:57Z

Python/bltinmodule.c

@@ -206,6 +298,9 @@ builtin___build_class__(PyObject *self, PyObject **args, Py_ssize_t nargs,
    Py_DECREF(meta);
    Py_XDECREF(mkw);
    Py_DECREF(bases);
+    if (modified_bases) {
+        Py_DECREF(old_bases);


Just

Py_XDECREF(old_bases);

This is not really possible, since this code is also executed on success, and in this case if there were no conversion, then bases should be DECREFed exactly once.

serhiy-storchaka · 2017-12-11T19:20:55Z

Python/bltinmodule.c

+        return NULL;
+    }
+    else {
+        old_bases = bases;


The code would look simpler if set the original bases to orig_bases.

orig_bases = _PyStack_AsTupleSlice(...); if (orig_bases == NULL) return NULL; bases = update_bases(...); if (bases == NULL) { Py_DECREF(orig_bases); return NULL; }

serhiy-storchaka · 2017-12-11T19:22:01Z

Python/bltinmodule.c

@@ -168,6 +255,11 @@ builtin___build_class__(PyObject *self, PyObject **args, Py_ssize_t nargs,
                             NULL, 0, NULL, 0, NULL, 0, NULL,
                             PyFunction_GET_CLOSURE(func));
    if (cell != NULL) {
+        if (modified_bases) {


if (bases != orig_bases) {

modified_bases is not needed.

ilevkivskyi · 2017-12-12T00:28:55Z

@serhiy-storchaka Thank you for another review! I converted the code to the single pass as you suggested. The code indeed looks cleaner, however performance is worse now. For normal (non-generic) classes there is slow-down of up to 5%. Do you have any ideas about how to boost performance here?

ilevkivskyi · 2017-12-12T07:56:02Z

@serhiy-storchaka Ah, sorry, I forgot to merge master before bechmarking. In fact, there are no visible slow-downs. (Which makes sense, since I just added few pointer comparisons.)

serhiy-storchaka · 2017-12-12T01:09:02Z

Python/bltinmodule.c

+            PyErr_Clear();
+            if (new_bases) {
+                if (PyList_Append(new_bases, base) < 0) {
+                    Py_DECREF(meth);


meth is NULL here.

serhiy-storchaka · 2017-12-12T07:33:52Z

Python/bltinmodule.c

+        if (!new_base) {
+            goto error;
+        }
+        if (PyTuple_Check(new_base)) {


I suggest to swap branches and move the success branch to upper level. I think this will make the code clearer.

if (!PyTuple_Check(new_base)) { ... goto error; } if (!new_bases) { ...

serhiy-storchaka · 2017-12-12T07:36:12Z

Python/bltinmodule.c

+            }
+            continue;
+        }
+        if (!(meth = _PyObject_GetAttrId(base, &PyId___mro_entries__))) {


It would look clearer if make an assignment on a separate line.

serhiy-storchaka · 2017-12-12T07:37:43Z

Python/bltinmodule.c

+    PyObject *stack[1] = {bases};
+    assert(PyTuple_Check(bases));
+
+    /* We have a separate cycle to calculate replacements with the idea that in


Is this comment outdated?

serhiy-storchaka · 2017-12-12T07:46:38Z

Python/bltinmodule.c

+
+    /* We have a separate cycle to calculate replacements with the idea that in
+       most cases we just scroll quickly though it and return original bases */
+    for (i = 0; i < nargs; i++) {


For performance try to add a separate fast loop for non-generic types.

for (i = 0; i < nargs && PyType_Check(args[i]); i++) { } if (i == nargs) { return bases; } new_bases = PyList_New(i); ... for (; i < nargs; i++) {

This doesn't give any visible difference, so I will just keep the single loop (As I mentioned before, a more careful benchmark showed that it is already good).

serhiy-storchaka · 2017-12-12T07:47:38Z

Python/bltinmodule.c

+            if (!new_bases) {
+                /* If this is a first successful conversion, create new_bases list and
+                copy previously encountered bases. */
+                new_bases = PyList_New(i);


Check for error.

serhiy-storchaka · 2017-12-12T07:52:26Z

Python/bltinmodule.c

+            if (!_PyList_Extend((PyListObject *)new_bases, new_base)) {
+                goto error;
+            }
+            Py_DECREF(Py_None);


This looks confusing. Add a comment to this non-obvious code.

Alternatively you can use a public PyList_SetSlice instead of _PyList_Extend if you prefer.

j = PyList_GET_SIZE(new_bases); if (PyList_SetSlice(new_bases, j, j, new_base) < 0) { goto error; }

serhiy-storchaka · 2017-12-12T07:59:02Z

Python/bltinmodule.c

@@ -46,12 +47,85 @@ _Py_IDENTIFIER(stderr);

 #include "clinic/bltinmodule.c.h"

+static PyObject*
+update_bases(PyObject* bases, PyObject** args, int nargs)


Add the const qualifier. See bpo-32240.

It is more common to add a space before * instead of after it.

serhiy-storchaka · 2017-12-12T08:00:48Z

Python/bltinmodule.c

+    result = PyList_AsTuple(new_bases);
+    Py_DECREF(new_bases);
+    return result;
+error:


This is just my personal preference, but I prefer to add an empty line after return and before a label (closing brace can replace an empty line). This make the code cleaner to me.

ilevkivskyi · 2017-12-14T11:51:52Z

@serhiy-storchaka Does this PR look OK to you now? (Taking into account that Guido likes the current implementation "strategy" of __class_getitem__, see https://bugs.python.org/issue32226#msg308171)

I am really sorry for constantly pushing you on this. There are few other things that depend on this PR

serhiy-storchaka

LGTM. This introduces a small performance regression, but it can be be fixed later.

ilevkivskyi · 2017-12-14T22:30:48Z

LGTM. This introduces a small performance regression, but it can be be fixed later

OK, thanks! Then I will merge this now.

ilevkivskyi and others added 30 commits November 11, 2017 10:01

POC implementation of __base_subclass__

26afdc6

POC implementation of __class_getitem__

3285fb6

Modify __base_subclass__ API to support dynamic evaluation and base r…

779f85d

…emoval

Make __base_subclass__ faster and safer

5d5211d

Factor out base update in a separate helper

41fa7e9

Formatting

5aeebab

Also make __class_getitem__ safer

e858ea7

Simplify some code

5b8d453

Initial work on typing2

b3e52f1

Remove test implementation for typing

6e7b575

Rename base_subclass to subclass_base

a8437fa

Start adding tests

c2d8ac2

Add tests for __subclass_base__

bc06c6b

Add tests for __class_getitem__

86c5d61

Fix trailing whitespace

2cef78a

Alternative implementation of __class_getitem__

b9bbf7c

Simplify code and fix reference counting

2495e94

Rename __base_subclass__ to __mro_entry__

9ca8dfc

Add types.resolve_bases

74bd36f

Make Python and C versions as similar as possible

6f20c45

More tests

bcfbcf6

Fail fast in type.__new__

d7bd630

Fix a test

29e54c3

Test error message

cba2a58

Test error message

966a9ea

Test error message

734c7e9

Allow expansion to multiple bases

152d2f6

Prohibit returning non-tuple

1a218e0

Rename to __mro_entries__

7cc8d8f

Merge remote-tracking branch 'upstream/master' into base-subclass

57f477c

bedevere-bot added awaiting changes and removed awaiting review labels Dec 7, 2017

ilevkivskyi added 2 commits December 8, 2017 01:27

Address CR (part 1)

978588c

Address CR (part 2) use a temporary tuple instead of modifying args i…

7bff68d

…n place

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 8, 2017

Remove checks for callability of special methods

b3d85ff

serhiy-storchaka reviewed Dec 11, 2017

View reviewed changes

ilevkivskyi added 3 commits December 12, 2017 00:06

Some simplifications

687e1e7

More simplifications: use single pass

4426e8c

Some fixes + performance

b7d28d8

Merge remote-tracking branch 'upstream/master' into base-subclass

dce0218

serhiy-storchaka reviewed Dec 12, 2017

View reviewed changes

Re-organize code and fix formatting

caf9238

serhiy-storchaka approved these changes Dec 14, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 14, 2017

ilevkivskyi merged commit 2b5fd1e into python:master Dec 14, 2017

bedevere-bot removed the awaiting merge label Dec 14, 2017

ilevkivskyi deleted the base-subclass branch December 14, 2017 22:33

ericsnowcurrently mentioned this pull request Aug 3, 2024

__class_getitem__ Unexpectedly Falls Back to the Metaclass #122634

Open

Uh oh!

bpo-32226: Implementation of PEP 560 (core components) #4732

bpo-32226: Implementation of PEP 560 (core components) #4732

Uh oh!

Conversation

ilevkivskyi commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi commented Dec 7, 2017

Uh oh!

ilevkivskyi commented Dec 8, 2017

Uh oh!

bedevere-bot commented Dec 8, 2017

Uh oh!

ilevkivskyi commented Dec 10, 2017

Uh oh!

ilevkivskyi commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Dec 12, 2017

Uh oh!

ilevkivskyi commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Dec 14, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Dec 14, 2017

Uh oh!

Uh oh!

ilevkivskyi commented Dec 5, 2017 •

edited

Loading