Skip to content

Conversation

@davidhewitt
Copy link
Member

Related to #4379, also closes #5062

This PR removes all types and structures starting with _Py from pyo3-ffi. These are private and may be freely altered in patch releases (as seen in #5062), so we should not expose these to users, and should only bother defining them in pyo3-ffi when they're necessary to satisfy public CPython ABI.

As part of this PR I changed the opaque_struct! macro to require explicitly opting-in to pub (or pub(crate)), rather than always making the defined item public.

@davidhewitt davidhewitt added CI-skip-changelog Skip checking changelog entry and removed CI-skip-changelog Skip checking changelog entry labels Apr 11, 2025
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks! Were you planning to just limit this to underscore apis? There are things like PyCodeObject and PyHeapTypeObject that aren't exactly public api either.

@davidhewitt davidhewitt requested a review from mejrs April 11, 2025 10:45
@davidhewitt
Copy link
Member Author

There are things like PyCodeObject and PyHeapTypeObject that aren't exactly public api either.

As per #5064 (comment) have made PyCodeObject opaque.

There are definitely other structures which should not be public, but given that each time we change this we might break downstream code I would prefer to just remove these when there's a good reason, e.g. the CPython headers hide the symbol or explicitly document as private API. Also if we removed PyHeapTypeObject, probably warrants its own PR at the very least.

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks, that's a nice line diff.

@davidhewitt davidhewitt enabled auto-merge April 11, 2025 19:33
@clin1234
Copy link
Contributor

Wholeheartedly agree, especially when CPython's C interfaces changes in prerelease builds!

@davidhewitt davidhewitt added this pull request to the merge queue Apr 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 12, 2025
@davidhewitt davidhewitt added this pull request to the merge queue Apr 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 12, 2025
@mejrs
Copy link
Member

mejrs commented Apr 13, 2025

@davidhewitt the auto merge didn't quite work:

 error: type `_PyCompilerSrcLocation` is more private than the item `PyFutureFeatures::ff_location`
    --> pyo3-ffi/src/cpython/compile.rs:53:5
     |
  53 |     pub ff_location: _PyCompilerSrcLocation,
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ field `PyFutureFeatures::ff_location` is reachable at visibility `pub`
     |
  note: but type `_PyCompilerSrcLocation` is only usable at visibility `pub(self)`
    --> pyo3-ffi/src/cpython/compile.rs:36:1
     |
  36 | struct _PyCompilerSrcLocation {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: `-D private-interfaces` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(private_interfaces)]`
  
  error: could not compile `pyo3-ffi` (lib) due to 1 previous error

@davidhewitt
Copy link
Member Author

Thanks, I've decided to fix by restricting visibility of PyFutureFeatures struct to < 3.10 (it was unused in the public C API after the PyFuture_From* functions were made internal in 3.10).

@ngoldbaum
Copy link
Contributor

Let's try to merge once again.

@ngoldbaum ngoldbaum added this pull request to the merge queue Apr 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 21, 2025
@ngoldbaum
Copy link
Contributor

Windows build failed with:

  error[E0609]: no field `co_freevars` on type `code::PyCodeObject`
    --> pyo3-ffi\src\cpython\code.rs:89:35
     |
  89 |     crate::PyTuple_GET_SIZE((*op).co_freevars)
     |                                   ^^^^^^^^^^^ unknown field
     |
     = note: available field is: `0`
  
  For more information about this error, try `rustc --explain E0609`

@davidhewitt davidhewitt enabled auto-merge April 23, 2025 21:02
@davidhewitt
Copy link
Member Author

I'm just going to remove that function; if we can't depend on the layout of PyCodeObject the current implementation makes no sense. If a user reports this as an issue, we can think about solutions then.

@davidhewitt davidhewitt added this pull request to the merge queue Apr 23, 2025
Merged via the queue into PyO3:main with commit caf35f9 Apr 23, 2025
43 of 44 checks passed
@davidhewitt davidhewitt deleted the remove-private-types branch April 23, 2025 22:06
newcomertv pushed a commit to newcomertv/pyo3 that referenced this pull request Apr 28, 2025
* remove private types from `pyo3-ffi`

* newsfragments

* make `PyCodeObject` opaque on all versions

* fix msrv

* fix msrv, try 2

* remove unused constant

* sync `cpython/compile.rs` with header at 3.13

* fix size of INT_MAX

* remove `PyCode_GetNumFree`
linkmauve added a commit to linkmauve/pyo3 that referenced this pull request Oct 30, 2025
Thanks to the typos[1] tool!

Previously _PyCoMontoringData was ignored as part of PyO3#5064[2], which is
obviously the wrong name.  This PR changes it for the real
_PyCoMonitoringData name, but since it should be private that shouldn’t
affect any real user of the ffi crate.

[1] https://github.com/crate-ci/typos
[2] PyO3#5064
github-merge-queue bot pushed a commit that referenced this pull request Oct 31, 2025
Thanks to the typos[1] tool!

Previously _PyCoMontoringData was ignored as part of #5064[2], which is
obviously the wrong name.  This PR changes it for the real
_PyCoMonitoringData name, but since it should be private that shouldn’t
affect any real user of the ffi crate.

[1] https://github.com/crate-ci/typos
[2] #5064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi-check fails on 3.13.3

4 participants