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.

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.

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.

Your version looks good to me, Yury (and good idea to add the second flag).

Thanks, Nick. Do you want me to commit the patch?
(Larry will x-ray the patch before including it in 3.4rc2)

Yeah, I think push this (with NEWS etc) for 3.4.1, then create the cherry
pick patch for Larry.

My only question: do they need to be independent flags?  ISTM that they're always either both True or both False.

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

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.

Committed in 65fb95578f94 
.. forgot to add the issue # in the commit message :(

Issue #20688 to track cherry-picking this into 3.4.0

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

pull_requests: + pull_request20834

messages: + msg211649

keywords: + patch
stage: test needed -> patch review