Uh oh!
There was an error while loading. Please reload this page.
ADFA-3319: add basic K2-based Kotlin diagnostic provider - #1112
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
2ba6d7eCompareSigned-off-by: Akash Yadav <akashyadav@appdevforall.org>
2ba6d7e to
90a3de1CompareSigned-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
90a3de1 to
54ca7a9CompareSigned-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 DSLcomposite-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 Providerlsp/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 Infrastructurelsp/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 Configurationsubprojects/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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Uh oh!
There was an error while loading. Please reload this page.
* fix: add ability to re-write class name references in desugar plugin * feat: integrate Kotlin analysis API * fix: update kotlin-android to latest version fixes duplicate class errors for org.antrl.v4.* classes * fix: remove UnsafeImpl It is now included in the embeddable JAR (named UnsafeAndroid) with proper relocations. * fix: update kotlin-android to latest version * fix: replace usages of Unsafe with UnsafeImpl * fix: make Kotlin LSP no-op * feat: configure K2 standalone session when setting up LSP * fix: JvmTarget resolution fails for input "21" * fix: do not early-init VirtualFileSystem * fix: remove replaceClass desugar instruction for Unsafe * fix: ensure boot class path is added as dependency to Android modules * feat: add diagnostic provider for Kotlin * fix: remove unnecessary log statement * fix: update to latest kotlin-android release * fix: always re-initialize K2 session on setupWithProject * fix: diagnostics are always collected from the on-disk file * fix: remove unused class Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
See ADFA-3319 for more details.
Must be merged after: