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: Python `functools.wraps` doesn't deal with defaults correctly
Type: enhancement Stage: patch review
Components: Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: JelleZijlstra, Thor Whalen, Thor Whalen2, domdfcoding, donovick, gregory.p.smith, ncoghlan, terry.reedy
Priority: normal Keywords: patch

Created on 2020-07-07 22:00 by Thor Whalen2, last changed 2022-04-11 14:59 by admin.

Pull Requests
URL Status Linked Edit
PR 21379 closed Thor Whalen2, 2020-07-07 22:10
Messages (15)
msg373254 - (view) Author: Thor Whalen (Thor Whalen2) Date: 2020-07-07 22:00
# PROBLEM

When using `functools.wraps`, the signature claims one set of defaults, but the (wrapped) function uses the original (wrappee) defaults.

Why might that be the desirable default?

# PROPOSED SOLUTION
Adding '__defaults__',  '__kwdefaults__' to `WRAPPER_ASSIGNMENTS` so that actual defaults can be consistent with signature defaults.

# Code Demo

```python
from functools import wraps
from inspect import signature

def g(a: float, b=10):
    return a * b

def f(a: int, b=1):
    return a * b

assert f(3) == 3

f = wraps(g)(f)

assert str(signature(f)) == '(a: float, b=10)'  # signature says that b defaults to 10
assert f.__defaults__ == (1,)  # ... but it's a lie!
assert f(3) == 3 != g(3) == 30  # ... and one that can lead to problems!
```

Why is this so? Because `functools.wraps` updates the `__signature__` (including annotations and defaults), `__annotations__`, but not `__defaults__`, which python apparently looks to in order to assign defaults.

One solution is to politely ask wraps to include these defaults.

```python
from functools import wraps, WRAPPER_ASSIGNMENTS, partial

my_wraps = partial(wraps, assigned=(list(WRAPPER_ASSIGNMENTS) + ['__defaults__', '__kwdefaults__']))

def g(a: float, b=10):
    return a * b

def f(a: int, b=1):
    return a * b

assert f(3) == 3

f = my_wraps(g)(f)

assert f(3) == 30 == g(3)
assert f.__defaults__ == (10,)  # ... because now got g defaults!
```

Wouldn't it be better to get this out of the box?

When would I want the defaults that are actually used be different than those mentioned in the signature?

<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
bpo-NNNN: Summary of the changes made
```

Where: bpo-NNNN refers to the issue number in the https://bugs.python.org.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `master`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `master`.

-->
msg373255 - (view) Author: Thor Whalen (Thor Whalen2) Date: 2020-07-07 22:10
Posted to stackoverflow to gather opinions about the issue: https://stackoverflow.com/questions/62782230/python-functools-wraps-doesnt-deal-with-defaults-correctly

Also made GitHub PR: https://github.com/python/cpython/pull/21379
msg373256 - (view) Author: Thor Whalen (Thor Whalen2) Date: 2020-07-07 22:18
Further, note that even with the additional '__defaults__', and '__kwdefaults__', `functools.wraps` breaks when keyword only arguments involved:

```
from functools import wraps, WRAPPER_ASSIGNMENTS, partial

# First, I need to add `__defaults__` and `__kwdefaults__` to wraps, because they don't come for free...
my_wraps = partial(wraps, assigned=(list(WRAPPER_ASSIGNMENTS) + ['__defaults__', '__kwdefaults__']))

def g(a: float, b=10):
    return a * b

def f(a: int,  *, b=1):
    return a * b

# all is well (for now)...
assert f(3) == 3
assert g(3) == 30
```

This:
```
my_wraps(g)(f)(3)
```
raises TypeError (missing required positional argument 'b'), expected

Note that `wraps(g)(f)(3)` doesn't throw a TypeError, but the output is not consistent with the signature (inspect.signature(wraps(g)(f)) is (a: float, b=10), so 3 should be multiplied by 10). This is because __defaults__ wasn't updated. See for example, that third-party from boltons.funcutils import wraps works as expected. And so do (the very popular) wrapt and decorator packages. Boltons works for wraps(f)(g), but not wraps(g)(f) in my example. 

See: https://stackoverflow.com/questions/62782709/pythons-functools-wraps-breaks-when-keyword-only-arguments-involved
msg373499 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2020-07-11 01:58
Is this actually a bugfix?
msg374818 - (view) Author: Thor Whalen (Thor Whalen) * Date: 2020-08-04 14:59
Hi Terry, sorry for the later reply.

Is this a bugfix? Well, I'm not sure what you would call a bug. Can't one
always redefine a bug to be a feature, and visa versa?

I would definitely say that the behavior (seeing one default in the
signature, but a different one actually taking effect) is probably not a
good one -- as this could lead to very hard to find... bugs. It seems in
fact that third party "fix your decorators" packages such as `wrapt` and
`boltons.funcutils` agree, since their implementation of `wraps` doesn't
have this... "misaligned-by-default feature" that `functools.wraps` does.

Unless I'm missing something, my guess of why `functools.wraps` doesn't
include what I put in my pull request is that it breaks some tests. But I
had a look at the failing test and it seems that it is the test that is
"wrong" (i.e. tests for a behavior that really shouldn't be the default).

See comment:
https://github.com/python/cpython/pull/21379#issuecomment-655661983

The question is: Is there a lot of code out there that depends on this
misaligned behavior. My guess is not.

On Fri, Jul 10, 2020 at 9:58 PM Terry J. Reedy <report@bugs.python.org>
wrote:

>
> Terry J. Reedy <tjreedy@udel.edu> added the comment:
>
> Is this actually a bugfix?
>
> ----------
> nosy: +terry.reedy
> versions: +Python 3.10 -Python 3.8
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue41232>
> _______________________________________
>
msg374879 - (view) Author: Dominic Davis-Foster (domdfcoding) * Date: 2020-08-05 12:51
To me your examples seem like a misuse of functools.wraps. IIUC its purpose is to make a wrapper that accepts solely *args and **kwargs appear to be the wrapped function; it doesn't seem to be intended to be used when the wrapper takes arguments of different types or that have different default values. 

I can't think of a situation where you'd use wraps with a decorator that doesn't take just *args and **kwargs. That's not to say there aren't occasions where you'd want to to that, just that wraps isn't the right tool.
msg374881 - (view) Author: Thor Whalen (Thor Whalen) * Date: 2020-08-05 13:36
You are the guardians of the great python, so we can leave it at that if
you want. That said for posterity, I'll offer a defense.

The same "the tools does what it does, and if you need something else, use
another tool" argument could have been applied to vote against adding
`__annotations__` etc. back when it was lacking.
If there were clear use cases for having signature defaults be different
from actual defaults, I'd also agree with the argument.
If it were a complicated fix, I'd also agree with you.

But it's a simple fix that would help avoiding unintended misalignments.

I may be missing something, but the only positive reasons I see for keeping
it the way it is are: (1) backcompatibility safety, and (2) not having to
change the (in my opinion incorrect) tests.

If it is kept as such, I think a documentation warning would go a long way
in making users avoid the trap I myself fell into.

On Wed, Aug 5, 2020 at 8:51 AM Dominic Davis-Foster <report@bugs.python.org>
wrote:

>
> Dominic Davis-Foster <dominic@davis-foster.co.uk> added the comment:
>
> To me your examples seem like a misuse of functools.wraps. IIUC its
> purpose is to make a wrapper that accepts solely *args and **kwargs appear
> to be the wrapped function; it doesn't seem to be intended to be used when
> the wrapper takes arguments of different types or that have different
> default values.
>
> I can't think of a situation where you'd use wraps with a decorator that
> doesn't take just *args and **kwargs. That's not to say there aren't
> occasions where you'd want to to that, just that wraps isn't the right tool.
>
> ----------
> nosy: +domdfcoding
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue41232>
> _______________________________________
>
msg374900 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2020-08-05 21:22
Thor, in the future, when you reply by email, snip off the messages you are replying to.  When you your message is added to the webpage below the earlier message, the copy become extraneous noise.  Quoting the hidden boilerplate on PRs is also useless.

For the purpose of this tracker, a bug is a discrepancy between the code and the docs.  Since I have hardly used wraps nor read it docs in detail, I have no particular opinion.
msg392648 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-05-02 01:03
I'm working on a more specialized case of this for functools.lru_cache in bpo-issue44003.  But I believe this issue also makes sense.  It is basically suggesting that __defaults__ and __kwdefaults__ should be included in the default set of assigned= attributes that functools.update_wrapper applies.

The argument in favor of this is that it already has __annotations__ in the default set.  With that, the default isn't really correct for use on any decorator that modifies the meaning or overall call signature of the function if it happens to have annotations.  as __annotations__ and __defaults__ and __kwdefaults__ are all abstract callable introspection interfaces.

From Python's perspective some may call each of these an Enhancement rather than a Bugfix.  I'm fine with that.  It doesn't come up super often but would make Python things more consistent if it were done.

Your PR makes sense to me, though we'll need to understand and fix that test failure you identified where it appears to be potentially testing for the wrong thing (I haven't studied it enough study to say yet).
msg392658 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2021-05-02 03:52
We should not do this, because the wrapping function may have different defaults, and updating __defaults__ would make it use the wrapped function's defaults.

Example:

>>> def f(y=1):
...     print(y)
... 
>>> f()
1
>>> f.__defaults__
(1,)
>>> f.__defaults__ = ()
>>> f()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: f() missing 1 required positional argument: 'y'
msg392660 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-05-02 04:29
Even if we shouldn't blindly propagate defaults (wrong in some wrapping scenarios), there is still a problem here as noted in Thor's opening message.

A bug exists between functools.wraps and inspect.signature.  The signature reported is wrong.  why?
msg392663 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2021-05-02 04:36
That's because inspect.signature by default follows the `.__wrapped__` attribute, so it gives you the signature for the *wrapped* function. That behavior is occasionally problematic (I ran into it in the context of https://github.com/quora/pyanalyze/issues/82), but I don't think it can be safely changed.
msg392664 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-05-02 04:42
That makes sense. It seems like we're missing information signature() would need to be able to differentiate between when it should default to follow .__wrapped__ vs when it shouldn't.  Neither default can satisfy everyone.
msg392665 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2021-05-02 04:52
We could add a new argument to `@functools.wraps()` to differentiate between a wrapper with the same signature and one with a different signature.

Here's a possible design:
* functools.wraps adds a new keyword-only argument signature_changed. It defaults to False for backward compatibility.
* If signature_changed is True:
  * __annotations__ are not copied
  * __wrapped__ is not set on the wrapping function. Instead, we set a new attribute __wrapped_with_changed_signature__ (that's a pretty terrible name, open to suggestions). This will make inspect.signature not look at the wrapped function.
msg392837 - (view) Author: Thor Whalen (Thor Whalen) * Date: 2021-05-03 19:52
On the surface, seems like a fair design to me: Back-compatible yet solves
this misalignment that bugged me (literally).
It would also force the documentation of `functools.wraps` to mention this
"trap", through describing the `signature_changed` flag.
As for the `__wrapped_with_changed_signature__`; yes, it's terrible. But I
have no better. At least, it's very descriptive!

On Sat, May 1, 2021 at 9:52 PM Jelle Zijlstra <report@bugs.python.org>
wrote:

>
> Jelle Zijlstra <jelle.zijlstra@gmail.com> added the comment:
>
> We could add a new argument to `@functools.wraps()` to differentiate
> between a wrapper with the same signature and one with a different
> signature.
>
> Here's a possible design:
> * functools.wraps adds a new keyword-only argument signature_changed. It
> defaults to False for backward compatibility.
> * If signature_changed is True:
>   * __annotations__ are not copied
>   * __wrapped__ is not set on the wrapping function. Instead, we set a new
> attribute __wrapped_with_changed_signature__ (that's a pretty terrible
> name, open to suggestions). This will make inspect.signature not look at
> the wrapped function.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue41232>
> _______________________________________
>
History
Date User Action Args
2022-04-11 14:59:33adminsetgithub: 85404
2021-05-03 19:52:34Thor Whalensetmessages: + msg392837
2021-05-02 04:52:39JelleZijlstrasetmessages: + msg392665
2021-05-02 04:42:50gregory.p.smithsetmessages: + msg392664
2021-05-02 04:36:05JelleZijlstrasetmessages: + msg392663
2021-05-02 04:29:51gregory.p.smithsetmessages: + msg392660
2021-05-02 03:52:37JelleZijlstrasetnosy: + JelleZijlstra
messages: + msg392658
2021-05-02 01:03:59gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg392648
2020-08-07 03:01:38donovicksetnosy: + donovick
2020-08-05 21:22:55terry.reedysetmessages: + msg374900
2020-08-05 13:36:45Thor Whalensetmessages: + msg374881
2020-08-05 12:51:16domdfcodingsetnosy: + domdfcoding
messages: + msg374879
2020-08-04 14:59:30Thor Whalensetnosy: + Thor Whalen
messages: + msg374818
2020-07-11 01:58:22terry.reedysetnosy: + terry.reedy

messages: + msg373499
versions: + Python 3.10, - Python 3.8
2020-07-08 06:42:47rhettingersetassignee: ncoghlan

nosy: + ncoghlan
2020-07-07 22:18:12Thor Whalen2setmessages: + msg373256
2020-07-07 22:10:41Thor Whalen2setkeywords: + patch

stage: patch review
messages: + msg373255
pull_requests: + pull_request20523
2020-07-07 22:00:22Thor Whalen2create