boost-ext / di

C++14 Dependency Injection Library
https://boost-ext.github.io/di
1.17k stars 140 forks source link

Multiple instance values not injected correctly #300

Closed scottmcnab closed 5 years ago

scottmcnab commented 6 years ago

I cannot seem to get multiple instance objects to inject correctly. When the collection is injected into the target object, the injected value objects appear default constructed.

For example, consider binding a collection of simple objects with value semantics:

struct CustomValue {
    std::string name;
    std::string description;
}

auto injector = di::make_injector( 
    di::bind<CustomValue []>().to( {
        CustomValue { "foo", "foo description" },
        CustomValue { "bar", "bar description" }
    });
);

Expected Behavior

auto v = injector.create<std::vector<CustomValue>>();
assert(v.size() == 2);
assert(v[0].name == "foo");                    // This assert fails!
assert(v[0].name == "foo description"); // This fails too
assert(v[1].name == "bar");                    // ditto
assert(v[1].name == "bar description"); // ditto

Actual Behavior

auto v = injector.create<std::vector<CustomValue>>();
assert(v.size() == 2);
assert(v[0].name == "");             // This string should not be empty! 
assert(v[0].description == "");    // Neither should this one.
assert(v[1].name == "");             // ditto
assert(v[1].description == "");    // ditto

It looks to me that the CustomValue collection is being injected with default-constructed values, instead of values created by bind<>. Oddly enough, the size of the bound array is correct (i.e. 2 items).

Any idea how to fix/work around this? I'd really like to use this library in my current project, however this is a show-stopper.

Thanks

Specifications

kris-jusiak commented 6 years ago

Thanks, @scottmcnab for reporting this issue.

The problem is MSVC only. I've just tested your example with gcc/clang and everything is working just fine.

I don't have an easy access to MSVC ATM but I bet it's related to the way MSVC treats aggregates and resolves using uniform initialization.

I think I'm not 100% sure, though, that the workaround might be to explicitly specify the copy constructor for CustomValue. That should trick the resolving part and inject name, description.

struct CustomValue {
    CustomValue(const std::string& name, const std::string& description)
        : name(name), description(description)
    { }
    CustomValue(const CustomValue&) = default;

    std::string name;
    std::string description;
}

I'll dig in some more when I'll have access to MSVC.

scottmcnab commented 6 years ago

Thanks for the suggestion. I tried implementing custom constructor and copy constructor for CustomValue and it didn't help. For some reason the value objects still get created empty.

FWIW, I defined: CustomValue() = delete; and the problem still occurs, so I don't think its case of the default constructor somehow being called.

What is strange is when I placed a breakpoint on the copy constructor, the right-hand-side object that is passed by reference is an empty instance! I don't know how this is possible when the default constructor is deleted...

Interestingly, if I change the copy constructor to explicit:

    explicit CustomValue(const CustomValue& rhs) : name(rhs.name), description(rhs.description) {}

Then I start getting compiler errors of the form:

test.cpp(58): error C2440: 'initializing': cannot convert from 'CustomValue' to 'CustomValue' test.cpp(58): note: Constructor for struct 'CustomValue' is declared 'explicit' test.cpp(59): error C2440: 'initializing': cannot convert from 'CustomValue' to 'CustomValue' test.cpp(59): note: Constructor for struct 'CustomValue' is declared 'explicit' test.cpp(81): error C2664: 'boost::di::v1_0_2::core::dependency<TScope,T,TExpected,boost::di::v1_0_2::no_name,void> &boost::di::v1_0_2::core::dependency<TScope,TExpected,TExpected,boost::di::v1_0_2::no_name,void>::to<>(...) noexcept const': cannot convert argument 1 from 'initializer list' to 'std::initializer_list &&' 1> with 1> [ 1> TScope=boost::di::v1_0_2::scopes::deduce, 1> T=CustomValue [], 1> TExpected=CustomValue [] 1> ] test.cpp(84): note: Reason: cannot convert from 'initializer list' to 'std::initializer_list' test.cpp(84): note: Element '1': no conversion from 'CustomValue' to 'CustomValue' test.cpp(84): note: Element '2': no conversion from 'CustomValue' to 'CustomValue'

Any idea how this might be happening?

scottmcnab commented 6 years ago

Actually, I think the errors with explicit keyword may be a red herring:

In my actual code, CustomValue is defined in a separate DLL and linked to the test .exe using declspec(dllexport) / declspec(dllimport). When I copy the same class into the test .exe directly (i.e. to bypass any DLL symbol issues), I don't get these compiler errors. I do still get the same incorrect binding result however.

kris-jusiak commented 6 years ago

Maybe, as a wknd, until the issue will get fixed properly, you can try binding it directly to a vector instead. It won't be that generic as [], meaning it won't out of the box be working for other containers but maybe it will unblock you? MSVC is always full of surprises ;)

struct CustomValue {
    std::string name;
    std::string description;
};

int main() {
  auto injector = di::make_injector(
      di::bind<>().to(
        std::vector<CustomValue>{
          CustomValue { "foo", "foo description" },
          CustomValue { "bar", "bar description" }
        }
      )
  );

  auto v = injector.create<std::vector<CustomValue>>();
  assert(v.size() == 2);
  assert(v[0].name == "foo");
  assert(v[0].description == "foo description");
  assert(v[1].name == "bar");
  assert(v[1].description == "bar description");
};
kris-jusiak commented 5 years ago

It seems that we are hitting an undefined behaviour with initializer_lists here :thinking:

An object of type std::initializer_list<E> is constructed from an initializer list as if the implementation allocated an array of N elements of type E, where N is the number of elements in the initializer list. Each element of that array is copy-initialized with the corresponding element of the initializer list, and the std::initializer_list<E> object is constructed to refer to that array. If a narrowing conversion is required to initialize any of the elements, the program is ill-formed.

The newest clang and clang with optimizations also stopped working.

The solution is to extend the lifetime of the initializer list.

auto il = {
        CustomValue { "foo", "foo description" },
        CustomValue { "bar", "bar description" }
    };

auto injector = di::make_injector( 
    di::bind<CustomValue []>().to(il);
);

The above seems to work in clang/gcc/msvc.