ADFA-2417: Add LSP support for Kotlin#907
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Kotlin LSP subsystem (parser, syntax/tree model, symbol model, indexing, semantic analysis, server and LSP providers), integrates it into the app/editor (diagnostics UI and clipboard copy), and updates build configuration and .gitignore for the new Kotlin LSP modules and generated stdlib index. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (User)
participant TDS as KotlinTextDocumentService
participant Scheduler as AnalysisScheduler
participant Parser as KotlinParser
participant Builder as SymbolBuilder
participant Analyzer as SemanticAnalyzer
participant Index as ProjectIndex
participant Client as LanguageClient
Editor->>TDS: didOpen / didChange(uri, content)
TDS->>Scheduler: scheduleAnalysis(uri)
Scheduler->>Parser: parse(source)
Parser-->>Scheduler: ParseResult (SyntaxTree)
Scheduler->>Builder: build symbol table (SyntaxTree)
Builder-->>Scheduler: SymbolTable
Scheduler->>Analyzer: analyze(SymbolTable, SyntaxTree)
Analyzer-->>Scheduler: Diagnostics, FileIndex
Scheduler->>Index: update ProjectIndex with FileIndex
Scheduler->>Client: publishDiagnostics(uri, diagnostics)
Editor->>TDS: completion/hover/definition request
TDS->>Index: query (ProjectIndex/AnalysisContext)
Index-->>TDS: results
TDS-->>Editor: LSP response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](/s/com/githubusercontent/avatars/G.https/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/IndexEntry.kt`:
- Around line 245-333: ClassEntry.companions are never emitted or counted:
update ClassEntry.toIndexedSymbols() to iterate companions and add IndexedSymbol
entries just like members (use name = member.name, fqName =
"$fqName.${member.name}", kind = parseKind(member.kind), packageName =
packageName, containingClass = fqName, signature = member.sig, parameters =
member.params.map { it.toIndexedParameter() }, returnType = member.ret,
visibility = parseVisibility(member.vis), deprecated = member.dep) and update
StdlibIndexData.totalCount to include companions (add it to the class member
count when iterating classes.values, e.g. count += it.members.size +
it.companions.size) so companion APIs are indexed and counted.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ProjectIndex.kt`:
- Around line 108-119: The current deduplication in getAllFunctions() using
distinctBy { it.fqName } collapses overloaded functions; update
getAllFunctions() (and similarly findBySimpleName(), findByPackage(),
findByPrefix(), getAllProperties(), getAllClasses(), findExtensions()) to avoid
losing overloads by either removing the fqName-only dedupe or change the dedupe
key to include signature/parameter info (e.g., use a composite key like
"${it.fqName}|${it.signature}" or a unique identifier field on IndexedSymbol);
ensure aggregation from fileIndexes, stdlibIndex, and classpathIndex still
merges sources but preserves distinct overload entries rather than collapsing by
fqName alone.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/AnalysisScheduler.kt`:
- Around line 154-159: analyzeSync uses runBlocking(analysisDispatcher) which
deadlocks if called on the same single-threaded dispatcher; update analyzeSync
to detect when the current thread is the analysis thread and in that case call
performAnalysis(state) directly, otherwise continue to use
runBlocking(analysisDispatcher) { performAnalysis(state) }; reference
analysisDispatcher, analyzeSync, and performAnalysis and, if needed, capture the
analysis thread when creating analysisDispatcher (e.g. store Thread in a field)
to perform the thread equality check.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/KotlinLanguageServer.kt`:
- Around line 110-113: The loadStdlibIndex(InputStream) method currently leaves
the InputStream open; modify loadStdlibIndex to ensure the stream is closed
after loading (e.g., wrap the call to
StdlibIndexLoader.loadFromStream(inputStream) in a Kotlin use{} block or a
try-with-resources equivalent) so the InputStream is closed even on exceptions,
then set the result on projectIndex via
projectIndex.setStdlibIndex(stdlibIndex).
- Around line 229-235: The exit() method in KotlinLanguageServer currently calls
exitProcess(0/1) which will kill the embedding IDE; instead remove direct
exitProcess calls in KotlinLanguageServer.exit() and either invoke a provided
host callback (e.g., a nullable shutdownHandler or onExit lambda you add to the
class) or guard the exit with an "outOfProcess" flag so exitProcess is only
called for standalone deployments; update references to shutdownRequested and
ensure exit() calls the host-provided callback or sets a status/result for the
host to act on rather than calling exitProcess directly.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/KotlinTextDocumentService.kt`:
- Around line 62-75: The logs in KotlinTextDocumentService.kt currently print
raw document text for each change (in the loop over params.contentChanges and
the Log.d calls), which can leak user code and grow logs; replace those
Log.d(...) invocations to avoid logging change.text directly—log only safe
metadata such as change.text.length, a capped preview (e.g., first N chars) or a
hash, and keep context (index, range, version); update both the full-sync branch
where documentManager.update(...) is called and the incremental branch around
documentManager.applyChange(...) to use the sanitized message instead of the raw
text.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/DefinitionProvider.kt`:
- Around line 118-131: The Location constructors are being passed plain
filesystem paths (ref.filePath / location.filePath) instead of URIs; update the
loop in DefinitionProvider (the block using projectIndex.findSymbolReferences
and creating Location) to call DocumentState.pathToUri(ref.filePath) when
constructing Location, and likewise fix the other occurrences by converting
paths to URIs in symbolToLocation and indexedSymbolToLocation methods in
DefinitionProvider and where KotlinWorkspaceService passes
location.filePath/filePath to Location so all Location(...) calls receive a URI
string.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt`:
- Around line 36-46: The literal branch in the hoverContent when-expression is
unreachable because node.kind.isKeyword matches literals first; update the when
order so literals are checked before keywords (i.e., move the
node.kind.isLiteral branch above node.kind.isKeyword) or alter the isKeyword
check to exclude literals; specifically modify the when in HoverProvider.kt
(hoverContent, node.kind) so getLiteralHover(node) can run before
getKeywordHover(node.kind) while keeping the SIMPLE_IDENTIFIER branch intact.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/ScopeKind.kt`:
- Around line 162-178: The canContainTypes and canContainProperties getters
currently omit ENUM, causing nested enum types/properties to be ignored; update
the boolean logic in the properties canContainTypes and canContainProperties to
include ENUM (same as isClassScope's treatment of ENUM) so enums are allowed to
contain nested type declarations and properties; locate the properties named
canContainTypes and canContainProperties in ScopeKind and add ENUM to their
membership checks.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/Symbol.kt`:
- Around line 599-618: The render() currently appends the nullable marker after
a function type without parentheses, producing invalid Kotlin for nullable
function types; update render() so when functionTypeInfo != null and isNullable
is true you wrap the entire function type in parentheses before appending '?'
(e.g., produce "((...)->...)?" rather than "(...)->...?"); implement this by
emitting the opening parenthesis before rendering the function type and the
closing parenthesis after it when functionTypeInfo != null && isNullable, then
append '?' and keep the existing type-argument/nullability behavior for the
non-function branch unchanged.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/SymbolBuilder.kt`:
- Around line 48-58: The debug Log.d calls in SymbolBuilder.build (and other
methods in SymbolBuilder) unconditionally construct and emit source/S-expression
snippets which can leak user code and hurt performance; wrap these logging sites
with a guard that checks logging is enabled (e.g.,
android.util.Log.isLoggable("SymbolBuilder", android.util.Log.DEBUG) or a new
private helper method like shouldDebug() / debugLog { ... }) and only build the
debug string inside the guarded block or lambda so strings aren’t constructed
when debug is off; apply the same guarded-logging pattern to the other Log.d
calls in this class to avoid PII/perf risk.
🟡 Minor comments (29)
app/src/main/java/com/itsaky/androidide/utils/DiagnosticsFormatter.kt-14-14 (1)
14-14: Thread safety:SimpleDateFormatis not thread-safe.
SimpleDateFormatis shared at the object level and can produce incorrect results or throw exceptions whenformat()is called concurrently from multiple threads.🛡️ Proposed fix: Create a new instance per call or use thread-safe alternative
object DiagnosticsFormatter { - private val dateFormat = SimpleDateFormat("yyyy-MM-dd HH:mm:ss", Locale.US) + private const val DATE_FORMAT_PATTERN = "yyyy-MM-dd HH:mm:ss" fun format(diagnostics: Map<File, List<DiagnosticItem>>): String { if (diagnostics.isEmpty()) return "No diagnostics" val builder = StringBuilder() builder.appendLine("=== Diagnostics Report ===") - builder.appendLine("Generated: ${dateFormat.format(Date())}") + builder.appendLine("Generated: ${SimpleDateFormat(DATE_FORMAT_PATTERN, Locale.US).format(Date())}")app/src/main/res/layout/layout_editor_bottom_sheet.xml-101-112 (1)
101-112: MissingcontentDescriptionfor accessibility.The FAB should have a
contentDescriptionattribute to support screen readers. However, the proposed string resource@string/desc_copy_diagnosticsdoes not exist and must be created. Also note that the other FABs in this file (clear_fab,share_output_fab) are also missing contentDescription attributes for consistency.♿ Proposed fix: Add contentDescription
<com.google.android.material.floatingactionbutton.FloatingActionButton android:id="@+id/copy_diagnostics_fab" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_above="@id/share_output_fab" android:layout_alignParentEnd="true" android:layout_marginStart="16dp" android:layout_marginTop="16dp" android:layout_marginEnd="16dp" android:layout_marginBottom="16dp" android:src="@drawable/ic_copy" - android:visibility="gone" /> + android:visibility="gone" + android:contentDescription="@string/desc_copy_diagnostics" />Add the string resource to values/strings.xml:
<string name="desc_copy_diagnostics">Copy diagnostics</string>lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ProjectIndex.kt-139-149 (1)
139-149: Race condition inupdateFilebetween removal and addition to secondary indexes.The sequence
put→removeFromPackageIndex(old)→addToPackageIndex(new)is not atomic. A concurrent read between removal and addition could miss the symbol. Consider using a lock or performing operations in a single synchronized block if strict consistency is required.lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/DiagnosticProvider.kt-104-110 (1)
104-110: Adjacent ranges are treated as overlapping.With half‑open ranges,
end == startshould be non‑overlapping. Current comparisons include adjacency, which can return diagnostics outside the requested range.✅ Suggested fix (treat adjacency as non-overlap)
- if (a.end.line == b.start.line && a.end.character < b.start.character) return false - if (a.start.line == b.end.line && a.start.character > b.end.character) return false + if (a.end.line == b.start.line && a.end.character <= b.start.character) return false + if (a.start.line == b.end.line && a.start.character >= b.end.character) return falselsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/SymbolBuilder.kt-1684-1686 (1)
1684-1686: Swallowed exception hides parsing issues.
extractTypecatchesExceptionand returns null without logging, which makes failures hard to debug and triggers the detekt warning. At least log the exception (ideally via the guarded logger).✅ Suggested fix (log the exception)
- } catch (e: Exception) { - null - } + } catch (e: Exception) { + android.util.Log.w(logTag, "extractType failed for ${node.kind}", e) + null + }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/parser/KotlinParser.kt-74-79 (1)
74-79: Wire the actual ABI version fromTSLanguageKotlinor remove this property.The
languageVersionproperty documents tree-sitter language version for "debugging version mismatches," but tree-sitter only exposes an ABI version (not a Kotlin version). Returning a hardcoded 0 is misleading. Either retrieve the ABI version from the language bindings or drop the property if it's unused.lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/SymbolIndex.kt-289-289 (1)
289-289: Incorrect val/var distinction logic.The condition
returnType != nulldoesn't correctly distinguish betweenvalandvarproperties—both have return types. TheIndexedSymbollacks anisVarfield. CurrentlytoSyntheticSymbolhardcodesisVar = false(line 365). Consider adding anisVarfield toIndexedSymbolif mutable property tracking is needed.lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/PackageIndex.kt-68-71 (1)
68-71: Duplicate handling inconsistency between maps.When
add()is called with a symbol whosefqNamealready exists,fqNameMapoverwrites the entry (correct), butpackageMapappends a new entry (creates duplicate). This leads to inconsistent state wheresize(fromfqNameMap) doesn't match actual symbol count inpackageMap. TheClasspathIndex.addSymbol(lines 75-78) guards against this.🛡️ Proposed fix to handle duplicates
override fun add(symbol: IndexedSymbol) { + val existing = fqNameMap[symbol.fqName] + if (existing != null) { + packageMap[existing.packageName]?.remove(existing) + } fqNameMap[symbol.fqName] = symbol packageMap.getOrPut(symbol.packageName) { mutableListOf() }.add(symbol) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/parser/SyntaxTree.kt-69-72 (1)
69-72: Documentation says "bytes" but implementation uses characters.The
sourceLengthproperty is documented as "length of the source text in bytes" butsource.lengthreturns the character count. For ASCII-only sources these are equal, but for UTF-8 encoded sources with multi-byte characters, they differ. Tree-sitter typically operates on byte offsets. Clarify the intended semantics.📝 Suggested documentation fix if character-based is intentional
/** - * The length of the source text in bytes. + * The length of the source text in characters. */ val sourceLength: Int get() = source.lengthOr if byte-based is needed:
- val sourceLength: Int get() = source.length + val sourceLength: Int get() = source.toByteArray(Charsets.UTF_8).sizelsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/parser/SyntaxTree.kt-129-150 (1)
129-150: Byte/character offset confusion inpositionFromOffset.The documentation refers to "byte offset" but the implementation treats the offset as a character index (iterating
sourceby character). This discrepancy could cause incorrect position calculations for source files containing multi-byte UTF-8 characters (e.g., comments or strings with non-ASCII characters).📝 Suggested documentation correction
If the intention is character-based offsets:
/** - * Converts a byte offset to a [Position]. + * Converts a character offset to a [Position]. * - * `@param` byteOffset Byte offset in the source + * `@param` byteOffset Character offset in the source * `@return` Position (line and column) */ - fun positionFromOffset(byteOffset: Int): Position { + fun positionFromOffset(offset: Int): Position {lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/StdlibIndexLoader.kt-182-190 (1)
182-190: Incorrect superTypes for Boolean and Char primitives.All primitives are assigned
superTypes = listOf("kotlin.Number", "kotlin.Comparable<...>"), butBooleanandChardo not extendkotlin.Numberin Kotlin. This could lead to incorrect type inference or resolution results downstream.🐛 Proposed fix
private fun addPrimitiveTypes(index: StdlibIndex) { - val primitives = listOf( + val numericPrimitives = listOf( "Int" to "32-bit signed integer", "Long" to "64-bit signed integer", "Short" to "16-bit signed integer", "Byte" to "8-bit signed integer", "Float" to "32-bit floating point", - "Double" to "64-bit floating point", - "Boolean" to "Boolean value (true/false)", - "Char" to "16-bit Unicode character" + "Double" to "64-bit floating point" ) - for ((name, _) in primitives) { + for ((name, _) in numericPrimitives) { index.addSymbol(IndexedSymbol( name = name, fqName = "kotlin.$name", kind = IndexedSymbolKind.CLASS, packageName = "kotlin", superTypes = listOf("kotlin.Number", "kotlin.Comparable<kotlin.$name>") )) } + + // Boolean and Char don't extend Number + index.addSymbol(IndexedSymbol( + name = "Boolean", + fqName = "kotlin.Boolean", + kind = IndexedSymbolKind.CLASS, + packageName = "kotlin", + superTypes = listOf("kotlin.Comparable<kotlin.Boolean>") + )) + + index.addSymbol(IndexedSymbol( + name = "Char", + fqName = "kotlin.Char", + kind = IndexedSymbolKind.CLASS, + packageName = "kotlin", + superTypes = listOf("kotlin.Comparable<kotlin.Char>") + )) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/AnalysisCache.kt-87-94 (1)
87-94: Potential race condition in eviction logic.The
evictIfNeededmethod is not atomic: between checkingcache.size >= maxCacheSizeand finding/removing the oldest entry, concurrent modifications can occur. Additionally,minByOrNulliterates the entire map which is not guaranteed to be consistent under concurrent modification. Consider synchronizing or using a more robust concurrent eviction strategy.🔒 Potential mitigation
private fun <V : Timestamped> evictIfNeeded(cache: ConcurrentHashMap<String, V>) { - if (cache.size >= maxCacheSize) { - val oldestEntry = cache.entries - .minByOrNull { it.value.timestamp } - ?.key - oldestEntry?.let { cache.remove(it) } + // Best-effort eviction under concurrent access + while (cache.size >= maxCacheSize) { + val snapshot = cache.entries.toList() + val oldest = snapshot.minByOrNull { it.value.timestamp }?.key ?: break + if (cache.remove(oldest) != null) break } }Alternatively, consider using a bounded cache implementation like Caffeine or Guava's Cache with built-in eviction policies.
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/FileIndex.kt-75-87 (1)
75-87:add()may create duplicate entries insymbolsByName.If the same symbol is added twice (e.g., during re-indexing),
symbolsByNamewill contain duplicates becausegetOrPutappends without checking if the symbol already exists in the list.🛡️ Proposed fix to prevent duplicates
override fun add(symbol: IndexedSymbol) { + // Remove existing entry if present to prevent duplicates + symbolsByFqName[symbol.fqName]?.let { existing -> + symbolsByName[existing.name]?.remove(existing) + } symbolsByFqName[symbol.fqName] = symbol symbolsByName.getOrPut(symbol.name) { mutableListOf() }.add(symbol) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/KotlinWorkspaceService.kt-134-145 (1)
134-145: Potential concurrent modification when closing documents during folder removal.Iterating
documentManager.openUriswhile callingclose(uri)inside the loop could causeConcurrentModificationExceptionifopenUrisreturns a live collection view.🛡️ Proposed fix to avoid concurrent modification
private fun handleWorkspaceFolderRemoved(folder: WorkspaceFolder) { val folderUri = folder.uri - for (uri in documentManager.openUris) { - if (uri.startsWith(folderUri)) { + val urisToClose = documentManager.openUris.filter { it.startsWith(folderUri) } + for (uri in urisToClose) { documentManager.close(uri) projectIndex.removeFile(uri) - } } client?.logMessage(MessageParams(MessageType.Info, "Workspace folder removed: ${folder.uri}")) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/DocumentState.kt-101-108 (1)
101-108: Debug logging may expose document content in production.The verbose
Log.dcalls output deleted text and content near edits. While useful for debugging, consider guarding these with a debug flag or using a lower log level to avoid exposing document content in production logs.🛡️ Suggested approach
+private const val DEBUG_SYNC = false // Set to true for detailed sync debugging + fun applyChange(...) { // ... offset calculations ... - Log.d(TAG, "[DOC-SYNC] applyChange: uri=$uri, range=$startLine:$startChar-$endLine:$endChar") - Log.d(TAG, "[DOC-SYNC] offsets: start=$startOffset, end=$endOffset, contentLength=$oldLength") - Log.d(TAG, "[DOC-SYNC] deleted='$deletedText', newText='$newText' (${newText.length} chars)") + if (DEBUG_SYNC) { + Log.d(TAG, "[DOC-SYNC] applyChange: uri=$uri, range=$startLine:$startChar-$endLine:$endChar") + Log.d(TAG, "[DOC-SYNC] offsets: start=$startOffset, end=$endOffset, contentLength=$oldLength") + Log.d(TAG, "[DOC-SYNC] deleted='$deletedText', newText='$newText' (${newText.length} chars)") + }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/CompletionProvider.kt-669-704 (1)
669-704: Commas inside string literals may cause incorrect argument index calculation.
findCallContextcounts commas to determineargumentIndex, but doesn't skip commas inside string literals. For example,println("a,b,c", x)would reportargumentIndex=3instead of1.🐛 Proposed fix to handle string literals
private fun findCallContext(content: String, offset: Int): CallContext? { var parenDepth = 0 var argIndex = 0 var functionStart = -1 + var inString = false + var stringChar: Char? = null for (i in (offset - 1) downTo 0) { + val c = content[i] + + // Track string boundaries (simplified - doesn't handle escapes perfectly) + if ((c == '"' || c == '\'') && (i == 0 || content[i - 1] != '\\')) { + if (inString && c == stringChar) { + inString = false + stringChar = null + } else if (!inString) { + inString = true + stringChar = c + } + } + + if (inString) continue + - when (content[i]) { + when (c) { ')' -> parenDepth++ '(' -> {lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ClasspathIndexer.kt-63-65 (1)
63-65: Silently swallowed exceptions lose diagnostic context.Per static analysis hints, the exceptions at lines 63-65 (JAR indexing) and 87-89 (AAR indexing) are swallowed without any logging. While skipping corrupted archives is reasonable, consider logging at debug level to aid troubleshooting when symbols are unexpectedly missing.
🔧 Proposed fix to add debug logging
} catch (e: Exception) { - // Silently skip corrupted jars + // Skip corrupted jars but log for debugging + android.util.Log.d("ClasspathIndexer", "Failed to index JAR: ${jarFile.name}", e) }} catch (e: Exception) { - // Silently skip corrupted aars + // Skip corrupted AARs but log for debugging + android.util.Log.d("ClasspathIndexer", "Failed to index AAR: ${aarFile.name}", e) }Also applies to: 87-89
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ClasspathIndexer.kt-527-537 (1)
527-537: Missing bounds checks inreadU2andreadU4could cause crashes.These methods access
bytes[offset]throughbytes[offset + 3]without verifying the array bounds first. Malformed class files could causeArrayIndexOutOfBoundsExceptionthat propagates up.🛡️ Proposed defensive bounds check
private fun readU2(bytes: ByteArray, offset: Int): Int { + if (offset + 1 >= bytes.size) return 0 return ((bytes[offset].toInt() and 0xFF) shl 8) or (bytes[offset + 1].toInt() and 0xFF) } private fun readU4(bytes: ByteArray, offset: Int): Int { + if (offset + 3 >= bytes.size) return 0 return ((bytes[offset].toInt() and 0xFF) shl 24) or ((bytes[offset + 1].toInt() and 0xFF) shl 16) or ((bytes[offset + 2].toInt() and 0xFF) shl 8) or (bytes[offset + 3].toInt() and 0xFF) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/StdlibIndex.kt-194-207 (1)
194-207: Missing duplicate check inaddSymbol— potential for duplicate entries.Unlike
ClasspathIndex.addSymbol(shown in relevant snippets at lines 74-86), this implementation doesn't check if a symbol with the samefqNamealready exists before adding. This could lead to duplicate entries inallSymbolsif the same symbol is added multiple times.🔧 Proposed fix to add duplicate check
internal fun addSymbol(symbol: IndexedSymbol) { + if (symbolsByFqName.containsKey(symbol.fqName)) { + return + } + allSymbols.add(symbol) symbolsByFqName[symbol.fqName] = symbollsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/semantic/TypeInferrer.kt-906-950 (1)
906-950: Dead code:inferCallableReferenceis never invoked.The
inferCallableReferencemethod is fully implemented but is never called frominferTypeImpl. There's noSyntaxKind.CALLABLE_REFERENCEcase in the dispatch. Either add the missing case or remove this dead code.🔧 To enable callable reference inference, add to inferTypeImpl
SyntaxKind.COLLECTION_LITERAL -> inferCollectionLiteral(node, scope, expectedType) + SyntaxKind.CALLABLE_REFERENCE -> inferCallableReference(node, scope) + SyntaxKind.ASSIGNMENT -> ClassType.UNITlsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/parser/ParseResult.kt-280-289 (1)
280-289: Multi-line insertions not handled innewEndPositioncalculation.The
insertionfactory calculatesnewEndPositionby addinginsertedLengthto the column, which is incorrect for multi-line text insertions (e.g., inserting"hello\nworld"). The new end position should account for line breaks in the inserted content.🔧 Suggested approach
Consider accepting the actual
newEndPositionas a parameter, or document that this helper is only for single-line insertions:fun insertion(position: Position, byteOffset: Int, insertedLength: Int): SourceEdit { + // Note: This assumes single-line insertion. For multi-line insertions, + // use the primary constructor with explicit newEndPosition. return SourceEdit(Or refactor to accept the end position:
- fun insertion(position: Position, byteOffset: Int, insertedLength: Int): SourceEdit { + fun insertion(position: Position, byteOffset: Int, insertedLength: Int, newEndPosition: Position? = null): SourceEdit { return SourceEdit( startByte = byteOffset, oldEndByte = byteOffset, newEndByte = byteOffset + insertedLength, startPosition = position, oldEndPosition = position, - newEndPosition = position.copy(column = position.column + insertedLength) + newEndPosition = newEndPosition ?: position.copy(column = position.column + insertedLength) ) }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/CodeActionProvider.kt-165-194 (1)
165-194: Unused variablesymbolTableand fragilevalkeyword detection.
symbolTableis fetched on line 171 but never used — it can be removed.- The
findValDeclarationmethod usesindexOf("val ")which could match substrings in identifiers (e.g.,interval,approval). This could cause incorrect edits.🔧 Proposed fixes
private fun createChangeToVarAction( uri: String, diagnostic: Diagnostic, content: String ): List<CodeAction> { - val state = documentManager.get(uri) ?: return emptyList() - val symbolTable = state.symbolTable ?: return emptyList() - val position = diagnostic.range.start val lines = content.lines() if (position.line >= lines.size) return emptyList()For more robust
valdetection:private fun findValDeclaration(content: String, startLine: Int, startChar: Int): Pair<Int, Int>? { val lines = content.lines() + val valPattern = Regex("""(^|\s)val\s""") for (line in startLine downTo maxOf(0, startLine - 20)) { val lineContent = lines.getOrNull(line) ?: continue - val valIndex = lineContent.indexOf("val ") - if (valIndex >= 0) { - return line to valIndex + val match = valPattern.find(lineContent) + if (match != null) { + val valIndex = match.range.first + (if (match.groupValues[1].isNotEmpty()) 1 else 0) + return line to valIndex } }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/semantic/SemanticAnalyzer.kt-143-146 (1)
143-146: Remove debug logging before release.Multiple
android.util.Log.dcalls throughout this file (lines 143-146, 160-168, 343, 357, 364, 368, 392, 395, 447-459, 463, 466, 472) are Android-specific and inappropriate for an LSP core module. These should be removed or replaced with a cross-platform logging abstraction.lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/DocumentManager.kt-200-204 (1)
200-204: Thecharacterparameter is unused.The
getAtPositionmethod accepts acharacterparameter but never uses it. Either validate the character position against the line length, or remove the parameter if character-level validation is not needed.Proposed fix (option 1: use the parameter)
fun getAtPosition(uri: String, line: Int, character: Int): DocumentState? { val state = documents[uri] ?: return null if (line < 0 || line >= state.lineCount) return null + val lineLength = state.getLineLength(line) + if (character < 0 || character > lineLength) return null return state }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/SemanticTokenProvider.kt-330-356 (1)
330-356: Multiple force-unwraps (!!) oncreateTokenresults could cause NPE.
createTokenreturnsnullwhen the node text is empty (line 416). The force-unwraps at lines 341, 345, 349, 351-354 assume non-null but could crash if symbols have empty names.Proposed fix
private fun classifyFromSymbol(node: SyntaxNode, symbol: Symbol): SemanticToken? { return when (symbol) { is ClassSymbol -> { val kind = when { symbol.isInterface -> TYPE_INTERFACE symbol.isEnum -> TYPE_ENUM symbol.isObject -> TYPE_CLASS symbol.isAnnotation -> TYPE_DECORATOR else -> TYPE_CLASS } val mods = if (symbol.modifiers.isAbstract) MOD_ABSTRACT else 0 - createToken(node, kind, mods)!! + createToken(node, kind, mods) } is FunctionSymbol -> { val mods = buildFunctionModifiers(symbol) - createToken(node, TYPE_FUNCTION, mods)!! + createToken(node, TYPE_FUNCTION, mods) } is PropertySymbol -> { val mods = if (!symbol.isVar) MOD_READONLY else 0 - createToken(node, TYPE_PROPERTY, mods)!! + createToken(node, TYPE_PROPERTY, mods) } - is ParameterSymbol -> createToken(node, TYPE_PARAMETER, 0)!! - is TypeParameterSymbol -> createToken(node, TYPE_TYPE_PARAMETER, 0)!! - is TypeAliasSymbol -> createToken(node, TYPE_TYPE, 0)!! - else -> createToken(node, TYPE_VARIABLE, 0)!! + is ParameterSymbol -> createToken(node, TYPE_PARAMETER, 0) + is TypeParameterSymbol -> createToken(node, TYPE_TYPE_PARAMETER, 0) + is TypeAliasSymbol -> createToken(node, TYPE_TYPE, 0) + else -> createToken(node, TYPE_VARIABLE, 0) } }Note: This changes the return type to
SemanticToken?.lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ExtensionIndex.kt-68-78 (1)
68-78: Theremovemethod always returnstrueeven if the extension wasn't found.The method returns
trueunconditionally after line 77, regardless of whether the extension was actually present inallExtensions. This could mislead callers.Proposed fix
fun remove(extension: IndexedSymbol): Boolean { if (extension.receiverType == null) return false - allExtensions.remove(extension) + val removed = allExtensions.remove(extension) + if (!removed) return false + val normalizedReceiver = normalizeType(extension.receiverType) extensionsByReceiver[normalizedReceiver]?.remove(extension) if (extensionsByReceiver[normalizedReceiver]?.isEmpty() == true) { extensionsByReceiver.remove(normalizedReceiver) } return true }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/SymbolTable.kt-159-170 (1)
159-170: Remove debug logging statements before release.The
android.util.Logcalls are Android-specific and will fail on non-Android JVMs. Since this is an LSP core module, it should be platform-agnostic. These debug logs should be removed or replaced with a platform-independent logging framework.Proposed fix
fun allVisibleSymbols(position: Position): List<Symbol> { - android.util.Log.d("SymbolTable", "allVisibleSymbols: position=$position") - android.util.Log.d("SymbolTable", "fileScope.range=${fileScope.range}, children=${fileScope.children.size}") - for (child in fileScope.children) { - android.util.Log.d("SymbolTable", " child scope: kind=${child.kind}, range=${child.range}, symbols=${child.allSymbols.map { it.name }}") - } val scope = scopeAt(position) ?: fileScope - android.util.Log.d("SymbolTable", "found scope: kind=${scope.kind}, range=${scope.range}") val symbols = scope.collectAll { true } - android.util.Log.d("SymbolTable", "collectAll returned ${symbols.size} symbols: ${symbols.map { it.name }}") return symbols }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/semantic/OverloadResolver.kt-328-346 (1)
328-346: Add missing primitive array type handlers inresolveVarargElementType.The method only handles
kotlin.Array,kotlin.IntArray, andkotlin.LongArray. Kotlin also hasDoubleArray,FloatArray,ByteArray,ShortArray,CharArray, andBooleanArray. When these types are not recognized, the method falls back to returningClassType.ANY, causing loss of type information for vararg parameter compatibility checks.Note: The codebase already handles all these array types in
TypeInferrer(semantic type inference), so this gap inOverloadResolvercreates inconsistency.Proposed fix
private fun resolveVarargElementType( param: ParameterSymbol, function: FunctionSymbol, scope: Scope? ): KotlinType { val arrayType = resolveParamType(param, function, scope) return when { arrayType is ClassType && arrayType.fqName == "kotlin.Array" -> { arrayType.typeArguments.firstOrNull()?.type ?: ClassType.ANY } arrayType is ClassType && arrayType.fqName == "kotlin.IntArray" -> PrimitiveType.INT arrayType is ClassType && arrayType.fqName == "kotlin.LongArray" -> PrimitiveType.LONG + arrayType is ClassType && arrayType.fqName == "kotlin.DoubleArray" -> PrimitiveType.DOUBLE + arrayType is ClassType && arrayType.fqName == "kotlin.FloatArray" -> PrimitiveType.FLOAT + arrayType is ClassType && arrayType.fqName == "kotlin.ByteArray" -> PrimitiveType.BYTE + arrayType is ClassType && arrayType.fqName == "kotlin.ShortArray" -> PrimitiveType.SHORT + arrayType is ClassType && arrayType.fqName == "kotlin.CharArray" -> PrimitiveType.CHAR + arrayType is ClassType && arrayType.fqName == "kotlin.BooleanArray" -> PrimitiveType.BOOLEAN else -> ClassType.ANY } }lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/Symbol.kt-471-477 (1)
471-477: FixeffectiveBoundto match its documented contract.The comment states "first bound or Any?", but the implementation returns
nullwhen no bounds exist. This contradicts Kotlin's type system semantics, where unbounded type parameters implicitly haveAny?as their upper bound. A parallel implementation inKotlinType.ktalready does this correctly.Fix
- val effectiveBound: TypeReference? - get() = bounds.firstOrNull() + val effectiveBound: TypeReference? + get() = bounds.firstOrNull() ?: ANY_NULLABLE
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/IndexEntry.kt
Show resolved
Hide resolved
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/index/ProjectIndex.kt
Show resolved
Hide resolved
...otlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/AnalysisScheduler.kt
Show resolved
Hide resolved
...in-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/KotlinLanguageServer.kt
Show resolved
Hide resolved
...in-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/KotlinLanguageServer.kt
Show resolved
Hide resolved
...src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/DefinitionProvider.kt
Show resolved
Hide resolved
...core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt
Show resolved
Hide resolved
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/ScopeKind.kt
Show resolved
Hide resolved
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/Symbol.kt
Show resolved
Hide resolved
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/SymbolBuilder.kt
Show resolved
Hide resolved
|
@Daniel-ADFA If this is still in progress, please mark it as a draft. If it's ready for review, please split the changes into multiple PRs so that we can review it. |
app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt
Outdated
Show resolved
Hide resolved
editor/src/main/java/com/itsaky/androidide/editor/ui/EditorEventDispatcher.kt
Show resolved
Hide resolved
subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt
Show resolved
Hide resolved
…imental # Conflicts: # .gitignore # resources/src/main/res/values/strings.xml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt`:
- Around line 549-553: The clipboard copy sequence around
DiagnosticsFormatter.format(diagnostics) should be made defensive: obtain the
ClipboardManager with a safe cast (use
getSystemService(Context.CLIPBOARD_SERVICE) as? ClipboardManager) and check for
null before calling setPrimaryClip, and wrap setPrimaryClip in a try-catch that
catches SecurityException (and general Exception as a fallback); on failure call
the existing flashError/flashSuccess equivalent with a new error string resource
(e.g., msg_clipboard_copy_failed) instead of reusing msg_no_diagnostics_to_copy
so the user sees a distinct clipboard-failure message and the code won't crash.
app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt`:
- Around line 266-279: parseIntegerLiteral currently doesn't strip unsigned
suffixes so literals like 42u/42UL end up throwing NumberFormatException; update
parseIntegerLiteral to normalize and strip unsigned suffixes before parsing:
convert the literal to lowercase (or otherwise case-normalize), remove
underscores, then remove suffixes "ul" (or "lu" if you prefer to handle both
orders), "u" and "l" (in that order) so both "42u", "42UL", "42uL" are reduced
to the numeric digits, then proceed with the existing hex (startsWith("0x")) and
binary (startsWith("0b")) parsing paths and keep the same catch fallback;
reference parseIntegerLiteral and the cleaned variable when making the change.
- Around line 99-103: The lookup uses projectIndex.findByFqName(name) and a
limited findByPrefix fallback which both fail for simple identifiers; replace
that logic with a simple-name search by calling
projectIndex.findBySimpleName(name).firstOrNull() to populate indexedSymbol,
then return formatIndexedSymbolHover(indexedSymbol) as before; update the code
paths in HoverProvider (the indexedSymbol variable and the return that calls
formatIndexedSymbolHover) to use the findBySimpleName result.
In
`@lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/Symbol.kt`:
- Around line 474-477: The KDoc for effectiveBound is misleading: it claims
"first bound or Any?" but the getter (effectiveBound) currently returns null
when bounds is empty; update the implementation to match the doc by returning a
TypeReference that represents Any? when bounds.firstOrNull() is null (i.e.,
return bounds.firstOrNull() ?: a TypeReference for Any?), or alternatively
update the KDoc to say "or null" if you prefer nullable behavior—locate the
effectiveBound getter in Symbol.kt and either create/return the existing project
representation of Any? as a TypeReference or change the KDoc to accurately state
that it may return null.
🧹 Nitpick comments (2)
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt (2)
36-48: Previous literal/keyword ordering issue is resolved.The
isLiteralbranch now correctly precedes theisKeywordbranch, so literal hover is reachable. However, note thatisKeyword(lines 324-327) still returnstruefor literal kinds likeBOOLEAN_LITERALandINTEGER_LITERALbecause their enum names are all-uppercase with underscores. This is currently harmless due to the ordering, but it makes thewhenfragile — any future refactor that reorders or adds branches could reintroduce the bug.Consider tightening
isKeywordto explicitly exclude literals, or adding!isLiteralto the guard:Suggested hardening
private val SyntaxKind.isKeyword: Boolean get() = name.all { it.isUpperCase() || it == '_' } && this != SyntaxKind.SIMPLE_IDENTIFIER && - this != SyntaxKind.ERROR + this != SyntaxKind.ERROR && + !isLiteral
64-83: Extract duplicate utility methods to shared module.
findNodeAtPositionandcontainsPositionare duplicated in both HoverProvider.kt and DefinitionProvider.kt with identical implementations. Consider extracting these into a shared utility (e.g.,SyntaxNodeUtils.ktor extension functions) to prevent drift between the copies.
...core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt
Show resolved
Hide resolved
...core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/server/providers/HoverProvider.kt
Show resolved
Hide resolved
lsp/kotlin-core/src/main/java/org/appdevforall/codeonthego/lsp/kotlin/symbol/Symbol.kt
Show resolved
Hide resolved
2 participants
Add Kotlin LSP with semantic analysis, type inference, and stdlib indexing
Implement a full Kotlin Language Server built on tree-sitter parsing with:
document symbols, signature help, semantic tokens, code actions, diagnostics
kotlin-stdlib-generatormodule