Dagger: IllegalArgumentException via checkArgument in `d.i.c.writing.InjectionMethod` could benefit from a message.

[trevjonez]

I just started working in a new project and am hitting a possible blocker for teams trying to update from older versions of dagger. I went from 2.9 to 2.25.2 and got this:

e: [kapt] An exception occurred: java.lang.IllegalArgumentException
    at com.google.common.base.Preconditions.checkArgument(Preconditions.java:128)
    at dagger.internal.codegen.writing.InjectionMethod.invoke(InjectionMethod.java:98)
    at dagger.internal.codegen.writing.InjectionMethods$InjectionSiteMethod.invoke(InjectionMethods.java:347)
    at dagger.internal.codegen.writing.InjectionMethods$InjectionSiteMethod.lambda$invokeAll$0(InjectionMethods.java:315)

I set a breakpoint at the throw in the checkArgument and was able to back track to the source of the issue. What i found was something like this:

object SomeObject {
  lateinit var statefulThing: Foo
    @Inject set //params based on `setStatefulThing(SomeObject, Foo)` but expects `setStatefulThing(Foo)`
}

@Component(modules = [FooModule::class])
interface SomeObjComponent {
  fun inject(someObj: SomeObject)
}

@Module
class FooModule(val bar: String) {
  @Provides
  fun theFoo(): Foo = Foo(bar)
}

class Foo(val bar: String)

DaggerSomeObjComponent.create(FooModule("barValue"))
  .inject(SomeObject)

once I figured out where the issue was , I updated the injected property to:

object SomeObject {
  @Inject
  lateinit var statefulThing: Foo
}

Which results in the expected: injection into static fields is not supported

Yes the stateful nature of the property is less than ideal and the best solution is probably a scoped instance rather than language singleton but at the least a more helpful info dump should ease the pain of the up and coming.

When debugging all the necessary info is in scope, so this _should_™ be a fairly straight forward addition once message formatting was decided.

[0neel]

Another workaround worked for me is to rollback to 2.24.

[trevjonez]

Another better option would likely be to add an additional check to InjectValidator.java similar to the existing:

//from validateMembersInjectionType(TypeElement)
if (hasInjectedMembers) {
  checkInjectIntoPrivateClass(typeElement, builder);
  checkInjectIntoKotlinObject(typeElement, builder);
}

something like:

private void checkInjectIntoKotlinObject(
      TypeElement element, ValidationReport.Builder<TypeElement> builder) {
    if (kotlinMetadataUtil.isObjectClass(element)) {
      builder.addError(
          "Dagger does not support injection into kotlin object's",
          element);
    }
}

[thegeekyasian]

Another better option would likely be to add an additional check to InjectValidator.java similar to the existing:

//from validateMembersInjectionType(TypeElement)
if (hasInjectedMembers) {
  checkInjectIntoPrivateClass(typeElement, builder);
  checkInjectIntoKotlinObject(typeElement, builder);
}

something like:

private void checkInjectIntoKotlinObject(
      TypeElement element, ValidationReport.Builder<TypeElement> builder) {
    if (kotlinMetadataUtil.isObjectClass(element)) {
      builder.addError(
          "Dagger does not support injection into kotlin object's",
          element);
    }
}

Should I add this into the InjectValidtor.java?

[trevjonez]

I'd be happy to make the changes I was just hoping for some direction from the project maintaintainers before doing so.

[danysantiago]

I'm on board with disallowing injection of object classes and companion objects, encouraging a scoped binding and if that doesn't work, as a migration path one can still have a provision method in the component to set the object's property. @trevjonez if you make a pull request I'll gladly pick it up and merge it internally.