`IntoPyObject` for `#[pyo3(get)]` by Icxolu · Pull Request #4449 · PyO3/pyo3

Icxolu · 2024-08-17T19:51:06Z

This extends the #[pyo3(get)] conversion specialization to include IntoPyObject. I implemented the following priority list (or at least tried to 🙃 )

Py<T>
IntoPyObject for &T
Temp case: IntoPyObject + Clone for T (to prefer over IntoPy)
Compat Fallback: ToPyObject for T
Compat Fallback: IntoPy + Clone for T
Base case: IntoPyObject + Clone for T (currently only used to emit the correct diagnostic if no conversion trait is implemented)

Icxolu · 2024-08-17T20:46:55Z

Hmm, not sure if there is a way to rewrite PyO3GetFieldIntoPyObject without associated type bounds while still keeping the correct diagnostic... I tried

pub trait PyO3GetFieldIntoPyObject: for<'py> IntoPyObject<'py> + Clone
where
    for<'py> <Self as IntoPyObject<'py>>::Error: Into<PyErr>,
{
}

but this forces additional trait bounds on the usage site and causes the diagnostic of IntoPyObject to be emitted instead of this one.

davidhewitt

Nice, thanks!

Hmm, not sure if there is a way to rewrite PyO3GetFieldIntoPyObject without associated type bounds

Hmm, I had a quick go just now but no obvious success, will continue to explore another rmoment.

davidhewitt · 2024-08-17T21:48:06Z

+    let _holder = unsafe {
+        BoundRef::ref_from_ptr(py, &obj)
+            .downcast_unchecked::<ClassT>()
+            .try_borrow()?
+    };


I think we now have a lot of repetition of this pattern, maybe refactor into a function? Possibly try of the offset operation just below also.

mejrs · 2024-08-17T22:25:31Z

You can try using #[diagnostic::do_not_recommend] to suppress these helper traits from showing up in error messages. But I've not used it and I'm not sure if it has landed in stable.

davidhewitt · 2024-08-17T22:40:56Z

this forces additional trait bounds on the usage site and causes the diagnostic of IntoPyObject to be emitted instead of this one.

Given that PyO3GetFieldIntoPyObject exists to emit a helpful diagnostic, maybe can just gate it behind a cfg so that we only try to use it on sufficiently new compilers?

mejrs · 2024-08-17T23:04:13Z

Given that PyO3GetFieldIntoPyObject exists to emit a helpful diagnostic, maybe can just gate it behind a cfg so that we only try to use it on sufficiently new compilers?

I'm not particularly interested in putting in effort to prettify errors on old compilers. I would prefer having a simpler implementation.

davidhewitt · 2024-08-18T06:06:08Z

Given that PyO3GetFieldIntoPyObject exists to emit a helpful diagnostic, maybe can just gate it behind a cfg so that we only try to use it on sufficiently new compilers?

I'm not particularly interested in putting in effort to prettify errors on old compilers. I would prefer having a simpler implementation.

I think we mean the same thing, though the problem is that the trait exists to emit a diagnostic specific to this case!

Icxolu · 2024-08-18T11:37:19Z

Given that PyO3GetFieldIntoPyObject exists to emit a helpful diagnostic, maybe can just gate it behind a cfg so that we only try to use it on sufficiently new compilers?

This could work, luckily associated type bounds were stabilized in 1.79 which is also the version we use for diagnostic_namespace

davidhewitt

Overall this is looking good; just some final thoughts to refine this one off! 👀

davidhewitt · 2024-08-19T21:03:44Z

+where
+    ClassT: PyClass,
+    Offset: OffsetCalculator<ClassT, FieldT>,
+    FieldT: PyO3GetField,


I think there might be no need for this PyO3GetField trait any more, as it will never be used for a diagnostic? Might be able to simplify here.

Suggested change

FieldT: PyO3GetField,

FieldT: IntoPy<Py<PyAny>> + Clone

It might show if only IntoPy<PyObject> (and not IntoPyObject) is implemented while Clone is missing. But this is probably such a specific case (and we want people to use IntoPyObject anyway) that we should probably just remove it.

davidhewitt · 2024-08-19T21:06:01Z

+    // The bound goes here rather than on the block so that this impl is always available
+    // if no specialization is used instead
+    where
+        FieldT: PyO3GetFieldIntoPyObject,


Maybe if the other PyO3GetField trait can be removed, this one could be renamed to just PyO3GetField instead.

davidhewitt · 2024-08-19T21:08:45Z

+        PyMethodDefType::Getter(PyGetterDef {
+            name,
+            meth: pyo3_get_value_into_pyobject::<ClassT, FieldT, Offset>,
            doc,
        })


Because there is a separate IntoPyObject + Clone case, this bound can never be satisfied, right?

Suggested change

PyMethodDefType::Getter(PyGetterDef {

name,

meth: pyo3_get_value_into_pyobject::<ClassT, FieldT, Offset>,

doc,

})

unreachable!("exists purely to emit diagnostics on unimplemented traits")

If so, I guess the same applies to the other cfg-d variant just above too.

This is true until we can remove the ToPyObject and IntoPy cases, at which point we can use this case, and remove the special case for IntoPyObject. If you prefer, we can put unreachable until then.

Actually I had to put panic since unreachable with a message is not allowed in const yet.

davidhewitt · 2024-08-19T21:21:23Z

-   = note: implement `ToPyObject` or `IntoPy<PyObject> + Clone` for `PhantomData<i32>` to define the conversion
-   = note: required for `PhantomData<i32>` to implement `PyO3GetField`
-note: required by a bound in `PyClassGetterGenerator::<ClassT, FieldT, Offset, false, false>::generate`
+   = help: the trait `for<'py> IntoPyObject<'py>` is not implemented for `PhantomData<i32>`, which is required by `PhantomData<i32>: PyO3GetFieldIntoPyObject`


A thought: from testing locally, if I change the definition of PyO3GetFieldIntoPyObject to have a lifetime 'py, i.e. PyO3GetFieldIntoPyObject<'py>: IntoPyObject<'py> + Clone.

... then (after adjusting other uses accordingly), the error message looks more like this:

Suggested change

= help: the trait `for<'py> IntoPyObject<'py>` is not implemented for `PhantomData<i32>`, which is required by `PhantomData<i32>: PyO3GetFieldIntoPyObject`

= help: the trait `IntoPyObject<'_>` is not implemented for `PhantomData<i32>`, which is required by `for<'py> PhantomData<i32>: PyO3GetFieldIntoPyObject<'py>`

Is that better? I think it might be slightly because it moves the HRTB later in the message. I think it's functionally equivalent otherwise?

I think it should be equivalent to put the HRTB in the method bound instead of the the super trait, yes.

davidhewitt

Thanks, this one looks great to me now!

Icxolu added 3 commits August 17, 2024 21:29

add #[pyo3(get)] specialization for IntoPyObject

abed641

emit correct diagnostic if nothing is implemented

23103ea

update migration guide

033d8db

Icxolu added the CI-skip-changelog Skip checking changelog entry label Aug 17, 2024

davidhewitt reviewed Aug 17, 2024

View reviewed changes

Icxolu added 3 commits August 18, 2024 13:35

refactor mutable alias checking into a function

18e376b

refactor field ptr offset into function

2dc566a

cfg gate PyO3GetFieldIntoPyObject

9a12485

davidhewitt reviewed Aug 19, 2024

View reviewed changes

review feedback

35a4148

Icxolu mentioned this pull request Aug 20, 2024

Use vectorcall (where possible) when calling Python functions #4456

Merged

davidhewitt approved these changes Aug 21, 2024

View reviewed changes

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

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

Merged via the queue into PyO3:main with commit 7879d57 Aug 21, 2024

Icxolu deleted the pyclass-get branch August 22, 2024 16:15

Icxolu mentioned this pull request Oct 9, 2024

Contribution opportunity: improved conversion traits #4041

Closed

Icxolu mentioned this pull request Oct 29, 2024

require #[pyclass] to be Sync #4566

Merged

	= help: the trait `for<'py> IntoPyObject<'py>` is not implemented for `PhantomData<i32>`, which is required by `PhantomData<i32>: PyO3GetFieldIntoPyObject`
	= help: the trait `IntoPyObject<'_>` is not implemented for `PhantomData<i32>`, which is required by `for<'py> PhantomData<i32>: PyO3GetFieldIntoPyObject<'py>`

Uh oh!

Conversation

Icxolu commented Aug 17, 2024

Uh oh!

Icxolu commented Aug 17, 2024

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mejrs commented Aug 17, 2024

Uh oh!

davidhewitt commented Aug 17, 2024

Uh oh!

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

Uh oh!

Uh oh!

davidhewitt commented Aug 18, 2024

Uh oh!

Icxolu commented Aug 18, 2024

Uh oh!

davidhewitt left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mejrs commented Aug 17, 2024 •

edited

Loading