classification
Title: functools.update_wrapper mishandles __wrapped__
Type: behavior Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: ezio.melotti, georg.brandl, jcea, lukasz.langa, michael.foord, ncoghlan, python-dev, rhettinger, zzzeek
Priority: normal Keywords:

Created on 2013-03-19 17:35 by ncoghlan, last changed 2014-02-19 12:40 by ncoghlan. This issue is now closed.

Messages (12)
msg184648 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-19 17:35
functools.update_wrapper inadvertently overwrites the just set __wrapped__ attribute when it updates the contents of __dict__.

This means the intended __wrapped__ chain is never created - instead, for every function in a wrapper stack, __wrapped__ will always refer to the innermost function.

This means that using __wrapped__ to bypass functools.lru_cache doesn't work correctly if the decorated function already has __wrapped__ set.

Explicitly setting __signature__ fortunately still works correctly, since that is checked before recursing down through __wrapped__ in inspect.signature.
msg184649 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-19 17:44
There's an interesting backwards compatibility challenge here. We definitely need to fix the misbehaviour, since it can lead to some pretty serious bugs in user code when attempting to bypass the LRU cache decorator if the wrapped function itself had a __wrapped__ attribute.

However, Michael tells me that at least some third party clients of the introspection tools assumed the "__wrapped__ points to the bottom of the wrapper stack" behaviour was intentional rather than just me screwing up the implementation.

The existing docs for update_wrapper are unfortunately ambiguous because they use the term "original function" instead of "wrapped function".
msg184656 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-19 18:17
And as further evidence that I always intended this to be a wrapper chain: issue 13266 :)
msg184657 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-19 18:25
OK, thinking about this a little further, I think it's not as bad as I feared. The number of people likely to be introspecting __wrapped__ is quite small, and updating to the correct recursive code will still do the right thing in existing versions.

So long as we tell people this is coming, and get Georg to highlight it in the release notes for the corresponding 3.3.x point release, we should be able to fix the lru_cache bug without too much collateral damage.
msg189910 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-24 13:33
Georg, just a heads up that I was informed of a fairly significant bug in the __wrapped__ handling which some folks doing function introspection had been assuming was a feature.

I'd like to fix it for 3.3.3, but need your +1 as RM since it *will* break introspection code which assumes the current behaviour was intentional.
msg193086 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-15 11:13
New changeset 13b8fd71db46 by Nick Coghlan in branch 'default':
Close issue 17482: don't overwrite __wrapped__
http://hg.python.org/cpython/rev/13b8fd71db46
msg193087 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-07-15 11:19
I decided I can live with the risk of this biting someone in 3.3 - the combination of using multiple levels of wrapping *and* using __wrapped__ for more than merely introspection seems remote enough to make being conservative with the behavioural change the better course.
msg211578 - (view) Author: mike bayer (zzzeek) Date: 2014-02-19 00:58
this is actually biting me, I think, though I'm having a very hard time getting a small Python script to duplicate it :/.   https://bitbucket.org/zzzeek/alembic/issue/175/test-suite-failure-under-python34 refers to the current problems I'm having.   I am not really familiar with the ramifications of __wrapped__ so I need to learn some more about that but also I'm concerned that a standalone test which attempts to simulate everything as much as possible is not doing the same thing.
msg211583 - (view) Author: mike bayer (zzzeek) Date: 2014-02-19 01:15
i think I found the problem.  sorry for the noise.
msg211585 - (view) Author: mike bayer (zzzeek) Date: 2014-02-19 01:38
OK well, let me just note what the issue is, and I think this is pretty backwards-incompatible, and additionally I really can't find any reasonable way of working around it except for just deleting __wrapped__.  It would be nice if there were some recipe or documentation that could point people to how do do the following pattern:

import functools
import inspect

def my_wrapper(fn):
    def wrapped(x, y, z):
        return my_func(x, y)
    wrapped = functools.update_wrapper(wrapped, fn)
    return wrapped

def my_func(x, y):
    pass

wrapper = my_wrapper(my_func)

# passes for 2.6 - 3.3, fails on 3.4
assert inspect.getargspec(wrapper) == (['x', 'y', 'z'], None, None, None), inspect.getargspec(wrapper)

basically in Alembic we copy out a bunch of decorated functions out somewhere else using inspect(), and that code relies upon seeing the wrappers list of arguments, not the wrapped.   Not that Python 3.4's behavior isn't correct now, but this seems like something that might be somewhat common.
msg211606 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 05:26
Mike, could you file a new issue for that? It's a genuine regression in the
inspect module.
msg211610 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 12:40
New release blocker created as issue 20684
History
Date User Action Args
2014-02-19 12:40:46ncoghlansetmessages: + msg211610
2014-02-19 05:26:02ncoghlansetmessages: + msg211606
2014-02-19 01:38:12zzzeeksetmessages: + msg211585
2014-02-19 01:15:16zzzeeksetmessages: + msg211583
2014-02-19 00:58:50zzzeeksetnosy: + zzzeek
messages: + msg211578
2013-07-15 11:19:16ncoghlansetmessages: + msg193087
versions: - Python 3.3
2013-07-15 11:13:30python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg193086

resolution: fixed
stage: test needed -> resolved
2013-05-24 13:42:48lukasz.langasetnosy: + lukasz.langa
2013-05-24 13:33:13ncoghlansetnosy: + georg.brandl
messages: + msg189910
2013-05-24 13:27:13ncoghlansetassignee: ncoghlan
2013-03-25 03:16:24jceasetnosy: + jcea
2013-03-24 13:56:31ezio.melottisetnosy: + ezio.melotti
2013-03-19 18:25:44ncoghlansetmessages: + msg184657
2013-03-19 18:17:51ncoghlansetmessages: + msg184656
2013-03-19 18:16:05ncoghlanlinkissue13266 dependencies
2013-03-19 17:44:45ncoghlansetnosy: + rhettinger, michael.foord
title: functools.update_wrapper doesn't overwrite attributes correctly -> functools.update_wrapper mishandles __wrapped__
messages: + msg184649

versions: + Python 3.3
2013-03-19 17:35:42ncoghlancreate