classification
Title: Make PyUnicode_CompareWithASCIIString() never failing
Type: behavior Stage: resolved
Components: Interpreter Core, Unicode Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: ezio.melotti, larry, ned.deily, python-dev, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-11-26 15:48 by serhiy.storchaka, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
PyUnicode_CompareWithASCIIString-no-errors.patch serhiy.storchaka, 2016-11-26 16:06 review
PyUnicode_CompareWithASCIIString-no-errors-2.patch serhiy.storchaka, 2016-12-02 23:40 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (12)
msg281783 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-26 15:48
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 issue28701. 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().
msg281842 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-11-28 04:51
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.
msg281844 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-28 07:14
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.
msg281934 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-11-29 02:42
I'd like @haypo to review and approve this change as well since he was involved in the predecessor (Issue28701).  Victor?
msg281958 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-11-29 07:46
I reviewed  PyUnicode_CompareWithASCIIString-no-errors.patch.
msg281966 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-11-29 08:17
>> 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 issue28701.

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.
msg281968 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-29 08:21
Usually we don't add "versionchanged" for every fixed bug.
msg282259 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-02 23:40
Updated patch addresses some Victor's comments. But I disagree that this bugfix needs a versionchanged directive.
msg282457 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-12-05 20:07
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.
msg282467 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-05 21:58
PyUnicode_CompareWithASCIIString-no-errors-2.patch LGTM. Go ahead Serhiy.
msg282473 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-05 22:20
New changeset b431d39da67f by Serhiy Storchaka in branch '3.5':
Issue #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 #28808: PyUnicode_CompareWithASCIIString() now never raises exceptions.
https://hg.python.org/cpython/rev/5bdc8e1a50c8

New changeset db56e39ea067 by Serhiy Storchaka in branch 'default':
Issue #28808: PyUnicode_CompareWithASCIIString() now never raises exceptions.
https://hg.python.org/cpython/rev/db56e39ea067
msg282475 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-05 22:25
Thanks Ned and Victor.
History
Date User Action Args
2017-03-31 16:36:19dstufftsetpull_requests: + pull_request935
2016-12-05 22:25:35serhiy.storchakasetstatus: open -> closed
priority: release blocker -> normal
messages: + msg282475

resolution: fixed
stage: patch review -> resolved
2016-12-05 22:20:52python-devsetnosy: + python-dev
messages: + msg282473
2016-12-05 22:02:54serhiy.storchakasetassignee: serhiy.storchaka
2016-12-05 21:58:05vstinnersetmessages: + msg282467
2016-12-05 20:07:09ned.deilysetmessages: + msg282457
2016-12-02 23:40:09serhiy.storchakasetfiles: + PyUnicode_CompareWithASCIIString-no-errors-2.patch

messages: + msg282259
2016-11-29 08:21:58serhiy.storchakasetmessages: + msg281968
2016-11-29 08:17:59vstinnersetmessages: + msg281966
2016-11-29 07:46:19vstinnersetmessages: + msg281958
2016-11-29 02:42:30ned.deilysetpriority: normal -> release blocker
nosy: + larry
2016-11-29 02:42:04ned.deilysetmessages: + msg281934
2016-11-28 07:14:00serhiy.storchakasetnosy: + ned.deily
messages: + msg281844
2016-11-28 04:51:35xiang.zhangsetnosy: + xiang.zhang
messages: + msg281842
2016-11-26 16:06:06serhiy.storchakasetfiles: + PyUnicode_CompareWithASCIIString-no-errors.patch
2016-11-26 16:05:20serhiy.storchakasetfiles: - PyUnicode_CompareWithASCIIString-no-errors.patch
2016-11-26 15:48:34serhiy.storchakacreate