Permit use of customs scope annotations (with validation)

[cgruber]

Greg kick had a suggestion that I've been mulling around as to how to keep people from inappropriately declaring scoping annotations in ways that they then contradict in the graph, using plus/extension-graphs. Here's my (adapted) version of his suggestion.

The problem

Say you have some app which has some shorter-lifetime requesty-thing.

/** the current user */
@Singleton // because this needs to be memoized in the request-level graph
class User {
  @Inject User(SomeAppContext ctx) { ... }
}

@Module(library = true)
class AppModule {
  @Provides @Singleton SomeAppContext() { ... }
}

@Module(injects = MyPage.class, addsTo = AppModule.class)
class RequestModule { 
  @Provides MyPage providePage(User u) { ... }
}

but then uses it...

ObjectGraph og = ObjectGraph.create(AppModule.class);
ObjectGraph rg = og.plus(RequestModule.class);
User u = rg.get(User.class);

User is scoped to Singleton, and this is confusing to the user. Also, since SomeAppContext is also singleton, but in a different graph layer, the annotations are technically correct according to the spec (one-per-graph) but in-obvious to the colloquial use of the term Singleton, and the typical use in Guice.

Worse, this could be a mistake - an error incrementally introduced but uncaught with subtle effects until later. In a very complex app with hundreds of bound objects or more, this could be quite a buried little time-bomb.

Additionally, migration from Guice, or use of libraries which annotate could conflict because the semantics of @Singleton are different between the cases. A Guice user would want to mark User as @RequestScoped and adding multiple scoping annotations is not permitted.

Dagger won't support orthogonal scoping (multiple scopes in the same container) as we have chosen child/extension graphs as the means to do scoping. But we are implementing a standard (JSR-330) that was developed in the context of guice-style scopes. So let's play to the documentation strengths of such.

The proposal

Support the presence of guice-style scopes, but still require that things be in the right modules in the right graph level. Any @Scope annotation will work, but the scope annotation serves as a constraint on the modules as additional validation for that layer of the graph. There are two ways to do it.

Run time

Say you did this:

og = ObjectGraph.create(Singleton.class, modules...);
rg = og.plus(RequestScoped.class, modules...);

If (jit or explicit) bindings were attempted by classes which resulted in an object being @Singleton but bound in the (constrained) extension graph, then it would blow up, because such a singleton is mis-labeled.

Drawback

This variant would not detect these wiring/scoping errors until runtime, and is less "daggery" therefore.

Compile time

A variant that I prefer would do this at compile-time, by adding it to the module definition

@Module(scopes={RequestScoped.class})
class RequestModule {
  @Provides @RequestScoped Foo provideFoo() { ... }
}

This could provide compile-time validation. An un-scoped module included by a scope-constrained module would (because it is an includes) be validated within the context of that scope annotation. If one had a general purpose module, one would not constrain it. But for something that is absolutely clear that it shoudl only be request-scoped, do it.

eg.

/** the current user */
@RequestScoped
class User {
  @Inject User(SomeAppContext ctx) { ... }
}

@Module(library = true, scopes = Singleton.class)
class AppModule {
  @Provides @Singleton SomeAppContext() { ... }
}

@Module(
    injects = MyPage.class,
    addsTo = AppModule.class,
    scopes = RequestScoped.class)
class RequestModule { 
  @Provides MyPage providePage(User u) { ... }
}

A few important points

• If User were marked as Singleton, this would be a graph error at compile-time.
• If User were marked as RequestScoped and then JIT depended on by something in the @Singleton constrained graph, then it would be a graph error at compile-time.

  Drawback

This approach is slightly too strict. For instance: A @Singleton-annotated class depended on by only something in a @RequestScoped graph AND which was JIT bound only by that dependency (i.e. was not otherwise depended on by other singletons) would be a compile error unless it was declared as an "injects=" or in a @Provides method on the @Singleton-constrained module. Dagger needs some way to tell it that you expect a thing to be bound in a particular graph, and leaving it to JIT can result in a false-negative. The good news is that it's a false-failure (slightly too strict) which simply requires one teeny piece of spec (injects=MySingleton.class) to fix, so the fix is self-documenting, and it isn't a false-success which would go undetected.

Conclusion

The purpose of this is to allow people to use scope-specific annotations as they did in Guice, but with dagger-specific semantics preserved. The validation would force the correct use of the scoping annotations, and catch mis-allocation.

[cgruber]

For the record, I'm proposing this to the square/dagger project. we have complex-enough apps at google that I believe this would be valueable and we may take it on board in our fork if it's not seen as enough of a win in the main project. I'd be cool with that, but I think it really is useful in the main project, so I posted it here.

[swankjesse]

Let me explain it back to you to confirm that the idea transmitted successfully.

1. In Dagger @Singleton means one-instance-per-graph, which contradicts the conventional definition of one-instance-per-application.
2. It's helpful to name the extending graphs after the additional state that they carry. The User extending graph holds one user constant. The Request extending graph holds one request constant.
3. Replace the confusing word 'singleton' with the graph's name. It's not Preferences, it's @ForUser Preferences. It's not HttpHeaders it's @ForRequest HttpHeaders.

Is that the proposal?

[cgruber]

Also, adding @gk5885 to this since it was in discussion with him that this evolved (and now I remember his username)

[cgruber]

[reposting this because mail -> github SUCKS for formatting]

I think 2 might be mis-stated slightly, but is an ok proxy for the idea. I would name things after their lifetime/phase/whatever, but in practice I think it's the same thing.

I hadn't characterized it in terms of named graphs, but that's the essence, and then scoping annotations that are appropriate to the named graph.

In practice, client code would look a lot like Guice's use, but the underlying mechanism would simply use those signals for validation of user intent.

But… (3) is off. It's not HttpHeaders -> @ForRequest HttpHeaders. It's @Singleton HttpHeaders -> @ForRequest HttpHeaders. If you don't mark something with a scoping annotation, it will remain default scope.

And since it doesn't change the semantics, but merely provides validation/self-documentation, it should be optional. An un-named graph would behave exactly as it does today.

[gk5885]

It actually might be good to enforce that @Singleton can only be applied to the top-level injector since that seems like a point of confusion. The basic idea is that all scopes are implemented as 1-per-injector and the user has the capability to say that an injector is associated with a given scope. So, the top-level Injector is associated with the @Singleton scope. I can create a child injector associated with the request scope (and there should be infrastructure that does this correctly for, say, servlets). It's still implemented as 1-per-injector, but the injector is the child in this case.

[cgruber]

Hey @gk5885. So that makes sense, though optionally allowing someone to rename their top-level scope could be good. I've seen people prefer @PerVM (with a static injector), @PerApplication, @PerRequest, etc. YMMV. But by default, enforcing @Singleton at the top may prove helpful. That seems to match most people's colloquial use of the term, and Guice usage in practice.

[ghost]

Is this more complicated than necessary? Seems to me we could cover all of the same ground by giving @Singleton an optional scope parameter, something like: @Singleton(scope = ActivityModule.class). If I try to instantiate one against a graph that doesn't derive from that module, explode.

Or maybe it's a new annotation called @Scope, and @Singleton is just a shorthand for @Scope(TheRootModule.class)

(I'm assuming here that the @Scope or @Singleton annotation appears on a class definition. I'm always happiest when I can write a new class and not have to do parallel, invisible work in magical module class to make it actually work.)

[cgruber]

@RayFromSquare - the problem is that @Singleton and @Scope are JSR-330 types, and we can't just go changing them.

In practice, it's not going to be much more complex. From the dagger end-user's perspective, they can just ignore this, or they can use it. If they use it, it is merely a matter of passing in scoping annotations where you want to get the validation.

[ghost]

@cgruber My objection is about over-complexity, but your counter-argument is about naming. Something like, say, @PerModule(ActivityModule.class) still sounds simpler to me than the proposal above, and having Dagger interpret @Singleton as shorthand for @PerModule(MyRootActivity.class) shouldn't conflict with the JSR.

[ghost]

I'm thinking that GitHub's Comment and Close buttons should be just a little bit closer together. There's an entire millimeter of space there.

[JakeWharton]

If you had a 13" MacBook there would be two millimeters!

[cgruber]

My "counterargument" comes in three levels, or "scopes" if you will. ;)

My counterargument is about naming because the whole feature is about naming. The current problem IS that the name Singleton is misleading, and risks people misusing scopes through misconfiguration.

But, my counterargument is also about complexity, insofar as I disagree with your subjective assessment as to the comparative complexity of the solution, from an API perspective. I agree it is more complex than the current implementation, but I believe you get more value for that complexity than you seem to think. I don't think that @PerModule(MyModule.class) or @Scope(FooModule.class) is any less complex, it just shuffles the problem in a different way.

We can (and if I didn't say it, let me correct) treat the default as @Singleton being synonymous with your top level graph, but that's doable in my proposal as well. In fact, I would have suggested it as the default behaviour in my own proposal, except that I wanted to retain current behaviour if you didn't opt-in for backward compatibility.

Lastly, my counterargument is that the solution you propose cannot work the way you suggest, without assuming things, in advance, about the module set. Namely, @Singleton can't stand in for @PerModule(MyModule.class) without relying on the implicit knowledge that MyModule is the module used for the top level graph that will materialize that class. Even if we're ok with THAT, you would have to mark classes as @PerModule(SomeModule.class) in advance of knowing which module(s) will reach this class. It ties the class to the module, when that class could be a reusable library class reachable by many valid modules.

Scope hints on classes should be logical scopes, which are not tied to modules from the point of view of the so-decorated class. The module itself is what links a particular logical scope to a particular hierarchy of graphs.

[ghost]

@cgruber I'm wanting scopes badly enough to start working on them. What do you think about the tests @swankjesse and I whipped up this morning? I think they're in line with your thinking.

https://github.com/square/dagger/pull/309

All modules have a scope annotation. By default it is @Singleton. A provider method whose scope doesn't match the module is an error. Thus, if I am an @Request module it is an error for me to define an @Singleton provider method.

A class can provide a scope anntoation. If no module has that annotation, injecting such a class is a static error.

[ghost]

Unaddressed here is how to deal with unscoped (@Singleton) modules on a .plus() line. IIRC @swankjesse's thought was that we should be permissive in such cases, e.g. to allow an app to stitch third party modules into a narrower scope. So an @Singleton provider method in a module that is .plus'd into a child graph would behave as it does today.

I'm unclear, though, on the implications this has for classes that are annotated with @Singleton. I really dislike the current behavior, where they land in the narrowest graph. I want to be able to structure an app s.t. @Singleton always means the root graph, unambiguously. If that can be broken by a single module forgetting to declare a scope, life's not much improved.

[cgruber]

I'm OOO today but I'm actually on this and have something I hope to push this weekend. I can take a look tomorrow when I'm back.

[swankjesse]

@cgruber what do you have?

[cgruber]

And I agree, the narrowest graph is problematic. We've run into that which is part of why I'm working on a change.

[cgruber]

Similar to what your p/r API suggests. I have a partly working initial cut which I'm working trough the validation stuff for.

[gk5885]

I'm with @RayFromSquare on this one. Allowing @Singleton to mean anything besides one-per-root-graph has already been a source of confusion for new adopters. Plus, it's especially unintuitive for anybody migrating from Guice. I think I'd prefer a release with the existing behavior and warnings logged about how this is soon going to have different behavior (git does similar things and I like the approach). Then we can change it without too much worry.

(As an aside, I wonder who singleton is and whether he/she gets sick of random Java projects mentioning him/her all the time… :)

[cgruber]

Agreed on the subject of Singleton. I think that one gets reserved for root.

Just to clarify a titch more, the way I went is, you can mark a module as X.class (where X is decorated with @Scope etc.) but you cannot have modules composed at the same graph layer with multiple scopes (it'll crap out at compile time). Similarly, an item that is scoped that is explicitly arrived at from a differently scoped set of modules, breaks at compile time.

I'm struggling with JIT, only becasue I want to let it JIT at the right scope generation, but I don't want a stupid performance drain finding that generation, nor a horrid mess of signals internally to optimize on finding it.

My current behaviour is that _entirely_ unscoped generations (that is, no modules in that graph have any scope constraint) will back-ward compatibly let you use @Singleton out of the normal root-only case as a legacy support behaviour.

But unscoped modules that are included into a module with a scope, or vice versa, will make that graph generation scoped and obviously will cause a compile time error if those scopes are inconsistent, including adding @Singleton scoping where a generation was alternatively scoped.

We should break this permissiveness in 2.0. Hard. But for now, we forced this on users, and we shouldn't break them with an upgrade, since it'll take some time for people to un-stich and unwind @Singleton from their graphs in its current semantic of "memoize this."

[swankjesse]

+1 on your rules. Or something stricter, like dropping backwards compatibility for the entirely unscoped extensions.