From 64d5ed4c69d06008e6078664624acfa5052be938 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 11 Jul 2014 15:04:51 -0400 Subject: [PATCH 1/5] Bind dispached methods, and don't rely on "self" argument to infer methods. These two changes are not dependent on each other (sorry for having them in the same commit/PR). I don't know how solid either of the changes are, but I think they are worth exploring. Previously, MethodDispatcher's `__get__` would be called each time a method was called. Moreover, the method dispatcher does not *look* like a method. So, we now bind the method dispatcher as a method when `__get__` is called, which should only happen once. This is similar to how regular functions are bound as methods in Python, although I don't know exactly *when* the magic normally occurs for regular methods. The second tweak changes how methods are inferred. Previously, the signature was inspected, and it was determined to be a method if the first argument was named "self". This fails for functions declared outside of a class, or--goodness forbid--somebody doesn't use "self" as the first argument of a method. There is another way. In the standard Python data model, classes are the only objects that have the `__module__` attribute. Interestingly, this appears to be already defined in the local scope of the class definition. Hence, we can use `inspect` to check in `f_locals`. In practice, "self" should *always* be used as the first item in method signatures. In other words, I'm not concerned about false-negatives (i.e., when "self" is not used for methods). This means that we now have two criteria, each of which may give false-positives, but not false-negatives. Therefore, we may want to check for both "self" in the signature *and* "__module__" in the `f_locals`. --- multipledispatch/core.py | 3 +- multipledispatch/dispatcher.py | 16 ++++++---- multipledispatch/tests/test_core.py | 48 +++++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 11 deletions(-) diff --git a/multipledispatch/core.py b/multipledispatch/core.py index ecbf84a..16a4081 100644 --- a/multipledispatch/core.py +++ b/multipledispatch/core.py @@ -76,5 +76,4 @@ def ismethod(func): Note that this has to work as the method is defined but before the class is defined. At this stage methods look like functions. """ - spec = inspect.getargspec(func) - return spec and spec.args and spec.args[0] == 'self' + return '__module__' in inspect.currentframe().f_back.f_back.f_locals diff --git a/multipledispatch/dispatcher.py b/multipledispatch/dispatcher.py index 6fd3728..40b5b3d 100644 --- a/multipledispatch/dispatcher.py +++ b/multipledispatch/dispatcher.py @@ -1,3 +1,4 @@ +import types from .conflict import ordering, ambiguities, super_signature, AmbiguityWarning from warnings import warn from .utils import expand_tuples @@ -203,14 +204,17 @@ class MethodDispatcher(Dispatcher): Dispatcher """ def __get__(self, instance, owner): - self.obj = instance - self.cls = owner - return self - - def __call__(self, *args, **kwargs): + self.__name__ = str(self) + asmethod = types.MethodType(self, None, owner) + setattr(owner, self.name, asmethod) + if instance is None: + return getattr(owner, self.name) + return getattr(instance, self.name) + + def __call__(self, obj, *args, **kwargs): types = tuple([type(arg) for arg in args]) func = self.resolve(types) - return func(self.obj, *args, **kwargs) + return func(obj, *args, **kwargs) def str_signature(sig): diff --git a/multipledispatch/tests/test_core.py b/multipledispatch/tests/test_core.py index 373ce99..79088ef 100644 --- a/multipledispatch/tests/test_core.py +++ b/multipledispatch/tests/test_core.py @@ -189,6 +189,21 @@ def g(self, x): def test_methods_multiple_dispatch(): + class Foo(object): + @dispatch(A) + def f(self, y): + return 1 + + @dispatch(C) + def f(self, y): + return 2 + + foo = Foo() + assert foo.f(A()) == 1 + assert foo.f(C()) == 2 + + +def test_methods_multiple_dispatch_fail(): class Foo(object): @dispatch(A, A) def f(x, y): @@ -198,8 +213,35 @@ def f(x, y): def f(x, y): return 2 + @dispatch(int) + def f(x, y): # 'x' as self + return 1 + y foo = Foo() - assert foo.f(A(), A()) == 1 - assert foo.f(A(), C()) == 2 - assert foo.f(C(), C()) == 2 + raises(TypeError, lambda: foo.f(A(), A())) + raises(TypeError, lambda: foo.f(A(), C())) + raises(TypeError, lambda: foo.f(C(), C())) + assert foo.f(2) == 3 + + +def test_function_with_self(): + @dispatch(A, A) + def f(self, x): + return 1 + + @dispatch(A, C) + def f(self, x): + return 2 + + @dispatch(C, A) + def f(self, x): + return 3 + + @dispatch(C, C) + def f(self, x): + return 4 + + assert f(A(), A()) == 1 + assert f(A(), C()) == 2 + assert f(C(), A()) == 3 + assert f(C(), C()) == 4 From 95ebca45aed7d25d2c47f5dd5e6682abc8068aaa Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 11 Jul 2014 15:58:38 -0400 Subject: [PATCH 2/5] Python 3 fixes for MethodDispatcher as a method --- multipledispatch/dispatcher.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/multipledispatch/dispatcher.py b/multipledispatch/dispatcher.py index 40b5b3d..5a32118 100644 --- a/multipledispatch/dispatcher.py +++ b/multipledispatch/dispatcher.py @@ -205,11 +205,18 @@ class MethodDispatcher(Dispatcher): """ def __get__(self, instance, owner): self.__name__ = str(self) - asmethod = types.MethodType(self, None, owner) - setattr(owner, self.name, asmethod) - if instance is None: - return getattr(owner, self.name) - return getattr(instance, self.name) + try: + asmethod = types.MethodType(self, instance, owner) + except TypeError: + # Python3 is slightly different + if instance is None: + # no unbound methods + asmethod = self + else: + asmethod = types.MethodType(self, instance) + if instance is not None: + setattr(instance, self.name, asmethod) + return asmethod def __call__(self, obj, *args, **kwargs): types = tuple([type(arg) for arg in args]) From 3b921901e81b5d3728bebda6d5583ef390be0855 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Mon, 14 Jul 2014 13:41:07 -0400 Subject: [PATCH 3/5] Simpler method binding. --- multipledispatch/dispatcher.py | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/multipledispatch/dispatcher.py b/multipledispatch/dispatcher.py index 5a32118..acf7d52 100644 --- a/multipledispatch/dispatcher.py +++ b/multipledispatch/dispatcher.py @@ -204,19 +204,11 @@ class MethodDispatcher(Dispatcher): Dispatcher """ def __get__(self, instance, owner): - self.__name__ = str(self) - try: - asmethod = types.MethodType(self, instance, owner) - except TypeError: - # Python3 is slightly different - if instance is None: - # no unbound methods - asmethod = self - else: - asmethod = types.MethodType(self, instance) - if instance is not None: - setattr(instance, self.name, asmethod) - return asmethod + dispatcher = self + def method(self, *args, **kwargs): + return dispatcher(self, *args, **kwargs) + method.__name__ = self.name + return method.__get__(instance, owner) def __call__(self, obj, *args, **kwargs): types = tuple([type(arg) for arg in args]) From 21f8b052d0ced0ed5e0fb49262c7e9205eed9bc0 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Mon, 14 Jul 2014 14:17:58 -0400 Subject: [PATCH 4/5] Add method dispatch test. The old approach fails this test. The previous method of setting `self.obj` in `__get__` is not reentrant, and should not be used in multi-threaded applications. Moreover, this approach can fail in a single thread too. This is what the new test tests. --- multipledispatch/dispatcher.py | 1 - multipledispatch/tests/test_core.py | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/multipledispatch/dispatcher.py b/multipledispatch/dispatcher.py index acf7d52..bb5cb19 100644 --- a/multipledispatch/dispatcher.py +++ b/multipledispatch/dispatcher.py @@ -1,4 +1,3 @@ -import types from .conflict import ordering, ambiguities, super_signature, AmbiguityWarning from warnings import warn from .utils import expand_tuples diff --git a/multipledispatch/tests/test_core.py b/multipledispatch/tests/test_core.py index 79088ef..32fa778 100644 --- a/multipledispatch/tests/test_core.py +++ b/multipledispatch/tests/test_core.py @@ -245,3 +245,30 @@ def f(self, x): assert f(A(), C()) == 2 assert f(C(), A()) == 3 assert f(C(), C()) == 4 + + +def test_method_dispatch_is_safe(): + class Foo(object): + def __init__(self, x): + self.x = x + + @dispatch(int) + def f(self, y): + return self.x + y + + @dispatch(float) + def f(self, y): + return self.x - y + + foo1 = Foo(1) + foo2 = Foo(2) + assert foo1.f(1) == 2 + assert foo1.f(1.0) == 0.0 + assert foo2.f(1) == 3 + assert foo2.f(1.0) == 1.0 + f1 = foo1.f + f2 = foo2.f + assert f1(1) == 2 + assert f1(1.0) == 0.0 + assert f2(1) == 3 + assert f2(1.0) == 1.0 From 80fafcf240e889814b1377703028e4b74ea0a70f Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Mon, 14 Jul 2014 14:32:13 -0400 Subject: [PATCH 5/5] Also use 'self' to infer class method --- multipledispatch/core.py | 13 +++++++++++-- multipledispatch/tests/test_core.py | 9 +++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/multipledispatch/core.py b/multipledispatch/core.py index 16a4081..7f2ad04 100644 --- a/multipledispatch/core.py +++ b/multipledispatch/core.py @@ -57,7 +57,7 @@ def dispatch(*types, **kwargs): def _(func): name = func.__name__ - if ismethod(func): + if ismethod(func) and isinclass(): dispatcher = inspect.currentframe().f_back.f_locals.get(name, MethodDispatcher(name)) else: @@ -76,4 +76,13 @@ def ismethod(func): Note that this has to work as the method is defined but before the class is defined. At this stage methods look like functions. """ - return '__module__' in inspect.currentframe().f_back.f_back.f_locals + spec = inspect.getargspec(func) + return spec and spec.args and spec.args[0] == 'self' + + +def isinclass(n=1): + """ Is the nth previous frame in a class definition?""" + frame = inspect.currentframe().f_back # escape from current function + for _ in range(n): + frame = getattr(frame, 'f_back') + return '__module__' in frame.f_locals diff --git a/multipledispatch/tests/test_core.py b/multipledispatch/tests/test_core.py index 32fa778..4ae2b51 100644 --- a/multipledispatch/tests/test_core.py +++ b/multipledispatch/tests/test_core.py @@ -218,10 +218,11 @@ def f(x, y): # 'x' as self return 1 + y foo = Foo() - raises(TypeError, lambda: foo.f(A(), A())) - raises(TypeError, lambda: foo.f(A(), C())) - raises(TypeError, lambda: foo.f(C(), C())) - assert foo.f(2) == 3 + # We require the 'self' argument to be used to infer methods + assert foo.f(A(), A()) == 1 + assert foo.f(A(), C()) == 2 + assert foo.f(C(), C()) == 2 + assert raises(TypeError, lambda: foo.f(2)) def test_function_with_self():