gh-84644: Fix singledispatch annotation parsing #143465
Conversation
johnslavik
added
needs backport to 3.13
This comment was marked as resolved.
This comment was marked as resolved.
|
I was asked to filter out tests that were helpful at the time of writing this code with detecting backward compatibility problems, but already pass on main. If we're interested in increasing coverage, they'll be recoverable from a commit that removes them. I'll do it in the next few days. In the meantime, the implementation on its own can be reviewed. |
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
Looks reasonable to me!
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Done now. Ready for a proper review. |
The incorrect test was fixed separately; the merge resolves the conflict and reduces the diff size.
johnslavik
force-pushed
the
fix-singledispatch-annotation-parsing
branch
from
08eb19a to
3995e79
Compare
| return registry.get(match) | ||
|
|
||
| def _get_singledispatch_annotated_param(func, *, _inside_dispatchmethod=False): | ||
| """Finds the first positional and user-specified parameter in a callable |
There was a problem hiding this comment.
Nitpick: I'd use the imperative form of the verb instead of the descriptive form:
| """Finds the first positional and user-specified parameter in a callable | |
| """Find the first positional and user-specified parameter in a callable |
There was a problem hiding this comment.
Thanks. I had considered it and picked what was more consistent with the rest of the docstrings in this module. I'm open to changing to imperative form if it's preferred for new functionality.
| all(isinstance(arg, type) for arg in cls.__args__)) | ||
|
|
||
| def register(cls, func=None): | ||
| def register(cls, func=None, _inside_dispatchmethod=False): |
There was a problem hiding this comment.
Consider changing this from a boolean to a scope parameter, where scope is one of function or method. Such a form would be more descriptive and flexible for the possibility of additional scopes.
There was a problem hiding this comment.
I like this semantics.
scope sounds quite good as a name, but I reckon that I'd call it role or purpose, because any callable can be a standalone function or a method simply depending on its purpose in the code.
There was a problem hiding this comment.
OTOH, maybe we could change the parameter name to _skip_first_arg. That's what it's essentially doing.
I don't think this is likely there would exist any other scope / role / purpose than function (0) and method (1).
| with self.assertRaises(TypeError) as exc: | ||
| @i.register | ||
| def _() -> None: | ||
| return "My function doesn't take arguments" | ||
| self.assertStartsWith(str(exc.exception), msg_prefix) | ||
| self.assertEndsWith(str(exc.exception), "does not accept positional arguments.") | ||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register | ||
| def _(*, foo: str) -> None: | ||
| return "My function takes keyword-only arguments" | ||
| self.assertStartsWith(str(exc.exception), msg_prefix) | ||
| self.assertEndsWith(str(exc.exception), "does not accept positional arguments.") |
There was a problem hiding this comment.
I appreciate the tests, but I feel like this change possible deserves its own test cases and not simply expansions of the existing test cases. I haven't looked deeply, so my instincts may be wrong here, but do consider creating independent tests where feasible and not too intrusive on the existing tests.
There was a problem hiding this comment.
Thanks, I'll keep this open. I'll appreciate additional feedback about the preferred way forward.
|
|
||
| argname = _get_singledispatch_annotated_param( | ||
| func, _inside_dispatchmethod=_inside_dispatchmethod) | ||
| if argname is None: |
There was a problem hiding this comment.
Consider raising this exception in the call above, reducing the return signature of the method and keeping concerns together.
| argname = _get_singledispatch_annotated_param( | ||
| func, _inside_dispatchmethod=_inside_dispatchmethod) | ||
| if argname is None: | ||
| raise TypeError( |
There was a problem hiding this comment.
What was the behavior of singledispatch before this change? Would it wrap the function and then fail at runtime when called? If so, that shift in behavior from import-time to run-time could cause problems for functions that were improperly wrapped but never called. Is that the intention of this change?
jaraco
left a comment
jaraco
left a comment
There was a problem hiding this comment.
Overall, this looks good to me. I don't have enough ownership stake in the original implementation to say whether this change aligns with the goals of singledispatch, so I'd appreciate an additional approval from @rhettinger .
3 participants
A very practical but more general approach than GH-140255 to fixing annotation parsing in
functools.singledispatchandfunctools.singledispatchmethod.Aims to fix 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.