Issue41232
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.
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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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:33 | admin | set | github: 85404 |
2021-05-03 19:52:34 | Thor Whalen | set | messages: + msg392837 |
2021-05-02 04:52:39 | JelleZijlstra | set | messages: + msg392665 |
2021-05-02 04:42:50 | gregory.p.smith | set | messages: + msg392664 |
2021-05-02 04:36:05 | JelleZijlstra | set | messages: + msg392663 |
2021-05-02 04:29:51 | gregory.p.smith | set | messages: + msg392660 |
2021-05-02 03:52:37 | JelleZijlstra | set | nosy:
+ JelleZijlstra messages: + msg392658 |
2021-05-02 01:03:59 | gregory.p.smith | set | nosy:
+ gregory.p.smith messages: + msg392648 |
2020-08-07 03:01:38 | donovick | set | nosy:
+ donovick |
2020-08-05 21:22:55 | terry.reedy | set | messages: + msg374900 |
2020-08-05 13:36:45 | Thor Whalen | set | messages: + msg374881 |
2020-08-05 12:51:16 | domdfcoding | set | nosy:
+ domdfcoding messages: + msg374879 |
2020-08-04 14:59:30 | Thor Whalen | set | nosy:
+ Thor Whalen messages: + msg374818 |
2020-07-11 01:58:22 | terry.reedy | set | nosy:
+ terry.reedy messages: + msg373499 versions: + Python 3.10, - Python 3.8 |
2020-07-08 06:42:47 | rhettinger | set | assignee: ncoghlan nosy: + ncoghlan |
2020-07-07 22:18:12 | Thor Whalen2 | set | messages: + msg373256 |
2020-07-07 22:10:41 | Thor Whalen2 | set | keywords:
+ patch stage: patch review messages: + msg373255 pull_requests: + pull_request20523 |
2020-07-07 22:00:22 | Thor Whalen2 | create |