-
Notifications
You must be signed in to change notification settings - Fork 24
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
PEP 585 steps 7-8: Implement repr for GenericAlias #1
Conversation
eb6df6c
to
612a7f2
Compare
Thanks! There’s something called _UnicodeWriter that I was hoping to use... |
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.
This needs a lot more error checking, alas. :-(
Also (as I said) there's a neat internal API (that we are allowed to use here) named _PyUnicodeWriter
that should speed this up. Grep for examples if use.
I rewrote things with the I also added the test you requested. |
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.
This is great. Thanks so much for doing this for me!
Happy to help! Thank you for the review :) I noticed I missed a static (sorry): #3 If I can help with any of the other implementation, let me know. I can probably implement Then the next step would be making it available in Python code I suppose (since ABCs need it)? |
That would be cool -- though I don't think we even need
That's easy. I'm planning to add one line to types.py: Slightly more work is adding genericity to these classes, in C (follow the example of
And these classes that IIUC exist both in C and in Python, all in
I expect these to go smoothly. What won't be so easy is making We also should be able to pickle and unpickle The real test, however, will be making the classes in |
Hm, perhaps it would be good to consider allowing
I can work on making these generic, should be straightforward.
I submitted #2 for
Yes I think that will be the best way to do it unfortunately. |
(Let's keep adding to this thread with progress.)
Still to do and straightforward:
Difficult or controversial:
|
Update on the issues revealed by the test_typing.py failures. There are actually several unrelated issues. A. In typing.py, the dunders of a generic or parameterized class are different!
For example:
B. Generic classes as defined in typing.py only allow C. For example:
This is done by explicitly assinging to |
I'm inclined to rename But I'm inclined not to set |
Works for me!
Ah, sorry, I've just been running the test_genericalias, I'll run the full test suite in future :)
I'm a little concerned a builtin type could special case its behavior based on something like
Works for me, that is what I called it in my original implementation :)
This seems reasonable.
Yeah that seems too high a cost.
The point of |
No need, it takes forever. And I'm on it. |
Ok great! Also I'm happy to add tests for OrderedDict and Counter, might start adding |
Great! |
Okay so I fixed (I think) the test in #5. I opened #6 for I looked more into I will probably work on So that's the easy bits :) This leaves the difficult or controversial items as you said:
I think I missed the description of this?
See above proposal.
You seem to have a plan for this, so I'll focus on other things.
I think this makes a lot of sense.
All 3 of them seem to be protocol-like, so perhaps not? I don't have much knowledge in this part.
Well I tried the naive way and it didn't work :)
It looks like we could probably do this. |
Make sure you read Lukasz' email to typing-sig. He's okay with several of my proposals (esp.
If we're checking that, can't we just check that
Great!
I think I have a plan, let me do this after I've finished the new
Scroll up a few comments here. But no worries, I have a fix in my pep585b branch.
Heh,. I looked more carefully and these aren't even related to And the more I think about it, the less I want to touch typing.py... So let's concentrate on the C code and adding generics outside typing.py. (E.g. look for things that are generic in typeshed -- like queue.Queue.) |
Ah yeah I'll look through typeshed for things that are Generic and start adding them to tests (and fixing the ones that aren't!). |
Okay, I have now grepped through typeshed, here are all of the things that should be generic according to typeshed:
Need to modify for GenericAlias:
Need to make un-generic (oops):
How should we approach tackling these? I can start working on implementing them in smaller batches of PRs. Also for things that are actually private, should we even bother? If it is in typeshed perhaps that indicates people are using it? |
The PEP says in "forward compatibility" that the expectation is that all new types added to Python will support generic aliasing. The lack of such guarantee is a sure way for typing to always feel hald-baked and not treated seriously by Python core. In this sense, the more types we cover right now, and list those explicitly, the better. |
It’s your PEP, go for it! |
I personally was thinking that we don't enumerate them all in the PEP, but say "types in the standard library that are generic in typeshed should become generic" it makes the PEP easier to read. I do think we should make all of these types generic. But as Guido said, it's your PEP :P |
That's not a bad idea at all! I hope Łukasz likes it enough to add some form of this to his PEP. |
I just discovered an issue:
I guess we need to add a tp_hash implementation to the GenericAlias type. I guess it could follow this from typing.py: def __hash__(self):
if self.__origin__ is Union:
return hash((Union, frozenset(self.__args__)))
return hash((self.__origin__, self.__args__)) |
(Except there is no Union type in C. Though PEP 604 might add one.) |
I don't think we need to worry about Union until it is implemented in C, right? Because unless someone manually does |
The problem is that the Union in typing.py (I.e. the old one, in Python) doesn’t accept list[int]. Adding a tp_hash will fix that. Probably the same for Generic[list[int]]. |
Understood, I more meant that we don't have to special case the hash of GenericAlias like in the snippet above if
should be fine? |
Oh. Yes. Anyway, I pushed a |
Okay, I pushed a fix for But I don't think it will be easy to make @ambv Do you have an opinion here? Is it okay that |
btw, @gvanrossum it seems the tests are not being run on your PR to CPython due to a merge conflict. Also I found the Windows build fails, so I opened #16 |
OK, I did a merge and fixed the conflict. Hopefully this will run the tests? |
So I discovered that I was reminded of this by https://bugs.python.org/issue40098. I tested this using the following script:
|
Okay so the PEP has been accepted (congrats Łukasz!) here are the TODOs I see:
|
Yeah, that looks like about it. But we can do all those in follow-up PRs -- I don't want to weigh down the initial PR any more than we need to. Oh, and we'll need docs too (though for the alpha the PEP will suffice). |
Hi @ethanhs I mentioned your comment in https://bugs.python.org/issue39481 -- you may want to add yourself to the nosy list there. |
Hey Guido, thanks! I'll definitely do that. I also will probably start working through the list. |
``` Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7f008bf19667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667) #1 0x7f007a0bee4a in subprocess_fork_exec /home/heimes/dev/python/cpython/Modules/_posixsubprocess.c:774 #2 0xe0305b in cfunction_call Objects/methodobject.c:546 ``` Signed-off-by: Christian Heimes <[email protected]>
``` Direct leak of 8 byte(s) in 1 object(s) allocated from: GH-0 0x7f008bf19667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667) GH-1 0x7f007a0bee4a in subprocess_fork_exec /home/heimes/dev/python/cpython/Modules/_posixsubprocess.c:774 GH-2 0xe0305b in cfunction_call Objects/methodobject.c:546 ``` Signed-off-by: Christian Heimes <[email protected]> (cherry picked from commit 0d3350d) Co-authored-by: Christian Heimes <[email protected]>
* bpo-40791: Make compare_digest more constant-time. The existing volatile `left`/`right` pointers guarantee that the reads will all occur, but does not guarantee that they will be _used_. So a compiler can still short-circuit the loop, saving e.g. the overhead of doing the xors and especially the overhead of the data dependency between `result` and the reads. That would change performance depending on where the first unequal byte occurs. This change removes that optimization. (This is change #1 from https://bugs.python.org/issue40791 .)
* bpo-40791: Make compare_digest more constant-time. The existing volatile `left`/`right` pointers guarantee that the reads will all occur, but does not guarantee that they will be _used_. So a compiler can still short-circuit the loop, saving e.g. the overhead of doing the xors and especially the overhead of the data dependency between `result` and the reads. That would change performance depending on where the first unequal byte occurs. This change removes that optimization. (This is change GH-1 from https://bugs.python.org/issue40791 .) (cherry picked from commit 3172936) Co-authored-by: Devin Jeanpierre <[email protected]>
* bpo-40791: Make compare_digest more constant-time. The existing volatile `left`/`right` pointers guarantee that the reads will all occur, but does not guarantee that they will be _used_. So a compiler can still short-circuit the loop, saving e.g. the overhead of doing the xors and especially the overhead of the data dependency between `result` and the reads. That would change performance depending on where the first unequal byte occurs. This change removes that optimization. (This is change GH-1 from https://bugs.python.org/issue40791 .) (cherry picked from commit 3172936) Co-authored-by: Devin Jeanpierre <[email protected]>
) Fix test_gdb.test_pycfunction() for Python built with clang -Og. Tolerate inlined functions in the gdb traceback. When _testcapimodule.c is built by clang -Og, _null_to_none() is inlined in meth_varargs() and so gdb returns _null_to_none() as the frame #1. If it's not inlined, meth_varargs() is the frame #1.
…python#91466) Fix an uninitialized bool in exception print context. `struct exception_print_context.need_close` was uninitialized. Found by oss-fuzz in a test case running under the undefined behavior sanitizer. https://oss-fuzz.com/testcase-detail/6217746058182656 ``` Python/pythonrun.c:1241:28: runtime error: load of value 253, which is not a valid value for type 'bool' #0 0xbf2203 in print_chained cpython3/Python/pythonrun.c:1241:28 #1 0xbea4bb in print_exception_cause_and_context cpython3/Python/pythonrun.c:1320:19 #2 0xbea4bb in print_exception_recursive cpython3/Python/pythonrun.c:1470:13 #3 0xbe9e39 in _PyErr_Display cpython3/Python/pythonrun.c:1517:9 ``` Pretty obvious what the ommission was upon code inspection.
…es (#1… (python#108688) This reverts commit 08447b5. Revert also _ctypes.c changes of the PyDict_ContainsString() change, commit 6726626.
This is basically a direct port of the Python code, modulo the loop inside was broken out so I didn't have to concat a new tuple to the parameters tuple.