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.

Sign up for GitHub

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

Jump to bottom

Make PyUnicode_CompareWithASCIIString() never failing #72994

Closed
serhiy-storchaka opened this issue Nov 26, 2016 · 12 comments
Closed

Make PyUnicode_CompareWithASCIIString() never failing #72994

serhiy-storchaka opened this issue Nov 26, 2016 · 12 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-unicode type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 28808
Nosy @vstinner, @larryhastings, @ned-deily, @ezio-melotti, @serhiy-storchaka, @zhangyangyu
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • PyUnicode_CompareWithASCIIString-no-errors.patch
  • PyUnicode_CompareWithASCIIString-no-errors-2.patch
    		•

    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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-12-05.22:25:35.380>
created_at = <Date 2016-11-26.15:48:34.645>
labels = ['interpreter-core', 'type-bug', '3.7', 'expert-unicode']
title = 'Make PyUnicode_CompareWithASCIIString() never failing'
updated_at = <Date 2017-03-31.16:36:19.842>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:19.842>
actor = 'dstufft'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-12-05.22:25:35.380>
closer = 'serhiy.storchaka'
components = ['Interpreter Core', 'Unicode']
creation = <Date 2016-11-26.15:48:34.645>
creator = 'serhiy.storchaka'
dependencies = []
files = ['45656', '45734']
hgrepos = []
issue_num = 28808
keywords = ['patch']
message_count = 12.0
messages = ['281783', '281842', '281844', '281934', '281958', '281966', '281968', '282259', '282457', '282467', '282473', '282475']
nosy_count = 7.0
nosy_names = ['vstinner', 'larry', 'ned.deily', 'ezio.melotti', 'python-dev', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28808'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']
    @serhiy-storchaka
    Copy link
    Member Author

    PyUnicode_CompareWithASCIIString() never set an exception in 3.2 and earlier versions. Since 3.3 it sets an exception and returns -1 if the first argument is not ready Unicode object, but this was not documented until bpo-28701. Due to undocumenting this behavior many (if not all) callers don't check whether it returned an error.

    Proposed patch restores old behavior of PyUnicode_CompareWithASCIIString().

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-unicode type-bug An unexpected behavior, bug, or error labels Nov 26, 2016
    @zhangyangyu
    Copy link
    Member

    LGTM.

    Although at first I am not in favour of this change but searching Github it seems all usages of this API doesn't check the possible error.

    @serhiy-storchaka
    Copy link
    Member Author

    Thanks Xiang.

    Ned, I ask your permission for committing this patch in 3.6. This is not critical neither new bug, it was introduced in 3.3 and is rarely reproducible. But 3.6b4 is the first version in which the changed behavior was documented, and this patch reverts the documentation change together with fixing the behavior. If release 3.6.0 without this patch, this will encourage users to change their code for handling possible error in PyUnicode_CompareWithASCIIString(), but these changes will be not needed when commit the patch.

    @ned-deily
    Copy link
    Member

    I'd like @Haypo to review and approve this change as well since he was involved in the predecessor (bpo-28701). Victor?

    @vstinner
    Copy link
    Member

    I reviewed PyUnicode_CompareWithASCIIString-no-errors.patch.

    @vstinner
    Copy link
    Member

    > The function was already documented in Python 3.5, so please add a "..
    > versionchanged:: 3.6" to document the API chnange.

    No, this behavior is not documented in any released Python version. The note about the failure was added in bpo-28701.

    Ah wait, you want to push this change into Python 3.5? I would prefer to leave Python 3.5 unchanged.

    Even if you modify Python 3.5, you still have to mention the "versionchanged", since it's a behaviour change.

    @serhiy-storchaka
    Copy link
    Member Author

    Usually we don't add "versionchanged" for every fixed bug.

    @serhiy-storchaka
    Copy link
    Member Author

    Updated patch addresses some Victor's comments. But I disagree that this bugfix needs a versionchanged directive.

    @ned-deily
    Copy link
    Member

    This is currently blocking 360rc1. Victor, Serhiy, we either need to get this in now or wait until 3.6.1. I am leaning towards pushing the latest patch for 3.6 without having a decision about 3.5 yet.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 5, 2016

    PyUnicode_CompareWithASCIIString-no-errors-2.patch LGTM. Go ahead Serhiy.

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 5, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 5, 2016

    New changeset b431d39da67f by Serhiy Storchaka in branch '3.5':
    Issue bpo-28808: PyUnicode_CompareWithASCIIString() now never raises exceptions.
    https://hg.python.org/cpython/rev/b431d39da67f

    New changeset 5bdc8e1a50c8 by Serhiy Storchaka in branch '3.6':
    Issue bpo-28808: PyUnicode_CompareWithASCIIString() now never raises exceptions.
    https://hg.python.org/cpython/rev/5bdc8e1a50c8

    New changeset db56e39ea067 by Serhiy Storchaka in branch 'default':
    Issue bpo-28808: PyUnicode_CompareWithASCIIString() now never raises exceptions.
    https://hg.python.org/cpython/rev/db56e39ea067

    @serhiy-storchaka
    Copy link
    Member Author

    Thanks Ned and Victor.

    @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
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-unicode type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @serhiy-storchaka @zhangyangyu @ned-deily