types: refactor native_type macros

[davidhewitt]

I rearranged the pyobject_native_type macros in types/mod.rs to make them slightly easier to work with.

First, I created pyobject_native_type_base, which contains impls all native types should want, including PyAny. I discovered that a few native types such as &PyIterator and &PyException did not implement some of these. This also allowed me to delete a couple of handwritten impls for e.g. PyAny.

Second, pyobject_native_type_fmt doesn't need to be separate any more (used to be split out before the exception fmt was just made consistent in #1275) - I merged these impls into pyobject_native_type_base.

Next, I moved impls which PyAny doesn't want into pyobject_native_type_named.

Third, I made the checkfunction optional, which allowed me to remove some code in exceptions.rs which was basically a copy-paste of the default impl. To get the macros to compile I had to modify the type_param syntax slightly.

Finally, I renamed pyobject_native_type_convert to pyobject_native_type_info, as that's now all it was left with.

Overall this reduced LOC and added quite a few missing fmt and other impls to PyException and PyIterator.

It does require a couple of small code changes to rust-numpy though, so I will push a PR for that shortly...

[kngwyu]

Thanks,

[kngwyu]

I'm sorry but I don't understand why we need this complex one with an additional $.
Could you please explain this more?

[davidhewitt]

👍

So, first, for the exceptions macros, I wanted to make $checkfunction optional. They don't have their own checkfunctions, and so previously I had copy-pasted the default implementation of PyTypeInfo::is_type_of. By making $checkfunction optional I could get rid of this copy-and-paste.

However, then there were two optional ident arguments in a row: $checkfunction and $type_param. This is ambiguous for macro_rules, so it would not compile.

This meant I needed to change the syntax slightly. Because the generics are used least (i.e. only in rust-numpy as far as I know) I thought it was best to modify the syntax for the generics.

The solution here I made the additional optional form to pass to the macro generics: <T, D> . But because the generics: < > structure is all optional ($(generics: < ... >)? this is what causes the extra $ level.

I don't love it, but couldn't see many better options.

An alternative could be to change $name:ty to $name:ident < $($generics),* > and maybe add second macro arm to each for the generic-free case, but that's also kind of yucky....

I'm happy to accept any other proposals how to do this!

[kngwyu]

I'm sorry for the delay and will add a review after playing with macro soon.

[davidhewitt]

👍

I have been looking also at whether it's better to make these macros into proc-macros. But that will make the macros feature non-optional.

[kngwyu]

The only alternative syntax I came up with is $(, $checkfunction:path)? $(;$generics: ident)* (branch: https://github.com/PyO3/pyo3/tree/refactor-macros-alternative-syntax).
But I'm actually not sure which is better.
For simplifying macro definition I think ; is better but generics: <> would look clearer when calling the macros.
Since now the only caller (with generics) is rust-numpy, I'm slightly inclining in taking the merit in simplifying the definition.

BTW, why generics? I think they are actually type parameters in AST, though I think generics is also good.

[davidhewitt]

I picked "generics" as a short name for "generic type parameters" 👍

I think I agree let's go for the simpler macro-rules syntax as the generics are only used by numpy.

Maybe in the future if it turns out more users want this, we should consider making a proc-macro with better ergonomics. But for now this is fine.

[davidhewitt]

I also updated PyO3/rust-numpy#163 - so I think this PR is ready to merge, and then that one when we release 0.13.

[kngwyu]

Looks good, thank you!