Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

functools.update_wrapper mishandles __wrapped__ #61684

Closed
ncoghlan opened this issue Mar 19, 2013 · 12 comments
Closed

functools.update_wrapper mishandles __wrapped__ #61684

ncoghlan opened this issue Mar 19, 2013 · 12 comments
Assignees
Labels
type-bug

Comments

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 19, 2013

BPO 17482

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = 'https://github.com/ncoghlan'
closed_at = <Date 2013-07-15.11:13:30.625>
created_at = <Date 2013-03-19.17:35:42.781>
labels = ['type-bug']
title = 'functools.update_wrapper mishandles __wrapped__'
updated_at = <Date 2021-11-04.14:33:49.415>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2021-11-04.14:33:49.415>
actor = 'eryksun'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2013-07-15.11:13:30.625>
closer = 'python-dev'
components = []
creation = <Date 2013-03-19.17:35:42.781>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 17482
keywords = []
message_count = 12.0
messages = ['184648', '184649', '184656', '184657', '189910', '193086', '193087', '211578', '211583', '211585', '211606', '211610']
nosy_count = 0.0
nosy_names = []
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17482'
versions = []

@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Mar 19, 2013

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.

@ncoghlan ncoghlan added the type-bug label Mar 19, 2013
@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Mar 19, 2013

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

@ncoghlan ncoghlan changed the title functools.update_wrapper doesn't overwrite attributes correctly functools.update_wrapper mishandles __wrapped__ Mar 19, 2013
@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Mar 19, 2013

And as further evidence that I always intended this to be a wrapper chain: bpo-13266 :)

@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Mar 19, 2013

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.

@ncoghlan ncoghlan self-assigned this May 24, 2013
@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented May 24, 2013

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.

@python-dev
Copy link
Mannequin

@python-dev python-dev mannequin commented Jul 15, 2013

New changeset 13b8fd71db46 by Nick Coghlan in branch 'default':
Close bpo-17482: don't overwrite __wrapped__
http://hg.python.org/cpython/rev/13b8fd71db46

@python-dev python-dev mannequin closed this as completed Jul 15, 2013
@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Jul 15, 2013

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.

@zzzeek
Copy link
Mannequin

@zzzeek zzzeek mannequin commented Feb 19, 2014

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.

@zzzeek
Copy link
Mannequin

@zzzeek zzzeek mannequin commented Feb 19, 2014

i think I found the problem. sorry for the noise.

@zzzeek
Copy link
Mannequin

@zzzeek zzzeek mannequin commented Feb 19, 2014

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.

@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Feb 19, 2014

Mike, could you file a new issue for that? It's a genuine regression in the
inspect module.

@ncoghlan
Copy link
Contributor Author

@ncoghlan ncoghlan commented Feb 19, 2014

New release blocker created as bpo-20684

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug
Projects
None yet
Development

No branches or pull requests

2 participants