[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

Thor Whalen  added the comment:

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 
wrote:

>
> Jelle Zijlstra  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 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Jelle Zijlstra]

Jelle Zijlstra  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Jelle Zijlstra]

Jelle Zijlstra  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Jelle Zijlstra]

Jelle Zijlstra  added the comment:

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 "", line 1, in 
TypeError: f() missing 1 required positional argument: 'y'

--
nosy: +Jelle Zijlstra

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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).

--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Caleb Donovick]

Change by Caleb Donovick :


--
nosy: +donovick

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Terry J. Reedy]

Terry J. Reedy  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

Thor Whalen  added the comment:

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 
wrote:

>
> Dominic Davis-Foster  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 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Dominic Davis-Foster]

Dominic Davis-Foster  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

Thor Whalen  added the comment:

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 
wrote:

>
> Terry J. Reedy  added the comment:
>
> Is this actually a bugfix?
>
> --
> nosy: +terry.reedy
> versions: +Python 3.10 -Python 3.8
>
> ___
> Python tracker 
> 
> ___
>

--
nosy: +Thor Whalen

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Terry J. Reedy]

Terry J. Reedy  added the comment:

Is this actually a bugfix?

--
nosy: +terry.reedy
versions: +Python 3.10 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Raymond Hettinger]

Change by Raymond Hettinger :


--
assignee:  -> ncoghlan
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

Thor Whalen  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

Thor Whalen  added the comment:

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

--
keywords: +patch
message_count: 1.0 -> 2.0
pull_requests: +20523
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/21379

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41232] Python `functools.wraps` doesn't deal with defaults correctly

[Thor Whalen]

New submission from Thor Whalen :

# 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?



--
messages: 373254
nosy: Thor Whalen2
priority: normal
severity: normal
status: open
title: Python `functools.wraps` doesn't deal with defaults correctly
type: enhancement
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com