Throw TypeError when subclasses forget to call __init__

[virtuald]

Forgetting to call __init__ in inherited classes is one of the biggest issues my users run into (and it's frustrating because it typically causes a segfault), I'd love to see this merged. It would make pybind11 so much more usable!

... this probably causes a minor performance impact when creating new objects. I would expect it to be in the noise, though I haven't looked at the timing yet.

• Fixes Use pybind11 metaclass to detect when __init__ has not been called from subclass #2103

[wjakob]

This change looks great, and I am happy to merge it. Performance impact should be minimal as you say. But can you please fix pep8? (See the failing "STYLE" test).

[wjakob]

In particular, there is a long line with the error message string that should be split.

[virtuald]

FIxed, sorry for the delay!

[virtuald]

Ready for merge @wjakob , thanks!

[virtuald]

Still ready. :)

[virtuald]

Ping.

[virtuald]

Ping? :(

[YannickJadoul]

As gently pinged by @virtuald on Gitter, this should be ready to finish, as it was already reviewed.

So this functionality is not present when a custom metaclass is used? For completeness, it might be nice to mention that in the docs, still.

[wjakob]

Thanks @virtuald, and for the reminder @YannickJadoul.

[virtuald]

@YannickJadoul the docs I added indirectly say what you want?

The default pybind11 metaclass will throw a TypeError when it detects
that __init__ was not called by a derived class.

Implying that if you set your own metaclass then you'll get different behavior... ?

[YannickJadoul]

@YannickJadoul the docs I added indirectly say what you want?

OK, yes, that's a bit implicit, but good enough. Using custom metaclasses comes with a lot of caveats anyway, currently.

[rwgk]

It turns out this PR #2152 leads to a test breakage related to this OSS deepmind code:

https://github.com/deepmind/open_spiel/blob/30ab4d23fd306358e874904b9dfe1dd3d52c5bcd/open_spiel/python/pybind11/pyspiel.cc#L311

The situation top-down:

• Google has non-OSS code like this: class SomeGame(pyspiel.Game):
• py::class_<Game, std::shared_ptr<Game>> game(m, "Game"); (the OSS link above)
• class Game : public std::enable_shared_from_this<Game> { ... }; (link below)
• Game has no public constructor but 3 factory functions like: std::shared_ptr<const Game> LoadGame(const std::string& game_string); (same file line 882)

https://github.com/deepmind/open_spiel/blob/30ab4d23fd306358e874904b9dfe1dd3d52c5bcd/open_spiel/spiel.h#L633

When SomeGame is instantiated, pybind11 raises pyspiel.Game.__init__() must be called when overriding __init__.
But there is no __init__ that can be called.
What's the recommended solution to this issue?

[bstaletic]

But there is no __init__ that can be called.

Are you sure? "No __init__" is different than "no constructor".

>>> __import__("foo").S.__init__
<slot wrapper '__init__' of 'foo.S' objects>
>>> __import__("foo").S()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo.S: No constructor defined!

[rwgk]

When naively adding a call to __int__ I'm getting:

    pyspiel.Game.__init__(self)
TypeError: SomeGame: No constructor defined!

Looking at the wrapper code (pyspiel.cc link in previous comment), there isn't an init defined.

But I'm not sure where to start fixing. Fake an init (if that even makes sense)? Change pybind11 to not raise in this situation? Some other trick?

[henryiii]

This is just checking to see if this has been initialized; currently it is not. If pyspiel.Game is not default constructible, why would SomeGame be? It needs to call one of the factory functions, or a constructor of some sort, or otherwise pyspiel.Game is not fully constructed.

[rwgk]

This is just checking to see if this has been initialized; currently it is not. If pyspiel.Game is not default constructible, why would SomeGame be? It needs to call one of the factory functions, or a constructor of some sort, or otherwise pyspiel.Game is not fully constructed.

Thanks Henry, I'll dig in deeper to see what they are doing currently. I know eventually they are calling pyspiel.load_game(), but with some tricks in between.

[virtuald]

It's possible that inheriting from a class implemented only by a factory function might not be properly constructing the holder? Seems weird.

[rwgk]

Thanks @bstaletic, @henryiii, @virtuald for taking a look! That made it clear I wasn't overlooking something obvious. After digging in I found a surprising but simple explanation why there were no problems with the existing code (I even tried ASAN & MSAN before digging in): the only function of inheriting from pyspiel.Game was to pass the isinstance(game, pyspiel.Game) test in open_spiel/python/rl_environment.py. Yes, the instance was in fact not initialized, but no problem, because it was never used for anything else: perfect duck typing in disguise.

After that realization the fix was very simple (maybe to be refined by the open_spiel people):

1. Replace isinstance(game, pyspiel.Game) with hasattr(game, 'get_type') and hasattr(game, 'num_players') ("good enough as a duck type check").
2. Replace SomeGame(pyspiel.Game) with SomeGame(object).

Interesting to see how any little hole gets inadvertently (mis)used somehow. @virtuald, thanks so much for plugging this one!

[virtuald]

Should we adjust the error messages to make this sort of thing easier to diagnose in the future?

[YannickJadoul]

Should we adjust the error messages to make this sort of thing easier to diagnose in the future?

What would you propose?
Personally, I find the error message reasonably clear, and it seems that the hard part for @rwgk was to figure out why it didn't get called and why it never resulted in an error? :-)

[YannickJadoul]

Interesting to see how any little hole gets inadvertently (mis)used somehow. @virtuald, thanks so much for plugging this one!

Here's another chance to bring up your favorite law of software engineering, @bstaletic ;-)

[henryiii]

m.class_.Pet.__init__() must be called when overriding __init__

is a little bit cryptic, to @virtuald's point. How about something roughly like:

m.class_.Pet.__init__() must be called when instantiating class that inherits from it?

(I'm also fine with it as is, the biggest problem above was the usage of it, not the message)

[rwgk]

BTW: I worked out another suggested solution for the open_spiel folks, adding a .def(py::init(...)) with an existing factory function. (They didn't like my simpler solution, although they were willing to accept it to unblock the pybind11 update.)

@henryiii and @virtuald: I think some small tweaks to the docs and error message could indeed help a lot.
Specifically, in docs/advanced/classes.rst, around this source code example:

    class Dachshund(Dog):                                                       
        def __init__(self, name):                                               
            Dog.__init__(self) # Without this, undefined behavior may occur if the C++ portions are referenced.
            self.name = name                                                    

The comment seems to need updating, how about: # Without this, a TypeError is raised.

It would also be good to add a sentence like: An extension class without an __init__ cannot be used as a base class in Python. To enable inheritance in Python, __init__ must be implemented.

For the error message, @virtuald, do you know how much trouble it is to check if the base class implements __init__?
To be most helpful:

• If __init__ exists: m.class_.Pet.__init__() must be called when overriding __init__ in a derived class.
• If __init__ does not exist: m.class_.Pet has no __init__ and can therefore not be used as a base class in Python.

Do we have a mechanism for short URLs pointing into the docs?
Appending a link to the error messages would be ideal: one click and aha!
But the URL is a bit long:
https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python
(While we're at it, it might be nice to have a more specific anchor or subsection to point to.)

[YannickJadoul]

I've now just spent half an hour looking around into this code and pybind11's dark magic, thinking whether there ought to be a way to surpass this check when a class is used as "interface", but it feels rather dangerous since it allows to access uninitialized data from Python (triggering UB from Python).
(I've also been playing around with __new__ and calling a factory constructor, I haven't managed yet. This feels like something that could be nice to have, as well.)

There's a way to override isinstance, btw, https://docs.python.org/3/reference/datamodel.html#customizing-instance-and-subclass-checks, if this might help.

The docs should be updated indeed; that part must've been overlooked when merging this PR. I'll go over it and see if I can fix it.

Appending a link to the error messages would be ideal: one click and aha!

I'm not sure this is something we'd want in the error message, though. This is a message that users of a pybind11-library might get to see, and pybind11's docs are meant as a reference for the developers. You don't want to confuse Python users with C++ thing, if you ask me.

I was not a fan of the error message either (since it's basically just 2 steps: a. you need to call __init__ when subclassing, b. you can't call __init__ because there is no constructor => a + b means you can't subclass), but from the perspective of the Python user of a pybind11-library, this can potentially be more useful, yes. So if it's easy enough to add, this seems OK to me.

(While we're at it, it might be nice to have a more specific anchor or subsection to point to.)

Careful with this, though. Inheritance in pybind11 is complex and can be subtle, so I'd prefer users would read the whole section. And if someone already knows how pybind11 interacts with inheritance and subclassing, it should be easy enough to scan that section and find the relevant part, no?

[YannickJadoul]

@rwgk Actually, in the case of pyspiel, a "trampoline" class might be the way to go? This is basically meant as the glue between C++ and Python. It could also provide __init__ without making a bare pyspiel.Game itself constructable:

class X {
public:
        static X create() { return X(42); }
        int getZ() const { return z; }

        virtual ~X() = default;

private:
        X(int y) : z(y) {}

        int z;
};


void f(const X &x) {
        py::print(x.getZ());
}


class PyX : public X {
public:
        PyX() : X(X::create()) {}
};

PYBIND11_MODULE(example, m)
{
        py::class_<X, PyX>(m, "X")
                .def_property_readonly("z", &X::getZ)
                .def(py::init([]() { throw py::type_error("Cannot construct an X object without subclassing."); return static_cast<X*>(nullptr); },
                              []() { return PyX(); }));

        m.def("create", &X::create);
        m.def("f", &f);
}

This results in:

Python 3.8.0 (default, Oct 28 2019, 16:14:01) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import example
>>> example.X
<class 'example.X'>
>>> example.X()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Cannot construct an X object without subclassing.
>>> class Y:
...     
KeyboardInterrupt
>>> class Y(example.X):
...     pass
... 
>>> Y()
<__main__.Y object at 0x7f1bdfc8a360>
>>> class Y(example.X):
...     def __init__(self):
...             example.X.__init__(self)
... 
>>> Y()
<__main__.Y object at 0x7f1bdfc93f40>
>>> 

[YannickJadoul]

See #2429.

[rwgk]

It could also provide __init__ without making a bare pyspiel.Game itself constructable:

Hi @YannickJadoul, my current solution is this:

    py::class_<Game, std::shared_ptr<Game>> game(m, "Game");
    game.def("num_distinct_actions", &Game::NumDistinctActions)
+      .def(py::init([](const std::string& game_string) {
+       return std::const_pointer_cast<Game>(LoadGame(game_string));
+      }))

There is a list of known/allowed names to pick from. It works with one I picked randomly. I asked for advice about creating a "bare" game, to use your word for it. Let's see what they say.

To have an __init__ without args I could add an overload with a hard-wired game_string, e.g. based on their recommendation:

+      .def(py::init([]() {
+       return std::const_pointer_cast<Game>(LoadGame("backgammon"));
+      }))

Mostly out of curiosity, I also tried this:

+      .def(py::init([]() { return std::shared_ptr<Game>{}; }))

It builds fine but pyspiel.Game() segfaults straightaway, in the __init__ call itself (not the destructor, I convinced myself). I don't know what dereferences the pointer and why.

[YannickJadoul]

It builds fine but pyspiel.Game() segfaults straightaway, in the __init__ call itself (not the destructor, I convinced myself). I don't know what dereferences the pointer and why.

Nice, you bumped into a bug! :-D I managed to figure out what goes wrong; see #2430.

To have an __init__ without args I could add an overload with a hard-wired game_string, e.g. based on their recommendation:

Right, I understand. I was mostly trying to point out the trampoline class, which allows to add the __init__ only when subclassing. So pyspiel.Game itself would still not be default-constructible, and it would still force the use of these factory methods.

[virtuald]

Ha, I found a hacky way to determine if the constructor was overridden or not. I'll make a PR.