-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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.lru_cache omits __defaults__ attribute from wrapped function #88169
Comments
When the C implementation of functools.lru_cache was added in bpo/issue14373, it appears to have omitted setting
functools.update_wrapper() does set __defaults__ so this appears to just be an oversight in Modules/_functoolsmodule.c. |
Where does functools.update_wrapper() set __defaults__? |
That was anecdotal evidence:
|
the pure python functools.lru_cache doesn't get this right either. here's a desirable testcase for this bug:
results in
|
I don't think this should be done. We want the lru_cache to be a pass-through. Applying defaults or keyword-only/positional-only restrictions is the responsibility of the inner function. FWIW, here are the fields that Nick selected to be included in update_wrapper(): ('__module__', '__name__', '__qualname__', '__doc__', '__annotations__'). Those are sufficient to get help() to work which is all we were aiming for: >>> from functools import *
>>> @lru_cache
def cached_func(b=5):
pass
>>> help(cached_func)
Help on _lru_cache_wrapper in module __main__: cached_func(b=5) |
__defaults__ and __kwdefaults__ get used for code introspection. Just as __annotations__ does. __annotations__ is already available on the lru_cache wrapped function. All of those seem to go together from a runtime inspection point of view. |
An inner function can't know if somebody else might want to inspect it. This is a decorator that does not change anything about the argument signature of the wrapped function, carrying over the reference to meta-information about that by default seems to make sense. |
https://bugs.python.org/issue41232 covers the more general case of suggesting changing update_wrapper's behavior. That would alleviate the need to fix the pure python implementation within my own PR. |
I don't really like it. Carrying forward these attributes isn't the norm for wrapping functions. The __defaults__ argument is normally only used where it has an effect rather than in a wrapper where it doesn't. Given that it is mutable, it invites a change that won't work. For example: >>> def pow(base, exp=2):
return base ** exp
>>> pow.__defaults__
(2,)
>>> pow.__defaults__ = (3,)
>>> pow(2)
8 Also, an introspection function can only meaningfully use defaults when accompanied by the names of the fields: >>> pow.__code__.co_varnames
('base', 'exp') However, these aren't visible by directly introspecting the wrapper. FWIW, we've never had a user reported issue regarding the absence of __defaults__. If ain't broke, let's don't "fix" it. Nick and Serhiy, any thoughts? |
Oh, I didn't realize mutating those would actually change the code runtime behavior. But it makes sense, those are needed before the code object is entered. Yeah that is different, and suggests making this the default is not actually desired. (this issue and the other one) I guess our rule is that introspection code really must check for and be ready to handle .__wrapped__ if its goal is robustness? |
rejecting. code trying to make direct use of __defaults__ is likely better off using inspect.signature(). there might be an issue with inspect in some cases (https://bugs.python.org/issue41232) but I do not believe that is true for lru_cache wrapped things. |
I meant that I looked up the code of functools.update_wrapper() and did not see that it sets the __defaults__ attribute. In your example in msg392643 you use functools.update_wrapper() incorrectly. The first argument is wrapper, and the second argument is wrapped. updated_x is func. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: