PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A

@malemburg

BPO	7330
Nosy	@malemburg, @mdickinson, @vstinner, @ericvsmith, @ezio-melotti, @lekma, @akheron, @serhiy-storchaka
Files	issue_7330.diff unicode_fromformat_precision.patch unicode_fromformat_precision-2.patch unicode_fromformat_precision-3.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 = None
closed_at = <Date 2013-05-06.21:36:01.271>
created_at = <Date 2009-11-15.19:42:32.127>
labels = ['type-feature', 'expert-unicode']
title = 'PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A'
updated_at = <Date 2013-05-06.21:36:01.270>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2013-05-06.21:36:01.270>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-05-06.21:36:01.271>
closer = 'vstinner'
components = ['Unicode']
creation = <Date 2009-11-15.19:42:32.127>
creator = 'mark.dickinson'
dependencies = []
files = ['21324', '27465', '27479', '30144']
hgrepos = []
issue_num = 7330
keywords = ['patch', 'needs review']
message_count = 42.0
messages = ['95306', '95310', '111802', '111808', '111820', '111894', '112041', '112298', '117995', '117996', '117997', '127690', '128296', '128359', '128381', '128725', '128773', '128776', '128785', '128786', '128790', '128933', '129942', '130258', '131649', '131668', '131710', '131964', '131965', '131968', '132057', '144626', '147861', '147966', '172258', '172262', '172343', '177206', '188476', '188477', '188596', '188597']
nosy_count = 12.0
nosy_names = ['lemburg', 'mark.dickinson', 'vstinner', 'eric.smith', 'ron_adam', 'ezio.melotti', 'lekma', 'ysj.ray', 'python-dev', 'petri.lehtinen', 'serhiy.storchaka', 'Sean.Ochoa']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue7330'
versions = ['Python 3.4']

There seems to be something wrong with the width handling code in
PyUnicode_FromFormat; or perhaps I'm misusing it.

To reproduce: replace the line

return PyUnicode_FromFormat("range(%R, %R)", r->start, r->stop);

in range_repr in Objects/rangeobject.c with

return PyUnicode_FromFormat("range(%20R, %20R)", r->start, r->stop);

On my machine (OS X 10.6), this results in a segfault when invoking
range_repr:

Python 3.2a0 (py3k:76311M, Nov 15 2009, 19:16:40) 
[GCC 4.2.1 (Apple Inc. build 5646)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> range(0, 10)
Segmentation fault

Perhaps these modifiers aren't supposed to be used with a width?

It looks like PyUnicode_FromFormatV is computing callcount incorrectly.
It's looking for 'S', 'R', or 'A' immediately following '%', before the
width. It seems to me it should be treating them the same as 's',
although I'll admit to not having looked at it close enough to know
exactly what's going on.

The whole routine could use some attention, I think.

I feel it's not proper to allow the width restrict on types %S, %R, %A. These types correspond to PyObject_Str(), PyObject_Repr, PyObject_ASCII() respectively, the results of them are usually a complete string representation of a object. If you put a width restriction on the string, it's likely that the result string is intercepted and is of no complete meaning. If you really want to put a width restriction on the result, you can use %s instead, with one or two more lines to get the corresponding char* from the object.

Ray.Allen wrote:

Ray.Allen <ysj.ray@gmail.com> added the comment:

I feel it's not proper to allow the width restrict on types %S, %R, %A. These types correspond to PyObject_Str(), PyObject_Repr, PyObject_ASCII() respectively, the results of them are usually a complete string representation of a object. If you put a width restriction on the string, it's likely that the result string is intercepted and is of no complete meaning. If you really want to put a width restriction on the result, you can use %s instead, with one or two more lines to get the corresponding char* from the object.

I agree with that, but don't feel strongly about not allowing this
use case.

If it's easy to support, why not have it ? Otherwise, I'd be +1 on
adding a check and raise an error in case a width modifier is used
with these markers.

I think under the "we're all consenting adults" doctrine that it should be allowed. If you really want that behavior, why force the char*/%s dance at each call site when it's easy enough to do it in one place? I don't think anyone supplying a width would really be surprised that it would truncate the result and possibly break round-tripping through repr.

Besides, it's allowed in pure python code:
>>> '%.5r' % object()
'<obje'

You can write "%20s" as a argument for PyUnicode_FromFormat(), but it has no effect. The width and precision modifiers are not intended to apply to string formating(%s, %S, %R, %A), only apply to integer(%d, %u, %i, %x). Though you can write "%20s", but you cannot write "%20S", "%20R" and "%20A".

There can be several fixes:

make the presence of width and precision modifiers of %s, %S, %R, %A raise an Exception, like ValueError, instead of segment fault.
make the presence of width and precision modifiers of %s, %S, %R, %A have no effect, just like current %s.
make the presence of width and precision modifiers of %s, %S, %R, %A do have correct effect, like %r and %s in string formatting in python code.

Thanks to Eric's ideas. Now I'm sure I prefer the last fix. I will work out a patch for this.

Is this really worthy to fix?

Here is the patch, it add support to use width and precision formatters in PyUnicode_FromFormat() for type %s, %S, %R, %V, %U, %A, besides fixed two bugs, which at least I believe:

1. According to PyUnicode_FromFormat() doc: http://docs.python.org/dev/py3k/c-api/unicode.html?highlight=pyunicode_fromformat#PyUnicode_FromFormat, the "%A" should produce result of ascii(). But in the existing code, I only find code of  call to ascii(object) and calculate the spaces needed for it, but not appending the ascii() output to result. Also according to my simple test, the %A doesn't work, as the following simple test function:
static PyObject *
getstr(PyObject *self, PyObject *args)
{
    const char *s = "hello world";
    PyObject *unicode = PyUnicode_FromString(s);
    return PyUnicode_FromFormat("%A", unicode);
}
Which should return the result of calling ascii() with the object named *unicode* as its argument. The result should be a unicode object with string "hello world". But it actually return a unicode object with string "%A". This can be fixed by adding the following line:
                   case 'A':
in step 4.

another bug, here is a piece of code in Object/unicodeobject.c, PyUnicode_FromFormatV():

797 if (f == '%') {
798 #ifdef HAVE_LONG_LONG
799 int longlongflag = 0;
800 #endif
801 const char p = f;
802 width = 0;
803 while (ISDIGIT((unsigned)*f))
804 width = (width*10) + *f++ - '0';

Here the variable *width* cannot be correctly calculated, because the while loop will not execute, the *f currently is definitely '%'! So the width is always 0. But currently this doesn't cause error, since the following codes will ensure width >= MAX_LONG_CHARS:

834 case 'd': case 'u': case 'i': case 'x':
835 (void) va_arg(count, int);
836 #ifdef HAVE_LONG_LONG
837 if (longlongflag) {
838 if (width < MAX_LONG_LONG_CHARS)
839 width = MAX_LONG_LONG_CHARS;
840 }
841 else
842 #endif
843 /* MAX_LONG_CHARS is enough to hold a 64-bit integer,
844 including sign. Decimal takes the most space. This
845 isn't enough for octal. If a width is specified we
846 need more (which we allocate later). */
847 if (width < MAX_LONG_CHARS)
848 width = MAX_LONG_CHARS;

(currently width and precision only apply to integer types:%d, %u, %i, %x, not string and object types:%s, %S, %R, %A, %U, %V )

To fix, the following line:
801 const char* p = f;
should be:
801 const char* p = f++;
just as the similar loop in step 4, and add another line:
f--;
after calculate width to adapting the character pointer.

My patch fixed these two problems. Hoping somebody could take a look at it.

I update the patch. Hope somebody could do a review.

Oooops! Sorry for re-submit the request...

I opened other tickets related to PyUnicode_FromFormatV:

bpo-10833 :Replace %.100s by %s in PyErr_Format(): the arbitrary limit of 500 bytes is outdated
bpo-10831: PyUnicode_FromFormatV() doesn't support %li, %lli, %zi
bpo-10830: PyUnicode_FromFormatV("%c") doesn't support non-BMP characters on narrow build
bpo-10829: PyUnicode_FromFormatV() bugs with "%" and "%%" format strings

(see also bpo-10832: Add support of bytes objects in PyBytes_FromFormatV())

PyUnicode_FromFormatV() has now tests in test_unicode: issue_7330.diff should add new tests, at least to check that %20R doesn't crash.

Hum, the issue is still open, I will try to review it.

Issue bpo-13428 has been marked as a duplicate of this issue.

Hi!

I'd like to have this committed to be able to fix bpo-13349. So here's a review.

In Doc/c-api/unicode.rst, the two "versionchanged:: 3.3" directives can be merged
In tests, I'd use 'abcde' rather than 'xxxxx' to make sure that correct characters are copied to the output (hope you understand what I mean)
No test checks that width and precision work on characters rather than bytes
The changes to unicodeobject.c don't apply on top of current default branch.

I rewrote PyUnicode_FromFormatV() to use a single step instead of four: see issue bpo-16147. So it's now simpler to fix this issue. Here is a new patch to implement width and precision modifiers for %s, %A, %R, %S and %U formats.

I read again this old issue. I still think that it would be better to truncate to a number of *bytes* for "%s" format (and %V format when the first argument is NULL) to mimic printf(). The "replace" error handler of the UTF-8 decoder handles truncated string correctly. So I should update my patch.

Updated patch: precision for "%s" and "%V" (if the first PyObject* argument is NULL) formats is now a number of bytes, rather than a number of characters. width is still always a number of character.

The reason is that "%.100s" can be used for avoid a crash if the argument is not terminated by a null character (see issue bpo-10833).

I found one bug and add some nitpicks and optimization suggestion on Rietveld.

New version of my patch taking Serhiy's remarks into account:

add a check_format() function to cleanup unit tests
only call _PyUnicodeWriter_Prepare() once per formatted argument: compute the length and maximum character. Be more optimistic about sprintf() for integer and pointer: expect that the maximum character is 127 or less
uniformize code parsing width and precision
factorize code for '%s' and '%V'

Note: remove also _PyUnicode_WriteSubstring() from the patch, it was already added.

I didn't add the following optimization (proposed by Serhiy in his review) because I'm not convinced that it's faster, and it's unrelated to this issue:

if (width > (PY_SSIZE_T_MAX - 9) / 10
&& width > (PY_SSIZE_T_MAX - ((int)*f - '0')) / 10)
{ ... }

instead of

if (width > (PY_SSIZE_T_MAX - ((int)*f - '0')) / 10)
{ ... }

New changeset 9e0f1c3bf9b6 by Victor Stinner in branch 'default':
Issue bpo-7330: Implement width and precision (ex: "%5.3s") for the format string
http://hg.python.org/cpython/rev/9e0f1c3bf9b6

Finally, I closed this issue. Sorry for the long delay, but many other PyUnicode_FromFormat() issues had to be discussed/fixed before this one can be fixed. It was also much easier to fix this issue since my refactoring of PyUnicode_FromFormat() to only parse the format string once (thanks to the _PyUnicodeWriter API) instead of having 4 steps.

Thanks to Ysj Ray, thanks to reviewers.

This is one of the oldest issue that I had to fix :-)

Navigation Menu

Repository navigation

PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A #51579

20 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A #51579

Description

Activity

mdickinson commented on Nov 15, 2009

ericvsmith commented on Nov 15, 2009

ysjray commented on Jul 28, 2010

malemburg commented on Jul 28, 2010

ericvsmith commented on Jul 28, 2010

ysjray commented on Jul 29, 2010

ysjray commented on Jul 30, 2010

ysjray commented on Aug 1, 2010

ysjray commented on Oct 5, 2010

ysjray commented on Oct 5, 2010

ysjray commented on Oct 5, 2010

vstinner commented on Feb 1, 2011

20 remaining items

vstinner commented on Sep 29, 2011

vstinner commented on Nov 18, 2011

akheron commented on Nov 19, 2011

vstinner commented on Oct 6, 2012

vstinner commented on Oct 6, 2012

vstinner commented on Oct 7, 2012

serhiy-storchaka commented on Dec 9, 2012

vstinner commented on May 5, 2013

vstinner commented on May 5, 2013

python-dev commented on May 6, 2013

vstinner commented on May 6, 2013

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions