Skip to content
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

Annotated arguments, bindInstance and factories #98

Closed
TrevorMuraro opened this issue Feb 21, 2020 · 13 comments
Closed

Annotated arguments, bindInstance and factories #98

TrevorMuraro opened this issue Feb 21, 2020 · 13 comments

Comments

@TrevorMuraro
Copy link

I have started using parameterization, annotation and bindInstance to pass global constants to my objects, as such:

// fileConstants.h

namespace fileConstants {
constexpr std::string_view someFile = "/path/to/some.file";
}
// usesSomeFile.h

class UsesSomeFile
{
};

fruit::Component<UsesSomeFile> getUsesSomeFileComponent(std::string_view file = fileConstants::someFile);
// usesSomeFile.cpp

struct SomeFile {};

class UsesSomeFileImpl : public UsesSomeFile
{
public:
   INJECT(UsesSomeFileImpl(ANNOTATED(SomeFile, std::string_view) someFile));
};

fruit::Component<UsesSomeFile> getUsesSomeFileComponent(std::string_view file)
{
   return fruit::createComponent().bind<UsesSomeFile, UsesSomeFileImpl>()
        .bindInstance<fruit::Annotated<SomeFile, std::string_view>>(file);
}

This allows UsesSomeFile to be created with the hardcoded file path for the application, but to be given a different file path in tests. However, when I tried to do the same for a factory-generated class:

// usesSomeOtherFile.cpp

struct SomeOtherFile {};

class UsesSomeOtherFileImpl : public UsesSomeOtherFile
{
public:
   INJECT(UsesSomeOtherFileImpl(ANNOTATED(SomeOtherFile, std::string_view) someOtherFile));
};

fruit::Component<std::function<std::unique_ptr<UsesSomeOtherFile>()>> getUsesSomeOtherFileFactoryComponent(std::string_view file)
{
   return fruit::createComponent().bind<UsesSomeOtherFile, UsesSomeOtherFileImpl>()
        .bindInstance<fruit::Annotated<SomeOtherFile, std::string_view>>(file);
}

The program compiles, but throws std::bad_alloc on attempting to install getUsesSomeOtherFileFactoryComponent.

I couldn't find anything on using bindInstance with factories. Can I use bindInstance in a factory component definition at all? If not, is there any other way for me to pass the factory component function's parameter to the class constructor?

Also, another documentation request: while the Quick Reference says bindInstance can be used for annotated types, it doesn't say how. Specifying that it's bindInstance<fruit::Annotated<MarkerType, T>>(instance) would be helpful.

@poletti-marco
Copy link
Contributor

bindInstance takes a reference to an object, so in this case you're passing a reference to the parameter, but that won't outlive the injector, so by the time you try to inject it that string_view object has been destroyed.
The comment on the method definition mentions this but the reference documentation didn't, now added the following paragraphs there:

The caller of bindInstance must ensure that the provided reference is valid for the entire lifetime of the component and of any components or injectors that install this component. Note that bindInstance does not create a copy of the parameter.

Unlike other injected objects Fruit will not take ownership of the object passed to bindInstance, so it won't destroy that object. The typical use-case is to pass a static variable or an object constructed in main (or similar) that outlives all injectors in the program.

Also added a note on how to inject annotated values, thanks for the suggestion.

You can fix your example code by passing a string_view& as a parameter in getUsesSomeOtherFileFactoryComponent.

Side note: not sure why you're using factories, it's a very advanced API that should not be necessary in most cases (is it just for this parameter or is there some other reason to use it?), and adds complexity because now all places that inject that object need to also use factories (which in rare cases is justified, but often not).

Also note that annotated and assisted params are orthogonal; you can use annotated params without having to bind a factory, or factories with annotated params. Not sure if this was already clear, I just wanted to make sure it is.

Passing parameters to get*Component functions just to then bind different values is not necessary, consider calling bindInstance with your real constant in the application, and then using replace(...).with(...) to override that binding in unit tests.
https://github.com/google/fruit/wiki/Quick-reference#component-replacements

That way the code remains encapsulated: some other test (e.g. an integration test) might not need to override all values that need to be overridden in unit tests.

@poletti-marco
Copy link
Contributor

I hope that helps.
Please let me know if you're still having issues or if you have more questions.

@TrevorMuraro
Copy link
Author

I had tried references (fruit::Component<std::function<std::unique_ptr<UsesSomeOtherFile>()>> getUsesSomeOtherFileFactoryComponent(const std::string_view& file)), but got a massive pile of errors related to std::hash - I assume because references can't be hashed.

I'm using factories because I'm doing a lot of hardware interfacing and inter-thread communication between application-length threads, and that means many of the same thing in some cases - one example is the binary semaphores from my last issue, multiple of which are used by control threads to stop or start parts of persistent worker threads; another is I2C connections, where there may theoretically be up to 127 instances of the I2CConnection class spread throughout different device manager objects.

The bit about annotated parameters in factories was immediately clear, but it took me a bit to figure out how to use them with registerFactory (for any future readers, like this:)

fruit::Component<std::function<std::unique_ptr<Foo>()>> getFooFactoryComponent()
{
   return fruit::createComponent().bind<Foo, FooImpl>().
      registerFactory<std::unique_ptr<Foo>(fruit::Annotated<Bar, Baz> qux)>([](Baz* qux) {
         return std::unique_ptr<Foo>(new FooImpl(qux));
      });
}

About replace(...).with(...), I thought that could only replace entire get*Component functions, not individual bindings. Can I do this:

fruit::Component<std::function<std::unique_ptr<UsesSomeOtherFile>()>> getUsesSomeOtherFileFactoryComponent()
{
   return fruit::createComponent().bind<UsesSomeOtherFile, UsesSomeOtherFileImpl>()
      .bindInstance<fruit::Annotated<SomeOtherFile, std::string_view>>(fileConstants::someOtherFile);
}

and then override just the bindInstance?

@poletti-marco
Copy link
Contributor

I had tried references (fruit::Component<std::function<std::unique_ptr()>> getUsesSomeOtherFileFactoryComponent(const std::string_view& file)), but got a massive pile of errors related to std::hash - I assume because references can't be hashed.

I meant this, not putting the reference in the return type:

fruit::Component<std::function<std::unique_ptr<UsesSomeOtherFile>()>> getUsesSomeOtherFileFactoryComponent(std::string_view& file) {...}

So that Fruit gets a reference to the actual static var you defined somewhere rather than the param (which is a stack variable that is destroyed after the call to get*Component completes.

The bit about annotated parameters in factories was immediately clear, but it took me a bit to figure out how to use them with registerFactory [...]

Yes, that's a bit complex. Usually (as in this case AFAICT, unless there are additional complications) you can get away with using the INJECT() wrapper on the constructor instead, so that Fruit generates that boilerplate code for you.

About replace(...).with(...), I thought that could only replace entire get*Component functions, not individual bindings

That's true, but it's not really a limitation. You can extract the bindInstance in a separate get*Component function (say, bindInstanceComponent) install that in the current get*Component function you have for your application, and then call replace(bindInstanceComponent).with(bindInstanceComponentForTests).

@poletti-marco
Copy link
Contributor

[...] there may theoretically be up to 127 instances of the I2CConnection class spread throughout different device manager objects.

I see, then if you really need separate instances (and especially with so many of them) I agree factories are the way to go for classes like the I2CConnection you describe.

I was just warning you since I've seen people go down the "make everything a factory" route and then ending up not being able to do sth that would be simple in the non-factory case.

@TrevorMuraro
Copy link
Author

I meant this, not putting the reference in the return type:

Aside from not using const, that's the same thing (I just left the function body off).

I got a bunch of error messages like this:

/usr/include/c++/8/bits/functional_hash.h:101:12: note: ‘std::hash<const std::basic_string_view<char>&>::hash()’ is implicitly deleted because the default definition would be ill-formed:
     struct hash : __hash_enum<_Tp>
            ^~~~
/usr/include/c++/8/bits/functional_hash.h:101:12: error: no matching function for call to ‘std::__hash_enum<const std::basic_string_view<char>&, false>::__hash_enum()’
/usr/include/c++/8/bits/functional_hash.h:82:7: note: candidate: ‘std::__hash_enum<_Tp, <anonymous> >::__hash_enum(std::__hash_enum<_Tp, <anonymous> >&&) [with _Tp = const std::basic_string_view<char>&; bool <anonymous> = false]’
       __hash_enum(__hash_enum&&);

The same happens when using non-const std::string_view&, with additional error qualifiers dropped in binding reference of type "std::string_view &" to initializer of type "const std::string_view".

Which makes sense, I think - the Quick Reference specifies that get*Component parameters must be hashable, and I don't believe references are hashable.

That's true, but it's not really a limitation. You can extract the bindInstance in a separate get*Component function (say, bindInstanceComponent) install that in the current get*Component function you have for your application, and then call replace(bindInstanceComponent).with(bindInstanceComponentForTests)

Ah. I'd hoped to avoid that due to the multiplication of component definitions, but having experimented with potential alternatives it really is simpler and safer.

I see, then if you really need separate instances (and especially with so many of them) I agree factories are the way to go for classes like the I2CConnection you describe.

I was just warning you since I've seen people go down the "make everything a factory" route and then ending up not being able to do sth that would be simple in the non-factory case.

I've got a little of a bunch of things - basic injection where I can, factories for things that need either a bunch of instances with a varying parameter or multiple identical instances in different places, annotation for one case where there are exactly two instances of a class and they have different constants (so I have AFooImpl : public FooImpl and BFooImpl : public FooImpl, and they just pass different things to FooImpl's constructor), and lazy injection for spots where a throwing constructor needs to bring down part of the application but not all of it.

Thanks for the advice!

@poletti-marco
Copy link
Contributor

Oh, it looks like reference params to get*Component functions don't work (build error).
When I have some time I'll look into making them work, or at least failing with a nice error message.
For now, a workaround is to pass a pointer:

#include <string_view>
#include <fruit/fruit.h>

struct SomeOtherFile {};

struct UsesSomeOtherFile {
    virtual ~UsesSomeOtherFile() = default;
};

class UsesSomeOtherFileImpl : public UsesSomeOtherFile
{
public:
   INJECT(UsesSomeOtherFileImpl(ANNOTATED(SomeOtherFile, std::string_view&) someOtherFile)) {
       (void)someOtherFile;
   }
};

using Fun = std::function<std::unique_ptr<UsesSomeOtherFile>()>;

fruit::Component<Fun> getUsesSomeOtherFileFactoryComponent(std::string_view* file)
{
   return fruit::createComponent()
        .bind<UsesSomeOtherFile, UsesSomeOtherFileImpl>()
        .bindInstance<fruit::Annotated<SomeOtherFile, std::string_view>>(*file);
}

static std::string_view s = "something";

int main() {
    
    fruit::Injector<Fun> injector(getUsesSomeOtherFileFactoryComponent, &s);
    Fun f(injector);
    std::unique_ptr<UsesSomeOtherFile> p = f();
    (void)p;
}

@TrevorMuraro
Copy link
Author

Useful to know. Thanks!

@poletti-marco
Copy link
Contributor

For future reference, filed #99 to track the bug about the compile error with references.

@TrevorMuraro
Copy link
Author

TrevorMuraro commented Feb 21, 2020

I may have another one.

This:

// fileConstants.h

namespace fileConstants
{
struct FooFilePath{};
inline const std::string_view fooFile = "foo.json";
inline fruit::Component<fruit::Annotated<FooFilePath, std::string_view>> getFooFilePathComponent()
{
   return fruit::createComponent().bindInstance<fruit::Annotated<FooFilePath, std::string_view>>(fooFile);
}
}

gets this:

/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h: In instantiation of ‘struct fruit::impl::NonConstBindingRequiredButConstBindingProvidedError<fruit::Annotated<fileConstants::FooFilePath, std::basic_string_view<char> > >’:
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/component.defn.h:56:55:   required from ‘fruit::Component<Types>::Component(fruit::PartialComponent<Bindings ...>&&) [with Bindings = {fruit::impl::BindConstInstance<fruit::Annotated<fileConstants::FooFilePath, std::basic_string_view<char, std::char_traits<char> > >, std::basic_string_view<char, std::char_traits<char> > >}; Params = {fruit::Annotated<fileConstants::FooFilePath, std::basic_string_view<char, std::char_traits<char> > >}]’
/workspaces/embedded/include/constants/fileConstants.h:17:105:   required from here
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h:205:7: error: static assertion failed: The type T was provided as constant, however one of the constructors/providers/factories in this component requires it as a non-constant (or this Component declares it as a non-const provided/required type). If you want to only have a const binding for this type, you should change the places that use the type to inject a constant value (e.g. T, const T*, const T& and Provider<const T> are ok while you should avoid injecting T*, T&, std::unique_ptr<T> and Provider<T>) and if the type is in Component<...> make sure that it's marked as const there (e.g. Component<const T> and Component<Required<const T>> are ok while Component<T> and Component<Required<T>> are not. On the other hand, if you want to have a non-const binding for this type, you should switch to a non-const bindInstance (if you're binding an instance) or changing any installed component functions to declare the type as non-const, e.g. Component<T> or Component<Required<T>> instead of Component<const T> and Component<Required<const T>>.
       AlwaysFalse<T>::value,

But this:

// fileConstants.h

namespace fileConstants
{
struct FooFilePath{};
inline const std::string_view fooFile = "foo.json";
inline fruit::Component<fruit::Annotated<FooFilePath, const std::string_view&>> getFooFilePathComponent()
{
   return fruit::createComponent().bindInstance<fruit::Annotated<FooFilePath, const std::string_view&>>(fooFile);
}
}

Gets this:

/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h: In instantiation of ‘struct fruit::impl::NonClassTypeError<const std::basic_string_view<char>&, std::basic_string_view<char> >’:
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/component.h:93:25:   required from ‘class fruit::Component<fruit::Annotated<fileConstants::FooFilePath, const std::basic_string_view<char, std::char_traits<char> >&> >’
/workspaces/embedded/include/constants/fileConstants.h:15:105:   required from here
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h:59:17: error: static assertion failed: A non-class type T was specified. Use C instead.
   static_assert(AlwaysFalse<T>::value, "A non-class type T was specified. Use C instead.");

According to the comment in component.h, both should work.

(Line 15 is the line of fruit::Component and line 17 is the line of fruit::createComponent()).

Trying to bind fruit::Annotated<FooFilePath, const std::string_view*> (which also should work) or fruit::Annotated<FooFilePath, std::string_view*> (which shouldn't) also gets NonClassTypeError in both cases.

I originally got this with the 3.4.0 release, but I updated to master and it's still happening.

@TrevorMuraro TrevorMuraro reopened this Feb 21, 2020
@poletti-marco
Copy link
Contributor

poletti-marco commented Feb 21, 2020 via email

@TrevorMuraro
Copy link
Author

That also gets NonClassTypeError:

// fileConstants.h

namespace fileConstants
{
struct FooFilePath{};
inline const std::string_view fooFile = "foo.json";
inline const fruit::Component<fruit::Annotated<FooFilePath, const std::string_view>> getFooFilePathComponent()
{
   return fruit::createComponent().bindInstance<fruit::Annotated<FooFilePath, const std::string_view>>(fooFile);
}
}
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h: In instantiation of ‘struct fruit::impl::NonClassTypeError<const std::basic_string_view<char>, std::basic_string_view<char> >’:
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/component.defn.h:138:55:   required from ‘fruit::PartialComponent<fruit::impl::BindConstInstance<AnnotatedType, C>, Bindings ...> fruit::PartialComponent<Bindings>::bindInstance(const C&) [with AnnotatedType = fruit::Annotated<fileConstants::FooFilePath, const std::basic_string_view<char> >; C = std::basic_string_view<char>; Bindings = {}]’
/workspaces/embedded/include/constants/fileConstants.h:17:111:   required from here
/home/conan/.conan/data/fruit/3.4.0/_/_/package/b80d46004713aa37d6a90b42e2a326a056a237b5/include/fruit/impl/injection_errors.h:59:17: error: static assertion failed: A non-class type T was specified. Use C instead.
   static_assert(AlwaysFalse<T>::value, "A non-class type T was specified. Use C instead.");

@TrevorMuraro
Copy link
Author

Ah, never mind - I found the test case that shows the working solution.

namespace fileConstants
{
struct FooFilePath{};
inline const std::string_view fooFile = "foo.json";
inline const fruit::Component<fruit::Annotated<FooFilePath, const std::string_view>> getFooFilePathComponent()
{
   return fruit::createComponent().bindInstance<fruit::Annotated<FooFilePath, std::string_view>>(fooFile);
}
}

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