add various (Try)From impls to convert &Bound<T>

[Icxolu]

• adds TryFrom<&Bound<T>> for PyRef<T>, PyRefMut<T>, PyClassGuard<T> and PyClassGuardMut<T>
• adds From<&Bound<T>> for Bound<T> and Py<T>

This removes the now obsolete BoundRef type.

[davidhewitt]

Thanks very much for this. A few thoughts regarding the new From / TryFrom implementations:

• They add easy ways to implicitly do the equivalent of .clone(). That's probably not an issue, though maybe the reference counting being more explicit would be desirable? Then again, I suggested adding From<Borrowed<T>> for Py<T> some time ago and a bunch of the other conversions we have perform implicit ref counting ops, so maybe this ship already sailed.
• If we don't want to add more ways to do implicit reference counting, these probably need to be packaged up in helper traits private to the macros? Might not be so concise for the implementation. We probably can't seal the traits given users can currently add From<BoundRef> implementations downstream.
• Given users could theoretically plug in From<BoundRef> implementations, do we need a deprecation cycle? It's a type in the impl_ submodule so my instinct is no deprecation needed.
• At least with helper traits we could use #[diagnostic::on_unimplemented] to provide custom error messages for users which specifically mention #[pass_module] / #[classmethod] etc. Though From / TryFrom is already not too bad for users to correct.

If we're choosing to just use From / TryFrom instead of helper traits, I think I'd prefer those to be introduced in a precursor PR with tests & a changelog entry, given they're API addititions.

[Icxolu]

• They add easy ways to implicitly do the equivalent of .clone(). That's probably not an issue, though maybe the reference counting being more explicit would be desirable? Then again, I suggested adding From<Borrowed<T>> for Py<T> some time ago and a bunch of the other conversions we have perform implicit ref counting ops, so maybe this ship already sailed.

While I agree in principle that it's nice to have it explicit, I think we already do it in too many places to make this a good reason. I think it simplifies our implementation and the operation is quite cheap, so in my opinion these impls are acceptable.

• Given users could theoretically plug in From<BoundRef> implementations, do we need a deprecation cycle? It's a type in the impl_ submodule so my instinct is no deprecation needed.

I don't think this is needed. impl_ is #[doc(hidden)] and thus not part of our public api.

• At least with helper traits we could use #[diagnostic::on_unimplemented] to provide custom error messages for users which specifically mention #[pass_module] / #[classmethod] etc. Though From / TryFrom is already not too bad for users to correct.

I think the From/TryFrom error messages are usually quite good. Also with these impl now the error message start to mention the relevant type instead of our internal implementation detail, so users can easily look up in the docs what conversions are possible.

If we're choosing to just use From / TryFrom instead of helper traits, I think I'd prefer those to be introduced in a precursor PR with tests & a changelog entry, given they're API addititions.

Sure, I can split these off and open a separate PR for them.

[Icxolu]

Given that we have full coverage of these impl through our existing tests, would you like to see explicit units tests anyway?

[davidhewitt]

Given that we have full coverage of these impl through our existing tests, would you like to see explicit units tests anyway?

Good question. I guess if we add these in an initial PR without the changes here, we don't have coverage.

Perhaps I am asking for too much ceremony. How about we just adjust this PR in-place so that the title / OP / changelog focusses on the new API additions for users to see. The obsoletion & deletion of BoundRef in this patch can then be viewed a nice side effect of the API additions.

That avoids the need for extra PR / unit tests / ceremony, while keeping the messaging user-focussed.

[davidhewitt]

Thanks, great to move this forward!