-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
Conversation
Objects/descrobject.c
Outdated
}; | ||
|
||
static PyMemberDef ga_members[] = { | ||
{"__origin__", T_OBJECT, offsetof(gaobject, origin), READONLY}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe T_OBJECT_EX
?
There was a problem hiding this comment.
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.)
Objects/descrobject.c
Outdated
static PyObject * | ||
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds) | ||
{ | ||
if (kwds != NULL) { |
There was a problem hiding this comment.
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;
}
Objects/descrobject.c
Outdated
}; | ||
|
||
static PyObject * | ||
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
So I'm looking into the test failures. I've fixed two simple ones, but now I'm stuck with at least two:
The problem with test_typing.py is that when we have a class that derives from
its MRO contains |
Using a debug build I can repro the test_genericalias.py crash. The upper few lines of the stack trace are:
@ethanhs does this give you a clue? Methinks there's a refcount bug in ga_repr(). :-( |
@ethanhs Update: it's crashing in |
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 |
Ah, are you suggesting that we move those into the branch where we check fro (also thank you for fixing the crash!) |
Lib/test/test_genericalias.py
Outdated
|
||
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
…list, dict, set, frozenset
... collections.defaultdict
By Ethan Smith.
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
What's up with the Docs build failures in CI? |
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. |
Hey @ethanhs -- can you repro the Windows failure here on test_ctypes? |
@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. |
Thanks -- I'll just land. |
Oops. "Required statuses must pass before merging" |
Should not we add |
Done, both. And the test failure is resolved after the merge from master, so once this passes I'll land. |
@gvanrossum: Please replace |
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`.
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`.
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`.
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[UPDATE: @ambv agrees and has updated the PEP.]isinstance(x, list[int])
andissubclass(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.)There are also a few things that ought to be changed:
GenericAlias
.io.IOBase
generic. This was a misunderstanding, and we'll revert that.__parameters__
attribute can and probably should be computed lazily the first time it's needed.typing.py
uses?descrobject.c
, but that's debatable (it was just convenient).https://bugs.python.org/issue39481