Skip to content

Regression from v2.1.1 to v2.2.1 #1247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
daveah opened this issue Jan 11, 2018 · 5 comments
Closed

Regression from v2.1.1 to v2.2.1 #1247

daveah opened this issue Jan 11, 2018 · 5 comments

Comments

@daveah
Copy link

daveah commented Jan 11, 2018

Issue description

When moving from v2.1.1 to v2.2.1 a wrapper around a class with both std::vector and std::initializer_list templated constructors storing data in a boost variant no longer compiles.

Reproducible example code

#include "pybind11/pybind11.h"
#include <vector>
#include <initializer_list>
#include <boost/blank.hpp>
#include <boost/variant.hpp>

namespace py = pybind11;

class Foo {
public:
  template <typename TT>
  Foo(TT init) : Foo(std::vector<TT>(std::move(init))) {}
  template <typename TT>
  Foo(std::vector<TT> init) : _data(std::move(init)) {}
  template <typename TT>
  Foo(std::initializer_list<TT> init) : Foo(std::vector<TT>(init)) {}

private:
  using var = boost::variant<boost::blank, std::vector<uint32_t>>;

  var _data;
};

PYBIND11_PLUGIN(_foo) {
  py::module mm("_foo", "Foo");
  py::class_<Foo> foo(mm, "Foo");
  foo.def(py::init<uint32_t>());
  foo.def(py::init<std::vector<uint32_t>>());
  return mm.ptr();
};

Using boost 1.59, gcc 6.3.1 and python 3.5 this compiles fine with 2.1.1, but not with 2.2.1.

@jagerman
Copy link
Member

This was caused by #1015, which switched to using brace initialization so that you can initialize simple aggregate types with py::init<>, e.g.

struct Agg { int a; int b; };
// ...
m.def(py::init<int, int>());

Unfortunately, this causes one fairly rare corner case compatibility issue (also discussed in #1015) which is just exactly what you ran into: when the class has an initializer_list constructor it wins for brace initialization over a regular constructor. All of this means we end up invoking the Foo(std::initializer_list<std::vector<uin32_t>> init) template specialization, which, in your code, tries to invoke the Foo(std::vector<std::vector<uint32_t>> init) constructor, which of course fails (_data expects a uint32_t vector, not a uint32 vector-vector).

Long story short, it needs a workaround as mentioned in the docs -- see the Note here.

One option--the one mentioned in the docs--is to change your constructor definition to the new-in-2.2 style lambda init:

mm.def(py::init([](std::vector<uint32_t> v) { return new Foo(std::move(v)); }));

Alternatively, I suppose you could use SFINAE to restrict the types accepted by the initializer_list specialization, though that might be tricky/complicated/infeasible depending on the non-simplified code involved.

jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 11, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in pybind#1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
@daveah
Copy link
Author

daveah commented Jan 11, 2018

That makes sense. I've never been a fan of the decisions made in the standard regarding overload resolution in the presence of initialiser lists, but your workarounds sound completely reasonable. Thanks.

@jagerman
Copy link
Member

Also see #1249 (if you haven't noticed it yet), which I think should resolve the issue.

@daveah
Copy link
Author

daveah commented Jan 11, 2018

I noticed that as well - I'll check it out with the next release.

jagerman added a commit that referenced this issue Jan 12, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in #1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
@jagerman
Copy link
Member

Fixed by #1249.

jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 12, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in pybind#1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 12, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in pybind#1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 12, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in pybind#1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 13, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in pybind#1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
wjakob pushed a commit that referenced this issue Feb 7, 2018
This updates the `py::init` constructors to only use brace
initialization for aggregate initiailization if there is no constructor
with the given arguments.

This, in particular, fixes the regression in #1247 where the presence of
a `std::initializer_list<T>` constructor started being invoked for
constructor invocations in 2.2 even when there was a specific
constructor of the desired type.

The added test case demonstrates: without this change, it fails to
compile because the `.def(py::init<std::vector<int>>())` constructor
tries to invoke the `T(std::initializer_list<std::vector<int>>)`
constructor rather than the `T(std::vector<int>)` constructor.

By only using `new T{...}`-style construction when a `T(...)`
constructor doesn't exist, we should bypass this by while still allowing
`py::init<...>` to be used for aggregate type initialization (since such
types, by definition, don't have a user-declared constructor).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants