-
Notifications
You must be signed in to change notification settings - Fork 2k
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
InvocationTargetException with 2.40.4 #3090
Comments
Thanks for the report! Looks like the superficial validation that was put in to fix #3075 is over validating and failing on something we weren't validating (and apparently didn't need to validate) before. In order to fix this, I think we'll need to define our own custom I'll look into getting a fix for this today, and sorry for all of the recent issues. |
@bcorso https://github.com/google/dagger/releases/tag/dagger-2.40.5 is released, but this issue is still open. |
@TWiStErRob thanks. Closing now. Fixed by 745d30c |
2.40.5 did not fix this issue |
Hmm, I think we've reduced the scope of the validation about as far as we can at this point. In particular, we should only be validating exactly what we need for
It would be nice to know exactly what it's failing on in your case. If you don't have a repro project, is there anyway you can attach a debugger (e.g. here in I can help you get the debugger set up if you need. |
Thanks for reopening. There is a lot of Dagger stuff and many module & Dagger dependencies in the area with the build error, so I don't really know where to start. And I don't think I have ever debugged a build. I see that there were a few immediate reactions on this issue. Perhaps one of the other folks affected might be able to come up with a repro or point at a public project, if they still see the problem? @TWiStErRob @ahmedre @ronathan @bitvale |
If you're using AndroidStudio and gradle it's not so bad to debug. If you (or someone else) is interested:
Then it should break at any break points you've setup in your sources (using AndroidStudio). You can start by setting breakpoints at points within your stack trace where it's expected to fail. In the latest release, the error should go through our custom validation, |
debugging helped me figure out how to get it working with 2.40.5 -- for me, the annotation validation on 2.40.5 that was dying was in this annotation:
when it reaches I was able to fix it by adding:
to the app/build.gradle file (despite the fact that app doesn't directly use Compose at all). when I do this, the same invocation correctly gets return type of int and the build passes. |
@ahmedre, by any chance do you know which library contained the class annotated with |
@danysantiago yes, you are right - I didn't have the compose |
@ahmedre thanks for debugging and sharing! It sounds like at the very least we should try to surface the information to the user with an error that actually contains information about what it's failing on without having to use a debugger, as it seems this information was enough to help you resolve the issue in your build setup. This shouldn't be too difficult, so I'll try to get something out soon. However, I'd still like to find out a bit more about where this annotation is located (e.g. which element, which class) to ensure we're not over validating something we're not suppose to. |
Sounds like the issue might be easily resolvable, but the exception is not helping. Would it be possible to surface where a null type is coming from? (Since it's a synthetic Type anyway I'd imagine it could contain the member and containing class at the minimum too.) |
@bcorso not sure if this will help, but here's a simplified version of the class that was causing the issue: package com.quran.labs.androidquran.extra.feature.linebyline
import android.content.Context
import android.widget.FrameLayout
import androidx.compose.material.MaterialTheme
import androidx.compose.material.Text
import androidx.compose.ui.platform.ComposeView
import androidx.compose.ui.platform.ViewCompositionStrategy
import com.quran.data.page.provider.di.inject
class QuranLineByLineWrapperView(
context: Context,
currentPage: Int,
private val dualScreenMode: Boolean = false
) : FrameLayout(context) {
init {
inject(currentPage)
addView(generateComposeView())
}
private fun generateComposeView(): ComposeView {
return ComposeView(context).apply {
setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)
setContent {
MaterialTheme {
Text("hello")
}
}
}
}
} here's the view of the happy to give more info if you can point me at what else you'd need. the real class (without modifications) just has some actual |
Ah, thanks @ahmedre! Looks like in this case you hit the issue because we're validating annotations on the types (and super types) because we need to check for scope annotations. I think there's a way we can fix this case by checking if the annotation is annotated with Hopefully that will help reduce the chances of running into this issue even more. I'll also still go ahead and add a better error message for cases that fail in the future. |
This CL removes superficial validation in 3 special cases: 1. If a members injected type has no `@Inject` constructor, we don't need to do superficial validation on the type's annotations because Dagger does not create the type, so we ignore scopes even if they're present. 2. For each super class, we don't need to do superficial validation on the type's annotations because Dagger only relies on the scopes of the subtype. 3. For each super class, we don't need to do superficial validation on the constructor because Dagger only relies on the constructor of the subtype. This will hopefully help cases like #3090, where the current superficial validation can fail on non-Dagger related types. RELNOTES=N/A PiperOrigin-RevId: 416188247
This CL removes superficial validation in 3 special cases: 1. If a members injected type has no `@Inject` constructor, we don't need to do superficial validation on the type's annotations because Dagger does not create the type, so we ignore scopes even if they're present. 2. For each super class, we don't need to do superficial validation on the type's annotations because Dagger only relies on the scopes of the subtype. 3. For each super class, we don't need to do superficial validation on the constructor because Dagger only relies on the constructor of the subtype. This will hopefully help cases like #3090, where the current superficial validation can fail on non-Dagger related types. RELNOTES=N/A PiperOrigin-RevId: 416363991
Update:
@bubenheimer would it be possible for you to try out the new changes using Dagger's I'm hoping that either the issue is now gone due to #1, or you get a better error message due to #2 which should help us further debug the issue. |
@bcorso and @ahmedre thank you for the detective work and code changes! I've tested out HEAD-SNAPSHOT and the original issues I saw are no longer present. It's possible, though, that they disappeared due to other code changes in my own project. I've been able to compile all my module dependencies now, but the main app module that pulls in everything else is failing with what looks to be the same issue that @ahmedre tracked down. If that was intended to have been fixed or worked around it does not seem to be ready yet. I was able to build successfully with the same strategies that @ahmedre suggested. (Personally I prefer to have the main app build.gradle explicitly specify needed dependencies from modules in the same project, so that I still have some idea what dependencies each module actually needs.) Without being familiar with this specific GitHub issue, though, it still takes some guesswork to conclude that a missing compose dependency is to blame for a Dagger-related build failure. Edit: At least one of my prior issues was indeed fixed in HEAD-SNAPSHOT, thanks again! |
Hmm, this must be a slightly different issue than @ahmedre's. @ahmedre's particular case should no longer be an issue using
Yeah, unfortunately, we only give a better error message when there's an unexpected exception, but I'm working on getting better error messages in the more general case. It'll require some changes to one of our dependencies (XProcessing), but hopefully sometime January we should have something out. |
Thanks @bcorso. @ahmedre mentions @Inject fields in his actual class. In my case there is an @Inject constructor; it's an empty constructor, though; don't know if that can also become one of the lenient cases. Admittedly, having an empty @Inject constructor is questionable. Edit: FYI: after eliminating the empty @Inject constructor I ran into the same kind of problem with a non-empty @Inject constructor in another class, so I guess I really need to add the dependency in my case. |
Ah, yeah in this case we need to be able to resolve all of the type's annotations to know if any of them are scopes. This is true even if the constructor is empty because in either case Dagger is responsible for creating the type so it needs to know whether to scope it. This is still unfortunate though since Dagger doesn't really care about Hmm, one idea that may fix this is if we started propagating the scope information onto the generated For example, consider a class
This class compiles fine in the library where it's defined -- it only breaks in later compilations when
Let me run this by the team and see if this is something we'd be willing to try. |
This CL adds a repro test for the issues in #3090 when a transitive annotation is no longer on the classpath. Follow-up CLs will fix the remaining issues. RELNOTES=N/A PiperOrigin-RevId: 419130258
This CL adds a repro test for the issues in #3090 when a transitive annotation is no longer on the classpath. Follow-up CLs will fix the remaining issues. RELNOTES=N/A PiperOrigin-RevId: 419677447
Getting the same error with 2.40.5 when adding compose dependencies. As @bcorso mentioned, using |
FWIW, regarding |
@bcorso fyi we ran into the same issue with compose and using |
@marianeum thanks for the update! @bubenheimer FYI, we've gone ahead with the idea proposed in #3090 (comment) so hopefully that solves your issue as well (might be worth verifying again using the We're close to finishing up #2208 (which should make investigating these issues much easier when things fail), so I want to get that fix in before we do another official release but hopefully that will be soon (next week or two). |
Here is a reproducer i case you are still trying to debug this issue: |
I cannot reproduce this anymore in 2.41 on my project |
I could confirm that updating from 2.40.1 to 2.41 fixed that issue for me as well. |
2.40.4 has introduced another regression, my build now fails again, it worked (for the first time since 2.39.1) with 2.40.3. I see what seems to be the same kind of failure in at least 2 different modules in my closed-source Android app. I don't have a public repro, so I hope the stacktraces combined with knowledge about changes in 2.40.4 will suffice. I'm on JDK 11, Kotlin 1.6.0, Gradle 7.3.1, AGP 7.0.3:
The text was updated successfully, but these errors were encountered: