Skip to content

gh-85294: Handle missing arguments to @singledispatchmethod gracefully #21471

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 16, 2024
Merged

gh-85294: Handle missing arguments to @singledispatchmethod gracefully #21471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
46dbf3c
bpo-41122: Handle missing arguments to @singledispatchmethod gracefully
ammaraskar Jul 14, 2020
de12588
Fix test name
ammaraskar Jul 18, 2020
737a8d2
Merge branch 'main' into singledispatchmethod
serhiy-storchaka Feb 16, 2024
171f0a5
Remove _funcname attribute.
serhiy-storchaka Feb 16, 2024
1d48a93
Polishing.
serhiy-storchaka Feb 16, 2024
f41eafb
Update Lib/functools.py
serhiy-storchaka Feb 16, 2024
0f0466e
Revert "Update Lib/functools.py"
serhiy-storchaka Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Lib/functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,6 @@ def wrapper(*args, **kw):
if not args:
raise TypeError(f'{funcname} requires at least '
'1 positional argument')

return dispatch(args[0].__class__)(*args, **kw)

funcname = getattr(func, '__name__', 'singledispatch function')
Expand Down Expand Up @@ -968,7 +967,11 @@ def __get__(self, obj, cls=None):
return _method

dispatch = self.dispatcher.dispatch
funcname = getattr(self.func, '__name__', 'singledispatchmethod method')
def _method(*args, **kwargs):
if not args:
raise TypeError(f'{funcname} requires at least '
'1 positional argument')
return dispatch(args[0].__class__).__get__(obj, cls)(*args, **kwargs)
Comment on lines +972 to 975
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done benchmarks, but is there a risk that this could add unnecessary overhead to the happy path? What if we did something like this @serhiy-storchaka?

Suggested change
if not args:
raise TypeError(f'{funcname} requires at least '
'1 positional argument')
return dispatch(args[0].__class__).__get__(obj, cls)(*args, **kwargs)
try:
key = args[0].__class__
except IndexError:
raise TypeError(f'{funcname} requires at least '
'1 positional argument') from None
return dispatch(key).__get__(obj, cls)(*args, **kwargs)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks reasonable, but could you please benchmark this? Your code is not zero cost either, it adds STORE_FAST+LOAD_FAST instead of LOAD_FAST+TO_BOOL+POP_JUMP_IF_TRUE.

If there is a difference, we perhaps should apply the same optimization in singledispatch().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll benchmark it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I reverted this change because it complicates the code. And the solution for other issues may change it anyway. It is better to defer it until the code will be stabilized.


_method.__isabstractmethod__ = self.__isabstractmethod__
Expand Down
17 changes: 16 additions & 1 deletion Lib/test/test_functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2867,11 +2867,26 @@ def _(arg: typing.Union[int, typing.Iterable[str]]):

def test_invalid_positional_argument(self):
@functools.singledispatch
def f(*args):
def f(*args, **kwargs):
pass
msg = 'f requires at least 1 positional argument'
with self.assertRaisesRegex(TypeError, msg):
f()
msg = 'f requires at least 1 positional argument'
with self.assertRaisesRegex(TypeError, msg):
f(a=1)

def test_invalid_positional_argument_singledispatchmethod(self):
class A:
@functools.singledispatchmethod
def t(self, *args, **kwargs):
pass
msg = 't requires at least 1 positional argument'
with self.assertRaisesRegex(TypeError, msg):
A().t()
msg = 't requires at least 1 positional argument'
with self.assertRaisesRegex(TypeError, msg):
A().t(a=1)

def test_union(self):
@functools.singledispatch
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2020-07-13-23-59-42.bpo-41122.8P_Brh.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Failing to pass arguments properly to :func:`functools.singledispatchmethod`
now throws a TypeError instead of hitting an index out of bounds
internally.