ffi: update object.rs for 3.13 by davidhewitt · Pull Request #4447 · PyO3/pyo3

davidhewitt · 2024-08-17T15:11:31Z

I audited object.rs for differences against 3.13 and applied all the differences I found here. At the same time I also removed all _Py private APIs.

I found a free threading detail in PyType_HasFeature which we needed to account for (atomic load), cc @ngoldbaum

(Will push CHANGELOG later, out of time right now...)

davidhewitt · 2024-08-17T15:13:09Z

Not sure how I got the commit message / PR title wrong originally; fat finger error while being hurried by family probably 🙈

ngoldbaum · 2024-08-19T16:59:02Z

pyo3-ffi/src/object.rs

-pub unsafe fn PyType_HasFeature(t: *mut PyTypeObject, f: c_ulong) -> c_int {
-    (((*t).tp_flags & f) != 0) as c_int
+    #[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))]
+    let flags = (*ty).tp_flags.load(Ordering::Relaxed);


Good catch, totally missed this.

ngoldbaum · 2024-08-19T17:00:39Z

pyo3-ffi/src/object.rs

+
+extern "C" {
+    #[cfg(Py_3_13)]
+    #[cfg_attr(PyPy, link_name = "PyPy_GetConstant")]


I guess there's no harm and it will probably work this way, but it seems a little premature to add code to support PyPy 3.13, which won't be coming out for quite a while, if ever.

Yes I have gone back and forth on these "future" bindings for PyPy, but I've also had so many cases where the link_name attribute has randomly missing creating compile errors for users on PyPy I just think it's slightly better this way round now.

ngoldbaum · 2024-08-19T17:02:27Z

pyo3-ffi/src/object.rs

+        arg1: *mut PyObject,
+        arg2: *mut PyObject,
+        arg3: *mut *mut PyObject,
+    ) -> c_int;


I don't know if the pattern of doing a getitem and then clearing the error afterwards shows up in the PyO3 codebase, but this will be a nice thing to use if it does show up. It's much faster to not create the error object if you don't actually need it.

ngoldbaum · 2024-08-19T17:04:21Z

pyo3-ffi/src/object.rs

-    let local = (*ob).ob_ref_local.load(Relaxed);
-    if local == _Py_IMMORTAL_REFCNT_LOCAL {
-        return _Py_IMMORTAL_REFCNT;
+    #[cfg(Py_GIL_DISABLED)]


I like this pattern of exposing the function once and then using per-config blocks inside the function to do the per-config implementation. Much cleaner than having multiple copies of the signature. And it looks like it plays nicer with the doc build too.

ngoldbaum · 2024-08-19T17:07:09Z

pyo3-ffi/src/object.rs


 #[inline]
 pub unsafe fn Py_Is(x: *mut PyObject, y: *mut PyObject) -> c_int {
    (x == y).into()


Totally unrelated to this PR and a bit of a minor corner case, but this is wrong on pypy, see pypy/pypy#4044

Thanks for flagging, worth fixing anyway while I'm here!

mejrs

Thanks; here is what I could review on my phone :)

mejrs · 2024-08-20T13:10:50Z

pyo3-ffi/src/impl_/mod.rs

@@ -0,0 +1,20 @@
+#[cfg(Py_GIL_DISABLED)]
+mod atomic_c_ulong {
+    pub(crate) struct GetAtomicCULong<const SIZE: usize>();


Is the size in bits or in bytes? I would naively expect the latter, but have no opinion. Maybe we should call it "width" if we want the former.

Thanks, I was thinking it's easier to think in bits here but size_of() returns bytes, so this is currently bugged 👍

Renamed to WIDTH 👍

mejrs · 2024-08-20T13:18:24Z

newsfragments/4447.added.md

@@ -0,0 +1 @@
+Add Python 3.13 FFI definitions `PyObject_GetOptionalAttr`, `PyObject_GetOptionalAttrString`, `PyObject_HasAttrWithError`, `PyObject_HasAttrStringWithError`, `Py_CONSTANT_*` constants, `Py_GetConstant`, `Py_GetConstantBorowed`, and `PyType_GetModuleByDef`.


davidhewitt · 2024-08-21T09:11:49Z

I will merge this to unblock forward progress; if there is more to do here then I can follow up. Thanks both for reviews!

pyo3-ffi/src/object.rs

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

ngoldbaum · 2024-08-24T12:37:41Z

I'm not sure why the tests didn't catch it when this was merged, but git bisect says this PR is causing the test failures with cargo test --features=abi3 on main with 3.13:

failures:

---- binary_arithmetic stdout ----
thread 'binary_arithmetic' panicked at tests/test_arithmetics.rs:265:9:
Did not raise PyTypeError: ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- rich_comparisons_python_3_type_error stdout ----
thread 'rich_comparisons_python_3_type_error' panicked at tests/test_arithmetics.rs:568:9:
Did not raise PyTypeError: ()

---- return_not_implemented::reverse_arith stdout ----
thread 'return_not_implemented::reverse_arith' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__radd__(Other()) is NotImplemented

---- return_not_implemented::ordering stdout ----
thread 'return_not_implemented::ordering' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__lt__(Other()) is NotImplemented

---- return_not_implemented::arith stdout ----
thread 'return_not_implemented::arith' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__add__(Other()) is NotImplemented

---- return_not_implemented::equality stdout ----
thread 'return_not_implemented::equality' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__eq__(Other()) is NotImplemented

---- return_not_implemented::bitwise stdout ----
thread 'return_not_implemented::bitwise' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__and__(Other()) is NotImplemented

---- return_not_implemented::inplace_arith stdout ----
thread 'return_not_implemented::inplace_arith' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__iadd__(Other()) is NotImplemented

---- return_not_implemented::inplace_bitwise stdout ----
thread 'return_not_implemented::inplace_bitwise' panicked at tests/test_arithmetics.rs:669:13:
class Other: pass
assert c2.__iand__(Other()) is NotImplemented


failures:
    binary_arithmetic
    return_not_implemented::arith
    return_not_implemented::bitwise
    return_not_implemented::equality
    return_not_implemented::inplace_arith
    return_not_implemented::inplace_bitwise
    return_not_implemented::ordering
    return_not_implemented::reverse_arith
    rich_comparisons_python_3_type_error

test result: FAILED. 6 passed; 9 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

error: test failed, to rerun pass `--test test_arithmetics`

davidhewitt · 2024-08-24T18:00:16Z

3.13 doesn't block merges yet, but you're right, I will investigate.

davidhewitt force-pushed the object-3-13 branch from 65594c5 to 48c6a3b Compare August 17, 2024 15:12

davidhewitt changed the title ~~clean up parsing and docs for module macro options~~ ffi: update object.rs for 3.13 Aug 17, 2024

davidhewitt added 2 commits August 17, 2024 22:03

ffi: update object.rs for 3.13

f46ce03

add newsfragment

3863c96

davidhewitt force-pushed the object-3-13 branch from 48c6a3b to 3863c96 Compare August 17, 2024 21:41

fixup _Py_IsImmortal cfgs

9de58cc

ngoldbaum reviewed Aug 19, 2024

View reviewed changes

davidhewitt added 2 commits August 19, 2024 22:54

also update cpython/object.rs

e908677

fix Py_Is on PyPy 3.10

6db725b

davidhewitt force-pushed the object-3-13 branch from 9e7897b to 6db725b Compare August 19, 2024 21:55

Py_GetConstant & Py_GetConstantBorrowed are pub

b57a74f

mejrs reviewed Aug 20, 2024

View reviewed changes

[review] mejrs feedback

6ef05f7

davidhewitt added this pull request to the merge queue Aug 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2024

alex reviewed Aug 21, 2024

View reviewed changes

pyo3-ffi/src/object.rs Outdated Show resolved Hide resolved

correct typo

97072db

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

davidhewitt enabled auto-merge August 21, 2024 16:07

davidhewitt added this pull request to the merge queue Aug 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2024

fix freethreaded build

6ea76b0

davidhewitt enabled auto-merge August 21, 2024 18:40

davidhewitt added this pull request to the merge queue Aug 21, 2024

Merged via the queue into PyO3:main with commit 2f5b45e Aug 21, 2024

davidhewitt deleted the object-3-13 branch August 21, 2024 20:02

ngoldbaum mentioned this pull request Aug 24, 2024

skip tests using thread-unsafe constructs on free-threaded build #4476

Merged

davidhewitt mentioned this pull request Aug 25, 2024

fixup Py_NotImplemented in 3.13 abi3 #4494

Merged

		@@ -0,0 +1 @@
		Add Python 3.13 FFI definitions `PyObject_GetOptionalAttr`, `PyObject_GetOptionalAttrString`, `PyObject_HasAttrWithError`, `PyObject_HasAttrStringWithError`, `Py_CONSTANT_*` constants, `Py_GetConstant`, `Py_GetConstantBorowed`, and `PyType_GetModuleByDef`.

Conversation

davidhewitt commented Aug 17, 2024

Uh oh!

davidhewitt commented Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejrs left a comment

Choose a reason for hiding this comment

Uh oh!

mejrs Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Aug 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngoldbaum commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhewitt commented Aug 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidhewitt commented Aug 17, 2024 •

edited

Loading

mejrs Aug 20, 2024 •

edited

Loading

ngoldbaum commented Aug 24, 2024 •

edited

Loading