GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45476: Disallow using PyFloat_AS_DOUBLE() as l-value by vstinner · Pull Request #28976 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Oct 15, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The following "GET" and "AS" functions can no longer be used as
l-value (to modify a Python object):

  • PyByteArray_GET_SIZE()
  • PyBytes_GET_SIZE()
  • PyCFunction_GET_CLASS()
  • PyCFunction_GET_FLAGS()
  • PyCFunction_GET_FUNCTION()
  • PyCFunction_GET_SELF()
  • PyDict_GET_SIZE()
  • PyFloat_AS_DOUBLE()
  • PyFunction_GET_ANNOTATIONS()
  • PyFunction_GET_CLOSURE()
  • PyFunction_GET_CODE()
  • PyFunction_GET_DEFAULTS()
  • PyFunction_GET_GLOBALS()
  • PyFunction_GET_KW_DEFAULTS()
  • PyFunction_GET_MODULE()
  • PyHeapType_GET_MEMBERS()
  • PyInstanceMethod_GET_FUNCTION()
  • PyList_GET_SIZE()
  • PyMemoryView_GET_BASE()
  • PyMemoryView_GET_BUFFER()
  • PyMethod_GET_FUNCTION()
  • PyMethod_GET_SELF()
  • PySet_GET_SIZE()
  • PyTuple_GET_SIZE()
  • PyWeakref_GET_OBJECT()

These macros are modified to use the _Py_RVALUE() macro.

https://bugs.python.org/issue45476

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner vstinner added the skip news label Oct 15, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Oct 15, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 15, 2021
gareth-rees
gareth-rees reviewed Oct 15, 2021
View reviewed changes
Comment thread Include/pymacro.h Outdated

@gareth-rees gareth-rees Oct 15, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Typo: "expressing" for "expression"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner marked this pull request as draft October 15, 2021 13:51
rhettinger
rhettinger approved these changes Oct 17, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 17, 2021
@vstinner

vstinner commented Nov 16, 2021

Copy link
Copy Markdown
Member Author

I wrote PEP 670 https://www.python.org/dev/peps/pep-0670/ to decide what's the best approach to fix https://bugs.python.org/issue45476

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 29, 2021

Copy link
Copy Markdown
Member Author

I rebased the PR and reverted PyBytes_AS_STRING() and PyByteArray_AS_STRING() changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner changed the title bpo-45476: Add _Py_RVALUE() macro bpo-45476: Disallow using PyFloat_AS_DOUBLE() as l-value Nov 30, 2021
@vstinner
vstinner marked this pull request as ready for review November 30, 2021 13:07
@vstinner
bpo-45476: Disallow using PyFloat_AS_DOUBLE() as l-value
7ba9d58
The following "GET" and "AS" functions can no longer be used as
l-value (to modify a Python object):

* PyByteArray_GET_SIZE()
* PyBytes_GET_SIZE()
* PyCFunction_GET_CLASS()
* PyCFunction_GET_FLAGS()
* PyCFunction_GET_FUNCTION()
* PyCFunction_GET_SELF()
* PyDict_GET_SIZE()
* PyFloat_AS_DOUBLE()
* PyFunction_GET_ANNOTATIONS()
* PyFunction_GET_CLOSURE()
* PyFunction_GET_CODE()
* PyFunction_GET_DEFAULTS()
* PyFunction_GET_GLOBALS()
* PyFunction_GET_KW_DEFAULTS()
* PyFunction_GET_MODULE()
* PyHeapType_GET_MEMBERS()
* PyInstanceMethod_GET_FUNCTION()
* PyList_GET_SIZE()
* PyMemoryView_GET_BASE()
* PyMemoryView_GET_BUFFER()
* PyMethod_GET_FUNCTION()
* PyMethod_GET_SELF()
* PySet_GET_SIZE()
* PyTuple_GET_SIZE()
* PyWeakref_GET_OBJECT()

These macros are modified to use the _Py_RVALUE() macro.
@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

@corona10 @erlend-aasland @encukou: Would you mind to review this change? It's a C API incompatible change made on purpose to aid detecting bugs in C extensions when the C API is misused.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Nov 30, 2021

Copy link
Copy Markdown
Member

Is there any deprecation warning for these changes?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

I cannot find any of the 25 macros being used as a l-value in an assignement in the PyPI top 5000 packages. So the risk of breaking 3rd party packages is very low.

I searched for the following patterns:

$ ./search_pypi_top_5000.sh '(PyByteArray|PyBytes|PyCFunction|PyDict|PyFunction|PyHeapType|PyInstanceMethod|PyList|PyMemoryView|PyMethod|PySet|PyTuple|PyWeakref)_GET[a-zA-Z0_9_]+ *([a-zA-Z0-9_]+) *=[^<>=]'
pypi-top-5000_2021-08-17/Cython-0.29.24.tar.gz

$ ./search_pypi_top_5000.sh 'PyFloat_AS_DOUBLE.*[^<>=!]=[^<>=]'
# no result

Cython-0.29.24.tar.gz is a false positive because of my weak regex:

Cython-0.29.2: Cython/Compiler/Optimize.py:    PyBytes_GET_SIZE_func_type = PyrexTypes.CFuncType(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

Is there any deprecation warning for these changes?

No. Which kind of warning do you expect?

I am not aware of any technical way to emit a compiler warning when a macro is only used as an l-value, see previous discussion about the Py_TYPE() change: python/steering-council#79

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

@rhettinger: You may want to review the updated PR. I made minor changes since you approved the PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Nov 30, 2021

Copy link
Copy Markdown
Member

The SC decision you linked says:

these changes must be announced better and in general the deprecation notice needs to stay for at least two releases in the documentation.

How does this PR do that?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou encukou mentioned this pull request Nov 30, 2021
Request for PEP 387 exception: Py_SET_TYPE python/steering-council#79
Closed
@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

Documentation of the modified macros:

  • https://docs.python.org/dev/c-api/bytearray.html#c.PyByteArray_GET_SIZE "Macro version of PyByteArray_Size()."
  • https://docs.python.org/dev/c-api/bytes.html#c.PyBytes_GET_SIZE "Macro form of PyBytes_Size() but without error checking."
  • https://docs.python.org/dev/c-api/float.html#c.PyFloat_AS_DOUBLE
  • https://docs.python.org/dev/c-api/method.html#c.PyInstanceMethod_GET_FUNCTION "Macro version of PyInstanceMethod_Function() which avoids error checking."
  • https://docs.python.org/dev/c-api/list.html#c.PyList_GET_SIZE "Macro form of PyList_Size() without error checking."
  • https://docs.python.org/dev/c-api/memoryview.html#c.PyMemoryView_GET_BASE
  • https://docs.python.org/dev/c-api/memoryview.html#c.PyMemoryView_GET_BUFFER
  • https://docs.python.org/dev/c-api/method.html#c.PyMethod_GET_FUNCTION "Macro version of PyMethod_Function() which avoids error checking."
  • https://docs.python.org/dev/c-api/method.html#c.PyMethod_GET_SELF "Macro version of PyMethod_Self() which avoids error checking."
  • https://docs.python.org/dev/c-api/set.html#c.PySet_GET_SIZE "Macro form of PySet_Size() without error checking."
  • https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_GET_SIZE
  • https://docs.python.org/dev/c-api/weakref.html#c.PyWeakref_GET_OBJECT "Similar to PyWeakref_GetObject(), but implemented as a macro that does no error checking."

The following functions are not documented:

  • PyCFunction_GET_CLASS()
  • PyCFunction_GET_FLAGS()
  • PyCFunction_GET_FUNCTION()
  • PyCFunction_GET_SELF()
  • PyDict_GET_SIZE()
  • PyFunction_GET_ANNOTATIONS()
  • PyFunction_GET_CLOSURE()
  • PyFunction_GET_CODE()
  • PyFunction_GET_DEFAULTS()
  • PyFunction_GET_GLOBALS()
  • PyFunction_GET_KW_DEFAULTS()
  • PyFunction_GET_MODULE()
  • PyHeapType_GET_MEMBERS()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

Is there any deprecation warning for these changes?

For me, using these macros as l-value is an unintentional usage. I don't think that we have to go through a deprecation process for this specific corner case.

For "GET" functions, it sounds weird to me to use them to set an object attribute or to modify a Python object.

For the PyFloat_AS_DOUBLE() function, it's less clear to me what is the intended usage. A Python float object is immutable. Once it's created, it can be referenced at multiple places (stored in two lists). If a list stores the number 1.0, it would be surprising to modify it in-place somewhere else to change its value to 2.0. Maybe some weird projects use an in-place modification when the reference count is 1, to avoid the cost of creating a new object. It sounds dangerous but people are creative to abuse the C API :-) What reassures me is that in the PyPI top 5000, I cannot find a single project doing that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Nov 30, 2021
bpo-45476: Convert PyFloat_AS_DOUBLE() to static inline #28961
Closed
@encukou

encukou commented Nov 30, 2021

Copy link
Copy Markdown
Member

I like to think that the target audience for deprecation messages and "versionchanged" notices is someone who inherited a decades-old legacy project, and is tasked with keeping it usable without knowing much about Python and current best practices. If it works for that audience, it should work for everyone else as well.
Changing old API is tricky because you don't usually know how it was documented/used in the past -- it's entirely possible that reasonable people looked at the code rather than the documentation.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2021

Copy link
Copy Markdown
Member Author

I wrote PEP 674 "Disallow using macros as l-value" for this change: https://python.github.io/peps/pep-0674/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Feb 23, 2022
@vstinner
vstinner deleted the py_rvalue branch February 23, 2022 23:19
@vstinner vstinner mentioned this pull request Jul 27, 2022
[C API] PEP 674: Disallow using macros (Py_TYPE and Py_SIZE) as l-value #89639
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes
+1 more reviewer
@gareth-rees gareth-rees gareth-rees left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vstinner @encukou @gareth-rees @rhettinger @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.