Uh oh!
There was an error while loading. Please reload this page.
ADFA-5053: Lazy-load the embedded javac fork via a DexClassLoader carrier APK - #1638
Draft
davidschachterADFA wants to merge 24 commits into
Draft
ADFA-5053: Lazy-load the embedded javac fork via a DexClassLoader carrier APK#1638davidschachterADFA wants to merge 24 commits into
davidschachterADFA wants to merge 24 commits into
Conversation
CacheFSInfo/FSInfo/RelativePath/Context/PlatformUtils/Assert (package unchanged) need to stay resident so subprojects/projects can keep using them for classpath-jar indexing, while the rest of jdk-compiler's ~400 files (parser/Attr/Resolve/Symtab/Types/codegen) move into an isolated, DexClassLoader-loaded carrier in a later commit. Mechanical move only: jdk-compiler already depends on java-compiler (api(projects.buildDeps.javaCompiler)), so relocating files the other direction can't create a cycle, and every dependency of these six classes (java.util/nio, jdkx.tools.JavaFileObject, zipfs2's AndroidFsProvider) is already satisfiable from java-compiler alone. Without this move, isolating the rest of jdk-compiler would either duplicate these classes (a resident copy plus a carrier-dex copy) or strand them where subprojects/projects can't reach them. Verified :build-deps:java-compiler, :build-deps:jdk-compiler, :subprojects:javac-services, :subprojects:projects, and :lsp:java all still compile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
subprojects/projects (the foundational, always-resident project-model module used by every project regardless of language) depends on javac's file-system caching wrappers (CacheFSInfoSingleton, CachedJarFileSystem, CachingJarFileSystemProvider, JarPackageProviderImpl, AndroidFsProviderImpl) for live classpath-jar indexing. That has to stay resident even after the rest of javac's fork moves into an isolated DexClassLoader carrier in a later commit, so it needs its own module rather than living inside javac-services (which is becoming the heavy, isolated payload). Package name (com.itsaky.androidide.javac.services.fs) is unchanged, so no source outside build.gradle.kts files needed touching. javac-services depends on it directly now (previously that was implicit, via the two sharing one module); subprojects/projects and lsp/java depend on it directly too instead of transitively through javac-services. Verified :subprojects:javac-fs, :subprojects:javac-services, :subprojects:projects, and :lsp:java all compile, and existing unit tests for :subprojects:projects and :subprojects:javac-services pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ntil a real .java file is touched DefaultLanguageServerRegistry.onProjectInitialized dispatches setupWithProject to every registered language server unconditionally, regardless of project language. JavaLanguageServer.setupWithProject() referenced JavaCompilerService.NO_MODULE_COMPILER and called SourceFileManager.clearCache(), both of which trigger class-init that eagerly constructs real javac Context/ JavacFileManager machinery plus a full android.jar top-level-class scan -- on the first project open in the app's lifetime, Kotlin-only projects included. shutdown() had the same problem in reverse, on every project close. Same eager-load bug pattern ADFA-5010 fixed for the Kotlin Analysis API, and independently confirmed and sized (openjdk.tools.javac ~2,238 classes, ~3.7MB) while researching whether javac could get ADFA-5010's carrier-APK treatment. This fix is scoped to just the eager-construction bug -- no DexClassLoader/ carrier-APK split; javac/jdk-compiler stay in the main dex, just constructed lazily. setupWithProject() now only stashes the workspace; the actual reset (destroy NO_MODULE_COMPILER, clear file-manager/JAR-fs caches, index module classpaths) is deferred to ensureProjectReset(), called from getCompiler() and onContentChange() -- both already gated on DocumentUtils.isJavaFile(), so this now only runs on genuine Java-file interaction. shutdown() skips its javac-specific cleanup entirely if that never happened. Per-file LSP dispatch methods (complete/findReferences/findDefinition/expandSelection/signatureHelp) needed no changes: the editor's IDELanguage already resolves one language server per file before calling any of them, so they were never the source of the cross-language trigger. Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.
…ss and shutdown The previous fix's synchronized(this) block only guarded the *decision* to run ensureProjectReset() (claiming pendingWorkspace, flipping javaCompilerInitialized to true) -- not the destroy/rebuild work that followed, which ran unsynchronized. A concurrent getCompiler()/ onContentChange() call on another thread could see javaCompilerInitialized already true and proceed to use JavaCompilerProvider/SourceFileManager while the first thread was still mid-destroy or mid-rebuild. shutdown() didn't synchronize on anything at all, so it could run its own destroy()/ clearCache() concurrently with an in-flight reset, racing two teardown/ rebuild sequences against each other. Replaces the two ad-hoc @volatile fields with an explicit PENDING/RESETTING/INITIALIZED/SHUTDOWN state machine guarded by a single ReentrantLock (compilerLifecycleLock) held for the *entire* reset or shutdown, not just the state transition. Concurrent callers now genuinely block until an in-flight reset finishes (getCompiler()/onContentChange() already route through ensureProjectReset(), which now can't return early while another thread holds the lock), and shutdown() waits on the same lock before deciding whether there's anything to tear down. setupWithProject() also goes through the lock; if a new project arrives mid-reset, the in-progress reset's own finally block detects the newer pendingWorkspace and reverts to PENDING instead of incorrectly claiming INITIALIZED. Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.
…t-lock race Three issues from /code-review high, verified against the current code: 1. ensureProjectReset() nulled pendingWorkspace before the try block, so any exception during destroy/rebuild (e.g. a bad submodule's classpath) still let the finally claim INITIALIZED -- silently treating a half-torn-down compiler as ready, with no retry, for the rest of the session. Now an exception re-queues the workspace, reverts to PENDING, and rethrows. 2. analyze() never called ensureProjectReset() at all. diagnosticProvider .analyze() builds its own JavaCompilerService directly, bypassing getCompiler(), and analysis is often the *first* real .java-file interaction (auto-triggered on file open, ahead of any completion request) -- so the R.jar/file-manager cache clear this reset performs could be skipped for an entire session, leaving diagnostics resolving against a stale previous project's classpath. Now gated the same way getCompiler()/onContentChange() already are. 3. getCompiler() and onContentChange() released compilerLifecycleLock as soon as ensureProjectReset() returned, then used JavaCompilerProvider unlocked -- a concurrent reset for a newer project could destroy() those compilers in the gap. Both now hold the lock across the reset and the subsequent provider lookup/use (safe: ReentrantLock is reentrant, so ensureProjectReset()'s own internal withLock nests without deadlocking). Two other findings from the same pass were assessed and left as-is: - shutdown() blocking on an in-flight reset with no cancellation is real but performance-only (no crash/corruption), requires disproportionate cancellation plumbing through SourceFileManager/JavaCompilerService for a narrow, bounded-cost edge case. - KotlinLanguageServer's eager construction is a real observation about this branch's current state, but it's already fixed by the separate, not-yet- merged ADFA-5010 (PR #1635) -- out of scope here, not a gap in this PR. Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.
New IJavaCompilerSession/IJavaCompilerSessionFactory interfaces, mirroring lsp/kotlin-api's role from ADR 0011: this is the only type surface a resident JavaLanguageServer will be allowed to reference once JavaCompilerService and its Provider classes move into an isolated, DexClassLoader-loaded module in a later commit. Exposes the LSP operations directly (complete/findReferences/ findDefinition/expandSelection/signatureHelp/analyze/onContentChange) rather than a getCompiler(): JavaCompilerService accessor, since JavaCompilerService itself won't be a resident type. Confirmed no caller outside JavaLanguageServer.kt uses the current getCompiler() (it's @RestrictTo(LIBRARY_GROUP)), so dropping it from the bridge is safe. Not yet wired up -- JavaLanguageServer.kt still talks to the soon-to-be-isolated types directly; that happens once the isolated implementation module exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-impl Moves the whole javac-touching surface of lsp/java (compiler/, providers/ except providers/snippet, actions/, edits/, parser/, rewrite/, utils/ except AnalyzeTimer, visitors/, JavaCompilerProvider, and their models) into a new isolated module. JavaLanguageServer.kt stays in lsp/java as a thin resident wrapper: it keeps ADFA-5052's CompilerLifecycle state machine unchanged, but ensureProjectReset() now lazily extracts and DexClassLoader-loads a carrier APK (via the new JavaCompilerLoader, mirroring KotlinCompilerLoader/ADR 0011) instead of directly constructing JavaCompilerService/JavaCompilerProvider. Every LSP operation (complete/findReferences/findDefinition/expandSelection/signatureHelp/ analyze/onContentChange/formatCode/handleFailure) now delegates through the new IJavaCompilerSession bridge (lsp/java-api) instead of touching isolated types directly. Not yet wired to an actual carrier: no carrier module exists yet, so JavaCompilerLoader has nothing to load on-device. That's the next commit. Locally, :lsp:java-compiler-impl:compileV8DebugSources verifies the isolated payload builds standalone. Notable fixes needed along the way, all confirmed via compilation across :app, :editor, :lsp:java, and :lsp:java-compiler-impl: - google-java-format stays a resident dependency (JavaServerSettings' formatter options) rather than moving with javac -- ADFA-4549 never flagged it as bloat, and the isolated module sees it via compileOnly so the type identity still matches. - The debugger's breakpoint/stack-frame source-path resolution (debug/utils/ModelUtils.kt) took a real, if narrow, dependency on JavaCompilerProvider/SourceFileObject. Added IJavaCompilerSession.findSourceFilePath so it resolves through the bridge (returning a plain path, not the isolated SourceFileObject type) instead of needing the isolated module directly. - CancelChecker.kt and CompletableDeferredExts.kt were misplaced under lsp/java/utils despite being genuinely generic (editor's IDEEditor.kt and app's ProjectHandlerActivity.kt use the former for coroutine cancellation logging unrelated to javac; app's Resolvable.kt uses the latter for Deferred completion state). CancelChecker moved with the isolated payload (its CancelAbort check is genuinely javac-specific and classloader-identity-sensitive); the two generic call sites got their own small inline cancellation check instead. CompletableDeferredExts.kt moved back to stay resident. - Applied ADFA-5010's LSPEditorActions fix (replace-not-skip on register, plus a new unregisterCodeActions) here too: Java's own carrier needs the same protection against a stale session's actions outliving its DexClassLoader. - Moved the lsp/java unit tests that exercised isolated types (JavaCompilerService, CompletionProvider, JavaSelectionProvider, DefinitionProvider, etc.) into lsp/java-compiler-impl's own test sourceset, and rewrote their `server.<lspMethod>()` calls to construct the isolated providers directly -- going through JavaLanguageServer would now try to load a real carrier APK that doesn't exist in the unit test environment. Verified: :app, :editor, :lsp:java, :lsp:java-compiler-impl, :subprojects:projects all compile; unit tests pass for :lsp:java, :lsp:java-compiler-impl, and :subprojects:projects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lass leaks it exposed New subprojects/java-compiler-carrier: a never-installed com.android.application shell (isMinifyEnabled=false, mirrors subprojects/kotlin-compiler-carrier) that exists only so AGP produces a real classes.dex from lsp:java-compiler-impl, for JavaCompilerLoader to DexClassLoader at runtime. Assembling it for the first time surfaced that the isolation from the previous commits wasn't actually leak-proof -- verified with dexdump against both the carrier's and the main app's dex, not just by reading the Gradle config: - jdk-compiler's own build.gradle.kts had `api(projects.buildDeps.javaCompiler)` -- an api dependency propagates to every consumer's runtime/packaging classpath regardless of how *they* declare their own dependency on jdk-compiler, so no amount of compileOnly on the consumer side could stop java-compiler (CacheFSInfo, Context, etc.) from being duplicated into the carrier. Changed to compileOnly; the javac aggregate module still api's both itself for its own resident-only consumers. - lsp-java-compiler-impl itself directly depended on the libs.composite.javac aggregate (both jdk-compiler and java-compiler) instead of jdk-compiler alone -- switched to the split dependency. - javac-services depended on javac-fs via implementation instead of compileOnly, bundling the resident fs wrappers into the carrier too. - google-java-format's own build.gradle.kts had the same api(javac aggregate) problem as jdk-compiler -- fixed the same way. But google-java-format actually runs javac's real parser at runtime to reformat source, so unlike javapoet it has to move with javac into the isolated module, not stay resident: JavaServerSettings (resident) now exposes only a plain code-style int instead of a google-java-format JavaFormatterOptions/Style value, and the two isolated call sites (CodeFormatProvider, OrganizeImportsAction) build the real options object themselves. - javapoet, by contrast, turned out to be needed unconditionally and resident-side too (templates-api/templates-impl's "New Project" wizard, a completely separate use from the Java LSP's code-generation actions) and is lightweight (no jdk-compiler dependency at all) -- kept it fully resident, with lsp-java-compiler-impl seeing it via compileOnly. - app/build.gradle.kts had a stray direct `implementation(projects.subprojects .javacServices)` with zero actual source usage in app/ -- the same class of leftover ADFA-5010 found and removed for kotlin-analysis-api. This was the very last leak keeping the full heavy javac fork in the main app's dex even after every other fix above. Verified end-to-end via dexdump class-descriptor listings (not just `checkDuplicateClasses`, which only catches conflicts within one module's own build): CacheFSInfo/Context/Assert/PlatformUtils/CacheFSInfoSingleton/JavaPoet appear exactly once, in the main app's dex, never the carrier's; NBAttr/ ReusableCompiler/JavaCompilerService/JavaCompilerSessionImpl/google-java- format's Formatter appear exactly once, in the carrier's dex, never the main app's. :app:assembleV8Debug and :subprojects:java-compiler-carrier:assembleV8Debug both succeed; :lsp:java, :lsp:java-compiler-impl, :subprojects:projects, and :subprojects:javac-services unit tests all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
New copyJavaCompilerCarrierToAssets task (app/build.gradle.kts), mirroring
ADR 0011's copyKotlinCompilerCarrierToAssets pattern: builds
:subprojects:java-compiler-carrier:assembleV8Release and copies the unsigned
APK to app/src/main/assets/data/common/java-compiler-carrier.apk, wired into
preBuild so it's always current. No PNG-optimization step needed here (unlike
the Kotlin carrier) -- this module has no resources at all.
Includes the same evaluationDependsOn(":subprojects:java-compiler-carrier")
workaround the Kotlin carrier needed: without it, configureondemand=true only
reaches that (com.android.application) project lazily via the task's
cross-project dependsOn, which trips a "DefaultClassLoaderScope must be
locked" Gradle failure specific to that project type.
Verified: :app:copyJavaCompilerCarrierToAssets and :app:assembleV8Debug both
succeed, and the built APK's assets/data/common/java-compiler-carrier.apk
(~36MB) is a raw asset entry, not merged into app's own classes*.dex.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>-keep class openjdk.** { *; } is a no-op now that the javac fork lives only
in the isolated carrier's dex, never app's own -- confirmed via a real
:app:assembleV8Release build, dexdump-verified before and after: zero
Lopenjdk/** class descriptors were reachable-but-for-the-rule, and the small
resident set (CacheFSInfo/FSInfo/RelativePath, kept alive by genuine
reachability from subprojects/projects, not this rule) survives R8 shrinking
identically with the rule removed. jdkx.**'s keep rule stays -- unlike
Kotlin's Analysis API, some jdkx/java-compiler classes remain genuinely
resident (javac-fs, javapoet), so removing it isn't a pure no-op the way this
one was.
ADR 0012 documents the full decision: the ADR 0011 precedent this mirrors,
the subprojects/projects coupling that made this harder than Kotlin's case
and how the vendored-source relocation resolved it, the google-java-format
-vs-javapoet resident/isolated judgment calls, and the three real
duplicate-class-identity bugs (an `api` dependency in a vendored composite
build's own build.gradle.kts propagating to every consumer regardless of how
they declare their own dependency) found only by dexdumping the actual built
carrier and app dex, not by reading the Gradle config.
Verified: :app:assembleV8Release succeeds; the app launches without
crashing on a physical device (Pixel 6 Pro) with the rule removed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>…s to public Found via the interactive on-device pass (not by the build or unit tests): opening a real .java file crashed with IllegalAccessError: Method 'java.util.Optional openjdk.tools.javac.file. CacheFSInfo.getAttributes(java.nio.file.Path)' is inaccessible to class 'openjdk.tools.javac.file.JavacFileManager' (declaration of 'JavacFileManager' appears in .../java-compiler-carrier.apk!classes4.dex) ART treats two classes with the identical package name as different runtime packages when they're loaded by different classloaders -- same-package and protected access are resolved by classloader identity, not just the package string. CacheFSInfo/RelativePath are resident (java-compiler, per ADR 0012); JavacFileManager is isolated in the carrier (jdk-compiler). Calling a protected or package-private member across that boundary throws IllegalAccessError at runtime, with no build-time or unit-test signal at all. Widened the three members JavacFileManager/JRTIndex actually call this way: CacheFSInfo.getAttributes, RelativePath.RelativeFile.forClass, and RelativePath.RelativeDirectory.forPackage. Audited the rest of jdk-compiler for other cross-boundary protected/package-private accesses on the six resident leaf classes (CacheFSInfo, FSInfo, Context, Assert, PlatformUtils, RelativePath) via static call-site search -- no others found. Verified live on a physical device (Pixel 6 Pro): opening a real .java file now extracts and DexClassLoader-loads the carrier, constructs SourceFileManager, and runs a full diagnostic pass to completion with zero IllegalAccessError and zero crashes, across two clean app restarts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ADR 0012 Records the IllegalAccessError finding from the on-device verification pass as its own hazard class, distinct from the duplicate-class-identity one already documented: ART resolves same-package/protected access by classloader identity, not just the package name string, so a protected or package-private member on a resident class throws IllegalAccessError when called from isolated code, with zero build-time or unit-test signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Spotless ratchet is file-level: since ~150 files were git-mv'd into the new lsp/java-compiler-impl module, every one of them differs from origin/stage and gets reformatted in full on first touch. Run spotlessApply and manually fix the handful of ktlint violations it couldn't auto-correct (wildcard import, duplicate license header, comment placement, mixed &&/||, a var that should've been val, and lines pushed over the 140-col limit by tab-width expansion).
The ADR said all three of jdk-compiler/javapoet/google-java-format needed their api() dependency on java-compiler fixed to compileOnly, but the actual diff deliberately left javapoet's api() unchanged (correctly -- javapoet stays fully resident, so it has no isolated consumer to leak into). Only jdk-compiler and google-java-format needed the fix. Caught by an architecture-review pass before opening the PR.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
…rrier Both were declared implementation instead of compileOnly, duplicating these resident libraries -- including native .so payloads -- into the isolated carrier dex alongside the identical resident copies, defeating part of the dex-size point of the isolation and risking UnsatisfiedLinkError if both classloaders load the same-named .so. The carrier's DexClassLoader resolves them from its parent (the resident classloader) instead. Also adds kotlinx-coroutines-core as compileOnly, needed by a follow-up commit that translates CancelAbort into CancellationException before it crosses back out of the isolated session.
Six independent issues found during code review of the javac carrier split:
- JavaCompilerImpl.parse() closed the TSParseResult it got from
TSJavaParser's own LRU cache via .use{} -- since the cache returns the
same instance on a hit, this double-closed (and use-after-freed) the
underlying native parse tree on the next completion request for an
unchanged file. Stop closing it; the cache already owns its lifecycle.
- JavaLanguageServer.complete()/formatCode() resolved the compiler session
and released compilerLifecycleLock before using it, unlike
onContentChange() -- a concurrent project reset could destroy() the
session's compilers in the gap. Now hold the lock across both steps, same
as onContentChange(). The five suspend methods (findReferences etc.)
can't use the same fix: the Kotlin compiler rejects a suspension point
inside a lock's critical section outright. Documented why, since the
residual race there is narrower than it looks (JavaCompilerProvider's
map access is already synchronized).
- JavaLanguageServer.shutdown() gated its teardown on
compilerLifecycle == INITIALIZED, but setupWithProject() sets state back
to PENDING on every project switch even once the carrier's already
loaded -- a switch queued without a .java-file interaction yet left a
live session's DexClassLoader leaked. Gate on session existence instead.
- JavaCompilerLoader.close() mutated the session field without
synchronized(this), unlike getOrCreateSession(), risking a race between
the two.
- JavaCompilerSessionImpl.close() never destroyed NO_MODULE_COMPILER,
unlike resetProject() -- now each session owns its own discardable
DexClassLoader, so nothing could reach back to release it once replaced.
- CodeActionsMenu.children was a plain, unsynchronized LinkedHashSet
mutated from LSP-dispatch threads (session register/unregister)
concurrently with the UI thread rendering the menu. Switched to
CopyOnWriteArraySet.
- CachedJarFileSystem.packages was a plain, unsynchronized map written by
resident classpath indexing and read by the isolated compiler through
the same shared provider -- now crossing the classloader boundary too,
not just threads. Switched to ConcurrentHashMap.
- javac's cancellation signal, CancelAbort, is thrown deep inside the
isolated fork and is only classloader-identity-safe to recognize on the
side that threw it -- IDEEditor's isCancelled() check (which looks for
CancellationException) silently stopped recognizing it once javac moved
into the carrier, logging routine cancellations as real failures.
JavaCompilerSessionImpl now translates CancelAbort into
CancellationException before it crosses back to resident code.- SourceFileObject.equals() compared paths via the live Files.isSameFile() check while hashCode() hashed the raw Path, violating the equals/hashCode contract -- two objects Files.isSameFile() considered equal (e.g. a symlink, or a relative vs. canonicalized path) could land in different hash buckets, silently breaking the compile-cache map's lookups. Both now key off a single canonicalPath resolved once via toRealPath(). - MultipleClassImportEditHandler computed each new import's position independently against the same pre-edit AST, then applied them in sequence against the same live buffer -- an earlier insertion shifts every line after it, invalidating a later edit's pre-computed position. Apply bottom-to-top instead: inserting at a lower line never shifts anything above it. - Four unguarded edge cases in new rewrite handlers that threw instead of cancelling gracefully: GenerateRecordConstructor NPEs on an unresolvable type element/tree; CreateMissingMethod threw when a call sat in a field or static initializer (no enclosing method) and StringIndexOutOfBounds on an anonymous class's empty simple name; RemoveException read one past the end of the buffer when a trailing comma was its last character. - ModelUtils.asLspLocation()'s fallback treated JDI's sourcePath() (a package-relative string) as a real filesystem path when no compiler session was available yet -- e.g. the very first breakpoint hit in a session, before any .java file had loaded the carrier -- so File(path) silently failed to open. Resolve it against each module's compile source directories first, which works without a session; log clearly if that also fails instead of silently handing back an unusable path.
- JavaCompletionProviderTest's members() was missing its @test annotation, so JUnit silently skipped it -- any regression in that completion path would have passed CI unnoticed. - No test exercised JavaCompilerLoader at all. Added coverage for close()/currentSession()'s contract when no session was ever created. This does not cover the getOrCreateSession()-vs-close() race the synchronization fix in this branch addresses, or JavaLanguageServer's CompilerLifecycle state machine more broadly: getOrCreateSession() extracts a real carrier APK and DexClassLoader-loads it, neither of which works in this JVM unit-test environment (no carrier APK asset is present, and there's no on-device ART to load it into) -- the same constraint that made JavaCompletionProviderTest bypass JavaLanguageServer entirely. Closing that gap needs either a DI seam for the classloader construction or an on-device instrumented test.
…Context
ADR 0012 and JavaCompilerLoader's doc comment cited ADR 0011 and
KotlinCompilerLoader as already-established precedent ("mirrors ADR 0011
exactly"), but neither exists on this branch: ADFA-5010 (the sibling
ticket that introduces them) is a separate, unmerged branch. Added a note
clarifying the real relationship and pointing at the actual existing
precedent both tickets extend, PluginLoader.
Also documents why ReusableContext extends Context (isolated extending
resident) accessing its protected ht/key() members doesn't need the same
public-widening treatment ADR 0012 already applied to three sibling-access
cases: protected access via inheritance is governed by a different JVMS
5.4.4 rule than same-package-sibling access, with no runtime-package/
classloader-identity condition attached. No code change -- confirmed safe,
not a bug.…ppdevforall/CodeOnTheGo into task/ADFA-5053-lazy-load-javac-carrier
CI's "Build Universal APK" check has failed 3/3 times on this branch
(before and after this batch of fixes) with:
Property '$1' specifies file '.../java-compiler-carrier-v8-release-unsigned.apk'
which doesn't exist.
Reproduced locally: packageV8Release completes and reports success, but
the plain APK file is intermittently missing from disk immediately
afterward. The task only had dependsOn(":...:assembleV8Release") -- a
task-ordering hint, not a real value-based dependency -- wired to a
hardcoded path guessing the AGP-produced filename ("-unsigned" suffix
included). That's exactly the shape of bug that races the file's own
write-to-disk on some environments even though dependsOn ordering is
satisfied.
Fixed by exposing the release variant's real APK output directory via
AGP's variant artifacts API (variant.artifacts.get(SingleArtifact.APK))
instead of a hardcoded path -- this ties Gradle's dependency tracking to
the actual producing task's Provider, and also stops assuming the
"-unsigned" filename, which was never guaranteed to stay accurate.
Verified with 5 consecutive clean (--no-build-cache --rerun-tasks)
local rebuilds, plus a full :app:assembleV8Debug sanity build.…ava-compiler-impl androidx.annotation, guava, gson, androidx.core.ktx, and kotlin-stdlib were all `implementation` despite every one already being resident (loaded by the parent classloader by the time the carrier's DexClassLoader runs) -- the same pattern already used correctly for androidide.ts/common.editor two lines above. Changed all five to compileOnly. Verified via a clean rebuild that this alone doesn't shrink java-compiler-carrier.apk: androidx.core's resources and the other four libraries also arrive through implementation(javacServices), which pulls in :common (and guava, kotlin-stdlib) on its own account, independent of what this module declares directly. That's a separate, deeper fix blocked by an AGP consistent-classpath conflict -- tracked as its own follow-up. This change is still correct and harmless on its own terms; it just isn't where the carrier's bytes are.
3 tasks
hal-eisen-adfa
marked this pull request as draft
August 21, 2026 18:19
Five conflicts, all from this branch relocating files that stage edited in place. AddImportAction, AutoFixImportsAction, VariableToStatementAction and FieldToBlockAction: taken from stage at the relocated lsp/java-compiler-impl path. The raw diffs looked alarming -- 233, 221, 89 and 87 lines -- but ignoring whitespace this branch changed nothing substantive in any of them: every difference is ktlint rewrapping a parameter list or joining a wrapped call. Stage's edits are the real ones, so stage's content wins and the relocation is preserved. docs/adr: stage claimed 0011 through 0014 while this branch was open, so this branch's ADR moves from 0012 to 0015, along with the eight references to it in javac-services, javac-fs, lsp/java's JavaCompilerLoader, lsp/java-compiler-impl's build script and its own title. Stage's own "ADR 0012" references point at its volatile-build-metadata ADR and are left alone. The README rows for 0013 and 0014 also had their filenames off by one on stage; corrected while resolving the table. JavaCodeActionTooltipTagTest moves to lsp/java-compiler-impl. Stage added it under lsp/java, where JavaCodeActionsMenu used to live; this branch moved that object into the carrier module, and lsp/java does not depend on it (the dependency runs the other way). The impl module already has the test wiring and depends on lsp/java, so the test compiles and passes there unchanged. Verified: :lsp:java-compiler-impl, :lsp:java, :app and :subprojects:javac-services compile; 3 lsp/java, 19 lsp/java-compiler-impl, 60 common and 293 app tests pass; spotlessCheck is clean.
4 tasks
davidschachterADFA added a commit
that referenced
this pull request
Aug 26, 2026
38 commits behind, and stage shipped a whole Kotlin refactoring feature
into lsp/kotlin while this branch was moving the Analysis API out of it,
so most of the work was deciding which module each of stage's new files
belongs in rather than resolving text.
Seven conflicts. Six were files this branch relocated and stage edited
in place -- stage's content wins at the impl path, with the import moves
this branch made (diagnostic.DiagnosticAction and the index keys are in
lsp:kotlin-api now) reapplied. The seventh, KotlinLanguageServer, was
the one that mattered: stage changed only seven lines there, and they
implement findReferences via findUsagesAt. That would have been lost to
the module split, so Find Usages is plumbed through the bridge --
findReferences on IKotlinCompilationEnvironment, implemented in the
carrier as context(this) { findUsagesAt(params) } -- exactly like
complete, findDefinition, signatureHelp and collectDiagnostics.
Eleven of stage's production files need the Analysis API and moved into
the carrier with it: both extract actions and the nine planner/analysis
helpers under utils/refactor.
That left stage's refactoring UI needing five symbols the move had
buried in the carrier. They are pure -- an enum, a name validator, and
three indent/offset functions -- so they now live in lsp/kotlin as
RefactorNameValidation.kt and RefactorTextLayout.kt, which satisfies
both ADRs: the UI keeps them (0013) and the carrier still reads them
through the compileOnly bridge it already declares (0016). Several
helpers widened from internal to public for the same reason, each with
the reason in a comment; internal does not cross a module boundary.
All 44 Kotlin LSP test files moved to the carrier, with the four
compileOnly bridges repeated as testImplementation -- compileOnly does
not reach the test classpath, and lsp/kotlin cannot compile a fixture
that touches the Analysis API. That was the last thing keeping this
branch from having any Kotlin LSP tests at all.
This branch's ADR is renumbered 0011 -> 0016; stage took 0011 through
0014 while it was open, and 0015 is PR #1638's. Stage's own "ADR 0011"
citations point at its command-analysis-priority ADR and are untouched.
Verified: :lsp:kotlin, :lsp:kotlin-compiler-impl and :app compile; 444
kotlin-compiler-impl, 293 app, 60 common and 18 lsp/java tests pass;
spotlessCheck is clean.4 tasks
davidschachterADFA added a commit
that referenced
this pull request
Aug 26, 2026
…5068-javac-services-dep-scope This PR is stacked on #1638, not on stage. Merging stage directly gave five conflicts -- the same five #1638 had, because they are the same relocations -- and would have meant resolving them a second time and renumbering the shared ADR independently, leaving the two branches with different answers to the same question. Merging its actual base instead resolves cleanly: zero conflicts, and the ADR arrives already renumbered to 0015 with stage's 0011-0014 in place. Verified: :lsp:java-compiler-impl and :app compile, the java LSP tests pass, spotlessCheck is clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ADFA-5053: moves the embedded javac/jdk-compiler fork (~2,238 classes, ~3.7MB, the #2 DEX-bloat contributor per ADFA-4549) off
app's main DEX, mirroring ADFA-5010's Kotlin Analysis API treatment (PR #1635, ADR 0011).lsp/java-api(resident bridge interface),lsp/java-compiler-impl(isolated payload -- the actual javac-dependent code),subprojects/javac-fs(resident file/attribute-caching leaf classessubprojects/projectsneeds for every project regardless of language),subprojects/java-compiler-carrier(the never-installed carrier APK).JavaLanguageServerbecomes a thin wrapper; javac is loaded viaDexClassLoaderon first real.java-file interaction, reusing ADFA-5052'sCompilerLifecycle/ensureProjectReset()trigger machinery rather than rebuilding it.api()deps in vendoredbuild.gradle.ktsfiles; cross-classloaderprotected/package-private access throwingIllegalAccessErrorat runtime) are documented in ADR 0012.Stacked on #1637 (ADFA-5052) -- this branch is built on top of it and includes its commits, since ADFA-5053 reuses its lazy-trigger machinery rather than duplicating it. The diff here will shrink to just this ticket's commits once #1637 merges to
stage.Test plan
spotlessCheckclean, module compiles (:lsp:java-compiler-impl,:lsp:java,:lsp:java-api)lsp/javaunit tests moved intolsp/java-compiler-impl's own test sourcesetDexClassLoader-loads on first.javafile interaction; completion, diagnostics, navigation, and code actions work; zeroIllegalAccessErroracross two independent app-restart/retest cycles after the cross-classloader access fixapkanalyzer dex packagesbefore/after) -- not yet run in this session🤖 Generated with Claude Code