Skip to content

Commit f230235

Browse files
committed
chore: simplify comments
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
1 parent eedc5f7 commit f230235

4 files changed

Lines changed: 27 additions & 45 deletions

File tree

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ internal class CompilationEnvironment(
249249

250250
project.write {
251251
// Must run under the write lock so the session mutation can't race a concurrent
252-
// `analyze` (which only holds the read lock); see onFileContentChanged.
252+
// `analyze` (which only holds the read lock); see KtSymbolIndex.refreshToCurrent.
253253
if (ktFile != null) {
254254
KaSourceModificationService.getInstance(project)
255255
.handleElementModification(ktFile, typeProvider(ktFile))

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt‎

Lines changed: 20 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,9 @@ internal class KtSymbolIndex(
100100
private val refreshExecutor: ExecutorService =
101101
Executors.newFixedThreadPool(2) { r -> Thread(r, "KtCurrentFileRefresh").apply { isDaemon = true } }
102102

103-
/** path -> in-flight/last-launched refresh. Guarded by currentFiles.asMap().compute per key. */
103+
/** path -> in-flight/last-launched refresh; mutated only inside the per-key `compute` below. */
104104
private val currentFiles = ConcurrentHashMap<Path, CompletableFuture<VersionedKtFile>>()
105-
/** path -> version most recently launched. Read/written only inside the compute() critical section. */
105+
/** path -> last-launched version; read/written only inside that same `compute` section. */
106106
private val currentVersions = ConcurrentHashMap<Path, Int>()
107107

108108
fun syncIndexInBackground() {
@@ -204,34 +204,26 @@ internal class KtSymbolIndex(
204204
return future.thenApply { it.ktFile }
205205
}
206206

207-
/** Parses the live document for [path], registers it as the in-memory file, returns the stamped file. */
207+
/**
208+
* Parses [path]'s live document into a fresh [KtFile], registers it as the in-memory file, and
209+
* transitions the module's FIR session (invalidate + reindex) so later analysis sees the content.
210+
*
211+
* The result is stamped with [version] (captured when the refresh was launched) even though the
212+
* content is read later, here. FileManager writes version-before-content unsynchronized, so the
213+
* stamp may lag the content but never lead it: callers never get older-than-requested content, and
214+
* a lagging stamp only costs one redundant re-parse on the next request.
215+
*/
208216
private fun refreshToCurrent(path: Path, version: Int, old: KtFile?): VersionedKtFile {
209-
// [version] is the version observed (by getCurrentKtFile, reading ActiveDocument.version)
210-
// when this refresh was launched inside compute(); the content below is read live, possibly
211-
// later (this runs on refreshExecutor, after any prior refresh's prior.get()).
212-
// FileManager.onDocumentContentChange writes `version` before `content`, and neither field is
213-
// volatile/synchronized (a pre-existing race, not introduced here) — so a reader can observe a
214-
// `version` that is older than the `content` it later reads, never the reverse. Combined with
215-
// content advancing monotonically, this means what we parse here is always at-least-as-fresh
216-
// as the stamped [version]: callers never see stale (older-than-requested) content. The only
217-
// effect of the skew is a spurious cache miss and one redundant re-parse on the next request
218-
// for the newer version.
219217
val content = FileManager.getDocumentContents(path)
220218
val newKtFile = project.read { parser.createFile(path.pathString, content) }
221219
newKtFile.backingFilePath = path
222-
// Use the view provider's virtual file rather than KtFile.virtualFile: the latter is null for
223-
// non-physical PSI files (parser event system disabled, e.g. the unit-test environment), while
224-
// the view provider always exposes the backing light virtual file. For physical files (prod)
225-
// the two are the same object.
220+
// KtFile.virtualFile is null for non-physical PSI (unit-test env); the view provider's is
221+
// always present, and identical to it in production.
226222
ProjectStructureProvider.getInstance(project)
227223
.registerInMemoryFile(path.pathString, newKtFile.viewProvider.virtualFile)
228-
// Mirrors CompilationEnvironment.onFileContentChanged: invalidate the FIR session's view of
229-
// the (old) element under the write lock so it can't race a concurrent `analyze` (which only
230-
// holds the read lock), then re-index the new instance.
224+
// project.write serializes the mutation against a concurrent `analyze` (read lock); runWriteAction
225+
// supplies the platform write access handleElementModification asserts, which our RW lock does not.
231226
project.write {
232-
// handleElementModification publishes an out-of-block modification event, which the
233-
// platform's ThreadingAssertions require to run inside a write action (independent of our
234-
// own read/write lock above, which only serializes with `analyze`).
235227
ApplicationManager.getApplication().runWriteAction {
236228
KaSourceModificationService.getInstance(project)
237229
.handleElementModification(old ?: newKtFile, KaElementModificationType.Unknown)
@@ -263,13 +255,9 @@ internal class KtSymbolIndex(
263255
if (!DocumentUtils.isKotlinFile(path)) return null
264256

265257
if (FileManager.isActive(path)) {
266-
// Active document: peek the current-file cache without blocking. Calling
267-
// getCurrentKtFile(path).get() here would deadlock: getKtFile is invoked by
268-
// Analysis-API services (DeclarationsProvider, AnnotationsResolver,
269-
// DirectInheritorsProvider) while FIR resolution holds project.read, and a blocking
270-
// refresh needs project.write on the executor thread (reader-holds-lock waits on
271-
// writer -> deadlock). A peek miss falls back to the disk instance below; the file's
272-
// own refresh is already scheduled on edit and will be served on the next request.
258+
// Peek, never block: getKtFile runs under project.read inside Analysis-API services, so a
259+
// blocking getCurrentKtFile().get() (its refresh needs project.write) would deadlock. A miss
260+
// falls through to the disk instance; the edit already scheduled a refresh for next time.
273261
getCurrentKtFileIfPresent(path)?.let { return it }
274262
}
275263

@@ -309,9 +297,8 @@ internal class KtSymbolIndex(
309297
// (APPDEVFORALL-17R). This index owns `scope`.
310298
scope.coroutineContext[Job]?.cancelAndJoin()
311299

312-
// Drain the current-file refresh pool: refreshToCurrent runs project.read/write on it, so no
313-
// in-flight refresh may survive into the caller's project disposal (same rationale as the
314-
// scope join above). Bounded so a slow parse can't block shutdown indefinitely.
300+
// Drain the refresh pool before disposal: refreshToCurrent runs project.read/write on it (same
301+
// rationale as the scope join above). Bounded so a slow parse can't stall shutdown.
315302
refreshExecutor.shutdownNow()
316303
refreshExecutor.awaitTermination(CLOSE_DRAIN_TIMEOUT_SECONDS, TimeUnit.SECONDS)
317304
}

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/signaturehelp/KotlinSignatureHelp.kt‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,8 @@ internal suspend fun doSignatureHelp(params: SignatureHelpParams): SignatureHelp
8080
return SignatureHelp.empty()
8181
}
8282

83-
// Resolves to the current document version, parsing and registering it if a refresh hasn't
84-
// already landed. This is called outside any project.read/write block, so awaiting a blocking
85-
// refresh here is safe (unlike KtSymbolIndex.getKtFile, which is called by Analysis-API
86-
// services while a read lock is held).
83+
// Safe to await a (possibly blocking) refresh here: this runs outside any project.read/write
84+
// block, so it can't deadlock against the refresh's project.write (unlike KtSymbolIndex.getKtFile).
8785
val ktFile = env.ktSymbolIndex.getCurrentKtFile(params.file).await()
8886
if (ktFile == null) {
8987
logger.warn("File {} is not open", params.file)

‎lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/CurrentKtFileCacheTest.kt‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,9 @@ internal class CurrentKtFileCacheTest : KtLspTest() {
9191
changeDocument(path, "fun e(): Int = 1\nfun f(): Int = e()", 2)
9292
val v2 = env.ktSymbolIndex.getCurrentKtFile(path).get()!!
9393

94-
// The new top-level `f` calling `e` must resolve without UNRESOLVED_REFERENCE. The
95-
// `.defaultMessage` access must stay inside `env.analyze {}`: outside the analysis
96-
// session, reading a diagnostic's message would throw
97-
// KaInaccessibleLifetimeOwnerAccessException instead of producing a clean assertion
98-
// diff on a real regression.
94+
// `f` calling `e` must resolve (no UNRESOLVED_REFERENCE). Keep `.defaultMessage` inside
95+
// `env.analyze {}`: reading a diagnostic outside its analysis session throws
96+
// KaInaccessibleLifetimeOwnerAccessException instead of a clean assertion diff.
9997
val diagnosticMessages = env.analyze(v2) {
10098
v2.collectDiagnostics(
10199
org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS
@@ -146,8 +144,7 @@ internal class CurrentKtFileCacheTest : KtLspTest() {
146144
createSourceFile("J.kt", "fun j() {}")
147145
val path = sourcePath("J.kt")
148146
openDocument(path, "fun j() {}")
149-
// Note: getCurrentKtFile(path) is deliberately never called here, so no refresh has
150-
// been launched for this path yet.
147+
// getCurrentKtFile is deliberately never called, so no refresh has been launched for this path.
151148

152149
val peeked = env.ktSymbolIndex.getCurrentKtFileIfPresent(path)
153150

0 commit comments

Comments
 (0)