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.

[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.