bpo-32380 Fix singledispatch interaction with methods #4987

emmatyping · 2017-12-23T07:12:15Z

Singledispatch dispatches on the first argument, making it incompatible with classes. This change fixes that.

https://bugs.python.org/issue32380

Singledispatch used the first argument to dispatch on. This made it incomatible with methods. This change adds support for methods by dispatching on the second argument if the passed function is a method.

ilevkivskyi

Thanks! I think this should be implemented more carefully, see comments.

ilevkivskyi · 2017-12-23T09:05:58Z

Lib/functools.py

-        return dispatch(args[0].__class__)(*args, **kw)
+        # If our decorated function is in the first arg's attributes,
+        # we are using a method.
+        if getattr(args[0], func.__name__, False):


This looks like a fragile hack. Some arguments may have the same attribute just by coincidence.

Hm, yes, I wasn't sure if this was the best solution or not. The main issue is that the decorated function is not an instance of types.MethodType when the decorator reaches it. I also looked at frame stack hacks but that would cause conflicts if it was decorating closures. At this point it seems to me a new methodsingledispatch is in order, with dispatching on args[1]. e.g.:

def methodsingledispatch(func): wrapped = singledispatch(func) def wrapper(*args, **kwargs): wrapped.dispatch(args[1].__class__)(*args, **kw) wrapper.register = wrapped.register update_wrapper(wrapper, func) return wrapper

Alternatively, I think allowing the user to specify which arg index for singledispatch to dispatch on would also be a possible solution (and a nice feature to add in general). Then it would be as simple as methodsingledispatch = partial(singledispatch, index=1) or something like that.

This is an API change, so this probably should go through python-ideas/python-dev.

Okay, I will post to python-dev.

ilevkivskyi · 2017-12-23T09:06:53Z

Lib/functools.py

@@ -816,8 +815,15 @@ def register(cls, func=None):
        dispatch_cache.clear()
        return func

+


Why do you change the whitespace formatting?

ilevkivskyi · 2017-12-23T09:11:08Z

Doc/library/functools.rst

@@ -332,7 +332,7 @@ The :mod:`functools` module defines the following functions:
     False

   When called, the generic function dispatches on the type of the first
-   argument::
+   argument, unless it is used on class::


This should be more clear. For example, "Note: if it is used with instance or class methods, then self/cls is skipped, and the next argument is used." Also add an example illustrating this.

ilevkivskyi · 2017-12-23T09:13:47Z

Lib/test/test_functools.py

+        self.assertEqual(a.t(0), "int")
+        self.assertEqual(a.t(''), "str")
+        self.assertEqual(a.t(0.0), "base")
+


Please add more tests. At least cover these cases:

@classmethod and @staticmethod

method with zero args (it should fail gracefully)

calling via instance vs calling via class for all of the above

@classmethod and @staticmethod return descriptors, and singledispatch requires a callable, thus these are not compatible with singledispatch (or would require more significant changes than I'm intending). Generally, you put these around the decorator, but for singledispatch that won't work, as that would break register().

There should be at least one way to do it. Allowing to use singledispatch only with instance methods looks like an incomplete solution.

ilevkivskyi · 2017-12-23T09:14:13Z

Misc/NEWS.d/next/Library/2017-12-22-22-31-46.bpo-32380.vHdQW7.rst

@@ -0,0 +1,2 @@
+``functools.singledispatch`` now supports decorating and registering with
+class methods.


Just "methods."

bedevere-bot · 2017-12-23T09:16:28Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

emmatyping · 2017-12-25T02:27:15Z

Lib/functools.py

            try:
-                impl = registry[cls]
+                impl = dispatch_cache[cls]


This is the diff being weird, I did not change this. The block and main part of singledispatch was indented.

ncoghlan · 2017-12-25T22:52:30Z

Doc/library/functools.rst.bak

@@ -0,0 +1,511 @@
+:mod:`functools` --- Higher-order functions and operations on callable objects


This shouldn't have been included in the commit.

Thanks. I don't know why this isn't ignored by git. I will remove it.

ncoghlan

Thanks for the PR, but unfortunately, I don't believe this particular class-definition-time-only approach is ever going to work right given the semantics of the descriptor protocol.

Instead, something more like the functools.partialmethod will be needed, which defers some key tasks to its __get__ method implementation.

ncoghlan · 2017-12-25T22:53:10Z

Doc/library/functools.rst

@@ -357,6 +357,23 @@ The :mod:`functools` module defines the following functions:
   for the base ``object`` type, which means it is used if no better
   implementation is found.

+   You can also tell ``@singeldispatch`` to dispatch on another argument index


s/singeldispatch/singledispatch/ (typo fix)

ncoghlan · 2017-12-25T22:55:16Z

Lib/functools.py

@@ -753,76 +753,78 @@ def singledispatch(func):
    implementations can be registered using the register() attribute of the


Docstring needs to be updated for the new parameter

ncoghlan · 2017-12-25T22:56:12Z

Lib/functools.py

@@ -744,7 +744,7 @@ def _find_impl(cls, registry):
            match = t
    return registry.get(match)

-def singledispatch(func):
+def singledispatch(func=None, arg=0):


The name arg doesn't clear convey that this is the arg index the will be used to dispatch on. arg_index would be clearer.

(Although this will become a moot point if you switch to a separate singledispatchmethod as proposed below)

ncoghlan · 2017-12-25T23:08:54Z

Lib/test/test_functools.py

+                return "int"
+            @t.register(str)
+            def _(self, arg):
+                return "str"


As @ilevkivskyi noted, additional tests are needed to cover the interaction with other method decorators, especially classmethod, staticmethod, abstractmethod, and partialmethod.

I think you will find that adding a parameter to the existing singledispatch function isn't going to be sufficient to solve the problem in the general case (including supporting post-registration of new overloads after the class has been defined). Instead, you'll need something more along the lines of partialmethod which is aware of the descriptor protocol, and uses that to delegate to the underlying descriptor's __get__ method as necessary. While it's a bit long, singledispatchmethod would follow the existing convention for descriptor protocol aware variants of previously function-only APIs.

Hey @ncoghlan and @ethanhs, I'd been lurking the cpython PRs and had some time today, so I made a quick gist of a possible descriptor-aware implementation of singlemethoddispatch. It's far from complete, but it does work for instance, abstract and class methods. (I haven't tried static methods or partial methods yet). I thought I'd link to it here in case it can provide you with some direction / inspiration.

Nice to haves (for my implementation, at least), would be a mechanism which wraps up any methods registered by the decorated method in the same decorators in which that method is wrapped; e.g., if Foo.bar is a classmethod, then Foo.bar.register(int, baz) should make baz a class method too. On the other hand, that's complicated to implement and possibly not even the desired behavior, so I'm putting it on the back-burner for now.

Anyway, here's the gist. Heavily inspired by the partialmethod implementation, as you'll see: https://gist.github.com/ianhoffman/e25899e8fca3c1ef8772f331b14031d0

emmatyping · 2018-01-31T04:43:41Z

I'm afraid I don't have the bandwidth for this at the moment. @ianhoffman if you want to take over be my guest. I'll come back to it another time if not.

ilevkivskyi · 2018-02-03T16:14:02Z

It is sad to see this closed, I think it would be a useful addition, please keep me posted if you will resume your work.

emmatyping · 2018-02-03T20:19:18Z

I want to focus on PEP 561 and a personal project, but I might get back to this after a month or two.

bpo-32380 Fix singledispatch interaction with methods

9351f1c

Singledispatch used the first argument to dispatch on. This made it incomatible with methods. This change adds support for methods by dispatching on the second argument if the passed function is a method.

emmatyping requested review from ncoghlan and rhettinger as code owners December 23, 2017 07:12

the-knights-who-say-ni added the CLA signed label Dec 23, 2017

bedevere-bot added the awaiting review label Dec 23, 2017

ilevkivskyi requested changes Dec 23, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 23, 2017

bpo-32380: Implement arg indexing for singledispatch

a69278b

emmatyping commented Dec 25, 2017

View reviewed changes

emmatyping added 2 commits December 24, 2017 18:29

Fix whitespace issues

f19e7a3

Change test suite arguments to arg not index

6ac7fea

ncoghlan reviewed Dec 25, 2017

View reviewed changes

Remove bak file

042734d

ncoghlan requested changes Dec 25, 2017

View reviewed changes

blag mentioned this pull request Jan 22, 2018

RFC: Use singledispatch for registering additional custom serializer functions simplejson/simplejson#203

Closed

emmatyping closed this Jan 31, 2018

pganssle mentioned this pull request Mar 4, 2018

Problems with a decorator used for both methods and functions? python-variants/variants#25

Closed

emmatyping deleted the singledispatch branch March 15, 2018 09:04

emmatyping mentioned this pull request Mar 29, 2018

bpo-32380: Create functools.singledispatchmethod #6306

Merged

		@@ -816,8 +815,15 @@ def register(cls, func=None):
		dispatch_cache.clear()
		return func

		@@ -0,0 +1,2 @@
		``functools.singledispatch`` now supports decorating and registering with
		class methods.

		@@ -0,0 +1,511 @@
		:mod:`functools` --- Higher-order functions and operations on callable objects

		@@ -753,76 +753,78 @@ def singledispatch(func):
		implementations can be registered using the register() attribute of the

Uh oh!

bpo-32380 Fix singledispatch interaction with methods #4987

bpo-32380 Fix singledispatch interaction with methods #4987

Uh oh!

Conversation

emmatyping commented Dec 23, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping commented Jan 31, 2018

Uh oh!

ilevkivskyi commented Feb 3, 2018

Uh oh!

emmatyping commented Feb 3, 2018

Uh oh!

Uh oh!

emmatyping commented Dec 23, 2017 •

edited by bedevere-bot

Loading