ADFA-3320: Avoid re-creating full K2 session by invalidating source files when needed#1142
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
fixes duplicate class errors for org.antrl.v4.* classes Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
It is now included in the embeddable JAR (named UnsafeAndroid) with proper relocations. Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
…ject re-sync Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 Walkthrough
WalkthroughThis PR adds BuildRunType to build identifiers and analytics, propagates run-type usage across build services and test launcher, introduces a KotlinProjectModel-driven lifecycle for the Kotlin LSP compiler/compilation environment, adds ReplaceClassRef for desugaring, and updates related dependency and diagnostic cleanup code. Changes
Sequence Diagram(s)sequenceDiagram
participant PA as ProjectHandlerActivity
participant GBS as GradleBuildService
participant BI as BuildId
participant BM as BuildMetric
PA->>GBS: nextBuildId(BuildRunType.ProjectSync)
activate GBS
GBS->>BI: create BuildId(..., runType=ProjectSync)
BI-->>GBS: BuildId
GBS-->>PA: BuildId
deactivate GBS
PA->>BM: BuildMetric.asBundle(buildId)
activate BM
BM->>BI: read runType.typeName
BM-->>PA: Bundle including run_type
deactivate BM
sequenceDiagram
participant KLS as KotlinLanguageServer
participant KPM as KotlinProjectModel
participant CE as CompilationEnvironment
participant CMP as Compiler
KLS->>KPM: instantiate KotlinProjectModel()
KLS->>KPM: addListener(compilationEnv)
KLS->>KPM: update(workspace, platform)
activate KPM
KPM->>KPM: configure modules
KPM->>CE: notify onProjectModelChanged(STRUCTURE)
activate CE
CE->>CE: rebuild session & parser
CE-->>KPM: done
deactivate CE
KPM-->>KLS: update complete
KLS->>CMP: new Compiler(projectModel)
CMP->>CE: create defaultCompilationEnv(projectModel)
CMP-->>KLS: compiler ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt (1)
21-24: Useprivate const val serialVersionUID: Longin the companion object.For
Serializableclasses, the idiomatic Kotlin approach is:Suggested change
companion object { - `@JvmField` - val serialVersionUID = 1L + private const val serialVersionUID: Long = 1L }
const valproduces a true compile-time constant that compiles directly to a private static final field with optimal bytecode. The current@JvmField valworks at runtime but generates unnecessary getter methods and exposes the field publicly (missingprivate). Both are recognized by Java serialization, butconst valis the preferred pattern in Kotlin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt` around lines 21 - 24, The companion object exposes serialVersionUID as a public `@JvmField` val; change it to a true compile-time constant by replacing that declaration inside the companion object with a private const val serialVersionUID: Long = 1L so the compiler emits a private static final field (update the declaration in ReplaceClassRef's companion object).subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/BuildId.kt (1)
10-18: Consider declaring aserialVersionUIDnow thatBuildId's shape is changing.
BuildIdimplementsSerializableand this change adds a new non-optional fieldrunType. Any previously serializedBuildId(e.g. persisted cache, cross-process state) will now fail to deserialize withInvalidClassExceptionbecause the auto-generatedserialVersionUIDdepends on the class shape. Even if today all transport is via JSON-RPC, pinning aserialVersionUIDmakes future additions safer and decouples binary serial compatibility from incidental field changes.🛠️ Suggested addition
data class BuildId( val buildSessionId: String, val buildId: Long, val runType: BuildRunType, ) : Serializable { companion object { + private const val serialVersionUID: Long = 1L val Unknown = BuildId("unknown", -1, BuildRunType.TaskRun) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/BuildId.kt` around lines 10 - 18, BuildId implements Serializable and now has a new non-optional field, so add an explicit serialVersionUID to pin binary compatibility: inside BuildId's companion object (the one that currently contains Unknown) declare a private const val serialVersionUID: Long = 1L (or another stable long) so deserialization does not break when the auto-generated UID would change; bump this value only when making intentionally incompatible binary changes to BuildId.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/KotlinProjectModel.kt (1)
62-66:update()unconditionally firesSTRUCTURE.Minor: each
setupWithProjectcall — even when workspace/platform reference is effectively unchanged — will trigger a full session rebuild. Consider short-circuiting whenworkspace === this.workspace && platform == this.platform, or adding a separate hook to fireSOURCESfrom build-complete without a full structural rebuild (the enum anticipates this but no caller currently emitsSOURCES). Not blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/KotlinProjectModel.kt` around lines 62 - 66, The update method in KotlinProjectModel unconditionally calls notifyListeners(ChangeKind.STRUCTURE), causing full rebuilds even when nothing changed; modify KotlinProjectModel.update(workspace: Workspace, platform: TargetPlatform) to short-circuit early when the incoming workspace is the same instance and platform equals the current ones (e.g., if (workspace === this.workspace && platform == this.platform) return), otherwise assign and notify; alternatively consider emitting ChangeKind.SOURCES from the build completion path instead of always STRUCTURE, but for this change implement the identity/equality check in update to avoid unnecessary STRUCTURE notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt`:
- Around line 9-10: The KDoc is wrong because ReplaceClassRef's constructor
doesn't normalize class-name forms, causing ReplaceClassRef("a.b.C", ...) and
ReplaceClassRef("a/b/C", ...) to be unequal; fix by normalizing both fromClass
and toClass inside the ReplaceClassRef constructor (e.g., in an init block) to a
single canonical form (choose either dot-notation or internal slash-notation
consistent with fromInternal/toInternal), update the stored properties so
fromInternal/toInternal return the expected form, and ensure equals/hashCode
operate on the normalized values (use the same normalization routine used
elsewhere, or call the existing fromInternal/toInternal conversions to avoid
duplication).
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 124-134: rebuildSession currently disposes the old disposable and
replaces it before calling buildSession(), which leaves session/parser pointing
at a disposed project if buildSession() throws; change to construct a new
Disposable first, pass it into a new helper (e.g. buildSessionWith(disposable))
that creates the Project/session without touching the fields, and only after
buildSessionWith succeeds atomically replace disposable, session, and parser
(create KtPsiFactory from the new session) so failures do not leave the
environment in a disposed state; also ensure access to
rebuildSession/onProjectModelChanged that mutate disposable/session/parser is
synchronized to prevent concurrent swaps and leaks.
- Around line 161-192: The nullable-safe casts use redundant trailing question
marks; change the casts from "as? KotlinStandaloneModificationTrackerFactory?"
to "as? KotlinStandaloneModificationTrackerFactory" and from "as?
SimpleModificationTracker?" to "as? SimpleModificationTracker" so the as?
operator alone provides the nullable type; update these casts in the block where
modificationTrackerFactory and sourceModificationTracker are initialized (look
for KotlinStandaloneModificationTrackerFactory and SimpleModificationTracker
symbols).
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/KotlinProjectModel.kt`:
- Around line 33-56: The listeners collection in KotlinProjectModel is a plain
mutableListOf and is accessed by addListener, removeListener and notifyListeners
from multiple threads; replace it with a thread-safe structure (e.g.,
java.util.concurrent.CopyOnWriteArrayList) or protect all accesses with a single
lock so iteration during notifyListeners cannot throw
ConcurrentModificationException and adds/removes are not lost; update the
declaration of listeners and ensure addListener, removeListener and
notifyListeners use the new thread-safe approach.
- Around line 126-132: In KotlinProjectModel.kt update the logging in the block
that checks libDep (the if (libDep == null) branch) to fix the duplicated word
in the message and lower the severity: replace logger.error("Skipping
non-existent classpath classpath: {}", classpath) with a clearer message like
"Skipping non-existent classpath: {}" and use logger.warn(...) instead; keep
referencing the same variables (libDep, classpath) so behavior is unchanged.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 137-162: The else branch that calls
projectModel?.update(workspace, jvmPlatform) can rebuild the compilation session
and invalidate diagnostics, but KotlinDiagnosticProvider.analyzeTimestamps is
keyed by file mtimes so stale timestamps suppress updated diagnostics; after
projectModel?.update(...) (i.e., in the update/STRUCTURE path) call
KotlinDiagnosticProvider.clearTimestamps()
(diagnosticProvider?.clearTimestamps()) to reset analyzeTimestamps so
diagnostics will be recomputed; alternatively register KotlinDiagnosticProvider
as a ProjectModel listener and clear timestamps on ChangeKind.STRUCTURE to cover
other callers.
---
Nitpick comments:
In
`@composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt`:
- Around line 21-24: The companion object exposes serialVersionUID as a public
`@JvmField` val; change it to a true compile-time constant by replacing that
declaration inside the companion object with a private const val
serialVersionUID: Long = 1L so the compiler emits a private static final field
(update the declaration in ReplaceClassRef's companion object).
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/KotlinProjectModel.kt`:
- Around line 62-66: The update method in KotlinProjectModel unconditionally
calls notifyListeners(ChangeKind.STRUCTURE), causing full rebuilds even when
nothing changed; modify KotlinProjectModel.update(workspace: Workspace,
platform: TargetPlatform) to short-circuit early when the incoming workspace is
the same instance and platform equals the current ones (e.g., if (workspace ===
this.workspace && platform == this.platform) return), otherwise assign and
notify; alternatively consider emitting ChangeKind.SOURCES from the build
completion path instead of always STRUCTURE, but for this change implement the
identity/equality check in update to avoid unnecessary STRUCTURE notifications.
In
`@subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/BuildId.kt`:
- Around line 10-18: BuildId implements Serializable and now has a new
non-optional field, so add an explicit serialVersionUID to pin binary
compatibility: inside BuildId's companion object (the one that currently
contains Unknown) declare a private const val serialVersionUID: Long = 1L (or
another stable long) so deserialization does not break when the auto-generated
UID would change; bump this value only when making intentionally incompatible
binary changes to BuildId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8253da0-2808-4011-aae1-7bb7646e19f0
📒 Files selected for processing (12)
app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/analytics/gradle/BuildMetric.ktapp/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.ktcomposite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kteventbus-events/build.gradle.ktslsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/KotlinProjectModel.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.ktsubprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/BuildId.kttesting/tooling/src/main/java/com/itsaky/androidide/testing/tooling/ToolingApiTestLauncher.kt
See ADFA-3320 for more details.
Must be merged after: