simplify extract_argument code for &T: PyClass
#5408
Conversation
CodSpeed Performance ReportMerging #5408 will not alter performanceComparing Summary
|
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
This look great! I was actually looking yesterday as well to remove the IsOption special handling but ran into the case that we still need it for something like Option<&Bound>.
I think this is a much better and I really like that this only special cases the things that need it. Something like Option<&str> that can go through FromPyObject now will go through it instead of a mixture of PyFunctionArgument and FromPyObject.
Moving things out of the macros is always good 👍
Maybe we add a ui test for the new diagnostic?
| _, | ||
| { #pyo3_path::impl_::pyclass::IsOption::<#arg_ty>::VALUE } | ||
| >( | ||
| #pyo3_path::impl_::extract_argument::extract_argument( |
There was a problem hiding this comment.
Looks like the compiler is now smart enough to figure this out via inference? Great 💯
There was a problem hiding this comment.
Yes, I think due to the way the true / false const generic is working it happens to fall out nicely that each type only ever should have one implementation, so type inference can do it 🎉
| } | ||
| )? | ||
| } | ||
| quote_arg_span! { |
There was a problem hiding this comment.
Great to remove special casing of Options and reduce usage of option_wrapped_type 🥳
There was a problem hiding this comment.
Indeed, this felt like a great cleanup 😁
|
Also, this might mean that we can seal |
|
Good point, added UI test and sealed 👍 Turns out that to seal I had to use the same const generic on |
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
Turns out that to seal I had to use the same const generic on IMPLEMENTS_FROMPYOBJECT to ensure the specialized implementations did not overlap with the blankets. I think that's still fine, will proceed to merge.
Makes sense I agree.
2 participants
Following on from #4390 I was exploring
extract_argument.rsto understand if anything could be simplified, and I've found a way to move the definition ofPyFunctionArgumentfor the#[pyclass]type references out of the macros and into generic code.This should reduce the generated macro output a fair bit, hopefully beneficial for compile times 🤞
The main trick is to adjust the const generic specialization from
IsOptiontoIsFromPyObjectand then all the custom implementations inextract_argument.rsset that parameter tofalse, while all "normal" types will have that be discovered to be true.While at it, I added a
diagnostic::on_unimplementedto make the error messages a touch nicer.