-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
PEP 670: clarify cast; don't change return type #2349
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,12 +20,8 @@ them usable by Python extensions which cannot use macros or static | |||||
inline functions, like extensions written in a programming languages | ||||||
other than C or C++. | ||||||
|
||||||
Remove the return value of macros having a return value, whereas they | ||||||
should not, to aid detecting bugs in C extensions when the C API is | ||||||
misused. | ||||||
|
||||||
Some function arguments are still cast to ``PyObject*`` to prevent | ||||||
emitting new compiler warnings. | ||||||
Function arguments of pointer types are still cast and return types are | ||||||
not changed to prevent emitting new compiler warnings. | ||||||
|
||||||
Macros which can be used as l-value in an assignment are not converted | ||||||
to functions to avoid introducing incompatible changes. | ||||||
|
@@ -177,6 +173,7 @@ The following macros should not be converted: | |||||
* Macros which can be used as l-value in an assignment. This change is | ||||||
an incompatible change and is out of the scope of this PEP. | ||||||
Example: ``PyBytes_AS_STRING()``. | ||||||
* Macros having different return types depending on the code path. | ||||||
|
||||||
|
||||||
Convert static inline functions to regular functions | ||||||
|
@@ -196,74 +193,81 @@ Using static inline functions in the internal C API is fine: the | |||||
internal C API exposes implementation details by design and should not be | ||||||
used outside Python. | ||||||
|
||||||
Cast to PyObject* | ||||||
----------------- | ||||||
Cast pointer arguments | ||||||
---------------------- | ||||||
|
||||||
Existing cast | ||||||
''''''''''''' | ||||||
|
||||||
Currently, most macros accepting pointers cast pointer arguments to | ||||||
their expected types. For example, in Python 3.6, the ``Py_TYPE()`` | ||||||
macro casts its argument to ``PyObject*``:: | ||||||
|
||||||
When a macro is converted to a function and the macro casts its | ||||||
arguments to ``PyObject*``, the new function comes with a new macro | ||||||
which cast arguments to ``PyObject*`` to prevent emitting new compiler | ||||||
warnings. This implies that a converted function will accept pointers to | ||||||
structures inheriting from ``PyObject`` (ex: ``PyTupleObject``). | ||||||
#define Py_TYPE(ob) (((PyObject*)(ob))->ob_type) | ||||||
|
||||||
For example, the ``Py_TYPE(obj)`` macro casts its ``obj`` argument to | ||||||
``PyObject*``:: | ||||||
The ``Py_TYPE()`` macro accepts the ``PyObject*`` type, but also any | ||||||
pointer types, such as ``PyLongObject*`` and ``PyDictObject*``. | ||||||
|
||||||
#define _PyObject_CAST_CONST(op) ((const PyObject*)(op)) | ||||||
Add a new macro to keep the cast | ||||||
'''''''''''''''''''''''''''''''' | ||||||
|
||||||
static inline PyTypeObject* _Py_TYPE(const PyObject *ob) { | ||||||
When a macro is converted to a function and the macro casts at least one | ||||||
of its arguments, a new macro is added to keep the cast. The new macro | ||||||
and the function have the same name. Example with the ``Py_TYPE()`` | ||||||
macro converted to a static inline function:: | ||||||
|
||||||
static inline PyTypeObject* Py_TYPE(PyObject *ob) { | ||||||
return ob->ob_type; | ||||||
} | ||||||
#define Py_TYPE(ob) _Py_TYPE(_PyObject_CAST_CONST(ob)) | ||||||
#define Py_TYPE(ob) Py_TYPE((PyObject*)(ob)) | ||||||
|
||||||
The undocumented private ``_Py_TYPE()`` function must not be called | ||||||
directly. Only the documented public ``Py_TYPE()`` macro must be used. | ||||||
The cast is kept for all pointer types, not only ``PyObject*``. | ||||||
|
||||||
Later, the cast can be removed on a case by case basis, but that is out | ||||||
of scope for this PEP. | ||||||
Removing a cast to ``void*`` would emit a new warning if the function is | ||||||
called with a variable of ``const void*`` type. For example, the | ||||||
``PyUnicode_WRITE()`` macro casts its *data* argument to ``void*``, and | ||||||
so accepts ``const void*`` type, even if it writes into *data*. | ||||||
|
||||||
Remove the return value | ||||||
----------------------- | ||||||
Avoid the cast in the limited C API version 3.11 | ||||||
'''''''''''''''''''''''''''''''''''''''''''''''' | ||||||
|
||||||
When a macro is implemented as an expression, it has an implicit return | ||||||
value. This return value can be misused in third party C extensions. | ||||||
See `bpo-30459 <https://bugs.python.org/issue30459>`__ regarding the | ||||||
misuse of the ``PyList_SET_ITEM()`` and ``PyCell_SET()`` macros. | ||||||
The cast is removed from the limited C API version 3.11 and newer: the | ||||||
caller must pass the expected type, or perform the cast. An example with | ||||||
the ``Py_TYPE()`` function:: | ||||||
|
||||||
Such issue is hard to catch while reviewing macro code. Removing the | ||||||
return value aids detecting bugs in C extensions when the C API is | ||||||
misused. | ||||||
static inline PyTypeObject* Py_TYPE(PyObject *ob) { | ||||||
return ob->ob_type; | ||||||
} | ||||||
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 | ||||||
# define Py_TYPE(ob) Py_TYPE((PyObject*)(ob)) | ||||||
#endif | ||||||
|
||||||
The issue has already been fixed in public C API macros by the | ||||||
`bpo-30459 <https://bugs.python.org/issue30459>`__ in Python 3.10: add a | ||||||
``(void)`` cast to the affected macros. Example of the | ||||||
``PyTuple_SET_ITEM()`` macro:: | ||||||
|
||||||
#define PyTuple_SET_ITEM(op, i, v) ((void)(_PyTuple_CAST(op)->ob_item[i] = v)) | ||||||
Return type is not changed | ||||||
-------------------------- | ||||||
|
||||||
Example of macros currently using a ``(void)`` cast to have no return | ||||||
value: | ||||||
When a macro is converted to a function, its return type must not change | ||||||
to prevent emitting new compiler warnings. | ||||||
|
||||||
* ``PyCell_SET()`` | ||||||
* ``PyList_SET_ITEM()`` | ||||||
* ``PyTuple_SET_ITEM()`` | ||||||
* ``Py_BUILD_ASSERT()`` | ||||||
* ``_PyGCHead_SET_FINALIZED()`` | ||||||
* ``_PyGCHead_SET_NEXT()`` | ||||||
* ``_PyObject_ASSERT_FROM()`` | ||||||
* ``_Py_atomic_signal_fence()`` | ||||||
* ``_Py_atomic_store_64bit()`` | ||||||
* ``asdl_seq_SET()`` | ||||||
* ``asdl_seq_SET_UNTYPED()`` | ||||||
For example, Python 3.7 changed ``PyUnicode_AsUTF8()`` return type from | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Fix grammar error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This grammar error wasn't fixed |
||||||
``char*`` to ``const char*`` (`commit | ||||||
<https://github.com/python/cpython/commit/2a404b63d48d73bbaa007d89efb7a01048475acd>`__). | ||||||
The change emitted new compiler warnings when building C extensions | ||||||
expecting ``char*``. This PEP doesn't change the return type to prevent | ||||||
this issue. | ||||||
|
||||||
|
||||||
Backwards Compatibility | ||||||
======================= | ||||||
|
||||||
Removing the return value of macros is an incompatible API change made | ||||||
on purpose: see the `Remove the return value`_ section. | ||||||
The PEP is designed to avoid C API incompatible changes. | ||||||
|
||||||
Some function arguments are still cast to ``PyObject*`` to prevent | ||||||
emitting new compiler warnings. | ||||||
Only C extensions explicitly targeting the limited C API version 3.11 | ||||||
must now pass the expected types to functions: pointer arguments are no | ||||||
longer cast to the expected types. | ||||||
|
||||||
Function arguments of pointer types are still cast and return types are | ||||||
not changed to prevent emitting new compiler warnings. | ||||||
|
||||||
Macros which can be used as l-value in an assignment are not modified by | ||||||
this PEP to avoid incompatible changes. | ||||||
|
@@ -275,10 +279,6 @@ Rejected Ideas | |||||
Keep macros, but fix some macro issues | ||||||
-------------------------------------- | ||||||
|
||||||
Converting macros to functions is not needed to `remove the return | ||||||
value`_: adding a ``(void)`` cast is enough. For example, the | ||||||
``PyList_SET_ITEM()`` macro was already fixed like that. | ||||||
|
||||||
Macros are always "inlined" with any C compiler. | ||||||
|
||||||
The duplication of side effects can be worked around in the caller of | ||||||
|
@@ -600,6 +600,14 @@ References | |||||
(March 2021). | ||||||
|
||||||
|
||||||
Version History | ||||||
=============== | ||||||
|
||||||
* Version 2: No longer remove return values; remove argument casting | ||||||
from the limited C API. | ||||||
* Version 1: First public version | ||||||
|
||||||
|
||||||
Copyright | ||||||
========= | ||||||
|
||||||
|
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.
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.
Fix grammar errors, phrasing and syntax highlighting
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.
The grammar and phrasing errors weren't fixed here