bpo-41231: wraps default behavior with __annotations__
Hi!
Some time ago I opened a bug and sent a patch, but I realize that I should have started a chat here first, better late than never.
The default behavior of the `functools.wraps` decorator, is to copy over the `__annotations__` from the wrapped function to the wrapper function.
This is ok if the wrapper function has the same signature that the wrapped one (that I guess was the main goal behind it, just a simple wrapper).
The issue comes when the wrapper function has a different signature than the wrapped, for example, the `contextlib.contextmanager` decorator, changes the return value, returning a `_GeneratorContextManager`:
``` def contextmanager(func): """... """ @wraps(func) def helper(*args, **kwds): return _GeneratorContextManager(func, args, kwds) return helper ```
but as it uses `wraps`, the `__annotations__` will be copied over, and the new function will have an incorrect return type there.
In order to improve this, I have some proposals:
1. Keep the default behavior of `wraps`, but change the usage around the library to not copy over the `__annotations__` in places (like `contextmanager`) where the types change.
Advantages are that `wraps` keeps being backwards compatible, though the change in `contextmanager` might force some people to "fix" their annotations, I would consider that a "bugfix", more than a behavior change.
The disadvantage is that you have to know that `wraps` will overwrite the wrapped function annotations by default, and I think that it's broadly used when creating decorators that have different signature/types than the wrapped function, so people will have to explicitly change their code.
2. Change `wraps` so if there's any type of annotation in the wrapper function, will not overwrite it. This is what I did in the PR (though I'm not convinced).
Advantages are that only people that took the time to annotate the wrapper function will see the change, and that will be the change they expect (that's my guess though). It's a bit smart with it, so if you don't specify a return type, will get it from the wrapped function, or if you don't specify types for the arguments will get them from the wrapped function, filling only the gap that was not specified. For everyone else, `wraps` is backwards compatible.
Disadvantages, it's a bit more convoluted as it requires some logic to detect what annotations are defined in the wrapper and which ones in the wrapped and merge them.
3. Change the default behavior of `wraps` and don't overwrite the `__annotations__` property. This is non-backwards compatible, but imo the simplest.
Advantages are that it's very simple, and you'll end up with valid, straight-forward `__annotations__` in the wrapped function (that is, whatever you declare when writing the wrapped function).
Disadvantages, if the goal of the `wraps` decorator was as a helper when wrapping functions in a simple manner (not changing the signature), then this becomes a bit more inconvenient, as it will not copy the `__annotations__` as it was doing by default. It also changes the current default behavior, so it will potentially affect everyone that uses `wraps` currently.
Ideas?
Thanks!
"3" would "break stuff around". I'd be glad if "2" worked - but that have to be well documented and easy to see.
"1" is basically a workaround the current behavior, and people who care for it will have to continue doing that until Python 3.10 is widespread - but ultimately it basically requires that everyone doing that re-implements whatever logic you have in that PR for "2" (thus defeating the very purpose of "@wraps" - I'd rather rebind wrapper.__name__ and __wrapped__ manually)
TL;DR: I agree that "2" is the way to go.
On Fri, 7 Aug 2020 at 09:14, David Caro david@dcaro.es wrote:
Hi!
Some time ago I opened a bug and sent a patch, but I realize that I should have started a chat here first, better late than never.
The default behavior of the `functools.wraps` decorator, is to copy over the `__annotations__` from the wrapped function to the wrapper function.
This is ok if the wrapper function has the same signature that the wrapped one (that I guess was the main goal behind it, just a simple wrapper).
The issue comes when the wrapper function has a different signature than the wrapped, for example, the `contextlib.contextmanager` decorator, changes the return value, returning a `_GeneratorContextManager`:
def contextmanager(func): """... """ @wraps(func) def helper(*args, **kwds): return _GeneratorContextManager(func, args, kwds) return helperbut as it uses `wraps`, the `__annotations__` will be copied over, and the new function will have an incorrect return type there.
In order to improve this, I have some proposals:
- Keep the default behavior of `wraps`, but change the usage around the
library to not copy over the `__annotations__` in places (like `contextmanager`) where the types change.
Advantages are that `wraps` keeps being backwards compatible, though the change in `contextmanager` might force some people to "fix" their annotations, I would consider that a "bugfix", more than a behavior change.
The disadvantage is that you have to know that `wraps` will overwrite the wrapped function annotations by default, and I think that it's broadly used when creating decorators that have different signature/types than the wrapped function, so people will have to explicitly change their code.
- Change `wraps` so if there's any type of annotation in the wrapper
function, will not overwrite it. This is what I did in the PR (though I'm not convinced).
Advantages are that only people that took the time to annotate the wrapper function will see the change, and that will be the change they expect (that's my guess though). It's a bit smart with it, so if you don't specify a return type, will get it from the wrapped function, or if you don't specify types for the arguments will get them from the wrapped function, filling only the gap that was not specified. For everyone else, `wraps` is backwards compatible.
Disadvantages, it's a bit more convoluted as it requires some logic to detect what annotations are defined in the wrapper and which ones in the wrapped and merge them.
- Change the default behavior of `wraps` and don't overwrite the
`__annotations__` property. This is non-backwards compatible, but imo the simplest.
Advantages are that it's very simple, and you'll end up with valid, straight-forward `__annotations__` in the wrapped function (that is, whatever you declare when writing the wrapped function).
Disadvantages, if the goal of the `wraps` decorator was as a helper when wrapping functions in a simple manner (not changing the signature), then this becomes a bit more inconvenient, as it will not copy the `__annotations__` as it was doing by default. It also changes the current default behavior, so it will potentially affect everyone that uses `wraps` currently.
Ideas?
Thanks!
-- David Caro _______________________________________________ Python-ideas mailing list -- python-ideas@python.org To unsubscribe send an email to python-ideas-leave@python.org https://mail.python.org/mailman3/lists/python-ideas.python.org/ Message archived at https://mail.python.org/archives/list/python-ideas@python.org/message/LFF3EX... Code of Conduct: http://python.org/psf/codeofconduct/
On 08/07 10:49, Joao S. O. Bueno wrote:
"3" would "break stuff around". I'd be glad if "2" worked - but that have to be well documented and easy to see.
I've update the PR with improved documentation and comments to make it easier to see.
At this point I'd appreciate if I get some reviews to improve the patch, thanks!
"1" is basically a workaround the current behavior, and people who care for it will have to continue doing that until Python 3.10 is widespread - but ultimately it basically requires that everyone doing that re-implements whatever logic you have in that PR for "2" (thus defeating the very purpose of "@wraps" - I'd rather rebind wrapper.__name__ and __wrapped__ manually)
TL;DR: I agree that "2" is the way to go.
On Fri, 7 Aug 2020 at 09:14, David Caro david@dcaro.es wrote:
Hi!
Some time ago I opened a bug and sent a patch, but I realize that I should have started a chat here first, better late than never.
The default behavior of the `functools.wraps` decorator, is to copy over the `__annotations__` from the wrapped function to the wrapper function.
This is ok if the wrapper function has the same signature that the wrapped one (that I guess was the main goal behind it, just a simple wrapper).
The issue comes when the wrapper function has a different signature than the wrapped, for example, the `contextlib.contextmanager` decorator, changes the return value, returning a `_GeneratorContextManager`:
def contextmanager(func): """... """ @wraps(func) def helper(*args, **kwds): return _GeneratorContextManager(func, args, kwds) return helperbut as it uses `wraps`, the `__annotations__` will be copied over, and the new function will have an incorrect return type there.
In order to improve this, I have some proposals:
- Keep the default behavior of `wraps`, but change the usage around the
library to not copy over the `__annotations__` in places (like `contextmanager`) where the types change.
Advantages are that `wraps` keeps being backwards compatible, though the change in `contextmanager` might force some people to "fix" their annotations, I would consider that a "bugfix", more than a behavior change.
The disadvantage is that you have to know that `wraps` will overwrite the wrapped function annotations by default, and I think that it's broadly used when creating decorators that have different signature/types than the wrapped function, so people will have to explicitly change their code.
- Change `wraps` so if there's any type of annotation in the wrapper
function, will not overwrite it. This is what I did in the PR (though I'm not convinced).
Advantages are that only people that took the time to annotate the wrapper function will see the change, and that will be the change they expect (that's my guess though). It's a bit smart with it, so if you don't specify a return type, will get it from the wrapped function, or if you don't specify types for the arguments will get them from the wrapped function, filling only the gap that was not specified. For everyone else, `wraps` is backwards compatible.
Disadvantages, it's a bit more convoluted as it requires some logic to detect what annotations are defined in the wrapper and which ones in the wrapped and merge them.
- Change the default behavior of `wraps` and don't overwrite the
`__annotations__` property. This is non-backwards compatible, but imo the simplest.
Advantages are that it's very simple, and you'll end up with valid, straight-forward `__annotations__` in the wrapped function (that is, whatever you declare when writing the wrapped function).
Disadvantages, if the goal of the `wraps` decorator was as a helper when wrapping functions in a simple manner (not changing the signature), then this becomes a bit more inconvenient, as it will not copy the `__annotations__` as it was doing by default. It also changes the current default behavior, so it will potentially affect everyone that uses `wraps` currently.
Ideas?
Thanks!
-- David Caro _______________________________________________ Python-ideas mailing list -- python-ideas@python.org To unsubscribe send an email to python-ideas-leave@python.org https://mail.python.org/mailman3/lists/python-ideas.python.org/ Message archived at https://mail.python.org/archives/list/python-ideas@python.org/message/LFF3EX... Code of Conduct: http://python.org/psf/codeofconduct/
participants (2)
-
David Caro -
Joao S. O. Bueno