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

IllegalArgumentException via checkArgument in d.i.c.writing.InjectionMethod could benefit from a message. #1665

Closed
trevjonez opened this issue Nov 6, 2019 · 5 comments · Fixed by #1744

Comments

@trevjonez
Copy link

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
Copy link

0neel commented Jan 29, 2020

Another workaround worked for me is to rollback to 2.24.

@trevjonez
Copy link
Author

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
Copy link

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
Copy link
Author

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

@danysantiago
Copy link
Member

danysantiago commented Feb 11, 2020

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.

trevjonez added a commit to trevjonez/dagger that referenced this issue Feb 11, 2020
kluever pushed a commit that referenced this issue Feb 17, 2020
Fixes #1665
Closes #1739

RELNOTES=Disallow member injection of Kotlin object classes.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=294805598
kluever pushed a commit that referenced this issue Feb 17, 2020
Fixes #1665
Closes #1739

RELNOTES=Disallow member injection of Kotlin object classes.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=294805598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants