ADFA-3319: add basic K2-based Kotlin diagnostic provider#1112
ADFA-3319: add basic K2-based Kotlin diagnostic provider#1112itsaky-adfa merged 21 commits intostagefrom
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>
191fdbb to
2ba6d7e
Compare
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
2ba6d7e to
90a3de1
Compare
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
90a3de1 to
54ca7a9
Compare
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>
📝 WalkthroughRelease Notes: K2-based Kotlin Diagnostic Provider (ADFA-3319)Features
Dependencies
|
| Cohort / File(s) | Summary |
|---|---|
Desugaring DSL composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt |
New data class ReplaceClassRef for bytecode class-reference rewriting, accepting fromClass and toClass parameters with computed getters for internal ASM notation; implements Serializable with serialVersionUID. |
Kotlin Diagnostic Provider lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt |
New class providing on-demand Kotlin diagnostics with per-file analysis caching, PSI resolution, and severity mapping via Kotlin Analysis API; implements AutoCloseable. |
Kotlin Language Server Infrastructure lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt, lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.kt, lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt |
Added coroutine infrastructure with debounced analysis triggering, KotlinDiagnosticProvider lifecycle management, parser event system configuration parameter, and defaultKotlinParser property exposure. |
Build Configuration subprojects/kotlin-analysis-api/build.gradle.kts |
Updated external kt-android JAR asset reference tag and sha256 checksum. |
Sequence Diagram(s)
sequenceDiagram
actor Editor as Editor (Client)
participant LSR as Language Server
participant KDP as KotlinDiagnosticProvider
participant CMP as Compiler
participant PRJ as Kotlin Project
Editor->>LSR: Document Open/Change
LSR->>LSR: Cancel previous analyzeJob
LSR->>LSR: Schedule analysis (400ms debounce)
Note over LSR: Wait 400ms<br/>(debounce delay)
LSR->>KDP: analyze(selectedFile)
activate KDP
KDP->>CMP: Refresh & resolve PSI
CMP->>PRJ: Get KtFile
CMP-->>KDP: Return resolved PsiFile
KDP->>KDP: Create in-memory copy
KDP->>PRJ: Run analyzeCopy()<br/>(PREFER_SELF + ONLY_COMMON_CHECKERS)
PRJ-->>KDP: Return KaDiagnostics
KDP->>KDP: Map diagnostics to items<br/>(severity, ranges)
KDP-->>LSR: Return DiagnosticResult
deactivate KDP
LSR->>Editor: Publish Diagnostics
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- PR
#1097: Modifies the sameReplaceClassRefdata class in the desugaring DSL, indicating parallel or dependent work on bytecode class-reference rewriting features. - PR
#1098: Updates Kotlin analysis API integration insubprojects/kotlin-analysis-api, which complements the diagnostic provider infrastructure added in this PR.
Suggested reviewers
- jatezzz
- Daniel-ADFA
- jomen-adfa
Poem
🐰 A rabbit hops through code so keen,
With diagnostics now on screen,
Debouncing dreams and coroutines fast,
Kotlin analysis built to last! 🚀
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main change: adding a basic K2-based Kotlin diagnostic provider, which aligns with the implementation of KotlinDiagnosticProvider and related infrastructure. |
| Description check | ✅ Passed | The description provides relevant context by referencing the related issue ADFA-3319 and specifying merge dependencies, which is related to the changeset's purpose and scope. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/ADFA-3319-basic-diagnostics-provider
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 @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt (1)
338-342:⚠️ Potential issue | 🟡 Minor
onDocumentChangeanalyzesselectedFile, notevent.changedFile.The Kotlin-file guard checks
event.changedFile, butdebouncingAnalyze()uses whateverselectedFilecurrently is. If a background change event fires for a Kotlin file that isn't the currently selected one (e.g. formatter, external write, multi-document workflows), you'll (a) re-analyze the wrong file and (b) skip the actually-changed one. Consider passing the changed file through and/or makingselectedFilethe source of truth only for open/selected events.🤖 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/KotlinLanguageServer.kt` around lines 338 - 342, onDocumentChange currently checks DocumentUtils.isKotlinFile(event.changedFile) but calls debouncingAnalyze() which uses the global selectedFile, causing the wrong file to be analyzed; change debouncingAnalyze to accept a File/URI parameter (e.g., debouncingAnalyze(changedFile: VirtualFile/URI)) and update onDocumentChange to pass event.changedFile (DocumentChangeEvent.changedFile) to it, and update other callers (e.g., selection/open handlers) to call debouncingAnalyze(selectedFile) so the analyzed file is always the one provided rather than relying on the selectedFile field.
🧹 Nitpick comments (4)
composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt (1)
15-18: Clarify parameter docs to match accepted input formats.Class KDoc states both dot and slash notations are accepted, but these field docs currently describe dot-notation only. Align wording to avoid DSL misuse.
Doc-only tweak
- /** The class whose references should be replaced (dot-notation). */ + /** The class whose references should be replaced (dot or slash notation). */ val fromClass: String, - /** The class that should replace all [fromClass] references (dot-notation). */ + /** The class that should replace all [fromClass] references (dot or slash notation). */ val toClass: String,🤖 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 15 - 18, The KDoc for ReplaceClassRef accepts both dot and slash notation but the property docs for fromClass and toClass only mention dot-notation; update the property comments for fromClass and toClass to state they accept either dot (e.g. java.lang.String) or slash (e.g. java/lang/String) notation and mention which form the DSL will consume/emit if applicable so users aren't confused when using the ReplaceClassRef class.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt (1)
46-57: Nit: broadcatch (Throwable)— narrow it if possible.The re-throw of
CancellationExceptionis correct, but catchingThrowableswallowsErrors (e.g.StackOverflowError,OutOfMemoryError) and obscures genuine VM-level problems. Consider narrowing toException(and still re-throwingCancellationException). Based on learnings fromactions/src/main/java/com/itsaky/androidide/actions/internal/DefaultActionsRegistry.kt, prefer narrow exception handling in Kotlin files across this project.🤖 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/diagnostic/KotlinDiagnosticProvider.kt` around lines 46 - 57, The analyze function currently catches Throwable which can swallow VM Errors; change the catch to catch Exception instead while preserving the CancellationException rethrow logic: in analyze (and around doAnalyze and logger.error), catch Exception, if it is a CancellationException rethrow, otherwise log the error and return DiagnosticResult.NO_UPDATE; keep the same logger.error call and return behavior so only non-fatal Exceptions are handled.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt (1)
316-322: Minor: non-atomic cancel/reassign ofanalyzeJobunder ASYNC subscribers.
onDocumentOpen/onDocumentChangerun on the EventBus ASYNC pool, so two events landing concurrently can both read the oldanalyzeJob, cancel it, and launch — leaking one job past the debounce window. Functionally still "eventually consistent", but worth guarding (e.g. a mutex,AtomicReference, or serializing via a single-dispatcher actor).🤖 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/KotlinLanguageServer.kt` around lines 316 - 322, The debouncingAnalyze() currently cancels and reassigns analyzeJob non-atomically which can race when onDocumentOpen/onDocumentChange run on the EventBus ASYNC pool; make the cancel+assign atomic by serializing access to analyzeJob in debouncingAnalyze — e.g. use an AtomicReference<Job?> for analyzeJob and update via getAndSet/cancel on the previous job, or protect the block with a Mutex, or route all debounce requests through a single-threaded coroutine actor/dispatcher; update the debouncingAnalyze method (and the analyzeJob field) to use one of these approaches so concurrent callers cannot leak a launched Job past the debounce window.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt (1)
38-45: Nit: document the newenableParserEventSystemparameter in the class KDoc.The KDoc block above the class enumerates the other parameters;
enableParserEventSystemis now public API and should be described there too (its semantics matter for anyone writing callers that don't need live PSI events).🤖 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/CompilationEnvironment.kt` around lines 38 - 45, Add a KDoc entry describing the new parameter enableParserEventSystem on the CompilationEnvironment class: explain that enableParserEventSystem controls whether the parser event/PSI live event system is enabled (true by default), its effect on callers that do not need live PSI events, and any notable semantics (e.g., default value, thread-safety or lifecycle implications). Update the KDoc parameter list in the class header to include `@param` enableParserEventSystem with this concise description so public API consumers of CompilationEnvironment see its purpose.
🤖 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 21-23: Replace the public `@JvmField` val serialVersionUID in the
companion object of ReplaceClassRef with a private compile-time constant by
changing it to private const val serialVersionUID to make it a true static
constant, restrict visibility, and remove the need for `@JvmField`; update the
companion object in ReplaceClassRef.kt accordingly.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt`:
- Around line 35-38: KotlinDiagnosticProvider.close() currently cancels the
externally provided scope (scope) which destroys the caller's server-wide
SupervisorJob; instead, make KotlinDiagnosticProvider own a child scope and only
cancel that in close(): create an internal CoroutineScope in the
KotlinDiagnosticProvider constructor derived from the passed-in scope (e.g.,
CoroutineScope(scope.coroutineContext +
SupervisorJob(scope.coroutineContext[Job]))) and replace uses of scope with
this.internalScope, then change close() to cancel only the internal child scope
(or keep close() a no-op and cancel per-analysis jobs explicitly); ensure
references to KotlinLanguageServer.recreateSession and
diagnosticProvider?.close() no longer terminate the server-wide scope.
- Around line 98-106: The code logs full source text and noisy per-keystroke
info at info level; in KotlinDiagnosticProvider.kt change the logger calls
around analyzeCopy (specifically the logger.info("ktFile.text={}",
inMemoryPsi.text) and other per-change messages like "Analyzing file" / "Found
{} diagnostics") to use logger.debug or logger.trace and remove or stop logging
inMemoryPsi.text (do not log full file contents); keep only minimal contextual
info (e.g., file id or path) at debug/trace so analyzeCopy, inMemoryPsi,
collectDiagnostics and the final diagnostics count do not emit sensitive/noisy
info at info level.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 131-133: The problem: calling diagnosticProvider?.close() from
recreateSession causes KotlinDiagnosticProvider.close() to cancel the shared
SupervisorJob (member scope) so subsequent debouncingAnalyze()/analyzeSelected()
launches are dropped; to fix, change KotlinDiagnosticProvider.close() so it does
NOT call scope.cancelIfActive(...) on the shared scope: instead give the
provider its own private CoroutineScope or Job for internal work and cancel that
(or cancel only its internal debounce/job fields), leaving the class member
scope (SupervisorJob) untouched; update KotlinDiagnosticProvider.close() to
cancel the provider-owned scope/job (or clear jobs) and remove any call that
cancels the shared scope; keep recreateSession calling
diagnosticProvider?.close() as-is.
---
Outside diff comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 338-342: onDocumentChange currently checks
DocumentUtils.isKotlinFile(event.changedFile) but calls debouncingAnalyze()
which uses the global selectedFile, causing the wrong file to be analyzed;
change debouncingAnalyze to accept a File/URI parameter (e.g.,
debouncingAnalyze(changedFile: VirtualFile/URI)) and update onDocumentChange to
pass event.changedFile (DocumentChangeEvent.changedFile) to it, and update other
callers (e.g., selection/open handlers) to call debouncingAnalyze(selectedFile)
so the analyzed file is always the one provided rather than relying on the
selectedFile field.
---
Nitpick comments:
In
`@composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.kt`:
- Around line 15-18: The KDoc for ReplaceClassRef accepts both dot and slash
notation but the property docs for fromClass and toClass only mention
dot-notation; update the property comments for fromClass and toClass to state
they accept either dot (e.g. java.lang.String) or slash (e.g. java/lang/String)
notation and mention which form the DSL will consume/emit if applicable so users
aren't confused when using the ReplaceClassRef class.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 38-45: Add a KDoc entry describing the new parameter
enableParserEventSystem on the CompilationEnvironment class: explain that
enableParserEventSystem controls whether the parser event/PSI live event system
is enabled (true by default), its effect on callers that do not need live PSI
events, and any notable semantics (e.g., default value, thread-safety or
lifecycle implications). Update the KDoc parameter list in the class header to
include `@param` enableParserEventSystem with this concise description so public
API consumers of CompilationEnvironment see its purpose.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt`:
- Around line 46-57: The analyze function currently catches Throwable which can
swallow VM Errors; change the catch to catch Exception instead while preserving
the CancellationException rethrow logic: in analyze (and around doAnalyze and
logger.error), catch Exception, if it is a CancellationException rethrow,
otherwise log the error and return DiagnosticResult.NO_UPDATE; keep the same
logger.error call and return behavior so only non-fatal Exceptions are handled.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 316-322: The debouncingAnalyze() currently cancels and reassigns
analyzeJob non-atomically which can race when onDocumentOpen/onDocumentChange
run on the EventBus ASYNC pool; make the cancel+assign atomic by serializing
access to analyzeJob in debouncingAnalyze — e.g. use an AtomicReference<Job?>
for analyzeJob and update via getAndSet/cancel on the previous job, or protect
the block with a Mutex, or route all debounce requests through a single-threaded
coroutine actor/dispatcher; update the debouncingAnalyze method (and the
analyzeJob field) to use one of these approaches so concurrent callers cannot
leak a launched Job past the debounce window.
🪄 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: 4048de85-ad9a-4763-b153-328f5f72dda4
📒 Files selected for processing (6)
composite-builds/build-logic/desugaring/src/main/java/com/itsaky/androidide/desugaring/dsl/ReplaceClassRef.ktlsp/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/diagnostic/KotlinDiagnosticProvider.ktsubprojects/kotlin-analysis-api/build.gradle.kts
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
See ADFA-3319 for more details.
Must be merged after: