Unable to scope provider with @ScopeInstances with Runtime checks

[philipbjorge]

This test fails with runtime checks on, not allowing you to bind providers with @ScopeInstances and @Scope...

toothpick/toothpick-runtime/src/test/java/toothpick/AllBindingsTest.java

Line 237 in faedb99

public void bindToProviderClass_shouldCreateNonInjectedSingleton_whenProviderClassIsAnnotatedProvidesSingleton() throws Exception {

[dlemures]

Hi @philipbjorge , thx for the issue and sorry for the wait. We have been a little busy lately.

Regarding the issue, it is right, there is a problem with @ScopeInstances:

According to the documentation, @ScopeInstances is used together with a scope annotation for providers to determine within which scope the instances are recycled.

However, those providers cannot be bound as we don't allow to bind classes that are annotated with scope annotations. Thus, @ScopeInstances is useless...

We are fixing the issue by allowing scoped annotated targets of bindings as long as the scoped annotation is supported by the scope where the binding is defined.

For example, this will be possible only for the root scope (root scope - singleton):

@Singleton
class Y {....}
...
bind(X.class).to(Y.class);

[philipbjorge]

@dlemures --
I would expect to be able to bind(X.class).to(Y.class); at any scope because Y is a singleton and accessible at any level in the scope chain. That being said, I can't really think of a valid usecase for this -- just pointing out that it is a little surpising.

I see how it will solve the @ScopeInstances issue and I think it will be a good direction for the API.

[dlemures]

So for the following example:

// Scope tree

       Scope A
       /    \
      /      \
Scope B       Scope C

• Z is a singleton
• Scope B contains the binding: X -> Z
• Scope C contains the binding: Y -> Z

All them inject the same instance:
@Inject Z (using Scope A) == @Inject X (using Scope B) == @Inject Y (using Scope C)

Is that what you mean?

@stephanenicolas I kind of agree with that behaviour... What are your thoughts?

[stephanenicolas]

I don't see how we can implement that :)

Moreover, I think it's just not a very good practice for a scope to define bindings at a upper level, the semantic is far from clear. So you wanna change the singleton for the current scope and child scopes or you would like something to be changed in the application scope ? It's just confusing to me.

Dagger components do that, but we use scopes to define bindings and thus from a scope changing a binding that is scoped in a different scope seems confusing to me.

And getting the same singleton for (z) in your example @dlemures seems quite impossible to me. I don't see how that could work in all cases. I am not even sure the semantic should mean this. I would really go against anything that adds complexity.

[stephanenicolas]

To dig a bit deeper, we just had a good talk with Daniel about this issue and how our fix should work.

Let's say we have an app scope, and an activity scope.
In the app scope we define

X --> Z, Z is @Singleton

Then, when we @Inject X, we get a singleton z0. When we @Inject Z, we get a different singleton z1.
If one wanted to have the same singleton, one would have to use the bindings:

z = new Z(..);
X --> z
Z --> z

That's the state of our current implementation, and we should not change it. It makes sense. A binding defines a "way to provide instances".

So, in the example of @dlemures , we can't afford the singleton to be the same between scopes. Not only the implementation would be quite tricky, but we don't even have this behavior inside a single scope itself for now ! And this seems quite far fetched.

If we would enable this, it would be a quite complex code change, just to accommodate this rare situation, which can already be achieved by simpler means (explicitly binding to the same instance).

And this rare use case would actually drive the feature that @philipbjorge was wondering about in the original post.

So, the easiest and most consistent change we can do is to:

• allow scope annotations to be used on targets of bindings
• but this scope annotation has to be supported by the scope that install the module containing this binding. This is the more consistent, and probably the simplest behavior.
• not allow the scope annotation to be supported by a parent, but only by the scope itself.

In short, the scopes of TP, when it comes to defining bindings, are not components a la dagger: they can only define things that will relate to the scope itself, not to a parent scope. This change is the simplest to enforce and it offers the simplest mental model for users I would say. It's just clearly coherent and there is no special rule to learn / remember.

We will start implementing it. We hope it's fine for you @philipbjorge . Thx again for raising this issue.
Btw, if you wanna become a contributor of TP, we can include you after a first PR getting merged in ;)

[stephanenicolas]

will be part of RC10