-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
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 all 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
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.
Show resolved
Hide 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). |
||
| """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: | ||
|
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 raising this exception in the call above, reducing the return signature of the method and keeping concerns together. |
||
| raise TypeError( | ||
|
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. 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? |
||
| 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.") | ||
|
Comment on lines
+3196
to
+3208
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. |
||
|
|
||
| with self.assertRaises(TypeError) as exc: | ||
| @i.register(42) | ||
| def _(arg): | ||
| return "I annotated with a non-type" | ||
| 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 | ||
| annotation or based on an irrelevant parameter type annotation. Contributed by Bartosz Sławecki. |
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.
Nitpick: I'd use the imperative form of the verb instead of the descriptive form:
Uh oh!
There was an error while loading. Please reload this page.
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.
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.