Is it valid to have both class_ & custom type_caster?

[rwgk]

Minimal reproducer of a situation I found in the wild. Is this valid?
We could add a static_assert to make this impossible. Good or bad idea?

[YannickJadoul]

Yes, this shouldn't really be a problem, but it's a weird situation, and I'd be quite surprised if it's useful.

Think of py::class_ as doing roughly two things:

1. Dynamically creating a new Python class, similar to having class Foo: ... or type("Foo", ..., ...) in a Python script/repl.
2. Registering this type in pybind11's internals such that the default, fallback type_caster knows that C++'s Foo should translate to Python's Foo and vice versa.

The second part is crucial, here, since this type_caster for py::class_ is the default one; the non-specialized template (implemented by type_caster_base<T>; see here)

This also means that's it's perfectly possible to overwrite this caster with a custom version. So what you're left with then is just the first point: a Python class that's +- "unrelated" to the C++ class.

[YannickJadoul]

a Python class that's +- "unrelated" to the C++ class.

At least, "unrelated" depending on what the custom type_caster is doing. That custom type caster might be even poking into pybind11's internals, using type_caster_base to still get pybind11's conversion between Python and C++ type.

[YannickJadoul]

That being said: as far as I'm concerned, this is a weird situation, and the two orthogonal components involved (class_ and type_caster) are extensively covered by existing test (especially after #2834 will get in).

[rwgk]

Thanks @YannickJadoul

I will mark this PR as draft until I have more time and data (feedback from the team that uses the mix in the wild).

I think there needs to be one of two outcomes:

• If we allow for the mix: add a test for it, even though it's weird, and add a pointer to the test in the docs.
• Or add a static_assert or runtime check to make the mix impossible.

[rwgk]

Small update: the case in the wild was this: tensorflow/tensorflow@85c6f51

In this case it was an oversight to have a mix. Thanks @hawkinsp for resolving this question so quickly!

I'm internally retesting with a static_assert, to see what else breaks.

[YannickJadoul]

Interesting, thanks!

On thing to keep in mind is that there might be type_casters out there that try to extend the default one? I.e., the combination of py::class_ with a custom type caster that (internally) uses py::detail::type_caster_base<T>/py::detail::type_caster_default might make perfect sense to me. It could e.g. use pybind11's link between C++ and Python types, but still add an extra check on top.
So we'd need to be careful about how to add such a check, at least.

[rwgk]

For the record: with our global testing, I found 10 additional types that are wrapped with py::class but use a type_caster that's not a subclass of type_caster_base.

To be investigated later. I need to de-prioritize this at the moment.

Leaving this as a Draft PR for now.

[EricCousineau-TRI]

FWIW I think it's a useful feature to (a) fail-fast on inadvertent mixing and (b) provide option for specializing when it's really what you want.

I've implemented a simple form of this in present rev of #2950. Can carve out if desired.

[rwgk]

FWIW I think it's a useful feature to (a) fail-fast on inadvertent mixing and (b) provide option for specializing when it's really what you want.

Yeah I agree that would be best.

It's a while ago I'm not sure I remember correctly, but I think I'd need to invest a few days of cleanup work before I could roll out that feature inside Google. I don't think I'll get to that anytime soon. — I opened this PR when I was still uncertain how much trouble I'm getting myself into working on the smart_holder code without this safety feature, but that's not a worry anymore, I got past that stage alright.

I've implemented a simple form of this in present rev of #2950. Can carve out if desired.

Sounds great! I'll close this PR now. — The comments here are valuable, but having this PR hang around open and stale not so much.