Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39481: Implementation for PEP 585 #18239

Merged
merged 78 commits into from
Apr 7, 2020
Merged

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 28, 2020

I consider this a finished prototype now. We've got everything working, and then some (e.g. pickle), and while we found a fair number of minor issues with PEP 585, the main conclusion is that the PEP is quite reasonable to implement.

A number of suggestions for changes PEP 585 that have basically been vetted by @ambv can be found in python/peps#1289.

There are a few places where I still disagree with @ambv. In particular, I don't think it makes sense to even allow isinstance(x, list[int]) and issubclass(C, list[int]) -- IMO these should just raise TypeError, and that's what this PR implements. (If @ambv convinces me to change my mind, it's easy to revert that.) [UPDATE: @ambv agrees and has updated the PEP.]

There are also a few things that ought to be changed:

  • We should add a nice docstring to GenericAlias.
  • We made io.IOBase generic. This was a misunderstanding, and we'll revert that.
  • The __parameters__ attribute can and probably should be computed lazily the first time it's needed.
  • Maybe we still need a cache, like typing.py uses?
  • Where should the code live? For now I put it in descrobject.c, but that's debatable (it was just convenient).

https://bugs.python.org/issue39481

};

static PyMemberDef ga_members[] = {
{"__origin__", T_OBJECT, offsetof(gaobject, origin), READONLY},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe T_OBJECT_EX?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not. The two attributes should never be allowed to be NULL. (This is a change from an earlier design.)

static PyObject *
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
if (kwds != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth to use Argument Clinic which generates simpler code using non-public C API similar to the following:

if (!_PyArg_NoKeywords("GenericAlias", kwds) ||
    !_PyArg_CheckPositional("GenericAlias", PyTuple_GET_SIZE(args), 2, 2))
{
    return NULL;
}

};

static PyObject *
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a __new__ method? Why not set tp_new = NULL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first had a tp_init. Then calling GenericAlias() somehow worked and produced an object with NULL attributes (or maybe it was some more clever incantation, I forget, but I definitely saw it). I do want the class instantiable from Python (so it can potentially be used from typing.py). Using tp_new instead solved that problem.

@gvanrossum gvanrossum changed the title WIP: PEP 585 bpo-39481: WIP: PEP 585 Jan 28, 2020
@gvanrossum
Copy link
Member Author

gvanrossum commented Jan 29, 2020

So I'm looking into the test failures. I've fixed two simple ones, but now I'm stuck with at least two:

  • test_typing.py
  • test_genericalias.py (i.e. the very PEP 585 test) crashes on Ubuntu, but not for me locally , see below (UPDATE: fixed)

The problem with test_typing.py is that when we have a class that derives from Tuple[xxx], like so:

from typing import Tuple
class C(Tuple[int]): pass
print(C.mro())  # C, tuple, typing.Generic, object

its MRO contains tuple before Generic, and the new tuple.__class_getitem__ shadows the intended Generic.__class_getitem__. I'm not sure what to do about this. @ambv @ilevkivskyi @serhiy-storchaka

@gvanrossum
Copy link
Member Author

Using a debug build I can repro the test_genericalias.py crash. The upper few lines of the stack trace are:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff72e217fa libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff72edebc1 libsystem_pthread.dylib`pthread_kill + 432
    frame #2: 0x00007fff72da8a1c libsystem_c.dylib`abort + 120
    frame #3: 0x00007fff72da7cd6 libsystem_c.dylib`__assert_rtn + 314
    frame #4: 0x00000001000df936 python.exe`_PyType_Lookup(type=0x0000000100401d30, name=0x00000001035f1d60) at typeobject.c:3155:5
    frame #5: 0x00000001000c0b8f python.exe`_PyObject_GenericGetAttrWithDict(obj=0x0000000100401ec8, name=0x00000001035f1d60, dict=0x0000000000000000, suppress=0) at object.c:1319:13
    frame #6: 0x00000001000c0ab3 python.exe`PyObject_GenericGetAttr(obj=0x0000000100401ec8, name=0x00000001035f1d60) at object.c:1407:12
    frame #7: 0x00000001000c0268 python.exe`PyObject_GetAttr(v=0x0000000100401ec8, name=0x00000001035f1d60) at object.c:1013:16
    frame #8: 0x00000001000c01a4 python.exe`PyObject_GetAttrString(v=0x0000000100401ec8, name="__module__") at object.c:918:11
    frame #9: 0x0000000100056154 python.exe`ga_repr_item(writer=0x00007ffeefbe9078, p=0x0000000100401ec8) at descrobject.c:1828:24
    frame #10: 0x0000000100054015 python.exe`ga_repr(self=0x00000001040349b0) at descrobject.c:1890:13
    frame #11: 0x00000001000beefb python.exe`PyObject_Repr(v=0x00000001040349b0) at object.c:543:11
    frame #12: 0x00000001001e9d49 python.exe`builtin_repr(module=0x0000000101221dd0, obj=0x00000001040349b0) at bltinmodule.c:2141:12

@ethanhs does this give you a clue? Methinks there's a refcount bug in ga_repr(). :-(

@gvanrossum
Copy link
Member Author

@ethanhs Update: it's crashing in repr(tuple[int, ...]). I think I know the problem. Fix coming up.

@gvanrossum
Copy link
Member Author

Okay, so test_typing.py is the only failing test (see above). But I think the gc_repr() code might be refactored a bit to avoid getting the __origin__ and __parameters__ attributes before we know they are needed.

@emmatyping
Copy link
Contributor

emmatyping commented Jan 29, 2020

But I think the gc_repr() code might be refactored a bit to avoid getting the origin and parameters attributes before we know they are needed.

Ah, are you suggesting that we move those into the branch where we check fro __qualname__ and __module__?

(also thank you for fixing the crash!)


class BaseTest(unittest.TestCase):
"""Test basics."""

def test_subscriptable(self):
for t in tuple, list, dict, set, frozenset, defaultdict, deque:
for t in tuple, list, dict, set, frozenset, defaultdict, deque, IOBase, Pattern, Match:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I suppose IOBase subclasses BufferedIOBase, RawIOBase and TextIOBase should not support indexing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also Python implementation _pyio.IOBase. I am not sure what to do with it. Maybe just ignore for now.

Copy link
Member Author

@gvanrossum gvanrossum Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added __class_getitem__ to _pyio.IOBase in 35cd5f477c9da985880130d72297335e010450e4.

I'm not sure whether it matters much that we take the __class_getitem__ away from the various subclasses (let the static type checkers worry about that).

@gvanrossum gvanrossum requested a review from 1st1 as a code owner January 30, 2020 00:05
gvanrossum and others added 2 commits April 6, 2020 12:01
@gvanrossum
Copy link
Member Author

What's up with the Docs build failures in CI?

@gvanrossum
Copy link
Member Author

Can't say I understand the Windows failures either. Anyway, I'm prepping this for merge by deleting the GitHub workflow edit. I'll be back later.

@gvanrossum
Copy link
Member Author

Hey @ethanhs -- can you repro the Windows failure here on test_ctypes?

@emmatyping
Copy link
Contributor

@gvanrossum I cannot repro locally. It seems to be unable to find the sqlite shared library, but I have no idea how that could be caused by this changeset.

@gvanrossum
Copy link
Member Author

Thanks -- I'll just land.

@gvanrossum
Copy link
Member Author

Oops. "Required statuses must pass before merging"

@serhiy-storchaka
Copy link
Member

Should not we add Py_GenericAlias and/or Py_GenericAliasType in PC/python3.def?

@gvanrossum
Copy link
Member Author

Should not we add Py_GenericAlias and/or Py_GenericAliasType in PC/python3.def?

Done, both. And the test failure is resolved after the merge from master, so once this passes I'll land.

@gvanrossum gvanrossum merged commit 48b069a into python:master Apr 7, 2020
@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@gvanrossum gvanrossum deleted the pep585 branch April 7, 2020 16:50
colesbury referenced this pull request in colesbury/nogil Oct 6, 2021
This implements things like `list[int]`,
which returns an object of type `types.GenericAlias`.
This object mostly acts as a proxy for `list`,
but has attributes `__origin__` and `__args__`
that allow recovering the parts (with values `list` and `(int,)`.

There is also an approximate notion of type variables;
e.g. `list[T]` has a `__parameters__` attribute equal to `(T,)`.
Type variables are objects of type `typing.TypeVar`.
icanhasmath pushed a commit to ActiveState/cpython that referenced this pull request Sep 27, 2023
This implements things like `list[int]`,
which returns an object of type `types.GenericAlias`.
This object mostly acts as a proxy for `list`,
but has attributes `__origin__` and `__args__`
that allow recovering the parts (with values `list` and `(int,)`.

There is also an approximate notion of type variables;
e.g. `list[T]` has a `__parameters__` attribute equal to `(T,)`.
Type variables are objects of type `typing.TypeVar`.
icanhasmath pushed a commit to ActiveState/cpython that referenced this pull request Sep 29, 2023
This implements things like `list[int]`,
which returns an object of type `types.GenericAlias`.
This object mostly acts as a proxy for `list`,
but has attributes `__origin__` and `__args__`
that allow recovering the parts (with values `list` and `(int,)`.

There is also an approximate notion of type variables;
e.g. `list[T]` has a `__parameters__` attribute equal to `(T,)`.
Type variables are objects of type `typing.TypeVar`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants