10000 gh-123378: fix some corner cases of `start` and `end` values in `PyUnicodeErrorObject` by picnixz · Pull Request #123380 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-123378: fix some corner cases of start and end values in PyUnicodeErrorObject #123380

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

Merged
merged 34 commits into from
Dec 4, 2024
Merged

gh-123378: fix some corner cases of start and end values in PyUnicodeErrorObject #123380

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
7a1574d
Fix `PyUnicode{Encode,Decode}Error_GetStart`.
picnixz Aug 26, 2024
6ef0c6d
blurb
picnixz Aug 27, 2024
60ab0bb
add tests
picnixz Aug 27, 2024
67b3d8e
fix NEWS
picnixz Aug 27, 2024
78fff57
remove a duplicated normal case
picnixz Aug 27, 2024
a6e6f80
handle start < 0
picnixz Aug 28, 2024
20c47ba
add C tests
picnixz Aug 28, 2024
51bc77e
add test coverage
picnixz Aug 28, 2024
b290e58
update docs
picnixz Aug 28, 2024
75398a9
fixup
picnixz Aug 28, 2024
546be87
update blurb
picnixz Aug 28, 2024
cded571
address Victor's review
picnixz Aug 29, 2024
1900d9a
refactor name
picnixz Aug 29, 2024
8acc563
fix refcounts
picnixz Aug 29, 2024
0538c83
remove debugging code
picnixz Aug 29, 2024
d5ea357
address Victor's review (round 2)
picnixz Aug 29, 2024
7c10769
handle negative 'start' and 'end' values
picnixz Aug 30, 2024
7ce2ef0
add C API tests
picnixz Aug 30, 2024
b55ca5a
add Python tests
picnixz Aug 30, 2024
4e34e5f
update docs
picnixz Aug 30, 2024
033a1ac
fix typo
picnixz Aug 30, 2024
c802e64
convert macros into `static inline` functions
picnixz Sep 13, 2024
66f33f8
Merge branch 'main' into fix/c-api-unicode-error-get-start
skirpichev Sep 20, 2024
6caf5b6
Merge remote-tracking branch 'upstream/main' into fix/c-api-unicode-e…
picnixz Oct 27, 2024
fcde448
post-merge cleanup
picnixz Oct 27, 2024
2e66302
Merge branch 'main' into fix/capi/unicode-error-start-end-123378
picnixz Nov 19, 2024
dc6b37b
Merge branch 'main' into fix/capi/unicode-error-start-end-123378
picnixz Nov 29, 2024
baa5cb2
fix typo
picnixz Dec 2, 2024
4c4808e
update NEWS and docs
picnixz Dec 2, 2024
efbdff1
add some assertion checks
picnixz Dec 2, 2024
180f3c2
add some assertion checks
picnixz Dec 2, 2024
4bd6e20
Merge branch 'main' into fix/c-api-unicode-error-get-start
picnixz Dec 2, 2024
5759a70
remove failing assertions for now
picnixz Dec 3, 2024
8c12171
fix docs
picnixz Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update docs
  • Loading branch information
@picnixz
picnixz committed Aug 30, 2024
commit 4e34e5fd13fb458f3835221e0e871a65e87663e0
20 changes: 18 additions & 2 deletions Doc/c-api/exceptions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,12 +853,23 @@ The following functions are used to create and modify Unicode exceptions from C.
*\*start*. *start* must not be ``NULL``. Return ``0`` on success, ``-1`` on
failure.

If the :attr:`UnicodeError.object` is an empty sequence, the resulting
*start* is ``0``. Otherwise, it is clipped to ``[0, len(object) - 1]``.

.. seealso:: :attr:`UnicodeError.start`

.. c:function:: int PyUnicodeDecodeError_SetStart(PyObject *exc, Py_ssize_t start)
int PyUnicodeEncodeError_SetStart(PyObject *exc, Py_ssize_t start)
int PyUnicodeTranslateError_SetStart(PyObject *exc, Py_ssize_t start)

Set the *start* attribute of the given exception object to *start*. *start*
must be non-negative. Return ``0`` on success, ``-1`` on failure.
Set the *start* attribute of the given exception object to *start*.
Return ``0`` on success, ``-1`` on failure.

.. note::

While passing a negative *start* does not raise an exception,
the corresponding getters will not consider it as a relative
offset.

.. c:function:: int PyUnicodeDecodeError_GetEnd(PyObject *exc, Py_ssize_t *end)
int PyUnicodeEncodeError_GetEnd(PyObject *exc, Py_ssize_t *end)
Expand All @@ -868,13 +879,18 @@ The following functions are used to create and modify Unicode exceptions from C.
*\*end*. *end* must not be ``NULL``. Return ``0`` on success, ``-1`` on
failure.

If the :attr:`UnicodeError.object` is an empty sequence, the resulting
*end* is ``0``. Otherwise, it is clipped to ``[1, len(object)]``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason why 0 isn't allowed? Empty substrings (start==end) can be useful for special cases.
OTOH, this PR allows “negative” substrings (start>end), which does not sound useful -- and it can throw off code like length = start-end.

IMO, end should be clipped to [clipped(start), len(object)].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason why 0 isn't allowed

The original code did:

    size = PyUnicode_GET_LENGTH(obj);
    if (*end<1)
        *end = 1;
    if (*end>size)
        *end = size;
    Py_DECREF(obj);
    return 0;

So it always clipped it to [1, len(object)] but put it at 0 for empty sequences. I just documented that behaviour correctly (otherwise it could be confusing).

OTOH, this PR allows “negative” substrings (start>end),

I don't know whether I should enforce it or not (we didn't decide on this matter, I only kept what was already there).

IMO, end should be clipped to [clipped(start), len(object)].

This could be a good alternative. I haven't thought of it, so I'll leave this comment opened to discuss your proposal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like not much discussion is coming.
Is there anyone whose opinion you'd like to hear?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd like to know what Victor and Serhiy think on that. The problem is I'm worried to break things even though we are fixing a bug. However, whatever we choose should not make the following crash:

str(UnicodeEncodeError('utf-8', '', -1, 0, ''))

Maybe we can start by fixing __str__? what do you think? (without touching on the getter/setters for now?). I once fixed it by changing the getter/setter implementation but then we didn't really know how to proceed forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A straightforward fix for __str__ sounds like it'd be easy to both review and backport.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on that then.
< 8000 include-fragment loading="lazy" src="/python/cpython/pull/123380/review_comment/1786123893/edit_form?textarea_id=discussion_r1786123893-body&comment_context=diff" data-nonce="v2:3077c472-45fa-9a4d-9599-dedf0914ae4d" data-view-component="true" class="previewable-comment-form js-comment-edit-form-deferred-include-fragment">

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #124935


.. c:function:: int PyUnicodeDecodeError_SetEnd(PyObject *exc, Py_ssize_t end)
int PyUnicodeEncodeError_SetEnd(PyObject *exc, Py_ssize_t end)
int PyUnicodeTranslateError_SetEnd(PyObject *exc, Py_ssize_t end)

Set the *end* attribute of the given exception object to *end*. Return ``0``
on success, ``-1`` on failure.

.. seealso:: :attr:`UnicodeError.end`

.. c:function:: PyObject* PyUnicodeDecodeError_GetReason(PyObject *exc)
PyObject* PyUnicodeEncodeError_GetReason(PyObject *exc)
PyObject* PyUnicodeTranslateError_GetReason(PyObject *exc)
Expand Down
9 changes: 7 additions & 2 deletions Doc/library/exceptions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,13 +642,18 @@ The following exceptions are the exceptions that are usually raised.

.. attribute:: start

The first index of invalid data in :attr:`object`. This value
must be non-negative.
The first index of invalid data in :attr:`object`.

This value should not be negative as it is interpreted as an
absolute offset but this constraint is not enforced at runtime.

.. attribute:: end

The index after the last invalid data in :attr:`object`.

This value should not be negative as it is interpreted as an
absolute offset but this constraint is not enforced at runtime.


.. exception:: UnicodeEncodeError

Expand Down
Loading
0