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

Allow _PyType_Lookup() to raise exceptions #75646

Open
scoder opened this issue Sep 14, 2017 · 6 comments
Open

Allow _PyType_Lookup() to raise exceptions #75646

scoder opened this issue Sep 14, 2017 · 6 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@scoder
Copy link
Contributor

scoder commented Sep 14, 2017

BPO 31465
Nosy @scoder, @vstinner, @serhiy-storchaka
PRs
  • gh-75646: allow _PyType_Lookup() to raise exceptions #3616
  • 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 = None
    created_at = <Date 2017-09-14.08:07:19.605>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Allow _PyType_Lookup() to raise exceptions'
    updated_at = <Date 2017-10-01.08:44:44.109>
    user = 'https://github.com/scoder'

    bugs.python.org fields:

    activity = <Date 2017-10-01.08:44:44.109>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2017-09-14.08:07:19.605>
    creator = 'scoder'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31465
    keywords = ['patch']
    message_count = 6.0
    messages = ['302156', '302211', '302336', '302413', '303009', '303456']
    nosy_count = 3.0
    nosy_names = ['scoder', 'vstinner', 'serhiy.storchaka']
    pr_nums = ['3616']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31465'
    versions = ['Python 3.7']

    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 14, 2017

    Follow-up to bpo-31336:
    The fact that _PyType_Lookup() does not propagate exceptions leaves some space for ambiguity. If, in a chain of MRO lookups, one would fail and a later one would succeed, is it correct to keep trying? What if the first failure actually failed to see an otherwise existing attribute in that class? We simply can't know because the lookup failed for *some* reason.

    I think, the only way to really avoid that ambiguity would be to allow _PyType_Lookup() to raise exceptions, and to make it fail on the first error.

    @scoder scoder added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 14, 2017
    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 14, 2017

    I'm working on a PR for this, but after changing all usages and fixing up some error handling here and there, it results in an interpreter crash for me. I'll try to debug it during the next days.

    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 16, 2017

    Test suite passes now. The crash was due to an uninitialised error flag in one case, which lead the C compiler to do incorrect optimisations on undefined behaviour.

    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 18, 2017

    Question: Do you think it's ok to change the signature of _PyType_Lookup() in this way by adding an error flag, or should I add a new function instead?

    There is no performance difference to PR 3279 since gcc should optimise this flag properly away in most cases, so it's not a question about performance but about backwards compatibility. It's an internal, private function, which suggests that it should be ok, but it's not my decision. Cython calls it in a couple of cases, which I will obviously adapt in the next release, if the signature change is accepted.

    Should the "no MRO after readying" error case be kept? It's the only non-exception error case, setting the error flag to 1. It seems ok to ignore this case, so it's not really an error, somehow. I would change it to return 0 (instead of 1), and thus change the error flag to simply "-1 on error (with exception set), 0 on success". Opinions?

    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 26, 2017

    Any comments on this?

    @serhiy-storchaka
    Copy link
    Member

    I'm positive to this change in general, but this is too complex and delicate code. In needs careful reviewing.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants