gh-84644: Fix singledispatch annotation parsing#143465
gh-84644: Fix singledispatch annotation parsing#143465johnslavik wants to merge 102 commits intopython:mainfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
I also think it'd be better as a new feature so that it pairs well with what we already shipped. Considering it relies on annotationlib, it wouldn't be possible to backport it to 3.13 either so I'd prefer keeping it in 3.15 only. |
Co-authored-by: Bénédikt Tran <[email protected]>
Hugo can choose to delete it anything
|
Thanks for the careful review, @picnixz :) I've also added a "What's New" entry. Do you think this looks good now? |
|
I'm still reflecting on whether we could somehow not touch this and instead leverage the fact that the user can easily workaround these bugs by using argument-based registration ( |
Yep. And it's things like this make it impossible for Cython to compile functions transparently. (Checking To me it looks like we're in a hole, and we need to stop digging. Or as PEP 20 puts it, refuse the temptation to guess. |
|
I have a similar hunch. This is a lot of fragile code to maintain in a code path that only supplies some simple convenience. Curiously enough, these issues aren't inherent to the original solution. Back when the annotation-based registration using That said, while I do believe this PR is the correct solution for any practical use, I think I'm in favor of documenting the constraint instead (given how easy and at hand it is to work around it). Thanks @encukou. |
|
Perhaps we can start with emitting a DeprecationWarning when singledispatch would pick up an annotation for the wrong parameter? |
|
Thanks @JelleZijlstra, sounds better to me! |
|
I assumed correct use of type hint based registration as in the contract. But for all bugs we're fixing here, yes, |
|
First step should be documenting the status quo, and backporting that.
Yeah, that can be limited to the cases where we know which argument is correct. |
|
FWIW, here's my take: Since it breaks alternative implementations of
So out of the 3 issues, only fixing the priority seems legitimate. Because that's legitimately an issue IMO that doesn't even appear inside classes, so with plain TL;DR: I also recommend the following:
|
@encukou, do you mean in general to add a note saying that we just pick up the first annotation?
@encukou, what about: @sdm.register # (singledispatchmethod registration)
def foo(self) -> int:
...There's no correct parameter. I guess this looks obviously wrong though so maybe it isn't worth it to consider it.
I'm glad that I've noted that at the very beginning of the PR :-)
@picnixz, I think Petr meant to mention Cython as an example project affected by limitations around heuristics for identifying implications of descriptors, not that this PR has any influence on Cython.
Good call. I could try to put together a Ruff rule for that. CC @AlexWaygood @JelleZijlstra
@picnixz, do you have some idea in mind how you'd phrase that?
It's already fixed independently for 3.15 -- see #143888 -- it's unfortunately unbackportable, so we'd have to backport my PR (this PR) to 3.13 and 3.14.
Agree that it's the most severe.
+1 |
A very practical but more general approach than GH-140255 to fixing annotation parsing in
functools.singledispatchandfunctools.singledispatchmethod.Fixes issues GH-84644, GH-130827, and GH-143886.
It can be broken if one uses a user-defined alternative implementation of
staticmethodor something analogous.Will break incorrect but working registrees.
I haven't investigated strippingAnnotatedtypeforms yet.Consulting a test which fails with this fix at https://github.com/python/cpython/pull/130309/changes#r2663516538 -- I think that the test is wrong.