ADFA-5231: Gate the resolution-side KtFile door and record ADR 0015 - #1747
ADFA-5231: Gate the resolution-side KtFile door and record ADR 0015#1747itsaky-adfa wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
|
||
| override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection<KtFile> = | ||
| ktFilesForPackage(packageFqName).toList() | ||
| override fun findFilesForFacadeByPackage(packageFqName: FqName): Collection<KtFile> = ktFilesForPackage(packageFqName).toList() |
There was a problem hiding this comment.
@itsaky-adfa The gate has an un-marked back door, so the headline claim does not hold.
@OptIn(ResolutionSideKtFileAccess::class) on DeclarationProvider.ktFilesForPackage (line 180) only covers the protected producer. AbstractDeclarationProvider re-exports those same live instances through public interface overrides: findFilesForFacadeByPackage (91), findFilesForFacade (78), findFilesForScript (94), getTopLevelCallableFiles (118).
DeclarationProvider is internal with an internal constructor, and project.createDeclarationProvider(scope, null) is already used in this module, so anywhere in lsp/kotlin — test source set included — this compiles with no diagnostic and reintroduces exactly the ADFA-3322 shape:
val f = project.createDeclarationProvider(scope, null)
.findFilesForFacadeByPackage(FqName("p")).first()
analyzeMaybeDangling(f, AnalysisPriority.COMMAND, checker) { /* ... */ }FIR then sees every top-level declaration twice — the regression this PR exists to prevent.
AnnotationsResolver.declarationsByAnnotation is the same class of hole one hop out: it returns KtAnnotated PSI, and .containingKtFile is the live instance.
Marking AbstractDeclarationProvider itself, or those four public overrides, would close it.
| **Positive** | ||
|
|
||
| - The invariant is now enforced by the compiler: code that reaches for a live `KtFile` outside `withLiveKtFile` / | ||
| `withLiveKtFileAsync` does not compile without an explicit `@OptIn` on one of the two markers, which makes every |
There was a problem hiding this comment.
@itsaky-adfa This consequence overclaims what the gate enforces.
code that reaches for a live
KtFileoutsidewithLiveKtFile/withLiveKtFileAsyncdoes not compile without an explicit@OptInon one of the two markers
Falsified by the unmarked public overrides on AbstractDeclarationProvider (findFilesForFacadeByPackage, findFilesForFacade, findFilesForScript, getTopLevelCallableFiles) — see the comment on DeclarationsProvider.kt. That route compiles clean anywhere in lsp/kotlin.
Since this ADR is the record future refactors will trust, the overclaim is precisely the risk the PR set out to remove: someone reads this guarantee, takes a KtFile off the declaration provider, analyses it, and gets no compiler error and no review signal.
Either name the declaration-provider re-export as a third known exception alongside the modified-file indexer, or widen the gate so the claim is true.
`internal` was not a gate: any file in this module, its test source set included, could take the live instance from `getKtFile` and analyse it unpinned, which is exactly the shape of the ADFA-3322 regression. The three Analysis API service providers that genuinely need to name the PSI for a path opt in per function, so each exemption stays visible in review. The three providers' whole-file reformat is the Spotless ratchet: touching one line in a file that predates the tab/ktlint convention pulls the file in entirely.
The escape hatch's justification covered analysis coherence only; its one caller does hand offsets from possibly-stale PSI into a buffer edit, which is the thing the isStale guards exist to prevent. And `LiveKtFile.analyzing` is not the only route to a live instance: the modified-file indexer is handed a raw one and analyses it unpinned. Both are pre-existing behaviour with follow-ups; the record should not assert otherwise.
da211d7 to
38d480c
Compare
Stack 5 of 5 for ADFA-5231. Closes the last hole in the compile-time gate and records the decision.
Why this layer exists
The earlier PRs made the raw accessors private, but the gate did not actually hold inside the module.
KtSymbolIndex.getKtFilewas plaininternaland returns the live instance for an open path, andanalyzeMaybeDanglingis plaininternal inline. So anywhere inlsp/kotlin- including its test source set, and including wherever the next refactor lands - this compiled with no diagnostic:and reintroduced the bug. That is a literal description of how ADFA-3322 undid ADFA-4165's fix, so leaving it open would have meant shipping a fix whose headline claim was false.
Both
getKtFileoverloads now carry@RequiresOptIn(ERROR)ResolutionSideKtFileAccess, with@OptInat the three service providers that legitimately need the resolution-side door (DeclarationsProvider,AnnotationsResolver,DirectInheritorsProvider), one internal use, and five test sites.@OptInis on the narrowest scope that works, so every exemption stays visible in review.Verified rather than asserted: an un-opted-in version of the snippet above now fails compilation with the marker error at the
getKtFileacquisition. Checked twice, independently, with the probe file removed and the module recompiled clean each time.ADR 0015
Records the invariant, its history, and the consequences - the point being that ADFA-4165's fix was a property maintained by code, so a refactor undid it, whereas this one is carried by types. It also records honestly what the design costs: the cross-request staleness window, the three ways staleness is handled (edit sites refuse, diagnostics discards and reschedules, navigation tolerates), the partial escape guard, and the known follow-ups. Alternatives considered - a runtime check, one mutable
KtFileper path reparsed in place, a lint rule - each with why it was not taken.Two corrections to claims elsewhere in the codebase are included:
KtFileExts's "all Analysis API access must go through this helper" is now structural for an open file, and the "only route to a liveKtFile" claim is qualified, because the modified-file indexer does analyse one unpinned (a tracked follow-up).Testing
461 tests, 0 failures. Compile clean.