From 7147ff36f45b4a4eef07515abc5fe6fd17f95a51 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 8 Aug 2026 14:53:06 -0700 Subject: [PATCH 1/2] ADFA-5068: Fix javac-services dependency scope without breaking AGP's consistency check javac-services declared kotlin-stdlib, guava, :common, and :logger as implementation even though all are resident, leaking androidx.core's 2.87MB of dead resources and duplicate guava/kotlin-stdlib bytecode into java-compiler-carrier.apk. Switching those to compileOnly broke AGP's compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath) check one level up in java-compiler-impl: removing :common's implicit implementation edge meant nothing but AGP's own low-pinned viewbinding was left providing androidx.annotation/kotlin-stdlib/org.jetbrains: annotations on the runtime classpath, while compileOnly deps (sora- editor, appcompat, material, lsp:indexing) still demanded much higher versions on the compile side. Relocated ReflectUtils (the one :common symbol javac-services actually uses; VMUtils and ILogger already lived in :shared/:logger) there, and added a constraints{} block in java-compiler-impl pinning the three conflicting artifacts to the versions already used everywhere else in the project -- harmonizing both classpaths without re-adding a real dependency edge. guava was dropped outright: javac-services' own code never references it directly. Verified via a from-scratch java-compiler-carrier release build: 32.4MB -> 10.2MB, resources.arsc 2.27MB -> 40 bytes (empty), dex 9.1+8.1+10.5+2.6MB across 5 files -> 8.4+2.1MB across 2. --- .../0012-lazy-load-javac-via-dexclassloader.md | 1 + gradle/libs.versions.toml | 2 ++ lsp/java-compiler-impl/build.gradle.kts | 17 +++++++++++++++++ .../com/itsaky/androidide/utils/ReflectUtils.kt | 0 .../itsaky/androidide/utils/ReflectUtilsTest.kt | 0 subprojects/javac-services/build.gradle.kts | 13 +++++++++---- 6 files changed, 29 insertions(+), 4 deletions(-) rename {common => shared}/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt (100%) rename {common => shared}/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt (100%) diff --git a/docs/adr/0012-lazy-load-javac-via-dexclassloader.md b/docs/adr/0012-lazy-load-javac-via-dexclassloader.md index d34bda2096..52319972a6 100644 --- a/docs/adr/0012-lazy-load-javac-via-dexclassloader.md +++ b/docs/adr/0012-lazy-load-javac-via-dexclassloader.md @@ -52,6 +52,7 @@ Investigation found this coupling narrower than it first looked: none of `CacheF - First `.java`-file interaction in a session now pays a one-time synchronous latency spike (asset extraction on first run + `DexClassLoader` construction + `JavaCompilerService`/`SourceFileManager` bootstrap) on top of ADFA-5052's own deferred-reset cost. - A third resident/isolated classloader boundary to reason about (after Kotlin's and the plugin system's). The same rule as ADR 0011 applies and now has two worked examples of getting it wrong: an `api` dependency anywhere in a vendored composite build's *own* `build.gradle.kts` propagates to every consumer's runtime classpath regardless of how the consumer declares its dependency — `compileOnly` has to be applied at the source of the leak, not just where it's consumed. A second, distinct rule this ADR adds: every resident member the isolated fork calls across the boundary must be `public` — `protected`/package-private access throws `IllegalAccessError` at runtime even when both classes share a package name, since ART checks classloader identity, not just the package string, and this has no build-time or unit-test signal at all. - The debugger's breakpoint/stack-frame source-path resolution (`JavaDebugAdapter`/`ModelUtils.asLspLocation`) took on a narrow, real dependency on `JavaCompilerProvider`/`SourceFileObject` that the isolation boundary can't ignore; it now resolves through `IJavaCompilerSession.findSourceFilePath` (returning a plain path, not the isolated `SourceFileObject` type) instead, and degrades to filename-only when no session exists yet. +- A third rule (ADFA-5068): converting a resident dependency from `implementation` to `compileOnly` can trip AGP's `compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)` check for any `com.android.library` module, if that dependency was incidentally anchoring a high-enough transitive version (of e.g. `androidx.annotation`, `kotlin-stdlib`, `org.jetbrains:annotations`) against AGP's own unconditionally-injected `androidx.databinding:viewbinding`, which pins those same artifacts much lower on the runtime side once nothing else pulls them in. The fix isn't to re-add the dependency (that reintroduces the duplication this rule exists to avoid) but a `constraints {}` block on `implementation` bumping just the conflicting artifact(s) to the version already used everywhere else — a constraint, unlike a dependency, only rescopes an edge that's already reachable (here, via viewbinding), so it doesn't add anything new to the carrier beyond a version bump on a few KB of annotation classes. See `lsp/java-compiler-impl/build.gradle.kts`'s `constraints` block and `subprojects/javac-services/build.gradle.kts`. ## Alternatives considered diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 897fb631c7..0c69068adb 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -173,6 +173,8 @@ common-javaparser = { module = "com.github.javaparser:javaparser-symbol-solver-c common-lang3 = { module = "org.apache.commons:commons-lang3", version = "3.14.0" } common-io = { module = "commons-io:commons-io", version = "2.15.1" } common-kotlin = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" } +common-kotlin-stdlib = { module = "org.jetbrains.kotlin:kotlin-stdlib", version.ref = "kotlin" } +common-jetbrains-annotations = { module = "org.jetbrains:annotations", version = "24.1.0" } common-kotlin-coroutines-core = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "kotlin-coroutines" } common-kotlin-coroutines-android = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-android", version.ref = "kotlin-coroutines" } common-jkotlin = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" } diff --git a/lsp/java-compiler-impl/build.gradle.kts b/lsp/java-compiler-impl/build.gradle.kts index 77f7c52f06..edc359a22f 100644 --- a/lsp/java-compiler-impl/build.gradle.kts +++ b/lsp/java-compiler-impl/build.gradle.kts @@ -40,6 +40,23 @@ kapt { } dependencies { + // javac-services' (and this module's own) compileOnly androidx-heavy deps (sora-editor, + // appcompat, material, lsp:indexing, etc.) pull androidx.annotation/kotlin-stdlib/ + // org.jetbrains:annotations at high versions on the compile classpath, but being compileOnly + // they're absent from the runtime classpath -- which then only sees AGP's unconditionally + // injected viewbinding's much lower transitive pins for the same artifacts. AGP's + // compile/runtime consistency check fails to reconcile the two. Constraining these three (not + // adding a dependency: each is already reachable via viewbinding, just at the wrong version) + // harmonizes both classpaths at the version already used everywhere else in the project, + // without adding a new edge. All three are a few KB of interfaces/annotations with no + // resources, unlike the androidx.core duplication this whole compileOnly effort exists to + // avoid -- see ADFA-5068. + constraints { + implementation(libs.androidx.annotation) + implementation(libs.common.kotlin.stdlib) + implementation(libs.common.jetbrains.annotations) + } + kapt(projects.annotationProcessors) // Resident (bundled in the main app dex via editor/editor-api/lsp:java/etc.) -- like the diff --git a/common/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt b/shared/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt similarity index 100% rename from common/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt rename to shared/src/main/java/com/itsaky/androidide/utils/ReflectUtils.kt diff --git a/common/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt b/shared/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt similarity index 100% rename from common/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt rename to shared/src/test/java/com/itsaky/androidide/utils/ReflectUtilsTest.kt diff --git a/subprojects/javac-services/build.gradle.kts b/subprojects/javac-services/build.gradle.kts index ab698259d4..df965cbc15 100644 --- a/subprojects/javac-services/build.gradle.kts +++ b/subprojects/javac-services/build.gradle.kts @@ -16,10 +16,15 @@ android { } dependencies { - implementation(libs.common.kotlin) - implementation(libs.google.guava) - implementation(projects.common) - implementation(projects.logger) + // Resident (see docs/adr/0012): kotlin-stdlib, :shared (ReflectUtils, VMUtils) and :logger + // (ILogger) are all already loaded by the parent classloader -- implementation here would + // duplicate their bytecode (and :common's androidx/guava graph, previously reached via + // projects.common) into the isolated carrier dex. compileOnly for the same reason as the + // block below. libs.google.guava was dropped entirely: this module's own code never + // references it directly -- it only ever arrived transitively through :common's api(guava). + compileOnly(libs.common.kotlin) + compileOnly(projects.shared) + compileOnly(projects.logger) // The actual javac fork this module wraps -- bundled with this module wherever it ends up // (isolated carrier, per ADFA-5053). From c79abe5476a6e480af057dd239e0b2dd3cb02d65 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 14:48:08 -0700 Subject: [PATCH 2/2] ADFA-5068: Address hal's four review findings Moving ReflectUtilsTest to :shared dropped all nine of its tests from CI: jacocoAggregateReport depended only on each subproject's testV8DebugUnitTest, and a plain java-library has `test`. Sonarqube reaches unit tests solely through that task, so the ADFA-4649 stripFinalModifier regression they guard has been unguarded since the move -- and :plugin-api and :logger were already invisible the same way. The aggregate now falls back to `test` where there is no testV8DebugUnitTest, and collects the flavorless class dirs and build/jacoco/test.exec so the coverage is reported, not merely executed. Verified: :shared:test, :logger:test and :plugin-api:test now appear in the task graph, and the nine tests pass. javac-services declares compileOnly(libs.androidx.annotation). Three sources import it, and dropping implementation(projects.common) removed the last declared provider; it compiled only because viewBinding dragged in androidx.annotation:1.0.0. That module has no layouts, so viewBinding is off there now, which makes the dependency real rather than incidental. The guava comment claimed the carrier no longer duplicates it. It does: :subprojects:java-compiler-carrier's v8ReleaseRuntimeClasspath still reaches com.google.guava:guava through :build-deps:google-java-format and javaparser-symbol-solver-core, neither of which goes via :common. Comment corrected to say so, and to say that keeping guava out means deciding about those two edges -- google-java-format needs it at runtime -- rather than a scope change. ADR 0012 said viewbinding is unconditionally injected by AGP. It is injected by our own convention plugin. That correction suggested a cheaper fix than the constraints block, so I tried it: with viewBinding off in both modules and the constraints removed, v8Release still fails on androidx.annotation:{strictly 1.0.0}, because the pin arrives through the runtime graph from the modules that still enable viewBinding. The constraints stay, the ADR now records both the correct attribution and why the cheaper fix needs to be project-wide. Co-Authored-By: Claude Opus 5 --- build.gradle.kts | 29 +++++++++++++++---- ...0012-lazy-load-javac-via-dexclassloader.md | 4 ++- lsp/java-compiler-impl/build.gradle.kts | 22 +++++++++----- subprojects/javac-services/build.gradle.kts | 29 ++++++++++++++++--- 4 files changed, 65 insertions(+), 19 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index b0f6438ace..5721ea3934 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -459,11 +459,16 @@ tasks.named("sonarqube") { tasks.register("jacocoAggregateReport") { val excludedProjects = emptySet() - // Depend only on testV8DebugUnitTest tasks in subprojects + // Android modules run testV8DebugUnitTest; flavorless ones (plain java-library: :shared, + // :logger, :plugin-api, :eventbus ...) have a plain `test` and no such task. Depending only on + // the former meant nothing in CI ever ran their tests -- sonarqube reaches unit tests solely + // through this task -- so moving a test into one of those modules silently stopped running it, + // which is how ADFA-4649's ReflectUtils regression tests went unguarded (found in review of + // ADFA-5068). dependsOn( subprojects .filterNot { it.name in excludedProjects } - .mapNotNull { it.tasks.findByName("testV8DebugUnitTest") }, + .mapNotNull { it.tasks.findByName("testV8DebugUnitTest") ?: it.tasks.findByName("test") }, ) reports { @@ -480,7 +485,9 @@ tasks.register("jacocoAggregateReport") { "**/*Test*.*", ) - // Collect kotlin and java class directories for v8Debug and v8DebugUnitTest variant + // Collect kotlin and java class directories for v8Debug and v8DebugUnitTest variant, plus the + // flavorless layout (build/classes/{kotlin,java}/main) so a java-library module's coverage is + // reported rather than merely executed. val classDirs = subprojects .filterNot { it.name in excludedProjects } @@ -498,6 +505,12 @@ tasks.register("jacocoAggregateReport") { fileTree(subproj.layout.buildDirectory.dir("intermediates/javac/v8DebugUnitTest/classes")) { exclude(fileFilter) }, + fileTree(subproj.layout.buildDirectory.dir("classes/kotlin/main")) { + exclude(fileFilter) + }, + fileTree(subproj.layout.buildDirectory.dir("classes/java/main")) { + exclude(fileFilter) + }, ) } @@ -511,9 +524,13 @@ tasks.register("jacocoAggregateReport") { val execFiles = subprojects .filterNot { it.name in excludedProjects } - .map { subproj -> - subproj.layout.buildDirectory.file( - "outputs/unit_test_code_coverage/v8DebugUnitTest/testV8DebugUnitTest.exec", + .flatMap { subproj -> + listOf( + subproj.layout.buildDirectory.file( + "outputs/unit_test_code_coverage/v8DebugUnitTest/testV8DebugUnitTest.exec", + ), + // Where a plain java-library's `test` task writes its coverage. + subproj.layout.buildDirectory.file("jacoco/test.exec"), ) } diff --git a/docs/adr/0012-lazy-load-javac-via-dexclassloader.md b/docs/adr/0012-lazy-load-javac-via-dexclassloader.md index 52319972a6..553658085b 100644 --- a/docs/adr/0012-lazy-load-javac-via-dexclassloader.md +++ b/docs/adr/0012-lazy-load-javac-via-dexclassloader.md @@ -52,7 +52,9 @@ Investigation found this coupling narrower than it first looked: none of `CacheF - First `.java`-file interaction in a session now pays a one-time synchronous latency spike (asset extraction on first run + `DexClassLoader` construction + `JavaCompilerService`/`SourceFileManager` bootstrap) on top of ADFA-5052's own deferred-reset cost. - A third resident/isolated classloader boundary to reason about (after Kotlin's and the plugin system's). The same rule as ADR 0011 applies and now has two worked examples of getting it wrong: an `api` dependency anywhere in a vendored composite build's *own* `build.gradle.kts` propagates to every consumer's runtime classpath regardless of how the consumer declares its dependency — `compileOnly` has to be applied at the source of the leak, not just where it's consumed. A second, distinct rule this ADR adds: every resident member the isolated fork calls across the boundary must be `public` — `protected`/package-private access throws `IllegalAccessError` at runtime even when both classes share a package name, since ART checks classloader identity, not just the package string, and this has no build-time or unit-test signal at all. - The debugger's breakpoint/stack-frame source-path resolution (`JavaDebugAdapter`/`ModelUtils.asLspLocation`) took on a narrow, real dependency on `JavaCompilerProvider`/`SourceFileObject` that the isolation boundary can't ignore; it now resolves through `IJavaCompilerSession.findSourceFilePath` (returning a plain path, not the isolated `SourceFileObject` type) instead, and degrades to filename-only when no session exists yet. -- A third rule (ADFA-5068): converting a resident dependency from `implementation` to `compileOnly` can trip AGP's `compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)` check for any `com.android.library` module, if that dependency was incidentally anchoring a high-enough transitive version (of e.g. `androidx.annotation`, `kotlin-stdlib`, `org.jetbrains:annotations`) against AGP's own unconditionally-injected `androidx.databinding:viewbinding`, which pins those same artifacts much lower on the runtime side once nothing else pulls them in. The fix isn't to re-add the dependency (that reintroduces the duplication this rule exists to avoid) but a `constraints {}` block on `implementation` bumping just the conflicting artifact(s) to the version already used everywhere else — a constraint, unlike a dependency, only rescopes an edge that's already reachable (here, via viewbinding), so it doesn't add anything new to the carrier beyond a version bump on a few KB of annotation classes. See `lsp/java-compiler-impl/build.gradle.kts`'s `constraints` block and `subprojects/javac-services/build.gradle.kts`. +- A third rule (ADFA-5068): converting a resident dependency from `implementation` to `compileOnly` can trip AGP's `compileClasspath.shouldResolveConsistentlyWith(runtimeClasspath)` check for any `com.android.library` module, if that dependency was incidentally anchoring a high-enough transitive version (of e.g. `androidx.annotation`, `kotlin-stdlib`, `org.jetbrains:annotations`) against `androidx.databinding:viewbinding`, which pins those same artifacts much lower on the runtime side once nothing else pulls them in. The fix isn't to re-add the dependency (that reintroduces the duplication this rule exists to avoid) but a `constraints {}` block on `implementation` bumping just the conflicting artifact(s) to the version already used everywhere else — a constraint, unlike a dependency, only rescopes an edge that's already reachable (here, via viewbinding), so it doesn't add anything new to the carrier beyond a version bump on a few KB of annotation classes. See `lsp/java-compiler-impl/build.gradle.kts`'s `constraints` block and `subprojects/javac-services/build.gradle.kts`. + + Two corrections to how this was first written (hal, #1643). **viewbinding is not AGP-imposed**: AGP adds it only when `buildFeatures.viewBinding` is on, and it is on for every module here because our own convention plugin sets it (`AndroidModuleConf.kt`). That matters because it suggests a cheaper fix — turn viewBinding off for the modules that have no layouts, removing the low pins at their source. **That fix was tried and is not sufficient at module scope**: with `viewBinding = false` on both `lsp/java-compiler-impl` and `subprojects/javac-services` and the constraints removed, `v8Release` still fails with `androidx.annotation:{strictly 1.0.0}`, because the pin arrives through the runtime graph from the other modules that still enable viewBinding. Removing these constraints for good means turning viewBinding off by default in the convention plugin and opting the modules with layouts back in — a project-wide change worth its own ticket, not a per-module tweak. ## Alternatives considered diff --git a/lsp/java-compiler-impl/build.gradle.kts b/lsp/java-compiler-impl/build.gradle.kts index edc359a22f..3b09238b85 100644 --- a/lsp/java-compiler-impl/build.gradle.kts +++ b/lsp/java-compiler-impl/build.gradle.kts @@ -42,15 +42,21 @@ kapt { dependencies { // javac-services' (and this module's own) compileOnly androidx-heavy deps (sora-editor, // appcompat, material, lsp:indexing, etc.) pull androidx.annotation/kotlin-stdlib/ - // org.jetbrains:annotations at high versions on the compile classpath, but being compileOnly - // they're absent from the runtime classpath -- which then only sees AGP's unconditionally - // injected viewbinding's much lower transitive pins for the same artifacts. AGP's - // compile/runtime consistency check fails to reconcile the two. Constraining these three (not - // adding a dependency: each is already reachable via viewbinding, just at the wrong version) - // harmonizes both classpaths at the version already used everywhere else in the project, - // without adding a new edge. All three are a few KB of interfaces/annotations with no - // resources, unlike the androidx.core duplication this whole compileOnly effort exists to + // org.jetbrains:annotations at high versions onto the compile classpath, but being compileOnly + // they are absent from the runtime classpath -- which sees viewbinding's much lower transitive + // pins for the same artifacts instead. AGP's compile/runtime consistency check cannot reconcile + // the two. Constraining these three (not adding a dependency: each is already reachable via + // viewbinding, just at the wrong version) harmonizes both classpaths at the version used + // everywhere else, without adding an edge. All three are a few KB of interfaces and annotations + // with no resources, unlike the androidx.core duplication this compileOnly effort exists to // avoid -- see ADFA-5068. + // + // viewbinding is not AGP's doing: our own convention plugin turns buildFeatures.viewBinding on + // for every module (AndroidModuleConf.kt). Turning it off here and in javac-services -- neither + // has a layout -- was tried and is not sufficient: v8Release still fails with + // androidx.annotation:{strictly 1.0.0}, because the pin arrives through the runtime graph from + // the modules that still enable viewBinding. Removing these constraints needs that project-wide, + // not per-module. constraints { implementation(libs.androidx.annotation) implementation(libs.common.kotlin.stdlib) diff --git a/subprojects/javac-services/build.gradle.kts b/subprojects/javac-services/build.gradle.kts index df965cbc15..4819c6f54f 100644 --- a/subprojects/javac-services/build.gradle.kts +++ b/subprojects/javac-services/build.gradle.kts @@ -13,19 +13,40 @@ android { isMinifyEnabled = false } } + + // This module has no layouts, and the convention plugin turns viewBinding on for every module + // (AndroidModuleConf.kt). Leaving it on pulled androidx.databinding:viewbinding in, whose + // transitive androidx.annotation:1.0.0 was the only thing making the imports below compile -- + // and whose low version was what java-compiler-impl's constraints block existed to harmonize. + buildFeatures { + viewBinding = false + } } dependencies { // Resident (see docs/adr/0012): kotlin-stdlib, :shared (ReflectUtils, VMUtils) and :logger // (ILogger) are all already loaded by the parent classloader -- implementation here would - // duplicate their bytecode (and :common's androidx/guava graph, previously reached via - // projects.common) into the isolated carrier dex. compileOnly for the same reason as the - // block below. libs.google.guava was dropped entirely: this module's own code never - // references it directly -- it only ever arrived transitively through :common's api(guava). + // duplicate their bytecode into the isolated carrier dex. compileOnly for the same reason as + // the block below. libs.google.guava was dropped because this module's own code never + // references it directly; it only ever arrived transitively through :common's api(guava). + // + // That does *not* mean guava stays out of the carrier. It is still on + // :subprojects:java-compiler-carrier's v8ReleaseRuntimeClasspath, reached through + // :build-deps:google-java-format and javaparser-symbol-solver-core -- neither of which goes via + // :common, so neither was affected by this change (hal, #1643). Keeping it out for real means + // deciding what to do about those two edges, and google-java-format needs guava at runtime, so + // it is not a scope change. The compileOnly(libs.google.guava) in java-compiler-impl upholds an + // invariant the build does not currently hold. compileOnly(libs.common.kotlin) compileOnly(projects.shared) compileOnly(projects.logger) + // Declared, not inherited: three sources here import androidx.annotation, and dropping + // implementation(projects.common) removed the last declared provider. It compiled anyway only + // because viewBinding (now off, above) dragged in androidx.annotation:1.0.0 transitively -- + // an accident of an unrelated build feature. + compileOnly(libs.androidx.annotation) + // The actual javac fork this module wraps -- bundled with this module wherever it ends up // (isolated carrier, per ADFA-5053). api(libs.composite.jdkCompiler)