Skip to content

/ cpython

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: WIP: PEP 585 #18239

Open
wants to merge 38 commits into
base: master
from
Open

bpo-39481: WIP: PEP 585 #18239

wants to merge 38 commits into from
+664 −11

Conversation

@gvanrossum
Copy link
Member

gvanrossum commented Jan 28, 2020 •
edited by bedevere-bot

https://bugs.python.org/issue39481
@gvanrossum gvanrossum requested a review from serhiy-storchaka Jan 28, 2020
@gvanrossum gvanrossum requested review from methane and rhettinger as code owners Jan 28, 2020
@gvanrossum gvanrossum removed the request for review from rhettinger Jan 28, 2020
@serhiy-storchaka
serhiy-storchaka reviewed Jan 28, 2020
View changes
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c
};

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

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Jan 28, 2020

Member

Maybe T_OBJECT_EX?

This comment has been minimized.

Sign in to view
Copy link
@gvanrossum

gvanrossum Jan 28, 2020

Author Member

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 Show resolved Hide resolved
Objects/descrobject.c Outdated
static PyObject *
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
if (kwds != NULL) {

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Jan 28, 2020

Member

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 Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c
};

static PyObject *
ga_new(PyTypeObject *type, PyObject *args, PyObject *kwds)

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Jan 28, 2020

Member

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

This comment has been minimized.

Sign in to view
Copy link
@gvanrossum

gvanrossum Jan 28, 2020

Author Member

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

gvanrossum commented Jan 29, 2020 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

gvanrossum commented Jan 29, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

gvanrossum commented Jan 29, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

gvanrossum commented Jan 29, 2020

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.
@ethanhs

This comment has been minimized.

Sign in to view
Copy link
Contributor

ethanhs commented Jan 29, 2020 •
edited

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!)
@serhiy-storchaka
serhiy-storchaka reviewed Jan 29, 2020
View changes
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:

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Jan 29, 2020

Member

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

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Jan 29, 2020

Member

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

This comment has been minimized.

Sign in to view
Copy link
@gvanrossum

gvanrossum Jan 29, 2020 •
edited

Author Member

I added __class_getitem__ to _pyio.IOBase in 35cd5f4.

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 Jan 30, 2020
gvanrossum and others added 14 commits Jan 27, 2020
@gvanrossum
PEP 585 step 2: Write a Py_GenericAlias() function that returns origin
d557a64
@gvanrossum
PEP 585 step 3: Add Py_GenericAlias() as __class_getitem__ to tuple, …
da33267 
…list, dict, set, frozenset
@gvanrossum
PEP 585 step 4: Write some tests
447fbe9
@gvanrossum
Define a GenericAlias (proxy) object (with lots of TODOs still)
83e2ba2
@gvanrossum
More tests
fb3e8d3
@gvanrossum
PEP 585 steps 5-6: Finish basic GenericAlias object and test
235180b
@gvanrossum
Fix bug with class methods
70f3abc
@gvanrossum
Define tp_new instead of tp_init
e4d1356
@gvanrossum
Test that list[int][int] fails
8f7a55e
@gvanrossum
Test that class L(list): pass; L[int] works
f2ba2e5
@ethanhs @gvanrossum
Implement PEP 585 for collections.deque and ... (#2)
0af74d1 
... collections.defaultdict
@gvanrossum
Respond to Serhiy's code review
b264294
@blurb-it @gvanrossum
📜🤖 Added by blurb_it.
0ddea56
@ethanhs @gvanrossum
PEP 585 steps 7-8: Implement repr for GenericAlias (#1)
91edf1c 
By Ethan Smith.
ethanhs and others added 8 commits Jan 29, 2020
@ethanhs @gvanrossum
Add static keyword to ga_repr_item (#3)
6b1a218
@gvanrossum
Add GenericAlias to types.py
ce21a96
@gvanrossum
Add test for types.py
dbe67d0 
Also strengthen test for repr(MyList[int])
@gvanrossum
Fix tests that were sensitive to the presence of __class_getitem__
15697f0 
Note that UserDict and UserList are now generic
@gvanrossum
Fix crash in repr(tuple[...]) -- call PyErr_Clear()
da15d43
@ethanhs @gvanrossum
Make re.Match, re.Pattern, and io.IOBase generic (#4)
647cda8
@gvanrossum
Restore parity between _io._IOBase and _pyio.IOBase
3824954
@gvanrossum
Make contextlib.Abstract*ContextManager generic, with tests
Loading status checks…
8aec6ae
@gvanrossum gvanrossum force-pushed the gvanrossum:pep585 branch from 1b763ec to 8aec6ae Jan 30, 2020
gvanrossum added 5 commits Jan 29, 2020
@gvanrossum
WIP: Tweak test_typing.py to pass with the new definition of __orig_c…
Loading status checks…
58af183 
…lass__
@gvanrossum
Rename __parameters__ to __args__
8da0a08
@gvanrossum
Rename parameters to args
eaecacb 
Keep not-yet-functional parameters, and add __parameters__ to match needed.
@gvanrossum
Properly implement __parameters__ as the tuple of args that are type …
98d479f 
…vars
@gvanrossum
Set __orig_class__ if possible when instantiating
82bc806
@gvanrossum gvanrossum requested a review from ilevkivskyi as a code owner Jan 30, 2020
@gvanrossum

This comment has been minimized.

Sign in to view
Copy link
Owner Author

gvanrossum commented on 58af183 Jan 30, 2020

These are temporary hacks -- once I have eq working this should be excised.
gvanrossum and others added 8 commits Jan 30, 2020
@gvanrossum
WIP: Implement ga_getitem so list[T][int] -> list[int]
2567ce3 
However, dict[T, int][str] -> dict[str], which is wrong!
Similar, dict[T, T][str] fails but should return dict[str, str].
Finally, dict[T, T].__parameters__ -> (T, T) but should be (T,).
@gvanrossum
Exclude duplicates from __parameters__
4f2ced6 
Also move declaration of Py_GenericAliasType to descrobject.h.
@gvanrossum
Implement ga_getitem properly, with tests
2c25fb0
@gvanrossum
Implement == and != for GA
Loading status checks…
9ed17ef
@ethanhs
Make collections.abc.* generic (#5)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bb9a11f
@ethanhs
Make isinstance and issubclass error for GenericAlias (#6)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6d3f535
@gvanrossum
Fix two more AssertIs calls in test_typing.py
4b3ecf2
@gvanrossum
Admit defeat -- comment out two expected TypeErrors in test_extended_…
Loading status checks…
2df602b 
…generic_rules_subclassing
@gvanrossum

This comment has been minimized.

Sign in to view
Copy link
Member Author

gvanrossum commented Jan 31, 2020

@serhiy-storchaka Can you review again? A lot has changed. (See also python/peps#1289.)
gvanrossum added 3 commits Jan 31, 2020
@gvanrossum
Fix whitespace to satisfy patchcheck.py
Loading status checks…
eede65a
@gvanrossum
Fix test_site.py
Loading status checks…
2b22f6e
@gvanrossum
Better way to fix test_site.py: don't import types from collections
Loading status checks…
065a032
@codecov

This comment has been minimized.

Sign in to view
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #18239 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18239      +/-   ##
==========================================
- Coverage   82.20%   82.12%   -0.08%     
==========================================
  Files        1957     1955       -2     
  Lines      589079   583703    -5376     
  Branches    44401    44407       +6     
==========================================
- Hits       484247   479380    -4867     
+ Misses      95174    94681     -493     
+ Partials     9658     9642      -16
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-12.86%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Lib/dbm/__init__.py 66.66% <0.00%> (-4.45%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.87%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
... and 316 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 065a032...065a032. Read the comment docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka

@methane methane

@1st1 1st1

@ilevkivskyi ilevkivskyi

Assignees
No one assigned
Labels
CLA signed DO-NOT-MERGE awaiting core review
Projects
None yet
Milestone
No milestone
5 participants
@gvanrossum @ethanhs @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.