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

inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains #64883

Closed
ncoghlan opened this issue Feb 19, 2014 · 12 comments
Closed

inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains #64883

ncoghlan opened this issue Feb 19, 2014 · 12 comments
Labels
release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ncoghlan
Copy link
Contributor

BPO 20684
Nosy @ncoghlan, @larryhastings, @1st1, @asottile
PRs
  • bpo-20684: Remove unused inspect._signature_get_bound_param #21100
  • Files
  • issue20684_ignore_wrapped_in_argspec.diff: Patch and tests
  • issue20684_ignore_wrapped_in_argspec_02.diff
  • 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 = None
    closed_at = <Date 2014-02-19.21:31:30.758>
    created_at = <Date 2014-02-19.12:40:21.646>
    labels = ['type-bug', 'library', 'release-blocker']
    title = 'inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains'
    updated_at = <Date 2020-07-30.18:16:47.283>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2020-07-30.18:16:47.283>
    actor = 'Anthony Sottile'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-02-19.21:31:30.758>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2014-02-19.12:40:21.646>
    creator = 'ncoghlan'
    dependencies = []
    files = ['34143', '34147']
    hgrepos = []
    issue_num = 20684
    keywords = ['patch', '3.4regression']
    message_count = 12.0
    messages = ['211609', '211614', '211649', '211650', '211651', '211652', '211653', '211654', '211657', '211659', '211661', '211667']
    nosy_count = 4.0
    nosy_names = ['ncoghlan', 'larry', 'yselivanov', 'Anthony Sottile']
    pr_nums = ['21100']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20684'
    versions = ['Python 3.4']

    @ncoghlan
    Copy link
    Contributor Author

    In a comment on bpo-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.

    @ncoghlan ncoghlan added release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 19, 2014
    @ncoghlan
    Copy link
    Contributor Author

    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.

    @ncoghlan ncoghlan changed the title inspect.getfullargspec (etc) incorrectly follow __wrapped__ chains inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains Feb 19, 2014
    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

    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.

    @ncoghlan
    Copy link
    Contributor Author

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

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

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

    @ncoghlan
    Copy link
    Contributor Author

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

    @larryhastings
    Copy link
    Contributor

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

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

    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.

    @ncoghlan
    Copy link
    Contributor Author

    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.

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

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

    @1st1 1st1 closed this as completed Feb 19, 2014
    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

    Issue bpo-20688 to track cherry-picking this into 3.4.0

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2014

    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.

    @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
    release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants