8000 gh-114563: C decimal falls back to pydecimal for unsupported format strings by belm0 · Pull Request #114879 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-114563: C decimal falls back to pydecimal for unsupported format strings #114879

8000
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

Jump to bottom
Merged
merged 12 commits into from
Feb 12, 2024
Merged

gh-114563: C decimal falls back to pydecimal for unsupported format strings #114879

merged 12 commits into from
Feb 12, 2024

Conversation

belm0
Copy link
Contributor
@belm0 belm0 commented Feb 1, 2024 •
edited
Loading

When the mpdecimal lib fails to parse a format string, we fall back to the pydecimal format implementation.

Notably this allows complex workarounds for 'z' format support to be removed. (mpdecimal added 'z' support in version 4.0.0, so this becomes efficient in the long term.)

  • remove 'z' format implementation from C decimal
  • fix 'z' format memory leak
  • fix 'z' format applied to type 'F'
  • fix missing '#' format support

Suggested and prototyped by Stefan Krah.

Fixes gh-114563, gh-91060

belm0 added 3 commits February 1, 2024 12:26
@belm0
add decimal tests
d49d679
@belm0
revert 'z' format support in C decimal
b526004
@belm0
pythongh-114563: C decimal falls back to pydecimal for unsupported fo…
6ac60d7 
…rmat strings

immediate merits:
  * eliminate complex workarounds for 'z' format support
    (NOTE: mpdecimal recently added 'z' support, so this becomes
    efficient in the long term.)
  * fix 'z' format memory leak
  * fix 'z' format applied to 'F'
  * fix missing '#' format support

Suggested and prototyped by Stefan Krah.

Fixes pythongh-114563, pythongh-91060
@belm0 belm0 requested a review from rhettinger as a code owner February 1, 2024 20:44
@bedevere-app bedevere-app bot mentioned this pull request Feb 1, 2024
Closed
@rhettinger rhettinger removed their request for review February 1, 2024 23:45
mdickinson
mdickinson reviewed Feb 10, 2024
View reviewed changes
mdickinson
mdickinson reviewed Feb 10, 2024
View reviewed changes
Copy link
Member
@mdickinson mdickinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, with one suggestion about the exception handling for the __format__ call.

@skrah Thank you for your help with this. If you happen to have the bandwidth and inclination to take a quick glance at this PR, I'd very much appreciate your input.

serhiy-storchaka
serhiy-storchaka reviewed Feb 10, 2024
View reviewed changes
@skrah
Copy link
Contributor
skrah commented Feb 10, 2024

If I checkout d96a8cd and then apply my own patches, I reach the exact same state as in this PR.

When comparing the result to 3.9, the diff in the dec_format area is clean and manageable.

I tested my own patches with deccheck (z-format extended version), so I approve!

@serhiy-storchaka's pydec_format changes look correct to me, but I'd have to look up some of the new functions, so I'll stay out of that part.

@belm0 @serhiy-storchaka
Apply suggestions from code review
d37b5b8 
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka
serhiy-storchaka reviewed Feb 10, 2024
View reviewed changes
serhiy-storchaka
serhiy-storchaka approved these changes Feb 10, 2024
View reviewed changes
Copy link
Member
@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Is it okay that the PyDecimal instance can have a different context?

@skrah
Copy link
Contributor
skrah commented Feb 10, 2024

Is it okay that the PyDecimal instance can have a different context?

This is an excellent observation! It is the one thing that cannot be caught by deccheck, because the _decimal and _pydecimal contexts are always in sync. _pydecimal.__format__ needs rounding and capitals.

Maybe the easiest thing would be to add another private method to _pydecimal, something like:

def fallback_format(self, specifier, rounding, caps):
    # make a new context with rounding and caps
    # return __format__ with that context

belm0
belm0 commented Feb 10, 2024
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed Feb 11, 2024
View reviewed changes
@skrah
Copy link
Contributor
skrah commented Feb 11, 2024

This is a clever use of the _decimal context! It works because ctx.rounding is a string and ctx.capitals is a bool. For others who happen to come by this issue, I want to (pedantically) point out that the _pydecimal and _decimal contexts are not in general interchangeable.

LGTM.

serhiy-storchaka
serhiy-storchaka reviewed Feb 11, 2024
View reviewed changes
Copy link
Member
@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Now please add a NEWS entry (it is a user visible change), and it will be done.

Describe what behavior was changed, not implementation details.

@serhiy-storchaka serhiy-storchaka merged commit 72340d1 into python:main Feb 12, 2024
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 12, 2024
@miss-islington-app
Copy link

Thanks @belm0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @belm0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @belm0 and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72340d15cdfdfa4796fdd7c702094c852c2b32d2 3.12

@miss-islington-app
Copy link

Sorry, @belm0 and @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72340d15cdfdfa4796fdd7c702094c852c2b32d2 3.11

@serhiy-storchaka
Copy link
Member

@belm0, do you mind to create a backport for 3.12?

It will be easier to backport then from 3.12 to 3.11 than directly to 3.11.

@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.11 only security fixes label Feb 12, 2024
@belm0 belm0 deleted the fix-issue-114563 branch February 12, 2024 15:06
@belm0 belm0 mentioned this pull request Feb 12, 2024
Closed
@bedevere-app
Copy link
bedevere-app bot commented Feb 12, 2024

GH-115353 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 12, 2024
@serhiy-storchaka serhiy-storchaka mentioned this pull request Feb 12, 2024
Closed
serhiy-storchaka pushed a commit that referenced this pull request Feb 12, 2024
@belm0
[3.12] gh-114563: C decimal falls back to pydecimal for unsupported f…
09c98e4 
…ormat strings (GH-114879) (GH-115353)

Immediate merits:
* eliminate complex workarounds for 'z' format support
  (NOTE: mpdecimal recently added 'z' support, so this becomes
  efficient in the long term.)
* fix 'z' format memory leak
* fix 'z' format applied to 'F'
* fix missing '#' format support

Suggested and prototyped by Stefan Krah.

Fixes gh-114563, gh-91060

(cherry picked from commit 72340d1)

Co-authored-by: John Belmonte <john@neggie.net>
Co-authored-by: Stefan Krah <skrah@bytereef.org>
belm0 added a commit to belm0/cpython that referenced this pull request Feb 13, 2024
@belm0 @skrah
[3.11] [3.12] pythongh-114563: C decimal falls back to pydecimal for …
ff75179 
…unsupported format strings (pythonGH-114879) (pythonGH-115353)

Immediate merits:
* eliminate complex workarounds for 'z' format support
  (NOTE: mpdecimal recently added 'z' support, so this becomes
  efficient in the long term.)
* fix 'z' format memory leak
* fix 'z' format applied to 'F'
* fix missing 'GH-' format support

Suggested and prototyped by Stefan Krah.

Fixes pythongh-114563, pythongh-91060

(cherry picked from commit 72340d1)

(cherry picked from commit 09c98e4)

Co-authored-by: John Belmonte <john@neggie.net>
Co-authored-by: Stefan Krah <skrah@bytereef.org>
@bedevere-app
Copy link
bedevere-app bot commented Feb 13, 2024

GH-115384 is a backport of this pull request to the 3.11 branch.

serhiy-storchaka pushed a commit that referenced this pull request Feb 13, 2024
@belm0
[3.11] gh-114563: C decimal falls back to pydecimal for unsupported f…
b86fdf2 
…ormat strings (GH-114879) (GH-115384)

Immediate merits:
* eliminate complex workarounds for 'z' format support
  (NOTE: mpdecimal recently added 'z' support, so this becomes
  efficient in the long term.)
* fix 'z' format memory leak
* fix 'z' format applied to 'F'
* fix missing 'GH-' format support

Suggested and prototyped by Stefan Krah.

Fixes gh-114563, gh-91060

(cherry picked from commit 72340d1)
(cherry picked from commit 09c98e4)

Co-authored-by: Stefan Krah <skrah@bytereef.org>
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@belm0 @skrah @fsc-eriker
pythongh-114563: C decimal falls back to pydecimal for unsupported fo…
3a33388 
…rmat strings (pythonGH-114879)

Immediate merits:
* eliminate complex workarounds for 'z' format support
  (NOTE: mpdecimal recently added 'z' support, so this becomes
  efficient in the long term.)
* fix 'z' format memory leak
* fix 'z' format applied to 'F'
* fix missing '#' format support

Suggested and prototyped by Stefan Krah.

Fixes pythongh-114563, pythongh-91060

Co-authored-by: Stefan Krah <skrah@bytereef.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdickinson mdickinson mdickinson left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@skrah skrah skrah approved these changes

Assignees

@serhiy-storchaka serhiy-storchaka

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible memory leak in _cdecimal.c with 'z' format
4 participants
@belm0 @skrah @serhiy-storchaka @mdickinson
0