classification
Title: inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: larry, ncoghlan, yselivanov
Priority: release blocker Keywords: 3.4regression, patch

Created on 2014-02-19 12:40 by ncoghlan, last changed 2014-02-19 22:18 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
issue20684_ignore_wrapped_in_argspec.diff ncoghlan, 2014-02-19 13:29 Patch and tests review
issue20684_ignore_wrapped_in_argspec_02.diff yselivanov, 2014-02-19 20:14 review
Messages (12)
msg211609 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 12:40
In a comment on issue 17482, Mike Bayer pointed out a backwards incompatibility resulting from changing inspect.getfullargspec (etc) to rely on inspect.signature: they now follow __wrapped__ chains, where previously they ignored them.

This means that instead of reporting the exact signature of the *wrapper*, they now report the signature of the wrapped function instead.

Since switching these functions from ignoring __wrapped__ to following it is an unintended backwards incompatible change, I'll tweak the code to bypass the unravelling of wrapper chains in the getfullargspec case.
msg211614 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 13:29
Attached patch moves the signature to an internal helper that takes an additional flag - whether or not to follow wrapper chains. getfullargspec() now calls this with the flag turned off, and that is passed down to any recursive calls.

I'll be offline again until tomorrow evening, so don't feel obliged to wait for me if the patch looks good or just needs minor tweaks before merging.

I also noticed a quirk with the getfullargspec -> signature fallback - we still drop the leading arg for partialmethod and other sources of signatures that aren't special cased. That's probably OK though - previously those wouldn't report signatures at all.
msg211649 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 20:14
Nick, thanks for writing this patch!

> That's probably OK though - previously those wouldn't report signatures at all.

I honestly don't think it is OK. I think we should stick to the behaviour of old 'getfullargspec' consistently, to make its behaviour always predictable.

I further tweaked your patch, please review the new one (*_02.diff)

The changes are relatively simple -- I've added a new bool flag to '_signature_internal' -- skip_bound_arg. When it is False, the logic for skipping bound args is off. It also made 'getfullargspec' implementation simpler.

Now 'getfullargspec()' should behave always like it did before + it will handle more callables.

Larry, if you have some time for this, I'd be glad to receive your feedback on this. This issue is quite important.
msg211650 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 20:49
Your version looks good to me, Yury (and good idea to add the second flag).
msg211651 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 20:53
Thanks, Nick. Do you want me to commit the patch?
(Larry will x-ray the patch before including it in 3.4rc2)
msg211652 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 20:58
Yeah, I think push this (with NEWS etc) for 3.4.1, then create the cherry
pick patch for Larry.
msg211653 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-19 21:02
My only question: do they need to be independent flags?  ISTM that they're always either both True or both False.
msg211654 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 21:05
> My only question: do they need to be independent flags?  ISTM that they're always either both True or both False.

I'd keep them independent solely to make the code easier to read/audit. As, for instance, a combined argument won't make sense for someone who just inspects the code of '_signature_from_builtin()' helper.
msg211657 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-19 21:24
I think in 3.5 it may actually make sense to expose these as options
(perhaps with different names), so yeah, I think separate flags is
reasonable.

The only plausible combined name would also be something like "legacy", and
that would need to be assigned to more meaningful local variables for
readability reasons anyway.
msg211659 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 21:31
Committed in 65fb95578f94 
.. forgot to add the issue # in the commit message :(
msg211661 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 21:35
Issue #20688 to track cherry-picking this into 3.4.0
msg211667 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-19 22:18
> I think in 3.5 it may actually make sense to expose these as options
> (perhaps with different names), so yeah, I think separate flags is
> reasonable.

+1 on thinking about exposing your 'follow_wrapper_chains' option in 3.5.

I'll create an issue.
History
Date User Action Args
2014-02-19 22:18:35yselivanovsetmessages: + msg211667
2014-02-19 21:35:43yselivanovsetmessages: + msg211661
2014-02-19 21:31:30yselivanovsetstatus: open -> closed
resolution: fixed
2014-02-19 21:31:09yselivanovsetmessages: + msg211659
2014-02-19 21:24:51ncoghlansetmessages: + msg211657
2014-02-19 21:05:05yselivanovsetmessages: + msg211654
2014-02-19 21:02:08larrysetmessages: + msg211653
2014-02-19 20:58:04ncoghlansetmessages: + msg211652
2014-02-19 20:53:34yselivanovsetmessages: + msg211651
2014-02-19 20:49:44ncoghlansetmessages: + msg211650
2014-02-19 20:14:31yselivanovsetfiles: + issue20684_ignore_wrapped_in_argspec_02.diff

messages: + msg211649
2014-02-19 13:29:42ncoghlansetfiles: + issue20684_ignore_wrapped_in_argspec.diff
title: inspect.getfullargspec (etc) incorrectly follow __wrapped__ chains -> inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains
messages: + msg211614

keywords: + patch
stage: test needed -> patch review
2014-02-19 12:40:21ncoghlancreate