Closed bubenheimer closed 2 years ago
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 SuperficialValidation
that only checks the types that Dagger is interested in. This should also help reduce the number of types Dagger needs to resolve.
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 https://github.com/google/dagger/commit/745d30cf28d2934989b9c5dca43ded16fb2b5cc1
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 @Inject
types:
@Inject
members)@Inject
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 DaggerSuperficialValidation
) and see if you can find out what it's failing on for you?
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:
Start a build in debug mode.
The command depends on if you're using Java annotation processing or KAPT.
# For java
./gradlew --no-daemon --rerun-tasks -Dorg.gradle.debug=true app:compileDebugJava
# For KAPT
./gradlew --no-daemon --rerun-tasks -Dorg.gradle.debug=true -Dkotlin.compiler.execution.strategy="in-process" -
Dkotlin.daemon.jvm.options="-Xdebug,-Xrunjdwp:transport=dt_socket\,address=5005\,server=y\,suspend=y"
app:compileDebugKotlin
Configure the debugger from AndroidStudio Create new ‘Run Configuration’ (Run -> Edit Configuration -> Add New Configuration -> Remote)
It should be setup with the proper flags already, but here they are:
Attach the debugger Just click the bug icon
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, DaggerSuperficialValidation#validateElement()
. You should even be able to right click on a break point and configure it to break on any exception.
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:
@androidx.compose.runtime.internal.StabilityInferred(parameters=0)
when it reaches validateAnnotationValues
, valueEntry
is a mapping between parameters()
and 0
, and the call to get the return type returns <nulltype>
, which ultimately bubbles up and causes this issue.
I was able to fix it by adding:
implementation "androidx.compose.runtime:runtime:$composeVersion"
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 @StabilityInferred
? That seems to indicate that your are consuming a library that had the Compose compiler applied but didn't have an api
dependency in Compose's runtime.
@danysantiago yes, you are right - I didn't have the compose runtime
dependency in the library at all (I had forgotten to add it), and adding it as api
fixes the problem as you suggested without having to add runtime
to app
.
@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 .class
file from jd-gui:
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 @Inject
fields and a more complicated ComposeView
being generated there.
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 @Scope
before validating the annotation values. In fact, we should probably do the same thing for qualifiers to avoid validating non-qualifier annotations on fields and parameters.
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.
Update:
Found a few more cases where we can skip type resolution validation for types Dagger doesn't care about (https://github.com/google/dagger/commit/6c45b9d476a6e0495745a5108a470260d9eef727).
Added better error message when we get unexpected exception like in @bubenheimer's original issue (https://github.com/google/dagger/commit/0ec80470f94a57c3adbf7aa17360acf5dfff2a2a).
@bubenheimer would it be possible for you to try out the new changes using Dagger's HEAD-SNAPSHOT
version?
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!
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.
Hmm, this must be a slightly different issue than @ahmedre's.
@ahmedre's particular case should no longer be an issue using HEAD-SNAPSHOT
(maybe @ahmedre can confirm this). In particular, that case was due to an annotation, @StabilityInferred
, added by the compose compiler during its compilation but missing from the classpath in later compilations. In HEAD-SNAPSHOT
, it should no longer fail because Dagger should just skip trying to resolve those annotations when there's no @Inject
constructor in the class (since scoping doesn't apply in that case).
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.
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.
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.
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 @StabilityInferred
, so you shouldn't have to add the api
dependency for it. The problem is just that we can't know that @StabilityInferred
is not a scope annotation unless we can resolve it.
Hmm, one idea that may fix this is if we started propagating the scope information onto the generated _Factory
class.
For example, consider a class Foo
that is annotated with @StabilityInferred
and has an @Inject
constructor:
@Singleton
@StabilityInferred(parameters=...)
class Foo {
@Inject
Foo() {}
}
This class compiles fine in the library where it's defined -- it only breaks in later compilations when StabilityInferred
is no longer on the classpath. Thus, it should be possible to detect the scope when compiling the library and add it onto the generated Foo_Factory
class, like this:
@Singleton // <-- add the scope here so that we can find it later without looking on Foo
@Generated("by Dagger")
public class Foo_Factory {
...
}
Let me run this by the team and see if this is something we'd be willing to try.
Getting the same error with 2.40.5 when adding compose dependencies. As @bcorso mentioned, using api
instead of implementation
for defining the compose dependencies in the app module's build.gradle, solves the issue. But, it definitely needs a proper fix.
FWIW, regarding StabilityInferred
, I filled an issue to the Compose team to make them aware of this issue: https://issuetracker.google.com/209688774
@bcorso fyi we ran into the same issue with compose and using HEAD-SNAPSHOT
solved it for us
@marianeum thanks for the update!
@bubenheimer FYI, we've gone ahead with the idea proposed in https://github.com/google/dagger/issues/3090#issuecomment-996088356 so hopefully that solves your issue as well (might be worth verifying again using the HEAD-SNAPSHOT
).
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: https://github.com/PaulWoitaschek/Voice/pull/1099
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: