Make PyUnicode_CompareWithASCIIString() never failing

[serhiy-storchaka]

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]

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

[zhangyangyu]

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]

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]

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

[vstinner]

I reviewed PyUnicode_CompareWithASCIIString-no-errors.patch.

[vstinner]

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

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

[serhiy-storchaka]

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

[ned-deily]

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]

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