-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-84644: Fix singledispatch annotation parsing #143465
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
base: main
Are you sure you want to change the base?
Changes from 70 commits
d3d6e64
6ea2a4a
0a39278
096fc3b
c8a5cdc
e1cde59
6bc698b
4ef7c7c
004c852
9bc1436
3115fd7
f6c102f
7968570
a808a1e
69b9978
ebdb68d
8d86f9e
82616f9
c9a1f1a
e878207
d3240a3
9240b0d
f6ccb97
6642321
0eaaa5b
345b7e9
8a46f3f
16f83ee
552daaf
113cc29
7c1bcea
444425c
9b26fb1
17dfb36
fbce76d
44b8bba
ec01821
57faa34
e4fb514
57965a9
eadc38f
682c41e
c406755
e238e6a
1e61429
19458fc
6390a82
3e33040
c50d344
32910f3
4283fba
17b5088
cdb7cca
62088c7
c497857
0f75d98
8350e71
50c0e64
052c2fd
30994eb
3edad44
fbc205e
b691969
0859bc0
7ac8275
fbe00f8
7ada2b0
ac2f5a2
ba46e43
3995e79
48d1bde
fc5df46
7fcf4d5
f7ec61d
946ccb8
2d1180a
cd10e91
0a622fb
9b2d20d
9191a22
ef74667
b55de76
9977251
d282c9a
4c43a9c
3b5a410
f6ce233
97a8515
62ea333
f0097c4
4e6a003
2d70e22
c716b57
a28e9af
cfb9194
b03891f
4c8e818
7fa9315
cee055a
4db216f
cc2bcc7
371bff5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| # import weakref # Deferred to single_dispatch() | ||
| from operator import itemgetter | ||
| from reprlib import recursive_repr | ||
| from types import GenericAlias, MethodType, MappingProxyType, UnionType | ||
| from types import FunctionType, GenericAlias, MethodType, MappingProxyType, UnionType | ||
| from _thread import RLock | ||
|
|
||
| ################################################################################ | ||
|
|
@@ -888,6 +888,48 @@ def _find_impl(cls, registry): | |
| match = t | ||
| return registry.get(match) | ||
|
|
||
| def _get_singledispatch_annotated_param(func, *, _inside_dispatchmethod=False): | ||
| """Finds the first positional and user-specified parameter in a callable | ||
| or descriptor. | ||
|
|
||
| Used by singledispatch for registration by type annotation of the parameter. | ||
| """ | ||
| # Pick the first parameter if function had @staticmethod. | ||
| if isinstance(func, staticmethod): | ||
| idx = 0 | ||
| func = func.__func__ | ||
| # Pick the second parameter if function had @classmethod or is a bound method. | ||
| elif isinstance(func, (classmethod, MethodType)): | ||
| idx = 1 | ||
| func = func.__func__ | ||
| # If it is a regular function: | ||
| # Pick the first parameter if registering via singledispatch. | ||
| # Pick the second parameter if registering via singledispatchmethod. | ||
| else: | ||
| idx = int(_inside_dispatchmethod) | ||
|
|
||
| # If it is a simple function, try to read from the code object fast. | ||
| if isinstance(func, FunctionType) and not hasattr(func, "__wrapped__"): | ||
| # Emulate inspect._signature_from_function to get the desired parameter. | ||
| func_code = func.__code__ | ||
| try: | ||
| return func_code.co_varnames[:func_code.co_argcount][idx] | ||
| except IndexError: | ||
| pass | ||
|
|
||
| # Fall back to inspect.signature (slower, but complete). | ||
| import inspect | ||
| params = list(inspect.signature(func).parameters.values()) | ||
| try: | ||
| param = params[idx] | ||
| except IndexError: | ||
|
johnslavik marked this conversation as resolved.
|
||
| pass | ||
| else: | ||
| # Allow variadic positional "(*args)" parameters for backward compatibility. | ||
| if param.kind not in (inspect.Parameter.KEYWORD_ONLY, inspect.Parameter.VAR_KEYWORD): | ||
| return param.name | ||
| return None | ||
|
|
||
| def singledispatch(func): | ||
| """Single-dispatch generic function decorator. | ||
|
|
||
|
|
@@ -935,7 +977,7 @@ def _is_valid_dispatch_type(cls): | |
| return (isinstance(cls, UnionType) and | ||
| all(isinstance(arg, type) for arg in cls.__args__)) | ||
|
|
||
| def register(cls, func=None): | ||
| def register(cls, func=None, _inside_dispatchmethod=False): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider changing this from a boolean to a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this semantics.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OTOH, maybe we could change the parameter name to I don't think this is likely there would exist any other scope / role / purpose than function (0) and method (1).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JelleZijlstra
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we intend I went with |
||
| """generic_func.register(cls, func) -> func | ||
|
|
||
| Registers a new implementation for the given *cls* on a *generic_func*. | ||
|
|
@@ -960,10 +1002,28 @@ def register(cls, func=None): | |
| ) | ||
| func = cls | ||
|
|
||
| argname = _get_singledispatch_annotated_param( | ||
| func, _inside_dispatchmethod=_inside_dispatchmethod) | ||
| if argname is None: | ||
|
johnslavik marked this conversation as resolved.
Outdated
|
||
| raise TypeError( | ||
|
johnslavik marked this conversation as resolved.
Outdated
|
||
| f"Invalid first argument to `register()`: {func!r} " | ||
| f"does not accept positional arguments." | ||
| ) | ||
|
|
||
| # only import typing if annotation parsing is necessary | ||
| from typing import get_type_hints | ||
| from annotationlib import Format, ForwardRef | ||
| argname, cls = next(iter(get_type_hints(func, format=Format.FORWARDREF).items())) | ||
| annotations = get_type_hints(func, format=Format.FORWARDREF) | ||
|
|
||
| try: | ||
| cls = annotations[argname] | ||
| except KeyError: | ||
| raise TypeError( | ||
| f"Invalid first argument to `register()`: {func!r}. " | ||
| "Use either `@register(some_class)` or add a type " | ||
| f"annotation to parameter {argname!r} of your callable." | ||
| ) from None | ||
|
|
||
| if not _is_valid_dispatch_type(cls): | ||
| if isinstance(cls, UnionType): | ||
| raise TypeError( | ||
|
|
@@ -1027,7 +1087,7 @@ def register(self, cls, method=None): | |
|
|
||
| Registers a new implementation for the given *cls* on a *generic_method*. | ||
| """ | ||
| return self.dispatcher.register(cls, func=method) | ||
| return self.dispatcher.register(cls, func=method, _inside_dispatchmethod=True) | ||
|
|
||
| def __get__(self, obj, cls=None): | ||
| return _singledispatchmethod_get(self, obj, cls) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2905,13 +2905,34 @@ def t(self, arg): | |
| def _(self, arg: int): | ||
| return "int" | ||
| @t.register | ||
| def _(self, arg: str): | ||
| def _(self, arg: complex, /): | ||
| return "complex" | ||
| @t.register | ||
| def _(self, /, arg: str): | ||
| return "str" | ||
| # See GH-130827. | ||
| def wrapped1(self: typing.Self, arg: bytes): | ||
| return "bytes" | ||
| @t.register | ||
| @functools.wraps(wrapped1) | ||
| def wrapper1(self, *args, **kwargs): | ||
| return self.wrapped1(*args, **kwargs) | ||
|
|
||
| def wrapped2(self, arg: bytearray) -> str: | ||
| return "bytearray" | ||
| @t.register | ||
| @functools.wraps(wrapped2) | ||
| def wrapper2(self, *args: typing.Any, **kwargs: typing.Any): | ||
| return self.wrapped2(*args, **kwargs) | ||
|
|
||
| a = A() | ||
|
|
||
| self.assertEqual(a.t(0), "int") | ||
| self.assertEqual(a.t(0j), "complex") | ||
| self.assertEqual(a.t(''), "str") | ||
| self.assertEqual(a.t(0.0), "base") | ||
| self.assertEqual(a.t(b''), "bytes") | ||
| self.assertEqual(a.t(bytearray()), "bytearray") | ||
|
|
||
| def test_staticmethod_type_ann_register(self): | ||
| class A: | ||
|
|
@@ -3172,12 +3193,27 @@ def test_invalid_registrations(self): | |
| @functools.singledispatch | ||
| def i(arg): | ||
| return "base" | ||
| 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.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I'll keep this open. I'll appreciate additional feedback about the preferred way forward.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests seem similar enough to the other cases in On the other hand the function is big enough that it might be nice to split it up. A nice split could be to have one test for cases where we can't get a type at all (i.e., there are no annotations in the place where we look for them) and another test case for cases where we do find an annotation, but singledispatch doesn't know what to do with it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this. I'll do that. Thanks, I haven't thought about it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JelleZijlstra, I removed all tests (including additional
I found these to be already covered after splitting, so I didn't add this aspect to my new cases (which reimplement the removed ones in a more structured way). |
||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register(42) | ||
| def _(arg): | ||
| return "I annotated with a non-type" | ||
|
johnslavik marked this conversation as resolved.
Outdated
|
||
| self.assertStartsWith(str(exc.exception), msg_prefix + "42") | ||
| self.assertEndsWith(str(exc.exception), msg_suffix) | ||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register | ||
| def _(arg): | ||
|
|
@@ -3187,6 +3223,33 @@ def _(arg): | |
| ) | ||
| self.assertEndsWith(str(exc.exception), msg_suffix) | ||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register | ||
| def _(arg, extra: int): | ||
| return "I did not annotate the right param" | ||
| self.assertStartsWith(str(exc.exception), msg_prefix + | ||
| "<function TestSingleDispatch.test_invalid_registrations.<locals>._" | ||
| ) | ||
| self.assertEndsWith(str(exc.exception), | ||
| "Use either `@register(some_class)` or add a type annotation " | ||
| f"to parameter 'arg' of your callable.") | ||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| # See GH-84644. | ||
|
|
||
| @functools.singledispatch | ||
| def func(arg):... | ||
|
|
||
| @func.register | ||
| def _int(arg) -> int:... | ||
|
|
||
| self.assertStartsWith(str(exc.exception), msg_prefix + | ||
| "<function TestSingleDispatch.test_invalid_registrations.<locals>._int" | ||
| ) | ||
| self.assertEndsWith(str(exc.exception), | ||
| "Use either `@register(some_class)` or add a type annotation " | ||
| f"to parameter 'arg' of your callable.") | ||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register | ||
| def _(arg: typing.Iterable[str]): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| :func:`functools.singledispatch` and :func:`functools.singledispatchmethod` | ||
| now require callables to be correctly annotated if registering without a type explicitly | ||
| specified in the decorator. The first user-specified positional parameter of a callable | ||
| must always be annotated. Before, a callable could be registered based on its return type | ||
|
johnslavik marked this conversation as resolved.
Outdated
|
||
| annotation or based on an irrelevant parameter type annotation. Contributed by Bartosz Sławecki. | ||
|
johnslavik marked this conversation as resolved.
Outdated
johnslavik marked this conversation as resolved.
Outdated
|
||
Uh oh!
There was an error while loading. Please reload this page.