Wrong PEP 515 parsing in decimal module (both C and Python versions)

[skirpichev]

BPO	44267
Nosy	@rhettinger, @facundobatista, @mdickinson, @skirpichev
Files	_pydecimal-pep515.diff 0001-bpo-44267-fix-parsing-Decimal-s-with-underscores.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 2021-05-30.09:05:38.324>
created_at = <Date 2021-05-30.04:12:10.330>
labels = ['extension-modules', 'library', '3.11']
title = 'Wrong PEP 515 parsing in decimal module (both C and Python versions)'
updated_at = <Date 2021-05-30.09:20:39.447>
user = 'https://github.com/skirpichev'

bugs.python.org fields:

activity = <Date 2021-05-30.09:20:39.447>
actor = 'Sergey.Kirpichev'
assignee = 'none'
closed = True
closed_date = <Date 2021-05-30.09:05:38.324>
closer = 'mark.dickinson'
components = ['Extension Modules', 'Library (Lib)']
creation = <Date 2021-05-30.04:12:10.330>
creator = 'Sergey.Kirpichev'
dependencies = []
files = ['50071', '50072']
hgrepos = []
issue_num = 44267
keywords = ['patch']
message_count = 8.0
messages = ['394750', '394755', '394756', '394757', '394758', '394759', '394760', '394761']
nosy_count = 4.0
nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'Sergey.Kirpichev']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44267'
versions = ['Python 3.11']

[skirpichev]

While working on bpo-44258 I discover that the decimal module doesn't follow specification in PEP-515: "The current proposal is to allow one underscore between digits, and after base specifiers in numeric literals." (c)

For example:
>>> float("1.1__1")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: could not convert string to float: '1.1__1'

but
>>> from decimal import Decimal as C
>>> from _pydecimal import Decimal as P
>>> C("1.1__1")
Decimal('1.11')
>>> P("1.1__1")
Decimal('1.11')

Maybe this requirement could be relaxed in PEP, but it seems - this was already discussed (see Alternative Syntax section). Hence, I think this is a bug.

Patch for _pydecimal attached.

[mdickinson]

There was some discussion of this on the python-dev mailing list at the time - see https://mail.python.org/pipermail/python-dev/2016-March/143556.html and the surrounding thread. It's probably best left alone.

[skirpichev]

On Sun, May 30, 2021 at 08:20:14AM +0000, Mark Dickinson wrote:

There was some discussion of this on the python-dev mailing list at the time

I see.

It's probably best left alone.

PEP-515 is clear. If this is not a bug - it should be adjusted (as it
claims to cover Decimal's among other stuff).

[mdickinson]

If this is not a bug - it should be adjusted

Standards Track PEPs are historical documents; it's quite common that an actual implementation ends up diverging from a PEP in small ways after the PEP is accepted, and we don't usually do post-hoc updates in those situations.

Possibly the decimal documentation could be updated, though.

[skirpichev]

On Sun, May 30, 2021 at 08:32:40AM +0000, Mark Dickinson wrote:

Standards Track PEPs are historical documents; it's quite common that an
actual implementation ends up diverging from a PEP in small ways after
the PEP is accepted, and we don't usually do post-hoc updates in those situations.

Well, then I something misunderstood in PEP-0:
--->8------
If changes based on implementation experience and user feedback are made
to Standards track PEPs while in the Accepted or Provisional State,
those changes should be noted in the PEP, such that the PEP accurately
describes the state of the implementation at the point where it is
marked Final.
---->8--------
I don't think that PEP describes the state of art in the decimal module.

Possibly the decimal documentation could be updated, though.

The current behaviour is documented. Do you mean we should document
disagreement with PEP as well?

Regarding mail thread: I don't think that following the PEP will
slow down string conversion. Also, probably we want that strings
that supported by float() and Decimal() were interchangeable.

[mdickinson]

Well, then I something misunderstood in PEP-0

Yep, you're absolutely right. I should have said "after the PEP is final", not "after the PEP is accepted". PEP-515 was marked final on April 28th, 2017.

The current behaviour is documented.

Thanks; I missed that. In that case, I don't think there's anything to do here documentation-wise.

[mdickinson]

Regarding mail thread: I don't think that following the PEP will
slow down string conversion.

Sorry, I just don't think it's worth re-opening this discussion; Stefan Krah had good reasons (not just speed) to avoid implementing a precise interpretation of PEP-515 for Decimal.

It would also be a backwards incompatible change at this point to start refusing strings that were previously accepted. As I said, it's probably best left alone at this point.

[skirpichev]

On Sun, May 30, 2021 at 08:58:56AM +0000, Mark Dickinson wrote:

Yep, you're absolutely right. I should have said "after the PEP is final"

Unfortunately, neither correction can fix that the PEP does not
"accurately describes the state of the implementation at the point where
it is marked Final."

It would also be a backwards incompatible change at this point to
start refusing strings that were previously accepted.

I'm not sure...

Well, it's not so clear which strings are accepted previously (i.e.
what's was documented). PEP-515 claims one. The docs says something
different:
---->8---
If value is a string, it should conform to the decimal numeric string
syntax after leading and trailing whitespace characters, as well as
underscores throughout, are removed
---->8-------

and
--->8------
Underscores are allowed for grouping, _as with integral and
floating-point literals in code_.
-->8-------

The 1-st sentence doesn't specify the way underscores are removed. And
given the 2-nd sentence: it's clearly can't be like the current
behaviour of the Decimal constructor.