Checking the null safety of dagger statically about dagger HOT 8 CLOSED

That's pretty awesome, Mohsen. Thanks.

At this point, I definitely wish we had a truly standardized @Nullable annotation. We could use the one in findbugs (with an optional maven dep, so no one would be "getting the transitive dep for free), but it's frustrating that there isn't a more centrally located standard one.

@swankjesse - how do you feel about actually using @Nullable (build-time dep only)?

from dagger.

@cgruber, @swankjesse

Do you find any of the nullness warnings marked in my change suspicious or they are always safe?

from dagger.

I was surprised that the requiredBy field needed @Nullable. It's too bad the nullness assertions are necessary. The @OneOf annotation implies that providesKey is non-null, but expressing this requires some ceremony with this framework. Similarly for the uses of LruCache.

from dagger.

Field dagger.internal.Binding.requiredBy is @Nullable because it's set to null at the following locations:

• https://github.com/reprogrammer/dagger/blob/master/core/src/main/java/dagger/internal/Binding.java#L26
• https://github.com/reprogrammer/dagger/blob/master/core/src/main/java/dagger/internal/SetBinding.java#L43

Similarly, field dagger.internal.Binding.provideKey is @Nullable because it's set to null at the following locations:

• https://github.com/reprogrammer/dagger/blob/master/core/src/main/java/dagger/internal/Binding.java#L26
• https://github.com/reprogrammer/dagger/blob/master/core/src/main/java/dagger/internal/Linker.java#L312

It looks to me that we can get rid of the above occurrences of the null value by introducing new classes. For instance, SetBinding sets provideKey and requiredBy to null. This may imply that Binding is not the right place for fields provideKey and requiredBy. Instead, these fields should belong to the subclasses that can set the fields to non-null values. I think this change would improve the design and make it easy to check the null-safety of the program at compile-time.

What do you think about such a change? Can you think of other changes to remove the null values?

from dagger.

On 6 Nov 2012, at 15:49, Mohsen Vakilian wrote:

It looks to me that we can get rid of the above occurrences of the
null value by introducing new classes. For instance, SetBinding
sets provideKey and requiredBy to null. This may imply that
Binding is not the right place for fields provideKey and
requiredBy. Instead, these fields should belong to the subclasses
that can set the fields to non-null values. I think this change would
improve the design and make it easy to check the null-safety of the
program at compile-time.

What do you think about such a change? Can you think of other changes
to remove the null values?

We talked about this earlier, and we've made some design decisions that
were intended to reduce object instances in order to keep Android zippy.
Having the same binding do duty for provideKey vs. requiredBy was one
such decision, though it's entirely possible that we can simply use the
same key for both, but have the binding in question say whether it's
supporting member injection or provision as a boolean (or bitfield
digit). I was thinking about that earlier, but haven't worked up a
design. It touches a few areas, and we had a few other things ahead on
my plate. I agree, though, these do make me cringe a bit.

c.

from dagger.

I'd like to close this since it isn't a very actionable issue. The only outstanding question is whether or not we want to adopt find bugs' @Nullable as a 'provided'. If not there's no action to be taken. If so, we can create a focused issue along the lines of "Introduce FindBugs Annotations".

from dagger.

SGTM

from dagger.

If you are considering the adoption of FindBugs analysis or annotations for nullness analysis, beware of its following two shortcomings:

• FindBugs may not find all nullness bugs.
• The semantics of FindBugs annotations can be misleading.

from dagger.