This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: @functools.wraps and abc.abstractmethod interoperability
Type: behavior Stage:
Components: Extension Modules, Library (Lib) Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Dennis Sweeney, erezinman, kamilturek
Priority: normal Keywords:

Created on 2021-01-23 14:26 by erezinman, last changed 2022-04-11 14:59 by admin.

Messages (8)
msg385538 - (view) Author: Erez Zinman (erezinman) Date: 2021-01-23 14:26
Consider the following code:

```
from abc import ABC, abstractmethod
from functools import wraps

class A(ABC):
    @abstractmethod
    def f(self):
        pass
    
    @wraps(f)
    def wrapper(self):
        print('f is called!')
        f()
        
class B(A):
    def f(self):
        print('f!')

B()
```

The last line of code results in the following error:
>>> TypeError: Can't instantiate abstract class B with abstract methods wrapper

That happens because `wraps` copies the `__dict__` of the original function. The problem is that at the point of declaration, the `__dict__` also contained `__isabstractmethod__=True` so it was copied as well, and it caused an error on the class' instantiation even though it contained no abstract methods. 
Moreover, this behavior is misleading because the the wrapper function is not abstract. 

Thanks.
msg388640 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2021-03-14 00:48
I don't think changing @wraps is what you want. Even if you manually set `wrapper.__isabstractmethod__ = False`, you won't reach `print('f is called!')`, since f() is overridden by the child. And if you do that, the ABC wouldn't have any abstract methods, since the name A.f points to the function defined at `def wrapper()`.

If I understand correctly, you want to modify the behavior of a method while also having it be abstract. These two goals are sort of in tension with one another, since defining f as abstract means your implementation won't be used, yet you want to specify part of the implementation. One solution would be to modify the subclass's methods to do what you want when the subclass is created.


from abc import ABC, abstractmethod
from functools import wraps

class A(ABC):
    @abstractmethod
    def f(self):
        pass

    def __init_subclass__(cls, **class_kwargs):
        old_f = cls.f
        @wraps(old_f)
        def wrapper(*args, **kwargs):
            print("f is called!")
            old_f(*args, **kwargs)
        cls.f = wrapper
        super().__init_subclass__()

class B(A):
    def f(self):
        print('f!')

class C(A):
    pass

B().f()
# f is called!
# f!

C()
# TypeError: Can't instantiate abstract class C with abstract method f
msg388641 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2021-03-14 00:51
> the ABC wouldn't have any abstract methods,

I was wrong about this since the @abstractmethod decorator adds 'f' to the __abstractmethods__ set of the ABC, but the rest of my comment stands.
msg389295 - (view) Author: Erez Zinman (erezinman) Date: 2021-03-22 10:07
Sorry for the late response. I forgot about that.

I believe one of us misunderstands the problem. The problem is that `functools.wraps` copies the `__isabstractmethod__` in the first place. 

Consider the following example:

```

class ModuleTemplate(ABC):

    @abstractmethod
    def _internal_main_operation(self, a: int, b: str, c: list) -> bool:
        pass

    @functools.wraps(_internal_main_operation)
    def external_main_operation(self, *args, **kwargs):
        print('LOG: Operation started.')
        try:
            ret = self._internal_main_operation(*args, **kwargs)
        except:
            print('LOG: Operation finished successfully.')
            raise
        else:
            print('LOG: Operation finished successfully.')

        return ret


class ModulePositiveCheck(ModuleTemplate):
    def _internal_main_operation(self, a: int, b: str, c: list) -> bool:
        return a > 0

```

In that case, I have a class that has a main operation that can be called either from the outside or internally. The outside call may be wrapped with some aspect-specific functionality like thread-safety, logging, exception handling, etc.; and the internal call implements the core logic of the class. 
In that (pretty common) pattern, I wouldn't want the `wraps` function to copy all the `abc` attributes because they're irrelevant. In fact, I'm having trouble thinking of a case where you WOULD like these attributes to be copied.


The solution here, I think, is to exclude these attributes from being copied within `functools.update_wrapper`. If you do want to allow copying these attributes (I don't see why, but anyway), you could add an `excluded` parameter to `update_wrapper`.
msg389306 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2021-03-22 11:22
Did you try adding updated=()?

    @functools.wraps(_internal_main_operation, updated=())
    def external_main_operation(self, *args, **kwargs):
msg389307 - (view) Author: Erez Zinman (erezinman) Date: 2021-03-22 12:15
I haven't because I don't need it anymore and it will surely work. Yet by doing so, other attributes will not be copied. I think that the `abc`-related attributes are irrelevant regardless, and should be omitted (at least by default).
msg389308 - (view) Author: Erez Zinman (erezinman) Date: 2021-03-22 12:16
This is an interoperability bug. Maybe not a severe one (due to the workaround), but it's still a bug.
msg389332 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2021-03-22 17:09
> other attributes will not be copied

The signature of wraps() is

    wraps(wrapped, assigned=('__module__', '__name__', '__qualname__', '__doc__', '__annotations__'), updated=('__dict__',))

Passing the updated=() will prevent the __dict__ from being updated, but those other attributes will still be assigned to:

    >>> from functools import wraps
    >>> def f(x: int) -> int:
    ...     "Square of a number"
    ...     return x ** 2
    ...
    >>> @wraps(f, updated=())
    ... def g(*args, **kwargs):
    ...     return f(*args, **kwargs)
    ...
    >>> help(g)
    Help on function f in module __main__:
    
    f(x: int) -> int
        Square of a number

> This is an interoperability bug

This is probably somewhat subjective, but I think the current behavior is okay: copy all of the attributes of the wrapped function into the wrapper. That's predictable, well-specified behavior, even if it has unexpected consequences in some situations -- I would say unusual situations, since 90% of the time I've seen @wraps used is in making custom decorators, where you really do mean to copy *all* of the attributes of the old function into the new one, and never think of the wrapped function again.

My thinking is also that to add a special case for abstract methods in functools would be to unnecessarily couple the functools module to implementation details of the ABC module. If someone using the ABC module wants to not update the __dict__ when using functools.wraps, there's already an easy switch for that, and it's completely orthogonal to what the __dict__ contains.

For an interesting precedent, @abstractclassmethod was created in https://bugs.python.org/issue5867 to solve a similar (I think) interoperability problem between @abstractmethod and @classmethod rather than adding a special case to @classmethod.

I would be interested in hearing if others want something to change about wraps().
History
Date User Action Args
2022-04-11 14:59:40adminsetgithub: 87176
2021-03-22 17:09:32Dennis Sweeneysetmessages: + msg389332
2021-03-22 12:16:43erezinmansetmessages: + msg389308
2021-03-22 12:15:41erezinmansetmessages: + msg389307
2021-03-22 11:22:33Dennis Sweeneysetmessages: + msg389306
2021-03-22 10:07:14erezinmansetmessages: + msg389295
2021-03-14 00:51:28Dennis Sweeneysetmessages: + msg388641
2021-03-14 00:48:30Dennis Sweeneysetnosy: + Dennis Sweeney
messages: + msg388640
2021-03-13 19:04:43kamiltureksetnosy: + kamilturek
2021-01-23 14:26:11erezinmancreate