From e80bd21d97bc2858ddd75fc34d9bf47aa7196102 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 14:17:35 +0000 Subject: [PATCH 01/16] ADFA-4826: Enable Compose in lsp/kotlin The refactoring bottom sheets are Compose (ADR 0009) and live in this module rather than a UI module because `editor` depends on it, not the reverse (ADR 0011). Adds the lifecycle-runtime-compose catalog entry for collectAsStateWithLifecycle(). --- gradle/libs.versions.toml | 2 ++ lsp/kotlin/build.gradle.kts | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 9c4e15649b..af4cc0b7f0 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -96,6 +96,8 @@ androidx-fragment = { module = "androidx.fragment:fragment", version.ref = "frag androidx-lifecycle-viewmodel-ktx = { module = "androidx.lifecycle:lifecycle-viewmodel-ktx", version.ref = "lifecycleViewmodelKtx" } androidx-lifecycle-process = { module = "androidx.lifecycle:lifecycle-process", version.ref = "lifecycleViewmodelKtx" } androidx-lifecycle-runtime-ktx = { module = "androidx.lifecycle:lifecycle-runtime-ktx", version.ref = "lifecycleViewmodelKtx" } +# Provides collectAsStateWithLifecycle(), the state-collection API mandated by ADR 0009. +androidx-lifecycle-runtime-compose = { module = "androidx.lifecycle:lifecycle-runtime-compose", version.ref = "lifecycleViewmodelKtx" } androidx-palette-ktx = { module = "androidx.palette:palette-ktx", version.ref = "paletteKtx" } androidx-preference-ktx = { module = "androidx.preference:preference-ktx", version.ref = "preferenceKtxVersion" } androidx-recyclerview-v132 = { module = "androidx.recyclerview:recyclerview", version.ref = "recyclerview" } diff --git a/lsp/kotlin/build.gradle.kts b/lsp/kotlin/build.gradle.kts index 9b16f87796..27f92b80a7 100644 --- a/lsp/kotlin/build.gradle.kts +++ b/lsp/kotlin/build.gradle.kts @@ -21,11 +21,18 @@ plugins { id("com.android.library") id("kotlin-android") id("kotlin-kapt") + alias(libs.plugins.kotlin.compose) } android { namespace = "${BuildConfig.PACKAGE_NAME}.lsp.kotlin" + // The refactoring bottom sheets are Compose (ADR 0009); they live here rather than in a UI + // module because `editor` depends on this module, not the reverse (ADR 0011). + buildFeatures { + compose = true + } + kotlin.compilerOptions { freeCompilerArgs.addAll("-Xcontext-parameters") } @@ -51,6 +58,22 @@ dependencies { implementation(projects.subprojects.projects) implementation(projects.subprojects.projectModels) + implementation(projects.commonCompose) + + implementation(platform(libs.compose.bom)) + implementation(libs.compose.runtime) + implementation(libs.compose.ui) + implementation(libs.compose.foundation) + implementation(libs.compose.material3) + implementation(libs.compose.ui.tooling.preview) + debugImplementation(libs.compose.ui.tooling) + + implementation(libs.androidx.fragment.ktx) + implementation(libs.androidx.lifecycle.runtime.ktx) + implementation(libs.androidx.lifecycle.viewmodel.ktx) + implementation(libs.androidx.lifecycle.runtime.compose) + implementation(libs.google.material) + implementation(libs.common.jsonrpc) implementation(libs.common.kotlin) implementation(libs.common.kotlin.coroutines.core) From 4113f41eff443c7c423dcc6d88220e3a3251c81e Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 14:18:00 +0000 Subject: [PATCH 02/16] ADFA-4826: Add extract-variable analysis, plan and rewrite One background analysis pass produces a plain-data ExtractionPlan covering every candidate expression - its legal scope chain, occurrence set and suggested name - so the UI does pure offset arithmetic and never touches PSI (ADR 0011). Occurrence matching is symbol-aware, not textual: two sites match only when they are structurally equal and every name reference resolves to the same declaration. Sites made unsound by an intervening write are excluded rather than warned about. --- .../utils/refactor/CandidateExpressions.kt | 220 ++++++++++ .../utils/refactor/ExtractVariableEdit.kt | 179 ++++++++ .../utils/refactor/ExtractVariablePlanner.kt | 132 ++++++ .../kotlin/utils/refactor/ExtractionPlan.kt | 164 ++++++++ .../kotlin/utils/refactor/NameSuggestion.kt | 155 +++++++ .../lsp/kotlin/utils/refactor/Occurrences.kt | 273 ++++++++++++ .../lsp/kotlin/utils/refactor/ScopeChain.kt | 262 ++++++++++++ .../utils/refactor/ExtractVariableEditTest.kt | 284 +++++++++++++ .../ExtractVariablePlanEndToEndTest.kt | 389 ++++++++++++++++++ .../utils/refactor/RefactorPrimitivesTest.kt | 142 +++++++ 10 files changed, 2200 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt new file mode 100644 index 0000000000..8c0510c27f --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt @@ -0,0 +1,220 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtAnnotationEntry +import org.jetbrains.kotlin.psi.KtAnonymousInitializer +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtBreakExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtConstantExpression +import org.jetbrains.kotlin.psi.KtContinueExpression +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtLiteralStringTemplateEntry +import org.jetbrains.kotlin.psi.KtLoopExpression +import org.jetbrains.kotlin.psi.KtOperationReferenceExpression +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtReturnExpression +import org.jetbrains.kotlin.psi.KtStringTemplateEntry +import org.jetbrains.kotlin.psi.KtStringTemplateExpression +import org.jetbrains.kotlin.psi.KtSuperExpression +import org.jetbrains.kotlin.psi.KtSuperTypeListEntry +import org.jetbrains.kotlin.psi.KtThrowExpression + +/** How many candidate expressions are ever offered. Keeps the chooser scannable on a phone. */ +const val MAX_CANDIDATES = 3 + +/** + * The purely syntactic result of resolving a cursor or selection to extraction targets. + * + * [expressions] is innermost-first and at most [MAX_CANDIDATES] long. [selectionMatchedInnermost] is + * true when the caller passed a non-empty selection whose trimmed range is exactly the innermost + * candidate's range -- the user has already said which expression they mean, so the UI can skip + * asking. + */ +data class CandidateSyntax( + val expressions: List, + val selectionMatchedInnermost: Boolean, +) { + companion object { + val NONE = CandidateSyntax(emptyList(), selectionMatchedInnermost = false) + } +} + +/** + * Resolves `[selectionStart, selectionEnd)` in [file] to candidate expressions. A cursor is the + * degenerate case where the two offsets are equal, so callers need only one code path. + * + * The selection is whitespace-trimmed first, because a touch-screen selection routinely carries a + * leading or trailing space. From the resulting innermost element the parent chain is walked + * outwards, keeping legal targets ([isLegalExtractionTarget]) and stopping at the enclosing + * declaration. Blocks and other illegal nodes along the way are skipped rather than terminating the + * walk, so `if (c) a else b` is still offered from inside one of its branches. + * + * Returns [CandidateSyntax.NONE] when the position cannot host an extraction at all -- see + * [isExtractionPosition]. + */ +fun candidateExpressionsAt( + file: KtFile, + selectionStart: Int, + selectionEnd: Int, +): CandidateSyntax { + val text = file.text + val (start, end) = trimToCode(text, selectionStart, selectionEnd) ?: return CandidateSyntax.NONE + + val anchor = innermostElementFor(file, start, end) ?: return CandidateSyntax.NONE + if (!isExtractionPosition(anchor)) return CandidateSyntax.NONE + + val collected = mutableListOf() + val seen = mutableSetOf>() + var element: PsiElement? = anchor + while (element != null && element !is KtFile) { + if (element is KtDeclaration && element !is KtFunctionLiteral) break + if (element is KtExpression && element.isLegalExtractionTarget()) { + val range = element.textRange.startOffset to element.textRange.endOffset + if (seen.add(range)) { + collected += element + if (collected.size == MAX_CANDIDATES) break + } + } + element = element.parent + } + + if (collected.isEmpty()) return CandidateSyntax.NONE + + val innermost = collected.first().textRange + val matched = + selectionStart != selectionEnd && + innermost.startOffset == start && + innermost.endOffset == end + return CandidateSyntax(collected, matched) +} + +/** + * Trims whitespace off both ends of `[start, end)`. Returns null when nothing but whitespace was + * selected. A cursor (start == end) is returned unchanged. + */ +internal fun trimToCode( + text: String, + start: Int, + end: Int, +): Pair? { + if (start < 0 || end > text.length || start > end) return null + if (start == end) return start to end + var s = start + var e = end + while (s < e && text[s].isWhitespace()) s++ + while (e > s && text[e - 1].isWhitespace()) e-- + return if (s == e) null else s to e +} + +/** + * The innermost element covering `[start, end)`. For a cursor, [KtFile.findElementAt] is tried at + * the offset and then just before it, so a caret sitting immediately after a token still resolves. + */ +private fun innermostElementFor( + file: KtFile, + start: Int, + end: Int, +): PsiElement? { + if (start == end) { + val at = file.findElementAt(start)?.takeUnless { it is PsiWhiteSpace } + val before = file.findElementAt((start - 1).coerceAtLeast(0))?.takeUnless { it is PsiWhiteSpace } + return at ?: before + } + val first = file.findElementAt(start) ?: return null + val last = file.findElementAt(end - 1) ?: return null + return PsiTreeUtil.findCommonParent(first, last) +} + +/** + * Whether [element] sits somewhere an extraction can legally be anchored. + * + * Rejects the positions where no `val` can precede the expression: + * - **annotation arguments** -- must be compile-time constants; + * - **default parameter values** -- evaluated per call, and a hoisted local would not be in scope; + * - **super-constructor delegation arguments** -- nothing can precede them; + * - **anything outside an executable body** -- notably a class-body property initializer, which has + * no block to insert into. Converting one to a getter would change compute-once into + * compute-per-access, so it is declined instead. + */ +internal fun isExtractionPosition(element: PsiElement): Boolean { + if (PsiTreeUtil.getParentOfType(element, KtAnnotationEntry::class.java, false) != null) return false + if (PsiTreeUtil.getParentOfType(element, KtSuperTypeListEntry::class.java, false) != null) return false + + val parameter = PsiTreeUtil.getParentOfType(element, KtParameter::class.java, false) + if (parameter != null && parameter.defaultValue?.isAncestorOf(element) == true) return false + + return enclosingExecutableBody(element) != null +} + +/** + * The nearest enclosing thing with a body that can hold statements: a lambda, a named or anonymous + * function, a property accessor, an `init` block, or a constructor. Null when [element] is not + * inside any of them. + */ +internal fun enclosingExecutableBody(element: PsiElement): PsiElement? { + var current: PsiElement? = element + while (current != null && current !is KtFile) { + if (current is KtFunctionLiteral) return current + if (current is KtDeclarationWithBody && current.bodyExpression?.isAncestorOf(element) == true) return current + if (current is KtAnonymousInitializer && current.body?.isAncestorOf(element) == true) return current + current = current.parent + } + return null +} + +private fun PsiElement.isAncestorOf(other: PsiElement): Boolean = PsiTreeUtil.isAncestor(this, other, false) + +/** + * Whether this expression is a thing whose value can be bound to a `val`. + * + * Excluded, and why: + * - blocks, loops, `return`/`throw`/`break`/`continue` -- no useful value to bind; + * - operator tokens and call callees (`foo` in `foo(x)`) -- fragments, not expressions; + * - the selector of a qualified expression (`b` in `a.b`) -- only meaningful with its receiver; + * - the left side of an assignment -- a write target, not a value; + * - `super` -- not a value; + * - **bare literals** (`1`, `"text"`) -- extracting them is pointless, and excluding them removes + * the only case where omitting a type annotation could change meaning (an `Int` literal where a + * `Long` is expected, or a bare `null` inferring `Nothing?`). + */ +internal fun KtExpression.isLegalExtractionTarget(): Boolean { + if (this is KtBlockExpression) return false + if (this is KtLoopExpression) return false + if (this is KtReturnExpression || this is KtThrowExpression) return false + if (this is KtBreakExpression || this is KtContinueExpression) return false + if (this is KtOperationReferenceExpression) return false + if (this is KtSuperExpression) return false + if (this is KtFunctionLiteral) return false + if (isBareLiteral()) return false + + val parent = parent + if (parent is KtQualifiedExpression && parent.selectorExpression === this) return false + if (parent is KtCallExpression && parent.calleeExpression === this) return false + if (parent is KtBinaryExpression && + parent.operationToken == KtTokens.EQ && + parent.left === this + ) { + return false + } + return true +} + +/** A numeric/boolean/char/null literal, or a string with no interpolation. */ +private fun KtExpression.isBareLiteral(): Boolean = + when (this) { + is KtConstantExpression -> true + is KtStringTemplateExpression -> entries.all { it.isLiteralEntry() } + else -> false + } + +private fun KtStringTemplateEntry.isLiteralEntry(): Boolean = this is KtLiteralStringTemplateEntry diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt new file mode 100644 index 0000000000..da41a5e2fa --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -0,0 +1,179 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.models.TextEdit +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.models.Range + +/** + * The one text replacement an extraction performs: replace `[span]` with [newText]. + * + * **Deliberately a single replacement, not a list of edits.** `IDELanguageClientImpl.applyActionEdits` + * applies each `TextEdit` in its own `runOnUiThread` with no `beginBatchEdit`, and every range is + * computed against the *original* text -- so a list of N edits would be applied against positions + * already shifted by its predecessors, and would cost the user N undo steps with a typing window + * between each. Rewriting one contiguous span sidesteps all of it. + */ +data class RewriteSpan( + val span: TextSpan, + val newText: String, +) + +/** + * Builds the extraction rewrite, or null when the inputs cannot produce one. + * + * [name] is the final variable name -- the caller has already validated it. [replaceAll] selects + * between every occurrence in [scope] and only [candidateSpan]. + * + * Occurrences are substituted right-to-left within the rewritten span so earlier substitutions + * cannot shift later offsets, and the whole span is emitted as one replacement. + */ +fun buildExtractVariableRewrite( + fileText: String, + candidateSpan: TextSpan, + scope: ScopeOption, + name: String, + replaceAll: Boolean, +): RewriteSpan? { + val targets = + (if (replaceAll) scope.occurrences else listOf(candidateSpan)) + .sortedBy { it.start } + .takeIf { it.isNotEmpty() } ?: return null + if (targets.any { it.end > fileText.length }) return null + + val expression = fileText.substring(candidateSpan.start, candidateSpan.end) + val declaration = "val $name = $expression" + + return when (val form = scope.anchorForm) { + AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, targets, declaration, name) + is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form, targets, declaration, name) + is AnchorForm.ConvertExpressionBody -> convertExpressionBodyRewrite(fileText, form, targets, declaration, name) + } +} + +/** + * Inserts the declaration as its own line before the first served occurrence's line, and rewrites + * everything from there through the last occurrence. + * + * The rewritten span starts at that line's start (not at the occurrence) so the declaration lands on + * a line of its own at the right indentation, and ends at the last occurrence so untouched trailing + * code is left alone. + */ +private fun existingBlockRewrite( + fileText: String, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val first = targets.first() + val last = targets.last() + val lineStart = lineStartOffset(fileText, first.start) + val indent = leadingIndentAt(fileText, first.start) + val newline = detectNewline(fileText) + + val body = replaceOccurrences(fileText, TextSpan(lineStart, last.end), targets, name) + return RewriteSpan( + span = TextSpan(lineStart, last.end), + newText = indent + declaration + newline + body, + ) +} + +/** Wraps a braceless statement in a block containing the declaration and the original statement. */ +private fun wrapInBracesRewrite( + fileText: String, + form: AnchorForm.WrapInBraces, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + // Occurrences in a braceless scope are confined to the statement itself (the frame's search + // range *is* this span), so no cross-span targets are possible; replaceOccurrences filters anyway. + val span = TextSpan(form.bodyStart, form.bodyEnd) + val newline = detectNewline(fileText) + val body = replaceOccurrences(fileText, span, targets, name) + + val newText = + buildString { + append('{').append(newline) + append(form.innerIndent).append(declaration).append(newline) + append(form.innerIndent).append(body).append(newline) + append(form.indent).append('}') + } + return RewriteSpan(span, newText) +} + +/** Converts `= expr` into a block body holding the declaration and a `return` of the rewritten body. */ +private fun convertExpressionBodyRewrite( + fileText: String, + form: AnchorForm.ConvertExpressionBody, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val bodySpan = TextSpan(form.bodyStart, form.bodyEnd) + val newline = detectNewline(fileText) + val body = replaceOccurrences(fileText, bodySpan, targets, name) + val returned = if (form.needsReturn) "return $body" else body + + val newText = + buildString { + append('{').append(newline) + append(form.innerIndent).append(declaration).append(newline) + append(form.innerIndent).append(returned).append(newline) + append(form.indent).append('}') + } + return RewriteSpan(TextSpan(form.assignStart, form.bodyEnd), newText) +} + +/** + * Returns `[span]`'s text with every occurrence inside it replaced by [name]. Substitutes + * right-to-left so an earlier replacement cannot invalidate a later offset. + */ +private fun replaceOccurrences( + fileText: String, + span: TextSpan, + targets: List, + name: String, +): String { + val builder = StringBuilder(fileText.substring(span.start, span.end)) + targets + .filter { it.start >= span.start && it.end <= span.end } + .sortedByDescending { it.start } + .forEach { builder.replace(it.start - span.start, it.end - span.start, name) } + return builder.toString() +} + +/** CRLF only when the file already uses it, so the edit does not mix line endings. */ +internal fun detectNewline(text: String): String = if (text.contains("\r\n")) "\r\n" else "\n" + +/** + * Converts a [RewriteSpan] into the `TextEdit` the language client consumes. [Position] carries + * line, column *and* index; all three are filled so neither the client's line/column path nor any + * index-based consumer sees a stale value. + */ +fun RewriteSpan.toTextEdit(fileText: String): TextEdit = + TextEdit( + range = + Range( + positionAt(fileText, span.start), + positionAt(fileText, span.end), + ), + newText = newText, + ) + +internal fun positionAt( + text: String, + offset: Int, +): Position { + val clamped = offset.coerceIn(0, text.length) + var line = 0 + var lineStart = 0 + var i = 0 + while (i < clamped) { + if (text[i] == '\n') { + line++ + lineStart = i + 1 + } + i++ + } + return Position(line, clamped - lineStart, clamped) +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt new file mode 100644 index 0000000000..bc39bde916 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt @@ -0,0 +1,132 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.read +import com.itsaky.androidide.lsp.kotlin.utils.renderName +import org.jetbrains.kotlin.analysis.api.KaExperimentalApi +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol +import org.jetbrains.kotlin.analysis.api.types.KaType +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.slf4j.LoggerFactory +import java.nio.file.Path + +private val logger = LoggerFactory.getLogger("ExtractVariablePlanner") + +/** + * Computes the whole [ExtractionPlan] in one background analysis pass. + * + * The current [KtFile] is fetched *before* entering [read] -- blocking on + * `getCurrentKtFile(...).get()` inside `project.read` deadlocks. + * + * Returns an empty plan both when there is genuinely nothing to extract and whenever anything in + * this pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on + * a scope with no exception handler, so an uncaught throw would crash the app. Degrading to an empty + * plan is always safe -- the action reports "nothing to extract" instead of rewriting anything. + */ +internal fun buildExtractionPlan( + env: AbstractCompilationEnvironment, + nioPath: Path, + selectionStart: Int, + selectionEnd: Int, + documentVersion: Int, + cancelChecker: ScheduledCancelChecker, +): ExtractionPlan = + runCatching { + val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return ExtractionPlan.empty() + env.project.read { + val syntax = candidateExpressionsAt(ktFile, selectionStart, selectionEnd) + if (syntax.expressions.isEmpty()) return@read ExtractionPlan.empty(ktFile.text, documentVersion) + + analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { + val candidates = syntax.expressions.mapNotNull { candidateFor(it) } + ExtractionPlan( + fileText = ktFile.text, + documentVersion = documentVersion, + candidates = candidates, + // Only meaningful while the innermost candidate survived filtering; otherwise the + // user's selection no longer corresponds to the first option shown. + selectionMatchedCandidate = + syntax.selectionMatchedInnermost && + candidates.firstOrNull()?.span?.start == + syntax.expressions + .first() + .textRange.startOffset, + ) + } + } + }.getOrElse { error -> + logger.warn("Failed to build extract-variable plan for {}", nioPath, error) + ExtractionPlan.empty() + } + +/** + * Turns one syntactic candidate into a [CandidateExpression], or null when it should not be offered. + * + * Dropped when the expression produces no useful value (`Unit`, `Nothing` -- `val u = println(x)` + * compiles but is pointless) or when nothing remains of its legal scope chain. + */ +@OptIn(KaExperimentalApi::class) +private fun KaSession.candidateFor(expression: KtExpression): CandidateExpression? { + val type = runCatching { expression.expressionType }.getOrNull() + if (type == null || isValuelessType(type)) return null + + val frames = truncateAtCeiling(enclosingScopeFrames(expression), referencedDeclarationCeiling(expression)) + if (frames.isEmpty()) return null + + val span = TextSpan(expression.textRange.startOffset, expression.textRange.endOffset) + val scopes = frames.map { scopeOptionFor(expression, span, it) } + val takenNames = visibleNamesAt(expression) + + return CandidateExpression( + label = collapseForLabel(expression.text), + span = span, + suggestedName = suggestVariableName(expression, runCatching { renderName(type) }.getOrNull(), takenNames), + takenNames = takenNames, + scopes = scopes, + ) +} + +/** Builds one scope option, resolving its occurrence set and fixing up expression-body details. */ +private fun KaSession.scopeOptionFor( + expression: KtExpression, + span: TextSpan, + frame: ScopeFrame, +): ScopeOption { + val matches = findOccurrences(expression, frame.scopeElement, frame.searchRange) + val writes = writeOffsetsFor(expression, frame.scopeElement) + val occurrences = excludeUnsoundOccurrences(matches, span, writes) + + val anchorForm = + when (val form = frame.anchorForm) { + is AnchorForm.ConvertExpressionBody -> form.copy(needsReturn = expressionBodyNeedsReturn(frame.scopeElement)) + else -> form + } + + return ScopeOption(label = frame.label, anchorForm = anchorForm, occurrences = occurrences) +} + +/** + * Whether converting an expression body to a block body needs a `return`. + * + * False only for a `Unit`-returning function, where `return expr` on a non-`Unit` expression would + * not compile and is unnecessary anyway. Defaults to true, which is right for everything else + * including property accessors. + */ +private fun KaSession.expressionBodyNeedsReturn(bodyExpression: PsiElement): Boolean { + val declaration = bodyExpression.parent as? KtDeclarationWithBody ?: return true + val returnType = + runCatching { ((declaration as? KtDeclaration)?.symbol as? KaCallableSymbol)?.returnType }.getOrNull() + ?: return true + return !isValuelessType(returnType) +} + +/** `Unit` and `Nothing` carry no value worth binding to a `val`. */ +private fun KaSession.isValuelessType(type: KaType): Boolean = runCatching { type.isUnitType || type.isNothingType }.getOrDefault(false) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt new file mode 100644 index 0000000000..47d1f43538 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -0,0 +1,164 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** A half-open offset range `[start, end)` into the analysed file's text. */ +data class TextSpan( + val start: Int, + val end: Int, +) { + init { + require(start <= end) { "start=$start > end=$end" } + } + + val length: Int get() = end - start + + fun overlaps(other: TextSpan): Boolean = start < other.end && other.start < end +} + +/** + * How the new declaration is woven into an anchor scope. Kotlin scopes are not all blocks, so + * three shapes are needed; [ExistingBlock] is by far the common one. + */ +sealed interface AnchorForm { + /** + * The scope already has a `{ ... }` body (function body, `if` block, lambda body, ...), so the + * declaration is simply a new statement line. + * + * Deliberately field-free: the insertion offset and indentation are both derived from the first + * occurrence being served, which is the candidate itself when replacing only one site and an + * earlier statement when replacing all. Storing a precomputed anchor would duplicate that and + * let the two drift apart. + */ + data object ExistingBlock : AnchorForm + + /** + * A braceless statement position -- `if (c) foo()`, a `when` entry, a braceless loop body. + * `[bodyStart, bodyEnd)` (the statement) is replaced by a braced block holding the declaration + * and the original statement. No `return` is involved. + */ + data class WrapInBraces( + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, + ) : AnchorForm + + /** + * An expression-bodied function or property accessor -- `fun area(r: Int) = r * r`. The `=` and + * the body are replaced by a block body. [needsReturn] is false only when the declaration + * returns `Unit`, where `return` is both unnecessary and wrong for a non-`Unit` expression. + */ + data class ConvertExpressionBody( + val assignStart: Int, + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, + val needsReturn: Boolean, + ) : AnchorForm +} + +/** + * One member of a candidate's legal scope chain: a place the declaration may go, together with the + * occurrences that are sound to replace there. + * + * [occurrences] is ascending by offset and always contains the candidate's own span, so + * `occurrences.size` is the count shown as "Replace all N occurrences". Narrowing to an inner scope + * can only shrink this set, never grow it. + */ +data class ScopeOption( + val label: String, + val anchorForm: AnchorForm, + val occurrences: List, +) + +/** + * A legal extraction target and everything the UI needs to act on it. + * + * [label] is the expression's source text with runs of whitespace collapsed, so a multi-line + * expression stays readable in a one-line list item. + * + * [scopes] is the legal scope chain, innermost first, and is never empty -- a candidate with no + * legal anchor is not a candidate. + */ +data class CandidateExpression( + val label: String, + val span: TextSpan, + val suggestedName: String, + val takenNames: Set, + val scopes: List, +) + +/** + * The complete result of the background analysis pass, and the central type of the extract/inline + * refactorings. + * + * ## Vocabulary + * + * Used verbatim throughout this package, its tests and its review comments -- prefer these over + * ad-hoc synonyms. + * + * - **Candidate expression** -- a [org.jetbrains.kotlin.psi.KtExpression] at the cursor or selection + * that is a legal extraction target. At most [MAX_CANDIDATES], ordered innermost-first. + * - **Legal scope chain** -- the ordered anchors available for the new declaration: outward from the + * candidate's own statement through enclosing blocks, crossing a lambda boundary only when nothing + * lambda-scoped is referenced, and stopping at the enclosing method body. + * - **Anchor scope** -- the chain member the user picked. The `val` is declared inside it. + * - **Anchor point** -- the exact insertion offset: immediately before the first statement *within the + * anchor scope* that contains a replaced occurrence. + * - **Occurrence** -- a site inside the anchor scope that is structurally equal to the candidate *and* + * whose every name reference resolves to the same symbol. Sites made unsound by an intervening + * reassignment are excluded, so an occurrence set is always safe to replace wholesale. + * - **Extraction plan** -- this type. + * + * ## Why plain data + * + * The user's choices (which expression, what name, which scope, replace-all or not) arrive *after* + * analysis, from a sheet. Rather than re-entering analysis on confirm, one background pass produces + * this plan for *all* candidates at once and the UI does pure string/offset arithmetic on it. That + * keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation + * unit-testable without an editor, an activity or Compose. + * + * [fileText] is the text the offsets here refer to, carried so the UI can build the replacement text + * without PSI; [documentVersion] is what makes that safe -- if the live document has moved on by the + * time the user confirms, the plan is discarded rather than applied against shifted offsets. + * + * [selectionMatchedCandidate] is true when the user's selection exactly matched the innermost + * candidate, meaning they already expressed which expression they want and the UI should not ask. + */ +data class ExtractionPlan( + val fileText: String, + val documentVersion: Int, + val candidates: List, + val selectionMatchedCandidate: Boolean, +) { + val isEmpty: Boolean get() = candidates.isEmpty() + + companion object { + fun empty( + fileText: String = "", + documentVersion: Int = -1, + ) = ExtractionPlan(fileText, documentVersion, emptyList(), selectionMatchedCandidate = false) + } +} + +/** + * Collapses whitespace runs so a multi-line expression reads as one line in a list item. + * + * The space before a `.` or `?.` is then removed: a wrapped call chain is the most common multi-line + * expression in Kotlin, and a plain collapse turns `items\n\t.filter { ... }` into + * `items .filter { ... }`, which reads as a typo in a list the user is choosing from. + */ +internal fun collapseForLabel( + text: String, + maxLength: Int = 80, +): String { + val collapsed = + text + .replace(WHITESPACE_RUN, " ") + .replace(SPACE_BEFORE_DOT, "$1") + .trim() + return if (collapsed.length <= maxLength) collapsed else collapsed.take(maxLength - 3) + "..." +} + +private val WHITESPACE_RUN = Regex("\\s+") +private val SPACE_BEFORE_DOT = Regex(" (\\??\\.)") diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt new file mode 100644 index 0000000000..3427571a18 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt @@ -0,0 +1,155 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.psi.KtArrayAccessExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.KtParenthesizedExpression +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtStringTemplateExpression + +/** Used when neither the expression's shape nor its type suggests anything better. */ +const val FALLBACK_NAME = "value" + +/** + * Kotlin's hard keywords -- the ones that are never valid identifiers. Soft and modifier keywords + * (`by`, `data`, `it`, ...) are legal names and are deliberately absent. + */ +private val HARD_KEYWORDS = + setOf( + "as", + "break", + "class", + "continue", + "do", + "else", + "false", + "for", + "fun", + "if", + "in", + "interface", + "is", + "null", + "object", + "package", + "return", + "super", + "this", + "throw", + "true", + "try", + "typealias", + "typeof", + "val", + "var", + "when", + "while", + ) + +/** Why a proposed name cannot be used. Null-free alternative to throwing for user input. */ +enum class NameProblem { + Blank, + NotAnIdentifier, + Keyword, + AlreadyTaken, +} + +/** + * Validates a user-supplied name against Kotlin's identifier rules and the names already visible at + * the anchor point. Returns null when the name is usable. + * + * Backtick-quoted names are rejected rather than supported: they are legal Kotlin but a poor + * suggestion for a generated local, and accepting them would mean validating the quoted form too. + */ +fun validateVariableName( + name: String, + takenNames: Set, +): NameProblem? { + if (name.isBlank()) return NameProblem.Blank + if (!isIdentifier(name)) return NameProblem.NotAnIdentifier + if (name in HARD_KEYWORDS) return NameProblem.Keyword + if (name in takenNames) return NameProblem.AlreadyTaken + return null +} + +private fun isIdentifier(name: String): Boolean { + if (name.isEmpty()) return false + if (!(name[0].isLetter() || name[0] == '_')) return false + return name.all { it.isLetterOrDigit() || it == '_' } +} + +/** + * Suggests a name for the value [expression] produces. + * + * Tried in order: + * 1. **The expression's shape** -- `items.size` -> `size`, `a.b.c()` -> `c`, `getFoo()` -> `foo`, + * `foo(x)` -> `foo`, an interpolated string -> `text`, `xs[i]` -> `xs` element naming. + * 2. **The resolved type**, lowercased -- `List` -> `list`, `Duration` -> `duration`. Pass null + * when the type is unavailable. + * 3. [FALLBACK_NAME]. + * + * The result is then made unique against [takenNames] by appending `1`, `2`, ... Shape beats type + * because `size`, `count` and `name` are far better names than `int` and `string`, and type-derived + * names collide constantly. + */ +fun suggestVariableName( + expression: KtExpression, + typeName: String?, + takenNames: Set, +): String { + val base = + nameFromShape(expression) + ?: typeName?.let(::nameFromType) + ?: FALLBACK_NAME + val sanitised = base.takeIf { isIdentifier(it) && it !in HARD_KEYWORDS } ?: FALLBACK_NAME + return makeUnique(sanitised, takenNames) +} + +private fun nameFromShape(expression: KtExpression): String? = + when (expression) { + is KtParenthesizedExpression -> expression.expression?.let(::nameFromShape) + is KtQualifiedExpression -> expression.selectorExpression?.let(::nameFromShape) + is KtCallExpression -> (expression.calleeExpression as? KtNameReferenceExpression)?.getReferencedName()?.let(::stripAccessorPrefix) + is KtNameReferenceExpression -> expression.getReferencedName().let(::stripAccessorPrefix) + is KtStringTemplateExpression -> "text" + is KtArrayAccessExpression -> expression.arrayExpression?.let(::nameFromShape) + else -> null + }?.takeIf { it.isNotBlank() } + +/** `getFoo` -> `foo`, `isReady` -> `ready`. Leaves anything else alone. */ +private fun stripAccessorPrefix(name: String): String { + for (prefix in ACCESSOR_PREFIXES) { + if (name.length > prefix.length && + name.startsWith(prefix) && + name[prefix.length].isUpperCase() + ) { + return name.substring(prefix.length).decapitaliseFirst() + } + } + return name +} + +private val ACCESSOR_PREFIXES = listOf("get", "is", "has") + +/** `List` -> `list`, `kotlin.time.Duration` -> `duration`, `Array` -> `array`. */ +private fun nameFromType(typeName: String): String? = + typeName + .substringBefore('<') + .substringAfterLast('.') + .trimEnd('?', '!') + .takeIf { it.isNotBlank() } + ?.decapitaliseFirst() + +private fun String.decapitaliseFirst(): String = if (isEmpty()) this else this[0].lowercaseChar() + substring(1) + +/** `size` -> `size1` -> `size2` until nothing in [takenNames] matches. */ +private fun makeUnique( + base: String, + takenNames: Set, +): String { + if (base !in takenNames) return base + var suffix = 1 + while ("$base$suffix" in takenNames) suffix++ + return "$base$suffix" +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt new file mode 100644 index 0000000000..61eea683ae --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt @@ -0,0 +1,273 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.symbols.KaSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaValueParameterSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaVariableSymbol +import org.jetbrains.kotlin.builtins.StandardNames +import org.jetbrains.kotlin.com.intellij.psi.PsiComment +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.idea.references.mainReference +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.KtUnaryExpression +import org.jetbrains.kotlin.psi.psiUtil.parents + +/** + * Whether [a] and [b] are the same expression for extraction purposes: structurally identical *and* + * every name reference in them resolving to the same declaration. + * + * The symbol check is the whole point. Text or structure alone would happily match `config.timeout` + * inside a nested lambda where `config` is a different `config`, or an `it` that means something + * else -- replacing those would silently change behaviour. The parent ticket (ADFA-3324) states the + * standard outright: text-based matching breaks things. + */ +internal fun KaSession.isSameExpression( + a: PsiElement, + b: PsiElement, +): Boolean { + if (a === b) return true + if (a.node?.elementType != b.node?.elementType) return false + + if (a is KtSimpleNameExpression && b is KtSimpleNameExpression) { + if (a.getReferencedName() != b.getReferencedName()) return false + if (!resolvesToSameDeclaration(a, b)) return false + } + + val childrenA = meaningfulChildren(a) + val childrenB = meaningfulChildren(b) + if (childrenA.size != childrenB.size) return false + if (childrenA.isEmpty()) return a.text == b.text + return childrenA.indices.all { isSameExpression(childrenA[it], childrenB[it]) } +} + +/** Whitespace and comments are formatting, not structure, so they never affect equality. */ +private fun meaningfulChildren(element: PsiElement): List = + element.children.filter { it !is PsiWhiteSpace && it !is PsiComment } + +/** + * Whether two same-named references point at the same declaration. + * + * Source declarations are compared by PSI identity, which is exactly the question being asked ("the + * same `val`?"). Symbols without source PSI -- library members, compiler-generated declarations -- + * fall back to symbol equality. Resolution over broken code throws, and a throw here must read as + * "not the same" rather than crash the action. + */ +private fun KaSession.resolvesToSameDeclaration( + a: KtSimpleNameExpression, + b: KtSimpleNameExpression, +): Boolean = + runCatching { + val symbolA = a.mainReference?.resolveToSymbols()?.firstOrNull() ?: return false + val symbolB = b.mainReference?.resolveToSymbols()?.firstOrNull() ?: return false + val psiA = symbolA.declarationPsi() + val psiB = symbolB.declarationPsi() + if (psiA != null || psiB != null) psiA === psiB else symbolA == symbolB + }.getOrDefault(false) + +private fun KaSymbol.declarationPsi(): PsiElement? = runCatching { psi }.getOrNull() + +/** + * Every site in [searchRoot] within [searchRange] that is the same expression as [candidate] and is + * itself a legal place to put the variable reference. + * + * The legality filter matters: in `a.a`, a candidate of `a` matches the selector too, but rewriting + * a selector would produce `v.v`. Overlapping matches are dropped so no site is rewritten twice. + * Ascending by offset, and always contains [candidate] itself. + */ +internal fun KaSession.findOccurrences( + candidate: KtExpression, + searchRoot: PsiElement, + searchRange: TextSpan, +): List { + val elementType = candidate.node?.elementType + val matches = + PsiTreeUtil + .collectElements(searchRoot) { element -> + element.node?.elementType == elementType && + element is KtExpression && + element.textRange.startOffset >= searchRange.start && + element.textRange.endOffset <= searchRange.end + }.filterIsInstance() + .filter { it === candidate || (it.isLegalExtractionTarget() && isSameExpression(candidate, it)) } + .map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) } + .sortedBy { it.start } + + val accepted = mutableListOf() + for (match in matches) { + if (accepted.none { it.overlaps(match) }) accepted += match + } + return accepted +} + +/** + * The innermost scope that must contain the declaration, or null when the candidate references + * nothing declared inside the enclosing scopes. + * + * This is what stops a hoist from escaping a lambda it depends on: if the candidate uses `it` or a + * lambda parameter, that lambda's body comes back as the ceiling and every outer rung of the scope + * chain is dropped by [truncateAtCeiling]. + */ +internal fun KaSession.referencedDeclarationCeiling(candidate: KtExpression): PsiElement? { + var deepest: PsiElement? = null + var deepestDepth = -1 + for (reference in candidate.collectDescendantsOfType()) { + val symbol = runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() ?: continue + val body = constrainingBodyFor(reference, symbol) ?: continue + val depth = depthOf(body) + if (depth > deepestDepth) { + deepest = body + deepestDepth = depth + } + } + return deepest +} + +/** + * The scope [reference] pins the declaration inside, or null when it constrains nothing. + * + * A declaration outside the candidate's own scopes -- a class member, a top-level property, anything + * from a library -- constrains nothing; only locals and parameters do. + * + * The implicit lambda parameter needs its own case: `it` has **no source PSI**, so the ordinary + * psi-based lookup finds nothing and would report "unconstrained", happily hoisting `it.length` clean + * out of its lambda into code that does not compile. A value-parameter symbol with no PSI, referenced + * by the name `it`, *is* by definition the implicit parameter of the innermost enclosing lambda -- a + * property of the language, not a guess about the text. + */ +private fun constrainingBodyFor( + reference: KtSimpleNameExpression, + symbol: KaSymbol, +): PsiElement? { + val declaration = runCatching { symbol.psi }.getOrNull() + if (declaration == null) { + if (symbol is KaValueParameterSymbol && reference.getReferencedName() == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString()) { + return PsiTreeUtil.getParentOfType(reference, KtFunctionLiteral::class.java, true)?.bodyExpression + } + return null + } + if (!PsiTreeUtil.isAncestor(reference.containingFile, declaration, false)) return null + return enclosingExecutableBody(declaration) +} + +private fun depthOf(element: PsiElement): Int = element.parents.count() + +private inline fun PsiElement.collectDescendantsOfType(): List = + PsiTreeUtil.collectElementsOfType(this, T::class.java).toList() + +/** + * Restricts [occurrences] to a contiguous run around [candidateSpan] that no write to a referenced + * mutable interrupts. + * + * A `var` the candidate reads can be reassigned between two occurrences, and then the two sites do + * not hold the same value even though they are the same expression: + * + * ``` + * var limit = 1 + * foo(limit + 1) // occurrence + * limit = 5 + * foo(limit + 1) // same expression, different value + * ``` + * + * Rather than warn, unsound sites are simply excluded, so "Replace all N occurrences" can never + * produce wrong code and N is always achievable. The walk grows outwards from the candidate -- never + * dropping the site the user actually selected -- and stops in each direction at the first write it + * would have to cross. + */ +internal fun excludeUnsoundOccurrences( + occurrences: List, + candidateSpan: TextSpan, + writeOffsets: List, +): List { + if (occurrences.isEmpty()) return occurrences + val ordered = occurrences.sortedBy { it.start } + val candidateIndex = ordered.indexOfFirst { it.start == candidateSpan.start && it.end == candidateSpan.end } + if (candidateIndex < 0) return listOf(candidateSpan) + + val writes = writeOffsets.sorted() + + fun writeBetween( + from: Int, + to: Int, + ): Boolean = writes.any { it in from until to } + + val accepted = mutableListOf(ordered[candidateIndex]) + for (i in candidateIndex - 1 downTo 0) { + if (writeBetween(ordered[i].end, ordered[candidateIndex].start)) break + accepted.add(0, ordered[i]) + } + for (i in candidateIndex + 1 until ordered.size) { + if (writeBetween(ordered[candidateIndex].end, ordered[i].start)) break + accepted += ordered[i] + } + return accepted +} + +/** + * Offsets of writes, within [searchRoot], to any mutable the candidate reads. Feeds + * [excludeUnsoundOccurrences]. + * + * Counts plain assignment, the augmented forms (`+=` and friends) and `++`/`--`. A `val` cannot be + * written, so only [KaVariableSymbol]s that report themselves mutable are tracked. + */ +internal fun KaSession.writeOffsetsFor( + candidate: KtExpression, + searchRoot: PsiElement, +): List { + val mutableDeclarations = + candidate + .collectDescendantsOfType() + .mapNotNull { reference -> + runCatching { + (reference.mainReference?.resolveToSymbols()?.firstOrNull() as? KaVariableSymbol) + ?.takeIf { !it.isVal } + ?.psi + }.getOrNull() + }.toSet() + if (mutableDeclarations.isEmpty()) return emptyList() + + return searchRoot + .collectDescendantsOfType() + .filter { it.isWriteTarget() } + .filter { reference -> + runCatching { + reference.mainReference + ?.resolveToSymbols() + ?.firstOrNull() + ?.psi + }.getOrNull() in mutableDeclarations + }.map { it.textRange.startOffset } +} + +/** Whether this reference is being written to rather than read. */ +private fun KtSimpleNameExpression.isWriteTarget(): Boolean { + val parent = parent + if (parent is KtBinaryExpression && parent.left === this && parent.operationToken in ASSIGNMENT_TOKENS) return true + if (parent is KtUnaryExpression && parent.operationToken in INCREMENT_TOKENS) return true + return false +} + +private val ASSIGNMENT_TOKENS = + setOf(KtTokens.EQ, KtTokens.PLUSEQ, KtTokens.MINUSEQ, KtTokens.MULTEQ, KtTokens.DIVEQ, KtTokens.PERCEQ) + +private val INCREMENT_TOKENS = setOf(KtTokens.PLUSPLUS, KtTokens.MINUSMINUS) + +/** + * Names a suggestion must avoid: every declaration name in the file. + * + * Deliberately conservative rather than scope-exact. A real scope query would need resolution and + * would let `size` be suggested in one function because the collision is in another -- correct, but + * the cost of being over-broad is only a `size1` where `size` would have done, while the cost of + * being under-broad is generated code that shadows something. Cheap, needs no analysis, and being + * purely syntactic it is unit-testable. + */ +internal fun visibleNamesAt(candidate: KtExpression): Set = + PsiTreeUtil + .collectElementsOfType(candidate.containingFile, KtDeclaration::class.java) + .mapNotNullTo(mutableSetOf()) { it.name } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt new file mode 100644 index 0000000000..79ac67d2fe --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -0,0 +1,262 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtAnonymousInitializer +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtContainerNodeForControlStructureBody +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtDoWhileExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtForExpression +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtIfExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtPropertyAccessor +import org.jetbrains.kotlin.psi.KtWhenEntry +import org.jetbrains.kotlin.psi.KtWhileExpression + +/** + * One rung of the legal scope chain, before occurrences are known. + * + * [scopeElement] is the PSI node that *is* the scope, used to decide whether a referenced + * declaration lives inside it (see [truncateAtCeiling]). [searchRange] bounds the occurrence search + * for this rung. [statementSpan] is the statement within this scope that contains the candidate -- + * the fallback anchor when only the selected occurrence is replaced. + */ +data class ScopeFrame( + val label: String, + val scopeElement: PsiElement, + val searchRange: TextSpan, + val statementSpan: TextSpan, + val anchorForm: AnchorForm, +) + +/** + * Enumerates the scopes [candidate] could be hoisted into, innermost first. + * + * Walks outward from the candidate's own statement. Each rung is one of the three [AnchorForm] + * shapes: a real block, a braceless statement position that needs braces, or an expression body that + * needs converting. The walk stops after the enclosing **named function, accessor or `init` block** + * body -- the ceiling agreed for this refactoring. A class body or file is never an anchor, so a + * property initializer outside any executable body yields nothing (already rejected earlier by + * [isExtractionPosition]). + * + * Lambda boundaries are *crossed* here: whether crossing is actually legal depends on what the + * candidate references, which needs resolution, so it is applied afterwards by [truncateAtCeiling]. + */ +fun enclosingScopeFrames(candidate: KtExpression): List { + val text = candidate.containingFile.text + val frames = mutableListOf() + var inner: PsiElement = candidate + + while (true) { + val parent = inner.parent ?: break + if (parent is KtFile) break + + val frame = frameFor(inner, text) + if (frame == null) { + // Most nodes are not themselves anchorable -- a value argument, an argument list, a lambda + // literal. Keep climbing rather than stopping, otherwise the chain would end at the first + // such node and, in particular, a candidate inside a lambda could never be hoisted out of + // it even when that is legal. + inner = parent + continue + } + + frames += frame + // A named function / accessor / init body is the ceiling: record it, then stop. + if (isCeilingBody(frame.scopeElement)) break + inner = frame.scopeElement.parent ?: break + } + return frames +} + +/** + * Drops the rungs that lie outside [ceiling] -- the innermost scope holding a declaration the + * candidate references. Passing null keeps the whole chain (nothing scoped inside was referenced). + * + * This is what enforces "crossing a lambda boundary is allowed only when nothing lambda-scoped is + * referenced": if the candidate uses `it` or a lambda parameter, the lambda body *is* the ceiling + * and every outer rung disappears. + */ +fun truncateAtCeiling( + frames: List, + ceiling: PsiElement?, +): List { + if (ceiling == null) return frames + val kept = frames.takeWhile { PsiTreeUtil.isAncestor(ceiling, it.scopeElement, false) || it.scopeElement === ceiling } + return kept.ifEmpty { frames.take(1) } +} + +/** + * Builds the rung whose scope directly contains [inner], or null when [inner] is not in a position + * this refactoring anchors in. + */ +private fun frameFor( + inner: PsiElement, + text: String, +): ScopeFrame? { + val parent = inner.parent ?: return null + + // A braceless control-structure body is wrapped in a container node, so the `if`/loop is the + // grandparent, not the parent. Without unwrapping, no braceless body is ever detected and the + // declaration silently hoists to the enclosing block instead of braces being added. + val controlOwner = (parent as? KtContainerNodeForControlStructureBody)?.parent + + if (parent is KtBlockExpression) { + val lineStart = lineStartOffset(text, inner.textRange.startOffset) + return ScopeFrame( + label = blockLabel(parent), + scopeElement = parent, + searchRange = parent.textRange.let { TextSpan(it.startOffset, it.endOffset) }, + statementSpan = TextSpan(lineStart, inner.textRange.endOffset), + anchorForm = AnchorForm.ExistingBlock, + ) + } + + val bracelessOwner = controlOwner ?: parent + val bracelessLabel = bracelessOwnerLabel(inner, bracelessOwner) + if (bracelessLabel != null) { + val indent = leadingIndentAt(text, bracelessOwner.textRange.startOffset) + val span = TextSpan(inner.textRange.startOffset, inner.textRange.endOffset) + return ScopeFrame( + label = bracelessLabel, + scopeElement = inner, + searchRange = span, + statementSpan = span, + anchorForm = + AnchorForm.WrapInBraces( + bodyStart = span.start, + bodyEnd = span.end, + indent = indent, + innerIndent = indent + detectIndentUnit(text), + ), + ) + } + + if (parent is KtDeclarationWithBody && parent.bodyExpression === inner && !parent.hasBlockBody()) { + val assign = parent.equalsToken ?: return null + val indent = leadingIndentAt(text, parent.textRange.startOffset) + val span = TextSpan(inner.textRange.startOffset, inner.textRange.endOffset) + return ScopeFrame( + label = declarationLabel(parent), + scopeElement = inner, + searchRange = span, + statementSpan = span, + anchorForm = + AnchorForm.ConvertExpressionBody( + assignStart = assign.textRange.startOffset, + bodyStart = span.start, + bodyEnd = span.end, + indent = indent, + innerIndent = indent + detectIndentUnit(text), + // Filled in by the caller, which has the resolved return type. + needsReturn = true, + ), + ) + } + + return null +} + +/** True for the body of a named function, accessor or `init` block -- where the chain stops. */ +private fun isCeilingBody(scopeElement: PsiElement): Boolean { + val owner = scopeElement.parent ?: return false + return when (owner) { + is KtNamedFunction, is KtPropertyAccessor, is KtAnonymousInitializer -> true + else -> false + } +} + +private fun blockLabel(block: KtBlockExpression): String = + when (val owner = block.parent) { + is KtNamedFunction -> "fun ${owner.name ?: ""}" + is KtPropertyAccessor -> if (owner.isGetter) "getter" else "setter" + is KtAnonymousInitializer -> "init block" + is KtFunctionLiteral -> "lambda" + is KtIfExpression -> if (owner.then === block) "if block" else "else block" + is KtForExpression -> "for loop" + is KtWhileExpression -> "while loop" + is KtDoWhileExpression -> "do-while loop" + is KtWhenEntry -> "when branch" + else -> "block" + } + +private fun declarationLabel(declaration: KtDeclarationWithBody): String = + when (declaration) { + is KtNamedFunction -> "fun ${declaration.name ?: ""}" + is KtPropertyAccessor -> if (declaration.isGetter) "getter" else "setter" + else -> "body" + } + +/** A label when [inner] is a braceless body, else null. */ +private fun bracelessOwnerLabel( + inner: PsiElement, + parent: PsiElement, +): String? = + when (parent) { + is KtIfExpression -> { + if (parent.then === inner) { + "if branch" + } else if (parent.`else` === inner) { + "else branch" + } else { + null + } + } + + is KtForExpression -> { + if (parent.body === inner) "for body" else null + } + + is KtWhileExpression -> { + if (parent.body === inner) "while body" else null + } + + is KtDoWhileExpression -> { + if (parent.body === inner) "do-while body" else null + } + + is KtWhenEntry -> { + if (parent.expression === inner) "when branch" else null + } + + else -> { + null + } + } + +/** Offset of the start of the line containing [offset]. */ +internal fun lineStartOffset( + text: String, + offset: Int, +): Int = text.lastIndexOf('\n', (offset - 1).coerceAtLeast(0)).let { if (it < 0) 0 else it + 1 } + +/** The run of spaces/tabs at the start of [offset]'s line. */ +internal fun leadingIndentAt( + text: String, + offset: Int, +): String { + val lineStart = lineStartOffset(text, offset) + return text.substring(lineStart, offset.coerceAtLeast(lineStart)).takeWhile { it == ' ' || it == '\t' } +} + +/** + * One indentation level for [text], inferred from its own lines: a tab if any line is tab-indented, + * otherwise the smallest positive run of leading spaces, defaulting to a tab (the project + * convention). Code-action edits bypass the editor's auto-indent, so emitted text must already match + * the file's style. Mirrors the detection in `ImplementMembersAction`. + */ +internal fun detectIndentUnit(text: String): String { + var minSpaces = Int.MAX_VALUE + for (line in text.splitToSequence('\n')) { + if (line.isEmpty()) continue + if (line[0] == '\t') return "\t" + if (line[0] != ' ') continue + val spaces = line.takeWhile { it == ' ' }.length + if (spaces in 1 until minSpaces) minSpaces = spaces + } + return if (minSpaces == Int.MAX_VALUE) "\t" else " ".repeat(minSpaces) +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt new file mode 100644 index 0000000000..334146c19e --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -0,0 +1,284 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Rewrite construction, with no PSI and no analysis session involved. + * + * Every assertion is on the **resulting file text** rather than on offsets. Indentation is the thing + * most likely to be wrong here -- code-action edits bypass the editor's auto-indent, so the emitted + * text has to be final -- and a range assertion cannot see an indentation bug at all. + */ +class ExtractVariableEditTest { + private fun apply( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + + private fun spanOf( + text: String, + snippet: String, + fromIndex: Int = 0, + ): TextSpan { + val start = text.indexOf(snippet, fromIndex) + require(start >= 0) { "'$snippet' not found" } + return TextSpan(start, start + snippet.length) + } + + private fun allSpansOf( + text: String, + snippet: String, + ): List { + val spans = mutableListOf() + var from = 0 + while (true) { + val start = text.indexOf(snippet, from) + if (start < 0) break + spans += TextSpan(start, start + snippet.length) + from = start + snippet.length + } + return spans + } + + private fun rewrite( + text: String, + candidate: TextSpan, + anchorForm: AnchorForm, + occurrences: List, + name: String, + replaceAll: Boolean, + ) = buildExtractVariableRewrite( + fileText = text, + candidateSpan = candidate, + scope = ScopeOption("scope", anchorForm, occurrences), + name = name, + replaceAll = replaceAll, + ) + + @Test + fun `inserts the declaration above the statement and replaces the selected occurrence`() { + val text = "fun f(items: List) {\n\tprintln(items.size * 2)\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `replace-all rewrites every occurrence and anchors above the first`() { + val text = + "fun f(items: List) {\n" + + "\tprintln(items.size * 2)\n" + + "\tlog(items.size * 2)\n" + + "\tuse(items.size * 2)\n" + + "}" + val occurrences = allSpansOf(text, "items.size * 2") + // The user selected the middle one; the declaration must still hoist above the first. + val candidate = occurrences[1] + + val result = rewrite(text, candidate, AnchorForm.ExistingBlock, occurrences, "size", replaceAll = true)!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "\tlog(size)\n" + + "\tuse(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `replace-all off leaves the other occurrences alone`() { + val text = + "fun f(items: List) {\n" + + "\tprintln(items.size * 2)\n" + + "\tlog(items.size * 2)\n" + + "}" + val occurrences = allSpansOf(text, "items.size * 2") + + val result = rewrite(text, occurrences[0], AnchorForm.ExistingBlock, occurrences, "size", replaceAll = false)!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "\tlog(items.size * 2)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `matches the file's space indentation rather than assuming tabs`() { + val text = "fun f(items: List) {\n println(items.size * 2)\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + + assertEquals( + "fun f(items: List) {\n" + + " val size = items.size * 2\n" + + " println(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `keeps CRLF line endings when the file uses them`() { + val text = "fun f(items: List) {\r\n\tprintln(items.size * 2)\r\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + + assertEquals( + "fun f(items: List) {\r\n" + + "\tval size = items.size * 2\r\n" + + "\tprintln(size)\r\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `deeper indentation is preserved`() { + val text = "class C {\n\tfun f(items: List) {\n\t\tprintln(items.size * 2)\n\t}\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + + assertEquals( + "class C {\n" + + "\tfun f(items: List) {\n" + + "\t\tval size = items.size * 2\n" + + "\t\tprintln(size)\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `wraps a braceless if branch in braces`() { + val text = "fun f(c: Boolean, a: A) {\n\tif (c) log(a.b)\n}" + val candidate = spanOf(text, "a.b") + val body = spanOf(text, "log(a.b)") + val form = + AnchorForm.WrapInBraces( + bodyStart = body.start, + bodyEnd = body.end, + indent = "\t", + innerIndent = "\t\t", + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "b", replaceAll = false)!! + + assertEquals( + "fun f(c: Boolean, a: A) {\n" + + "\tif (c) {\n" + + "\t\tval b = a.b\n" + + "\t\tlog(b)\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `converts an expression body to a block body with return`() { + val text = "fun area(r: Int) = r * r + r * r" + val occurrences = allSpansOf(text, "r * r") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = occurrences.first().start, + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = true, + ) + + val result = rewrite(text, occurrences.first(), form, occurrences, "square", replaceAll = true)!! + + assertEquals( + "fun area(r: Int) {\n" + + "\tval square = r * r\n" + + "\treturn square + square\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `omits return when the expression body function returns Unit`() { + val text = "fun show(a: A) = log(a.b)" + val candidate = spanOf(text, "a.b") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = text.indexOf("log(a.b)"), + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = false, + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "b", replaceAll = false)!! + + assertEquals( + "fun show(a: A) {\n" + + "\tval b = a.b\n" + + "\tlog(b)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `null when there is nothing to replace`() { + val text = "fun f() {}" + assertNull( + buildExtractVariableRewrite( + fileText = text, + candidateSpan = TextSpan(0, 3), + scope = ScopeOption("scope", AnchorForm.ExistingBlock, emptyList()), + name = "value", + replaceAll = true, + ), + ) + } + + @Test + fun `null when an occurrence lies outside the file`() { + val text = "fun f() {}" + assertNull( + buildExtractVariableRewrite( + fileText = text, + candidateSpan = TextSpan(0, 3), + scope = ScopeOption("scope", AnchorForm.ExistingBlock, listOf(TextSpan(0, text.length + 5))), + name = "value", + replaceAll = true, + ), + ) + } + + @Test + fun `position index line and column all agree`() { + val text = "aa\nbbb\nc" + val position = positionAt(text, text.indexOf('c')) + assertEquals(2, position.line) + assertEquals(0, position.column) + assertEquals(7, position.index) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt new file mode 100644 index 0000000000..11aff94443 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -0,0 +1,389 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The parts of the plan that need real symbol resolution: candidate filtering, the legal scope chain + * across lambda boundaries, occurrence matching by symbol identity, and reassignment soundness. + * + * Where a rewrite is produced, the assertion is on the **resulting file text** -- the only assertion + * that can catch an indentation or off-by-one error. + */ +class ExtractVariablePlanEndToEndTest : KtLspTest() { + private fun plan( + content: String, + start: Int, + end: Int = start, + ): ExtractionPlan { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + return buildExtractionPlan(env, path, start, end, documentVersion = 1, cancelChecker = noopCancelChecker()) + } + + private fun apply( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + + @Test + fun `offers the innermost three candidates, innermost first`() { + val content = + """ + package p + class B { fun c(): Int = 1 } + class A { val b: B = B() } + fun wrap(n: Int): Int = n + fun demo(a: A) { + wrap(a.b.c() * 2) + } + """.trimIndent() + + // Anchor on the call site, not the `fun c()` declaration that appears earlier in the file. + val result = plan(content, content.indexOf("a.b.c()") + "a.b.c".length) + + assertEquals( + listOf("a.b.c()", "a.b.c() * 2", "wrap(a.b.c() * 2)"), + result.candidates.map { it.label }, + ) + } + + @Test + fun `does not offer bare literals`() { + val content = + """ + package p + fun demo(n: Int): Int { + return n * 2 + } + """.trimIndent() + + val result = plan(content, content.indexOf("2", content.indexOf("n * 2"))) + + assertFalse(result.candidates.any { it.label == "2" }) + assertTrue(result.candidates.any { it.label == "n * 2" }) + } + + @Test + fun `offers nothing for a class-body property initializer`() { + val content = + """ + package p + fun compute(): Int = 1 + class C { + val x = compute() + compute() + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("compute() + compute()") + 1).isEmpty) + } + + @Test + fun `offers nothing for a default parameter value`() { + val content = + """ + package p + fun base(): Int = 1 + fun demo(n: Int = base() * 2) { + println(n) + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("base() * 2") + 1).isEmpty) + } + + @Test + fun `offers nothing when the cursor is in a comment`() { + val content = + """ + package p + fun demo() { + // nothing here + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("nothing")).isEmpty) + } + + @Test + fun `a selection matching an expression exactly short-circuits the chooser`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(n: Int) { + wrap(n * 2) + } + """.trimIndent() + val start = content.indexOf("n * 2") + + val result = plan(content, start, start + "n * 2".length) + + assertTrue(result.selectionMatchedCandidate) + assertEquals("n * 2", result.candidates.first().label) + } + + @Test + fun `an off-boundary selection still resolves, without short-circuiting`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(n: Int) { + wrap(n * 2) + } + """.trimIndent() + val start = content.indexOf("n * 2") + + // Selection stops mid-expression, as a touch-screen drag routinely does. + val result = plan(content, start, start + 3) + + assertFalse(result.selectionMatchedCandidate) + assertEquals("n * 2", result.candidates.first().label) + } + + @Test + fun `a shadowed name in a nested lambda is not the same expression`() { + val content = + """ + package p + class Config(val timeout: Int) + fun log(n: Int) {} + fun demo(config: Config, list: List) { + log(config.timeout) + list.forEach { config -> log(config.timeout) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("config.timeout") + 1) + val functionScope = + result.candidates + .first() + .scopes + .first() + + // `config` inside the lambda is a different declaration, so only one occurrence exists. + assertEquals(1, functionScope.occurrences.size) + } + + @Test + fun `the same expression in both branches of an if is one occurrence set`() { + val content = + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun warn(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) { + log(a.b) + } else { + warn(a.b) + } + } + """.trimIndent() + + val result = plan(content, content.indexOf("a.b") + 1) + val candidate = result.candidates.first { it.label == "a.b" } + // The outermost rung is the function body, which contains both branches. + val functionScope = candidate.scopes.last() + + assertEquals(2, functionScope.occurrences.size) + } + + @Test + fun `a reassignment between occurrences drops the unsound one`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(): Int { + var limit = 1 + wrap(limit + 1) + limit = 5 + wrap(limit + 1) + return limit + } + """.trimIndent() + + val result = plan(content, content.indexOf("limit + 1") + 1) + val candidate = result.candidates.first { it.label == "limit + 1" } + val functionScope = candidate.scopes.last() + + // Both sites are the same expression, but `limit = 5` makes the second a different value. + assertEquals(1, functionScope.occurrences.size) + } + + @Test + fun `a candidate using the implicit lambda parameter cannot be hoisted out of the lambda`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(items: List) { + items.forEach { log(it.length + 1) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("it.length + 1") + 1) + val candidate = result.candidates.first { it.label == "it.length + 1" } + + // `it` belongs to the lambda, so the lambda body is the only legal anchor. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + } + + @Test + fun `a lambda-invariant candidate can be hoisted to the enclosing function`() { + val content = + """ + package p + class Config(val timeout: Int) + fun log(n: Int) {} + fun demo(config: Config, items: List) { + items.forEach { log(config.timeout * 2) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("config.timeout * 2") + 1) + val candidate = result.candidates.first { it.label == "config.timeout * 2" } + + // Nothing lambda-scoped is referenced, so hoisting out to the function body is offered. + assertEquals(listOf("lambda", "fun demo"), candidate.scopes.map { it.label }) + } + + @Test + fun `suggests a name from the expression shape`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List) { + wrap(items.size * 2) + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size") + 1) + + assertEquals("size", result.candidates.first { it.label == "items.size" }.suggestedName) + } + + @Test + fun `does not suggest a name that is already taken`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List) { + val size = 0 + wrap(items.size * 2) + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size") + 1) + + assertEquals("size1", result.candidates.first { it.label == "items.size" }.suggestedName) + } + + @Test + fun `end to end rewrite replaces all occurrences in the function body`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List): Int { + wrap(items.size * 2) + return items.size * 2 + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size * 2") + 1) + val candidate = result.candidates.first { it.label == "items.size * 2" } + val scope = candidate.scopes.last() + assertEquals(2, scope.occurrences.size) + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "size", replaceAll = true) + assertNotNull(rewrite) + + assertEquals( + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List): Int { + val size = items.size * 2 + wrap(size) + return size + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } + + @Test + fun `end to end rewrite converts an expression-bodied function to a block body`() { + val content = + """ + package p + fun area(r: Int) = r * r + r * r + """.trimIndent() + + val result = plan(content, content.indexOf("r * r") + 1) + val candidate = result.candidates.first { it.label == "r * r" } + val scope = candidate.scopes.first() + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "square", replaceAll = true) + assertNotNull(rewrite) + + assertEquals( + """ + package p + fun area(r: Int) { + val square = r * r + return square + square + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } + + @Test + fun `end to end rewrite wraps a braceless if branch`() { + val content = + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) log(a.b + 1) + } + """.trimIndent() + + val result = plan(content, content.indexOf("a.b + 1") + 1) + val candidate = result.candidates.first { it.label == "a.b + 1" } + val scope = candidate.scopes.first() + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "offset", replaceAll = false) + assertNotNull(rewrite) + + assertEquals( + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) { + val offset = a.b + 1 + log(offset) + } + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt new file mode 100644 index 0000000000..1d212b8404 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt @@ -0,0 +1,142 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** The analysis-free primitives the refactoring is built from: name rules, indentation, soundness. */ +class RefactorPrimitivesTest { + @Test + fun `rejects blank names`() { + assertEquals(NameProblem.Blank, validateVariableName("", emptySet())) + assertEquals(NameProblem.Blank, validateVariableName(" ", emptySet())) + } + + @Test + fun `rejects non-identifiers`() { + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("1size", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("my size", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("size!", emptySet())) + // Backticked names are legal Kotlin but deliberately unsupported for a generated local. + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("`size`", emptySet())) + } + + @Test + fun `rejects hard keywords but allows soft ones`() { + assertEquals(NameProblem.Keyword, validateVariableName("val", emptySet())) + assertEquals(NameProblem.Keyword, validateVariableName("when", emptySet())) + assertEquals(NameProblem.Keyword, validateVariableName("this", emptySet())) + // `it`, `data` and `by` are soft keywords -- perfectly legal identifiers. + assertNull(validateVariableName("it", emptySet())) + assertNull(validateVariableName("data", emptySet())) + assertNull(validateVariableName("by", emptySet())) + } + + @Test + fun `rejects names already in use`() { + assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", setOf("size"))) + assertNull(validateVariableName("size", setOf("count"))) + } + + @Test + fun `accepts underscores and digits`() { + assertNull(validateVariableName("_size", emptySet())) + assertNull(validateVariableName("size2", emptySet())) + } + + @Test + fun `detects a tab indent unit`() { + assertEquals("\t", detectIndentUnit("fun f() {\n\tval x = 1\n}")) + } + + @Test + fun `detects the smallest space indent unit`() { + assertEquals(" ", detectIndentUnit("fun f() {\n val x = 1\n val y = 2\n}")) + assertEquals(" ", detectIndentUnit("fun f() {\n val x = 1\n}")) + } + + @Test + fun `falls back to a tab when nothing is indented`() { + assertEquals("\t", detectIndentUnit("fun f() {}")) + } + + @Test + fun `leading indent is read from the offset's own line`() { + val text = "class C {\n\t\tval x = 1\n}" + assertEquals("\t\t", leadingIndentAt(text, text.indexOf("val x"))) + assertEquals("", leadingIndentAt(text, text.indexOf("class"))) + } + + @Test + fun `line start is found for the first and later lines`() { + val text = "aa\nbbb\nc" + assertEquals(0, lineStartOffset(text, 1)) + assertEquals(3, lineStartOffset(text, 4)) + assertEquals(7, lineStartOffset(text, 7)) + } + + @Test + fun `label collapses whitespace and truncates`() { + assertEquals("items.filter { it > 0 }", collapseForLabel("items\n\t.filter { it > 0 }")) + assertEquals("a?.b", collapseForLabel("a\n\t?.b")) + assertEquals("aaaaaaa...", collapseForLabel("aaaaaaaaaaaa", maxLength = 10)) + } + + @Test + fun `trim drops surrounding whitespace from a selection`() { + val text = " items.size " + assertEquals(2 to 12, trimToCode(text, 0, text.length)) + } + + @Test + fun `trim leaves a cursor untouched and rejects a whitespace-only selection`() { + assertEquals(3 to 3, trimToCode("a b", 3, 3)) + assertNull(trimToCode("a b", 1, 5)) + } + + @Test + fun `soundness keeps every occurrence when nothing is written`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + assertEquals( + occurrences, + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = emptyList()), + ) + } + + @Test + fun `soundness drops occurrences separated from the candidate by a write`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + // A reassignment between the second and third sites: the third no longer holds the same value. + assertEquals( + listOf(TextSpan(10, 20), TextSpan(30, 40)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(45)), + ) + } + + @Test + fun `soundness drops earlier occurrences when the write precedes the candidate`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + assertEquals( + listOf(TextSpan(30, 40), TextSpan(50, 60)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(25)), + ) + } + + @Test + fun `soundness always keeps the occurrence the user selected`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + // Writes on both sides isolate the candidate, but it must never be dropped. + assertEquals( + listOf(TextSpan(30, 40)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(25, 45)), + ) + } + + @Test + fun `soundness falls back to the candidate alone when it is not among the occurrences`() { + assertEquals( + listOf(TextSpan(70, 80)), + excludeUnsoundOccurrences(listOf(TextSpan(10, 20)), TextSpan(70, 80), writeOffsets = emptyList()), + ) + } +} From 964d2104d39d4484a7364195f28f03bd97488ad9 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 14:18:22 +0000 Subject: [PATCH 03/16] ADFA-4826: Add the extract-variable Compose sheet One surface holding every choice - expression, name, scope, replace-all - because they are interdependent: a different expression changes the scope list and the occurrence count, and sequential dialogs would hide that. Each chooser is hidden when it has nothing to ask. State derives entirely from the plan, so the ViewModel is a plain unit test with no editor, activity or Compose. Uses the shared IdeTheme from common-compose. --- .../refactor/ui/ExtractVariableSheet.kt | 117 ++++++++++ .../ui/ExtractVariableSheetContent.kt | 200 ++++++++++++++++++ .../refactor/ui/ExtractVariableUiState.kt | 71 +++++++ .../refactor/ui/ExtractVariableViewModel.kt | 118 +++++++++++ .../ui/ExtractVariableViewModelTest.kt | 195 +++++++++++++++++ resources/src/main/res/values/strings.xml | 18 ++ 6 files changed, 719 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt new file mode 100644 index 0000000000..17ffdf7dba --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt @@ -0,0 +1,117 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import android.content.Context +import android.content.ContextWrapper +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.compose.runtime.getValue +import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.ViewCompositionStrategy +import androidx.fragment.app.FragmentActivity +import androidx.fragment.app.viewModels +import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.google.android.material.bottomsheet.BottomSheetDialogFragment +import com.itsaky.androidide.common.compose.IdeTheme +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan + +/** + * Hosts [ExtractVariableSheetContent]. + * + * The plan is handed in directly rather than through fragment arguments: it carries the file's text and + * offset spans, which is neither `Parcelable` nor meaningful to restore -- after process death the + * document may be entirely different. So [plan] is null on a recreated instance and the sheet dismisses + * itself, which is the same outcome the action's document-version guard would reach anyway. + */ +class ExtractVariableSheet : BottomSheetDialogFragment() { + private var plan: ExtractionPlan? = null + private var onChoice: ((ExtractionChoice) -> Unit)? = null + + private val viewModel: ExtractVariableViewModel by viewModels { + ExtractVariableViewModel.factory(requireNotNull(plan) { "sheet shown without a plan" }) + } + + override fun onCreateView( + inflater: LayoutInflater, + container: ViewGroup?, + savedInstanceState: Bundle?, + ): View? { + if (plan == null) { + dismissAllowingStateLoss() + return null + } + + return ComposeView(requireContext()).apply { + // The sheet's window is torn down with the fragment's view, so dispose with it. + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { + IdeTheme { + val state by viewModel.uiState.collectAsStateWithLifecycle() + ExtractVariableSheetContent( + state = state, + onEvent = ::handleEvent, + ) + } + } + } + } + + private fun handleEvent(event: ExtractVariableUiEvent) { + when (event) { + ExtractVariableUiEvent.Confirmed -> { + viewModel.choice()?.let { choice -> onChoice?.invoke(choice) } + dismiss() + } + + ExtractVariableUiEvent.Dismissed -> { + dismiss() + } + + else -> { + viewModel.onEvent(event) + } + } + } + + companion object { + private const val TAG = "extract_variable_sheet" + + /** + * Shows the sheet on [activity], calling [onChoice] once if the user confirms. + * + * Returns false when the sheet could not be shown, so the caller can report a failure rather + * than silently doing nothing. + */ + fun show( + activity: FragmentActivity, + plan: ExtractionPlan, + onChoice: (ExtractionChoice) -> Unit, + ): Boolean { + val manager = activity.supportFragmentManager + if (manager.isStateSaved || manager.isDestroyed) return false + ExtractVariableSheet() + .apply { + this.plan = plan + this.onChoice = onChoice + }.show(manager, TAG) + return true + } + } +} + +/** + * Finds the [FragmentActivity] hosting this context by unwrapping the [ContextWrapper] chain. + * + * A view inflated into an activity reports that activity as its context, but a theme overlay wraps it, + * so a direct cast is not reliable. `ActionData` carries only the editor's `Context`, and adding a + * `FragmentActivity` key would only move the same unwrapping one module upstream, into `editor`. + */ +fun Context.findFragmentActivity(): FragmentActivity? { + var context: Context? = this + while (context != null) { + if (context is FragmentActivity) return context + context = (context as? ContextWrapper)?.baseContext + } + return null +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt new file mode 100644 index 0000000000..25409974ee --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt @@ -0,0 +1,200 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.navigationBarsPadding +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.selection.selectable +import androidx.compose.foundation.selection.selectableGroup +import androidx.compose.foundation.selection.toggleable +import androidx.compose.material3.Button +import androidx.compose.material3.Checkbox +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedTextField +import androidx.compose.material3.RadioButton +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.pluralStringResource +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.Role +import androidx.compose.ui.text.font.FontFamily +import androidx.compose.ui.unit.dp +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.resources.R + +/** + * The extract-variable sheet: one surface holding every choice, with no navigation between steps. + * + * Expression, name, scope and replace-all are interdependent -- picking a different expression changes + * the scope list and the occurrence count -- so they are shown together, where that relationship is + * visible, rather than across sequential dialogs the user would have to back out of to explore. + * + * Stateless: all state arrives in [state] and every interaction leaves as an [ExtractVariableUiEvent]. + */ +@Composable +fun ExtractVariableSheetContent( + state: ExtractVariableUiState, + onEvent: (ExtractVariableUiEvent) -> Unit, + modifier: Modifier = Modifier, +) { + Column( + modifier = + modifier + .fillMaxWidth() + .navigationBarsPadding() + .padding(horizontal = 24.dp, vertical = 16.dp), + verticalArrangement = Arrangement.spacedBy(16.dp), + ) { + Text( + text = stringResource(R.string.title_extract_variable), + style = MaterialTheme.typography.titleLarge, + ) + + if (state.showCandidatePicker) { + LabelledSection(stringResource(R.string.label_extract_variable_expression)) { + OptionList( + options = state.candidateLabels, + selected = state.selectedCandidate, + monospace = true, + onSelect = { onEvent(ExtractVariableUiEvent.CandidateSelected(it)) }, + ) + } + } + + OutlinedTextField( + value = state.name, + onValueChange = { onEvent(ExtractVariableUiEvent.NameChanged(it)) }, + label = { Text(stringResource(R.string.label_extract_variable_name)) }, + isError = state.nameProblem != null, + singleLine = true, + supportingText = state.nameProblem?.let { problem -> { Text(stringResource(problem.messageRes())) } }, + modifier = Modifier.fillMaxWidth(), + ) + + if (state.showScopePicker) { + LabelledSection(stringResource(R.string.label_extract_variable_scope)) { + OptionList( + options = state.scopeLabels, + selected = state.selectedScope, + monospace = false, + onSelect = { onEvent(ExtractVariableUiEvent.ScopeSelected(it)) }, + ) + } + } + + if (state.showReplaceAll) { + val replaceAllLabel = + pluralStringResource( + R.plurals.label_extract_variable_replace_all, + state.occurrenceCount, + state.occurrenceCount, + ) + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = + Modifier + .fillMaxWidth() + .toggleable( + value = state.replaceAll, + role = Role.Checkbox, + onValueChange = { onEvent(ExtractVariableUiEvent.ReplaceAllChanged(it)) }, + ), + ) { + Checkbox( + checked = state.replaceAll, + // Null so the row, not the box, is the single accessibility target. + onCheckedChange = null, + ) + Text( + text = replaceAllLabel, + modifier = Modifier.padding(start = 8.dp), + ) + } + } + + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.End, + ) { + TextButton(onClick = { onEvent(ExtractVariableUiEvent.Dismissed) }) { + Text(stringResource(android.R.string.cancel)) + } + Button( + onClick = { onEvent(ExtractVariableUiEvent.Confirmed) }, + enabled = state.canConfirm, + modifier = Modifier.padding(start = 8.dp), + ) { + Text(stringResource(R.string.action_extract)) + } + } + } +} + +@Composable +private fun LabelledSection( + label: String, + content: @Composable () -> Unit, +) { + Column(verticalArrangement = Arrangement.spacedBy(4.dp)) { + Text(text = label, style = MaterialTheme.typography.labelLarge) + content() + } +} + +/** A radio group. Expression text is monospaced so a candidate reads as the code it is. */ +@Composable +private fun OptionList( + options: List, + selected: Int, + monospace: Boolean, + onSelect: (Int) -> Unit, +) { + Column( + modifier = Modifier.selectableGroup(), + verticalArrangement = Arrangement.spacedBy(8.dp), + ) { + options.forEachIndexed { index, option -> + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = + Modifier + .fillMaxWidth() + .selectable( + selected = index == selected, + role = Role.RadioButton, + onClick = { onSelect(index) }, + ), + ) { + RadioButton( + selected = index == selected, + onClick = null, + ) + + Text( + text = option, + style = + if (monospace) { + MaterialTheme.typography.bodyMedium.copy(fontFamily = FontFamily.Monospace) + } else { + MaterialTheme.typography.bodyMedium + }, + modifier = Modifier.padding(start = 8.dp), + ) + } + } + } +} + +/** The message shown under the name field for each way a name can be unusable. */ +internal fun NameProblem.messageRes(): Int = + when (this) { + NameProblem.Blank -> R.string.msg_extract_variable_name_blank + NameProblem.NotAnIdentifier -> R.string.msg_extract_variable_name_invalid + NameProblem.Keyword -> R.string.msg_extract_variable_name_keyword + NameProblem.AlreadyTaken -> R.string.msg_extract_variable_name_taken + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt new file mode 100644 index 0000000000..c5937f79dd --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt @@ -0,0 +1,71 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.CandidateExpression +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ScopeOption + +/** + * Everything the extract-variable sheet renders, derived entirely from the + * [com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan]. + * + * [showCandidatePicker] is false when the plan holds a single candidate, or when the user's selection + * already matched an expression exactly -- in both cases asking which expression they meant would be + * asking a question they have already answered. + * + * [occurrenceCount] counts every site the selected scope would rewrite, **including** the one the user + * selected, so "Replace all 3 occurrences" means three sites in total. [showReplaceAll] is false at a + * count of one, where the toggle would have nothing to do. + */ +data class ExtractVariableUiState( + val candidateLabels: List, + val selectedCandidate: Int, + val showCandidatePicker: Boolean, + val name: String, + val nameProblem: NameProblem?, + val scopeLabels: List, + val selectedScope: Int, + val occurrenceCount: Int, + val replaceAll: Boolean, +) { + val showReplaceAll: Boolean get() = occurrenceCount > 1 + + val showScopePicker: Boolean get() = scopeLabels.size > 1 + + val canConfirm: Boolean get() = nameProblem == null +} + +/** What the sheet reports back up; the ViewModel never touches the document itself. */ +sealed interface ExtractVariableUiEvent { + data class CandidateSelected( + val index: Int, + ) : ExtractVariableUiEvent + + data class NameChanged( + val name: String, + ) : ExtractVariableUiEvent + + data class ScopeSelected( + val index: Int, + ) : ExtractVariableUiEvent + + data class ReplaceAllChanged( + val replaceAll: Boolean, + ) : ExtractVariableUiEvent + + data object Confirmed : ExtractVariableUiEvent + + data object Dismissed : ExtractVariableUiEvent +} + +/** + * The user's finished decision, handed to the action to turn into an edit. + * + * Kept free of offsets and text so the sheet stays a pure chooser: resolving this into a rewrite, and + * checking the document has not moved on, both belong to the action. + */ +data class ExtractionChoice( + val candidate: CandidateExpression, + val scope: ScopeOption, + val name: String, + val replaceAll: Boolean, +) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt new file mode 100644 index 0000000000..6d9494593e --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt @@ -0,0 +1,118 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.lifecycle.ViewModel +import androidx.lifecycle.ViewModelProvider +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.validateVariableName +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow + +/** + * Derives the sheet's state from an [ExtractionPlan] and nothing else. + * + * The plan already contains every candidate's scope chain and occurrence set, so switching expression + * or scope is pure recomputation -- no analysis, no PSI, no I/O. That is what lets this class hold all + * the sheet's logic while remaining a plain unit test. + * + * A plain [ViewModelProvider.Factory] rather than a Koin definition (ADR 0006/0009 resolve ViewModels + * through Koin): this one is sheet-scoped, injects nothing, and takes the plan as a runtime argument, + * so a Koin definition would add indirection without providing anything. + */ +class ExtractVariableViewModel( + private val plan: ExtractionPlan, +) : ViewModel() { + private val _uiState = MutableStateFlow(initialState()) + val uiState: StateFlow = _uiState.asStateFlow() + + private fun initialState(): ExtractVariableUiState = stateFor(candidateIndex = 0, scopeIndex = 0, replaceAll = false, name = null) + + fun onEvent(event: ExtractVariableUiEvent) { + val current = _uiState.value + when (event) { + is ExtractVariableUiEvent.CandidateSelected -> { + if (event.index == current.selectedCandidate) return + // A different expression means a different suggested name, scope chain and count, so the + // name is re-suggested rather than carried over -- the old one described the old expression. + _uiState.value = stateFor(event.index, scopeIndex = 0, replaceAll = false, name = null) + } + + is ExtractVariableUiEvent.ScopeSelected -> { + if (event.index == current.selectedScope) return + _uiState.value = + stateFor(current.selectedCandidate, event.index, current.replaceAll, current.name) + } + + is ExtractVariableUiEvent.NameChanged -> { + _uiState.value = + current.copy( + name = event.name, + nameProblem = validateVariableName(event.name, candidate(current.selectedCandidate).takenNames), + ) + } + + is ExtractVariableUiEvent.ReplaceAllChanged -> { + _uiState.value = current.copy(replaceAll = event.replaceAll) + } + + ExtractVariableUiEvent.Confirmed, ExtractVariableUiEvent.Dismissed -> { + Unit + } + } + } + + /** The user's decision, or null when the name is unusable. */ + fun choice(): ExtractionChoice? { + val state = _uiState.value + if (!state.canConfirm) return null + val candidate = candidate(state.selectedCandidate) + val scope = candidate.scopes.getOrNull(state.selectedScope) ?: return null + return ExtractionChoice( + candidate = candidate, + scope = scope, + name = state.name, + // A single occurrence makes the toggle meaningless, and the sheet hides it; make sure a + // stale `true` from a previous candidate cannot leak into the choice. + replaceAll = state.replaceAll && state.occurrenceCount > 1, + ) + } + + private fun candidate(index: Int) = plan.candidates[index.coerceIn(plan.candidates.indices)] + + /** + * Recomputes the whole state for a (candidate, scope) pair. [name] carries the user's typed name + * across a scope change; pass null to take the candidate's suggestion. + */ + private fun stateFor( + candidateIndex: Int, + scopeIndex: Int, + replaceAll: Boolean, + name: String?, + ): ExtractVariableUiState { + val candidate = candidate(candidateIndex) + val boundedScope = scopeIndex.coerceIn(candidate.scopes.indices) + val scope = candidate.scopes[boundedScope] + val resolvedName = name ?: candidate.suggestedName + val occurrenceCount = scope.occurrences.size + + return ExtractVariableUiState( + candidateLabels = plan.candidates.map { it.label }, + selectedCandidate = candidateIndex.coerceIn(plan.candidates.indices), + showCandidatePicker = plan.candidates.size > 1 && !plan.selectionMatchedCandidate, + name = resolvedName, + nameProblem = validateVariableName(resolvedName, candidate.takenNames), + scopeLabels = candidate.scopes.map { it.label }, + selectedScope = boundedScope, + occurrenceCount = occurrenceCount, + replaceAll = replaceAll && occurrenceCount > 1, + ) + } + + companion object { + fun factory(plan: ExtractionPlan): ViewModelProvider.Factory = + object : ViewModelProvider.Factory { + @Suppress("UNCHECKED_CAST") + override fun create(modelClass: Class): T = ExtractVariableViewModel(plan) as T + } + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt new file mode 100644 index 0000000000..4f25b9a3aa --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt @@ -0,0 +1,195 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.AnchorForm +import com.itsaky.androidide.lsp.kotlin.utils.refactor.CandidateExpression +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ScopeOption +import com.itsaky.androidide.lsp.kotlin.utils.refactor.TextSpan +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The sheet's derivation logic, tested without Compose, a fragment or an activity. + * + * Every choice the sheet offers is recomputed from the plan, so all of this is exercisable as plain + * state transitions -- which is the point of keeping the plan plain data. + */ +class ExtractVariableViewModelTest { + private fun scope( + label: String, + occurrences: Int, + ) = ScopeOption( + label = label, + anchorForm = AnchorForm.ExistingBlock, + occurrences = (0 until occurrences).map { TextSpan(it * 10, it * 10 + 5) }, + ) + + private fun candidate( + label: String, + suggestedName: String, + scopes: List, + takenNames: Set = emptySet(), + ) = CandidateExpression( + label = label, + span = TextSpan(0, 5), + suggestedName = suggestedName, + takenNames = takenNames, + scopes = scopes, + ) + + private fun plan( + candidates: List, + selectionMatched: Boolean = false, + ) = ExtractionPlan( + fileText = "unused", + documentVersion = 1, + candidates = candidates, + selectionMatchedCandidate = selectionMatched, + ) + + private val threeCandidatePlan = + plan( + listOf( + candidate("items.size", "size", listOf(scope("lambda", 1), scope("fun demo", 3))), + candidate("items.size * 2", "size1", listOf(scope("fun demo", 2))), + candidate("wrap(items.size * 2)", "wrap", listOf(scope("fun demo", 1))), + ), + ) + + @Test + fun `starts on the innermost candidate, innermost scope, replace-all off`() { + val state = ExtractVariableViewModel(threeCandidatePlan).uiState.value + + assertEquals(0, state.selectedCandidate) + assertEquals(0, state.selectedScope) + assertEquals("size", state.name) + assertFalse(state.replaceAll) + assertTrue(state.canConfirm) + } + + @Test + fun `shows the candidate picker only when there is a real choice`() { + assertTrue(ExtractVariableViewModel(threeCandidatePlan).uiState.value.showCandidatePicker) + + val single = plan(listOf(candidate("items.size", "size", listOf(scope("fun demo", 1))))) + assertFalse(ExtractVariableViewModel(single).uiState.value.showCandidatePicker) + } + + @Test + fun `an exact selection suppresses the candidate picker`() { + // The user already said which expression they meant by selecting it. + val matched = plan(threeCandidatePlan.candidates, selectionMatched = true) + assertFalse(ExtractVariableViewModel(matched).uiState.value.showCandidatePicker) + } + + @Test + fun `changing the expression re-derives name, scopes and count`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + + viewModel.onEvent(ExtractVariableUiEvent.CandidateSelected(1)) + val state = viewModel.uiState.value + + assertEquals("size1", state.name) + assertEquals(listOf("fun demo"), state.scopeLabels) + assertEquals(2, state.occurrenceCount) + assertEquals(0, state.selectedScope) + } + + @Test + fun `changing the scope changes the occurrence count`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + assertEquals(1, viewModel.uiState.value.occurrenceCount) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertEquals(1, viewModel.uiState.value.selectedScope) + assertEquals(3, viewModel.uiState.value.occurrenceCount) + } + + @Test + fun `a scope change keeps the name the user typed`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("mySize")) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertEquals("mySize", viewModel.uiState.value.name) + } + + @Test + fun `the replace-all toggle is hidden at a single occurrence`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + assertFalse(viewModel.uiState.value.showReplaceAll) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertTrue(viewModel.uiState.value.showReplaceAll) + } + + @Test + fun `an invalid name blocks confirming`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("val")) + + assertEquals(NameProblem.Keyword, viewModel.uiState.value.nameProblem) + assertFalse(viewModel.uiState.value.canConfirm) + assertNull(viewModel.choice()) + } + + @Test + fun `a name colliding with a visible declaration is rejected`() { + val colliding = + plan(listOf(candidate("items.size", "size1", listOf(scope("fun demo", 1)), takenNames = setOf("size")))) + val viewModel = ExtractVariableViewModel(colliding) + + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("size")) + + assertEquals(NameProblem.AlreadyTaken, viewModel.uiState.value.nameProblem) + } + + @Test + fun `the choice carries the selected expression, scope, name and toggle`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("total")) + + val choice = viewModel.choice() + assertNotNull(choice) + assertEquals("items.size", choice!!.candidate.label) + assertEquals("fun demo", choice.scope.label) + assertEquals("total", choice.name) + assertTrue(choice.replaceAll) + } + + @Test + fun `replace-all cannot leak from a wider scope into a single-occurrence one`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + assertTrue(viewModel.uiState.value.replaceAll) + + // Back to the lambda scope, which has one occurrence and no visible toggle. + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(0)) + + assertFalse(viewModel.uiState.value.replaceAll) + assertFalse(viewModel.choice()!!.replaceAll) + } + + @Test + fun `switching expression resets replace-all`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + + viewModel.onEvent(ExtractVariableUiEvent.CandidateSelected(1)) + + assertFalse(viewModel.uiState.value.replaceAll) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 33f210b86b..2b657596a5 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -523,6 +523,24 @@ Suppress \'unchecked\' warning Uncomment line Convert to statement + + + Extract variable + Extract variable + Expression + Name + Declare in + + Replace %1$d occurrence + Replace all %1$d occurrences + + Extract + Enter a name + Not a valid Kotlin name + That is a Kotlin keyword + That name is already used + No expression to extract here + The file changed. Try extracting again. Select fields No fields selected No fields found From 14d1a30ee5aa38f1c29536b44694993aef4bc5a4 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 14:18:34 +0000 Subject: [PATCH 04/16] ADFA-4826: Wire up the extract-variable code action execAction runs the analysis off the UI thread and returns the plan; postExec shows the sheet and turns the user's choice into one spanning TextEdit. The document version is re-read on confirm - the editor stays reachable while the sheet is open, and applying spans computed against older text would corrupt the file. No prepare() visibility gate: deciding extractability needs an analysis session, far too costly for the UI thread. Records the placement decision as ADR 0011. --- ...oring-ui-lives-in-the-owning-lsp-module.md | 52 ++++++ docs/adr/README.md | 1 + .../androidide/idetooltips/TooltipTag.kt | 1 + lsp/kotlin/build.gradle.kts | 2 +- .../lsp/kotlin/KotlinCodeActionsMenu.kt | 2 + .../kotlin/actions/ExtractVariableAction.kt | 155 ++++++++++++++++++ .../kotlin/KotlinCodeActionTooltipTagTest.kt | 2 + 7 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt diff --git a/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md b/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md new file mode 100644 index 0000000000..92a2a0b16b --- /dev/null +++ b/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md @@ -0,0 +1,52 @@ +# 0012. Refactoring UI lives in the owning LSP module + +- **Status:** Proposed +- **Date:** 2026-08-03 +- **Deciders:** Code On The Go team + +## Context + +The K2 Kotlin LSP is gaining interactive refactorings: extract variable and extract method (ADFA-4826), inline variable (ADFA-4827), semantic rename (ADFA-4825). Unlike every existing Kotlin code action, these cannot be a single fire-and-forget edit — the user has to choose an expression, a name, a target scope, and whether to replace other occurrences. That is a real UI surface, not a `DialogUtils` one-liner. + +[ADR 0009](0009-jetpack-compose-for-new-ui.md) settles *what* that UI is built with (Compose, UDF, `ViewModel` + `StateFlow`). It says nothing about *where* language-specific UI lives, and the module graph makes that a genuine question: + +- `editor` depends on `lsp/kotlin` (`editor/build.gradle.kts`), so the dependency flows **LSP -> editor**. An LSP module cannot reach the editor or `app`. +- `ActionData` carries only a `Context` and the editor; there is no service-lookup mechanism for an LSP module to call *up* into a UI layer. +- `lsp/java` already owns UI code today — `AutoFixImportsAction` builds and shows a `DialogUtils` chooser directly. + +So a refactoring in `lsp/kotlin` either renders its own UI, or a new inversion mechanism has to be invented for it. + +## Decision + +**A language server module owns the UI for its own refactorings.** `lsp/kotlin` enables Compose and hosts the refactoring bottom sheets; the same applies to any future `lsp/*` module that grows an interactive refactoring. + +- Compose is enabled per-module exactly as `flamegraph`, `floating-window` and `profiler` do it: the `kotlin-compose` plugin, `compose = true`, and the Compose BOM with `ui`/`foundation`/`material3`. +- The UI is a `BottomSheetDialogFragment` hosting a `ComposeView`. The hosting `FragmentActivity` is found by walking `ContextWrapper.baseContext` up from `ActionData`'s `Context` — no new `ActionData` key, no change to the `editor` module. +- **The analysis/UI split is enforced by data, not by module boundaries.** The action's background pass produces a plain-data plan (candidate expressions, scope chains, occurrence ranges, suggested name, document version); the sheet performs no analysis and holds no PSI. All refactoring logic lives in pure functions, unit-testable without an editor, an activity, or Compose. +- ADR 0009 otherwise applies unchanged: `ViewModel` + `StateFlow`, sealed `UiEvent`, `collectAsStateWithLifecycle()`. + +## Consequences + +**Positive** +- No new indirection: one module, one PR per refactoring, no interface to register or resolve. +- Consistent with `lsp/java` already owning its dialogs, so there is one rule for LSP-owned UI rather than two. +- The plain-data plan boundary keeps the valuable logic testable regardless of where the UI sits, so the placement decision does not compromise test coverage. + +**Negative / costs** +- A language server module gains a UI surface, which is a layering smell: `lsp/kotlin` is no longer purely a language service. +- Compose and `lifecycle-viewmodel` are added to a module that previously had neither, growing its build surface and bringing ktlint's compose-rules ruleset to bear on it. +- Walking the `ContextWrapper` chain for a `FragmentActivity` is an implicit dependency on how the editor is hosted; a future change to that hosting breaks it at runtime rather than at compile time. +- If three or more `lsp/*` modules end up with Compose UI, extracting a shared UI module becomes worthwhile and this decision will need revisiting. + +## Alternatives considered + +- **Render in `editor`, invert via an interface.** Declare a refactoring-UI interface in `editorApi` or `lsp/models`, implement it in `editor`, have `lsp/kotlin` call up through it. Cleanest layering. Rejected: nothing registers such an implementation today, so it means inventing a service-lookup mechanism for one sheet, and the interface would be guessed from a single client. +- **Render in `app`.** `app` is the integration point and already hosts `BottomSheetDialogFragment`s and `ILanguageClient`. Rejected: same inversion problem, and it puts Kotlin-specific refactoring UI in the module where nothing else language-specific lives. +- **A new `lsp/kotlin-ui` module.** Keeps Compose out of `lsp/kotlin` without inverting. Rejected for now: a new Gradle module in a ~80-module build is disproportionate for one sheet. Reconsider once extract-method and inline-variable have landed and the UI surface is known. + +## Related + +- [ADR 0009](0009-jetpack-compose-for-new-ui.md) — Compose for new UI; this ADR answers *where*, not *what*. +- [ADR 0006](0006-koin-dependency-injection.md) — Koin DI, unchanged. +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) — the K2 Analysis API as the Kotlin semantic source of truth. +- [ARCHITECTURE.md](../../ARCHITECTURE.md) — module map, layering, UDF. diff --git a/docs/adr/README.md b/docs/adr/README.md index 7139d240d5..554f429bee 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -25,3 +25,4 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0009](0009-jetpack-compose-for-new-ui.md) | Build new UI in Jetpack Compose, not XML Views | Proposed | | [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed | | [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed | +| [0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt index 02a0571d1f..ac8fd24d98 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -93,6 +93,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH = "editor.codeactions.kotlin.trycatch" const val EDITOR_CODE_ACTIONS_KT_GOTO_DEF = "editor.codeactions.kotlin.gotodef" const val EDITOR_CODE_ACTIONS_KT_FIND_REFS = "editor.codeactions.kotlin.findrefs" + const val EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE = "editor.codeactions.kotlin.extractvariable" const val EXIT_TO_MAIN = "exit.to.main" diff --git a/lsp/kotlin/build.gradle.kts b/lsp/kotlin/build.gradle.kts index 27f92b80a7..d25dd4a40a 100644 --- a/lsp/kotlin/build.gradle.kts +++ b/lsp/kotlin/build.gradle.kts @@ -28,7 +28,7 @@ android { namespace = "${BuildConfig.PACKAGE_NAME}.lsp.kotlin" // The refactoring bottom sheets are Compose (ADR 0009); they live here rather than in a UI - // module because `editor` depends on this module, not the reverse (ADR 0011). + // module because `editor` depends on this module, not the reverse (ADR 0012). buildFeatures { compose = true } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt index 1188a15022..311d0dadd6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt @@ -6,6 +6,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.IActionsMenuProvider import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction @@ -39,5 +40,6 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { SurroundWithTryCatchAction(), NullSafetyAction(), ImplementMembersAction(), + ExtractVariableAction(), ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt new file mode 100644 index 0000000000..086c8060f7 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt @@ -0,0 +1,155 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.get +import com.itsaky.androidide.actions.requireContext +import com.itsaky.androidide.actions.requireEditor +import com.itsaky.androidide.actions.requireFile +import com.itsaky.androidide.idetooltips.TooltipTag +import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractVariableSheet +import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractionChoice +import com.itsaky.androidide.lsp.kotlin.refactor.ui.findFragmentActivity +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractVariableRewrite +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.toTextEdit +import com.itsaky.androidide.lsp.models.CodeActionItem +import com.itsaky.androidide.lsp.models.CodeActionKind +import com.itsaky.androidide.lsp.models.Command +import com.itsaky.androidide.lsp.models.DocumentChange +import com.itsaky.androidide.projects.FileManager +import com.itsaky.androidide.resources.R +import com.itsaky.androidide.tasks.createJobCancelChecker +import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo +import java.nio.file.Path + +/** + * Extracts the expression at the cursor, or the selected one, into a local `val`. + * + * The work is split so nothing heavy touches the UI thread: [execAction] runs one background analysis + * pass and returns a plain-data [ExtractionPlan] covering every candidate, then [postExec] shows the + * sheet and turns the user's choice into a single text edit with pure offset arithmetic. + */ +class ExtractVariableAction : BaseKotlinCodeAction() { + companion object { + const val ID = "ide.editor.lsp.kt.extractVariable" + } + + override var titleTextRes: Int = R.string.action_extract_variable + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE + + override val id: String = ID + override var label: String = "" + + // Analysis must not run on the UI thread. The selection is therefore read at the top of + // execAction on a background thread, as ImplementMembersAction does; a torn read while the user + // is mid-edit can only produce a plan the document-version guard then refuses to apply. + override var requiresUIThread: Boolean = false + + // Intentionally no prepare() visibility gate: deciding whether anything is extractable needs a K2 + // analysis session, far too costly for prepare() (UI thread). The action stays visible on any + // Kotlin file and reports "nothing to extract" instead. Matches OrganizeImportsAction and + // ImplementMembersAction. + + override suspend fun execAction(data: ActionData): ExtractionPlan { + val server = data.get() ?: return ExtractionPlan.empty() + val nioPath = data.requireFile().toPath() + val env = server.compilationEnvironmentFor(nioPath) ?: return ExtractionPlan.empty() + + val cursor = data.requireEditor().cursor + val selectionStart = minOf(cursor.left, cursor.right) + val selectionEnd = maxOf(cursor.left, cursor.right) + + return buildExtractionPlan( + env = env, + nioPath = nioPath, + selectionStart = selectionStart, + selectionEnd = selectionEnd, + documentVersion = documentVersionOf(nioPath), + // Ties the analysis to this action's coroutine: cancelling the action aborts the analysis. + cancelChecker = ScheduledCancelChecker(createJobCancelChecker()), + ) + } + + override fun postExec( + data: ActionData, + result: Any, + ) { + super.postExec(data, result) + if (result !is ExtractionPlan) return + + if (result.isEmpty) { + flashInfo(R.string.msg_extract_variable_nothing_to_extract) + return + } + + val activity = + data.requireContext().findFragmentActivity() + ?: run { + // A wiring problem rather than a user path: the editor is always hosted by one. + logger.warn("No FragmentActivity for the editor context. Cannot show the extract sheet.") + flashError(R.string.msg_cannot_perform_fix) + return + } + + val shown = ExtractVariableSheet.show(activity, result) { choice -> applyChoice(data, result, choice) } + if (!shown) { + logger.warn("Fragment manager unavailable. Cannot show the extract sheet.") + } + } + + /** + * Turns the user's choice into one edit and hands it to the language client. + * + * The document version is re-read here rather than trusted from the plan: the editor stays + * reachable while the sheet is open, and applying spans computed against older text would corrupt + * the file. Refusing is always safe; the user can invoke the action again. + */ + private fun applyChoice( + data: ActionData, + plan: ExtractionPlan, + choice: ExtractionChoice, + ) { + val file = data.requireFile() + val nioPath = file.toPath() + if (documentVersionOf(nioPath) != plan.documentVersion) { + flashInfo(R.string.msg_extract_variable_file_changed) + return + } + + val rewrite = + buildExtractVariableRewrite( + fileText = plan.fileText, + candidateSpan = choice.candidate.span, + scope = choice.scope, + name = choice.name, + replaceAll = choice.replaceAll, + ) ?: run { + logger.warn("Could not build an extract-variable rewrite for '{}'", choice.candidate.label) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val client = + data.languageClient ?: run { + logger.warn("No language client set. Cannot extract variable.") + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = listOf(DocumentChange(file = nioPath, edits = listOf(rewrite.toTextEdit(plan.fileText)))), + kind = CodeActionKind.QuickFix, + // The rewrite is emitted fully indented; CMD_FORMAT_CODE is a no-op for Kotlin anyway. + command = Command("", ""), + ), + ) + } + + /** -1 when the document is not open, which never matches a real version and so fails the guard. */ + private fun documentVersionOf(path: Path): Int = FileManager.getActiveDocument(path)?.version ?: -1 +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt index 352e81eaec..504b4e50de 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt @@ -5,6 +5,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.KotlinCodeActionsMenu.KT_LANG import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction @@ -41,6 +42,7 @@ class KotlinCodeActionTooltipTagTest { NullSafetyAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_NULL_SAFETY_FIX, ImplementMembersAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_IMPLEMENT_MEMBERS, SurroundWithTryCatchAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH, + ExtractVariableAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE, ) assertEquals(expected, actualTags) } From 74ffff8a3b643a0f95cc5b32d5991cce0f764651 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 14:19:21 +0000 Subject: [PATCH 05/16] ADFA-4826: Document the extract-variable requirements Requirements, scope, non-goals, acceptance criteria and the test split, following the kotlin-goto-definition.md template. Also carries the Language section for the whole refactoring family - extract method, inline variable and rename all reuse this vocabulary rather than restating it. --- docs/features/kotlin-extract-variable.md | 229 +++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 docs/features/kotlin-extract-variable.md diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md new file mode 100644 index 0000000000..bc45813e51 --- /dev/null +++ b/docs/features/kotlin-extract-variable.md @@ -0,0 +1,229 @@ +# Kotlin extract variable (K2 LSP) + +- **Ticket:** ADFA-4826 (subtask of ADFA-3317; split out of the closed ADFA-3324 "Refactoring"). Extract method was originally part of this subtask and is now ADFA-5080. +- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename and the sealed `RefactoringPlan` supertype shared with ADFA-5080 (see [Design](#design)). +- **Module:** `lsp/kotlin` + +Bind the expression at the cursor, or the selected one, to a new local `val`, and replace the occurrences of that expression with the new name. + +This is the first *interactive* Kotlin code action: the user chooses an expression, a name, a target scope and whether to replace other occurrences, so it needs a real UI surface rather than a fire-and-forget edit. Where that UI lives is [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do is the decline-rather-than-rewrite principle, recorded as ADR 0013 alongside extract method (ADFA-5080). + +## Language + +This section is the glossary for the whole refactoring family - extract variable, extract method (ADFA-5080), inline variable (ADFA-4827), rename (ADFA-4825). Prefer these terms over ad-hoc synonyms in code, tests, docs and review comments. + +**Selection**: +The user's raw offsets from the editor caret, before any processing. A cursor is the degenerate selection where start equals end. Trimmed and snapped before it becomes an extraction region, so it is *not* interchangeable with one. +_Avoid_: range (that's `Range`, the LSP line/column type), region. + +**Extraction region**: +The contiguous text an extraction reads its body from. For extract variable it is always an expression candidate; extract method adds statement ranges. +_Avoid_: target (overloaded with go-to-definition's target and with the insertion site), extent, fragment. + +**Expression candidate**: +A `KtExpression` at the selection that is a legal extraction target. Ordered innermost-first, at most `MAX_CANDIDATES` (3) of them, so the chooser stays scannable on a phone. +_Avoid_: candidate expression when naming code (the type is `CandidateExpression`, but the term is "expression candidate"), match, option. + +**Text span**: +A half-open offset range `[start, end)` into the analysed file's text - the type `TextSpan`. Purely positional; it carries no meaning about what it covers. +_Avoid_: range, offset pair. + +**Legal scope chain**: +The ordered anchors available for the new declaration, innermost first: outward from the candidate's own statement through enclosing blocks, crossing a lambda boundary only when nothing lambda-scoped is referenced, and stopping at the enclosing named function, accessor or `init` body. +_Avoid_: scope list, parent chain. + +**Anchor scope**: +The chain member the user picked. The `val` is declared inside it. + +**Anchor form**: +How the declaration is woven into an anchor scope, since not all Kotlin scopes are blocks: `ExistingBlock`, `WrapInBraces`, or `ConvertExpressionBody`. + +**Anchor point**: +The exact insertion offset - immediately before the first statement *within the anchor scope* that contains a replaced occurrence. + +**Occurrence**: +A site inside the anchor scope that is structurally equal to the candidate *and* whose every name reference resolves to the same declaration. Sites made unsound by an intervening write are excluded, so an occurrence set is always safe to replace wholesale. +_Avoid_: duplicate, match, usage. + +**Refactoring plan**: +The complete result of the background analysis pass - the sealed `RefactoringPlan`, carrying the analysed `fileText` and its `documentVersion`. Plain data: no PSI, no symbols, no session. `ExtractVariablePlan` is this refactoring's subtype. +_Avoid_: model, result, context. + +**Rewrite span**: +The single text replacement an extraction performs - a `TextSpan` plus its replacement text (`RewriteSpan`), converted to one `TextEdit` at the boundary. + +## Scope + +### In scope + +An expression inside any executable body: a function body, a property accessor, an `init` block, a constructor, or a lambda. Both a bare cursor and a selection, since a cursor is just the selection where start equals end. + +### Out of scope + +Positions where no `val` can precede the expression, all rejected up front by `isExtractionPosition`: + +- **Annotation arguments** - must be compile-time constants. +- **Default parameter values** - evaluated per call, and a hoisted local would not be in scope. +- **Super-constructor delegation arguments** - nothing can precede them. +- **Anything outside an executable body**, notably a class-body property initializer. Converting one to a getter would turn compute-once into compute-per-access, so it is declined rather than silently changing evaluation semantics. + +## Requirements + +**R1 - Trigger.** An "Extract variable" item (`action_extract_variable`) appears in the editor code-actions menu for Kotlin files, id `ide.editor.lsp.kt.extractVariable`, tooltip tag `EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE = "editor.codeactions.kotlin.extractvariable"`. Tooltip *content* is keyed by tag in the out-of-repo tooltips database, so the tag shows no text until a row exists for it - a hand-off item, not code. + +There is deliberately **no `prepare()` visibility gate**. Deciding whether anything is extractable needs a K2 analysis session, which is far too costly for `prepare()` (UI thread, per menu item). The action stays visible on any Kotlin file and reports "nothing to extract" instead, matching `OrganizeImportsAction` and `ImplementMembersAction`. `requiresUIThread = false`, so the selection is read on a background thread; a torn read while the user is mid-edit can only produce a plan the version guard (R3) then refuses. + +**R2 - Region.** The selection is whitespace-trimmed first, because a touch-screen selection routinely carries a leading or trailing space; a whitespace-only selection yields nothing. For a cursor, the element is looked up at the offset and then at `offset - 1`, so a caret resting just past a token still resolves. + +From the innermost element the parent chain is walked outwards, collecting legal targets and stopping at the enclosing declaration. Illegal nodes along the way are **skipped rather than terminating the walk**, so `if (c) a else b` is still offered from inside one of its branches. At most 3 candidates, innermost first, deduplicated by range. + +An expression is not a legal target when it is: a block, a loop, `return`/`throw`/`break`/`continue`, an operation reference, `super`, a lambda literal, the selector of a qualified expression (`b` in `a.b`), a call's callee (`foo` in `foo(x)`), the left side of an assignment, or a **bare literal**. Excluding bare literals removes the only case where omitting the type annotation could change meaning - an `Int` literal where a `Long` is expected, or a bare `null` inferring `Nothing?`. + +When the trimmed selection exactly equals the innermost candidate's range, the user has already said which expression they mean and the chooser is not shown (`selectionMatchedCandidate`). + +**R3 - Live offsets and the version guard.** Analysis runs against `ktSymbolIndex.getCurrentKtFile(path)`, PSI refreshed to the open document's current version - an offset resolved against stale text points at the wrong element. The `KtFile` is fetched *before* entering `project.read`: the refresh needs `project.write`, and awaiting it under the read lock deadlocks. + +The plan records the document version it was computed against. On confirm, the version is re-read and the edit is **refused** if it has moved on (`msg_extract_variable_file_changed`) - the editor stays reachable while the sheet is open, and applying spans computed against older text would corrupt the file. Refusing is always safe; the user can invoke the action again. + +**R4 - Value filter.** A candidate whose type is `Unit` or `Nothing` is dropped: `val u = println(x)` compiles but is pointless. A candidate whose legal scope chain is empty is dropped too - a candidate with no legal anchor is not a candidate. + +**R5 - Scope chain.** Anchors are enumerated outward from the candidate's own statement, each one of three anchor forms: + +| Anchor form | When | Emitted as | +|---|---|---| +| `ExistingBlock` | the scope already has a `{ ... }` body | a new statement line | +| `WrapInBraces` | a braceless statement position: `if (c) foo()`, a `when` entry, a braceless loop body | the statement is replaced by a braced block holding the declaration and the original statement | +| `ConvertExpressionBody` | an expression-bodied function or accessor, `fun area(r: Int) = r * r` | `=` and the body become a block body; `return` is added unless the declaration returns `Unit` | + +The walk stops after the enclosing named function, accessor or `init` body. A class body or file is never an anchor. Lambda boundaries are crossed during the syntactic walk, then **truncated afterwards** by the innermost scope holding a declaration the candidate references - so a candidate using `it` or a lambda parameter can never be hoisted out of that lambda. `it` needs its own case: it has no source PSI, so a value-parameter symbol with no PSI referenced by the name `it` is taken to be the innermost enclosing lambda's implicit parameter. That is a property of the language, not a guess about the text. + +Braceless control-structure bodies are wrapped in a container node, so the `if`/loop is the grandparent; without unwrapping, no braceless body is ever detected and the declaration silently hoists to the enclosing block instead of braces being added. + +**R6 - Occurrences.** Two sites are the same expression when they are structurally identical (whitespace and comments ignored) *and* every name reference in them resolves to the same declaration. The symbol check is the point: text or structure alone would match `config.timeout` inside a nested lambda where `config` is a different `config`. ADFA-3324 states the standard outright - text-based matching breaks things. + +Source declarations are compared by PSI identity, which is exactly the question being asked ("the same `val`?"); symbols without source PSI fall back to symbol equality. A resolution failure reads as "not the same" rather than propagating. + +Matches must themselves be legal targets - in `a.a`, a candidate of `a` matches the selector too, and rewriting it would produce `v.v`. Overlapping matches are dropped so no site is rewritten twice. + +An occurrence set is then restricted to a contiguous run around the candidate that **no write to a referenced mutable interrupts**: + +```kotlin +var limit = 1 +foo(limit + 1) // occurrence +limit = 5 +foo(limit + 1) // same expression, different value +``` + +Unsound sites are excluded rather than warned about, so "Replace all N occurrences" can never produce wrong code and N is always achievable. The walk grows outward from the candidate - never dropping the site the user selected - and stops in each direction at the first write it would cross. Writes counted: plain assignment, the augmented forms, and `++`/`--`, against any `var` the candidate reads. + +Occurrence sets are ascending by offset and always contain the candidate's own span, so `occurrences.size` is the count shown in "Replace all N occurrences". Narrowing to an inner scope can only shrink the set, never grow it. + +**R7 - Name.** The suggestion is derived from the expression's shape first (`items.size` -> `size`, `getFoo()` -> `foo`, an interpolated string -> `text`), then its rendered type (`List` -> `list`), then `"value"`; shape beats type because `size`, `count` and `name` are far better names than `int` and `string`. It is then uniquified with a numeric suffix. + +Validation returns a `NameProblem` - `Blank`, `NotAnIdentifier`, `Keyword`, `AlreadyTaken` - rather than throwing, since the input is a text field. Only Kotlin's **hard** keywords are rejected; soft and modifier keywords (`by`, `data`, `it`) are legal names. Backtick-quoted names are rejected: legal Kotlin, but a poor generated local, and accepting them would mean validating the quoted form too. + +Taken names are every declaration name in the file - deliberately conservative rather than scope-exact. Being over-broad costs a `size1` where `size` would have done; being under-broad generates code that shadows something. It is also purely syntactic, so it needs no analysis and is unit-testable. + +**R8 - Sheet.** One surface holding every choice, with no navigation between steps: expression chooser, name field, scope chooser, replace-all checkbox, Cancel/Extract. The four are interdependent - a different expression changes the scope list and the occurrence count - so they are shown together where that relationship is visible, rather than across sequential dialogs the user would have to back out of to explore. + +Each chooser is hidden when it has nothing to ask: the expression chooser when there is one candidate or the selection already matched one, the scope chooser when the chain has one rung, the replace-all checkbox at an occurrence count of one. Changing the expression re-suggests the name, because the old one described the old expression. + +**R9 - Edit.** Exactly **one** `TextEdit`, built as a `RewriteSpan` covering one contiguous span. `IDELanguageClientImpl.applyActionEdits` applies each edit in its own `runOnUiThread` with no `beginBatchEdit`, and every range is interpreted against the *current* text - so a list of N edits would be applied against positions already shifted by its predecessors and would cost N undo steps with a typing window between each. Occurrences are substituted right-to-left within the span so an earlier substitution cannot shift a later offset. + +The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. + +**R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. + +**R11 - Failure isolation.** Anything thrown in the analysis pipeline degrades to an empty plan and a log line. The action framework catches only `IllegalArgumentException` and this runs on a scope with no exception handler, so an uncaught throw would crash the app; reporting "nothing to extract" is always safe. A missing `FragmentActivity` or fragment manager logs and flashes `msg_cannot_perform_fix` rather than failing silently. + +## Non-goals + +- **Extract to a `val` outside an executable body** - a class property or a top-level `val`. That is a different refactoring with different scope rules. +- **Extract `var`, `lateinit`, or a property with accessors.** Always a `val`. +- **An explicit type annotation** on the generated declaration. Bare literals are excluded (R2) precisely so inference cannot change meaning. +- **Occurrences outside the anchor scope**, or across files. +- **Renaming the declaration in place after the edit** - ADFA-4825. +- **Formatting the result.** `CMD_FORMAT_CODE` is a no-op for Kotlin; R9 emits indented text instead. +- **Extract method** - ADFA-5080, which shares this vocabulary and these primitives. + +## Acceptance criteria + +1. "Extract variable" appears in the code-actions menu of a Kotlin file and is absent in a non-Kotlin file. +2. A cursor inside `a + b * c` offers the innermost-first candidates and extracting the selected one produces `val = ...` on its own line above, correctly indented. +3. A selection that exactly matches an expression skips the expression chooser. +4. A caret immediately after an identifier resolves the same as one inside it. +5. A cursor on a bare literal, on whitespace, in a comment, or in an annotation argument reports "No expression to extract here". +6. An expression appearing three times in the same block reports "Replace all 3 occurrences" and rewrites all three. +7. The same expression with an intervening reassignment of a `var` it reads offers only the contiguous sound run. +8. An expression using `it` inside a lambda offers no anchor outside that lambda. +9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. +10. Extracting from `fun area(r: Int) = r * r` converts it to a block body with `return`. +11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return`. +12. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. +13. Editing the file while the sheet is open, then confirming, reports "The file changed. Try extracting again." and leaves the file untouched. +14. One undo restores the file exactly. +15. A file indented with spaces receives space-indented output; a CRLF file keeps CRLF. + +## Design + +Per [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md), `lsp/kotlin` owns its refactoring UI, and the analysis/UI split is enforced **by data rather than by module boundaries**: the background pass produces a plain-data plan, and the sheet holds no PSI and performs no analysis. + +``` +ExtractVariableAction.execAction (background) lsp/kotlin/actions + server.compilationEnvironmentFor(path) ?: empty plan + cursor -> [selectionStart, selectionEnd) + -> buildExtractionPlan(...) utils/refactor/ExtractVariablePlanner.kt + ktFile = env.ktSymbolIndex.getCurrentKtFile(path).get() [R3: before project.read] + env.project.read { + candidateExpressionsAt(ktFile, start, end) utils/refactor/CandidateExpressions.kt [R2] + analyzeMaybeDangling(INTERACTIVE, cancelChecker) { [R10] + per candidate: type filter [R4] + enclosingScopeFrames + truncateAtCeiling ScopeChain.kt / Occurrences.kt [R5] + findOccurrences + excludeUnsoundOccurrences Occurrences.kt [R6] + suggestVariableName + visibleNamesAt NameSuggestion.kt / Occurrences.kt [R7] + } + } + <- ExtractVariablePlan (plain data, no PSI) + +ExtractVariableAction.postExec (UI thread) + empty -> flashInfo("No expression to extract here") [R11] + findFragmentActivity() -> ExtractVariableSheet.show refactor/ui [R8] + ExtractVariableViewModel: StateFlow, sealed UiEvent + on confirm -> ExtractionChoice + version re-read; mismatch -> refuse [R3] + buildExtractVariableRewrite -> RewriteSpan -> toTextEdit utils/refactor/ExtractVariableEdit.kt [R9] + client.performCodeAction(one DocumentChange, one TextEdit) +``` + +Components: + +- **`utils/refactor/ExtractionPlan.kt`** - `TextSpan`, `AnchorForm`, `ScopeOption`, `CandidateExpression`, the plan, `collapseForLabel`. To be renamed to `ExtractVariablePlan` under a sealed `RefactoringPlan` carrying `fileText`, `documentVersion` and the shared version guard, so ADFA-5080 adds a subtype rather than renaming this one. Both refactorings share these *primitives*, not the aggregate: extract method has no scope chain, so `ScopeOption`/`AnchorForm`/`CandidateExpression` are not shared. +- **`CandidateExpressions.kt`** - purely syntactic, no analysis session, hence unit-testable on its own (R2). +- **`ScopeChain.kt`** - the syntactic chain and the three anchor forms (R5); indentation and newline detection shared with the edit builder. +- **`Occurrences.kt`** - symbol-aware structural equality, the occurrence search, the unsoundness filter, the referenced-declaration ceiling, and `visibleNamesAt` (R5, R6, R7). +- **`NameSuggestion.kt`** - suggestion and validation, no analysis session (R7). +- **`ExtractVariableEdit.kt`** - `RewriteSpan`, the three anchor-form rewrites, `toTextEdit` (R9). Pure text and offsets. +- **`refactor/ui/`** - `ExtractVariableSheet` (a `BottomSheetDialogFragment` hosting a `ComposeView`), stateless `ExtractVariableSheetContent`, `ExtractVariableViewModel` + `ExtractVariableUiState` + sealed `ExtractVariableUiEvent`. `LabelledSection` and `OptionList` become shared with ADFA-5080. The ViewModel uses a plain `ViewModelProvider.Factory` rather than a Koin definition: it is sheet-scoped, injects nothing, and takes the plan as a runtime argument. +- **`ExtractVariableAction`** extending `BaseKotlinCodeAction`, registered in `KotlinCodeActionsMenu`; the only class that touches the editor, the document version or the language client. +- **`common-compose`** - `IdeTheme`/`IdeColorScheme`, shared with `profiler` and `floating-window` so the sheet matches the IDE's theme. + +## Verification + +Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), split so a failure localises to one layer: + +- **`RefactorPrimitivesTest`** - no analysis session: selection trimming, candidate collection and the legal-target rules (R2), indent/newline detection, name suggestion and validation (R7), the unsoundness filter as a pure function (R6). +- **`ExtractVariablePlanEndToEndTest`** - analysis-backed: the value filter (R4), scope chains and the lambda ceiling (R5), occurrence sets including the `it` and same-name-different-symbol cases (R6). +- **`ExtractVariableEditTest`** - pure text: the three anchor forms, right-to-left substitution, indentation and CRLF (R9). +- **`ExtractVariableViewModelTest`** - state derivation: chooser visibility, candidate switching re-suggesting the name, replace-all clamping, `choice()` refusing an invalid name (R8). +- **`KotlinCodeActionTooltipTagTest`** - every action carries a tooltip tag (R1). + +`prepare()`/`ActionData` and the sheet itself are not unit-testable, consistent with the other Kotlin code actions. They are covered by on-device QA from the acceptance criteria, recorded in ADFA-4826's "Steps to QA" field. + +## Related + +- [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module +- ADR 0013 - refactorings decline rather than rewrite unselected code (lands with extract method, ADFA-5080) +- [ADR 0009](../adr/0009-jetpack-compose-for-new-ui.md) - Compose, UDF, `ViewModel` + `StateFlow` +- [ADR 0010](../adr/0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth +- ADFA-5080 - extract method, the sibling refactoring; it reuses this vocabulary and these primitives +- [ARCHITECTURE.md](../../ARCHITECTURE.md) From 7ef73c72fd8c2629362e2004cab87332dc7a5c26 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 14:34:01 +0000 Subject: [PATCH 06/16] ADFA-4826: Stop offering the lambda that wraps the expression --- docs/features/kotlin-extract-variable.md | 4 ++-- .../utils/refactor/CandidateExpressions.kt | 5 ++++ .../ExtractVariablePlanEndToEndTest.kt | 23 +++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index bc45813e51..f6fe22d263 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -1,7 +1,7 @@ # Kotlin extract variable (K2 LSP) - **Ticket:** ADFA-4826 (subtask of ADFA-3317; split out of the closed ADFA-3324 "Refactoring"). Extract method was originally part of this subtask and is now ADFA-5080. -- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename and the sealed `RefactoringPlan` supertype shared with ADFA-5080 (see [Design](#design)). +- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename (the sealed `RefactoringPlan` supertype it will sit under has landed). - **Module:** `lsp/kotlin` Bind the expression at the cursor, or the selected one, to a new local `val`, and replace the occurrences of that expression with the new name. @@ -77,7 +77,7 @@ There is deliberately **no `prepare()` visibility gate**. Deciding whether anyth From the innermost element the parent chain is walked outwards, collecting legal targets and stopping at the enclosing declaration. Illegal nodes along the way are **skipped rather than terminating the walk**, so `if (c) a else b` is still offered from inside one of its branches. At most 3 candidates, innermost first, deduplicated by range. -An expression is not a legal target when it is: a block, a loop, `return`/`throw`/`break`/`continue`, an operation reference, `super`, a lambda literal, the selector of a qualified expression (`b` in `a.b`), a call's callee (`foo` in `foo(x)`), the left side of an assignment, or a **bare literal**. Excluding bare literals removes the only case where omitting the type annotation could change meaning - an `Int` literal where a `Long` is expected, or a bare `null` inferring `Nothing?`. +An expression is not a legal target when it is: a block, a loop, `return`/`throw`/`break`/`continue`, an operation reference, `super`, a lambda (the `{ ... }` expression and the literal inside it -- outside its call site the parameter types are gone, so `val v = { it.length + 1 }` does not compile), the selector of a qualified expression (`b` in `a.b`), a call's callee (`foo` in `foo(x)`), the left side of an assignment, or a **bare literal**. Excluding bare literals removes the only case where omitting the type annotation could change meaning - an `Int` literal where a `Long` is expected, or a bare `null` inferring `Nothing?`. When the trimmed selection exactly equals the innermost candidate's range, the user has already said which expression they mean and the chooser is not shown (`selectionMatchedCandidate`). diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt index 8c0510c27f..2f6b6fd3ef 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt @@ -17,6 +17,7 @@ import org.jetbrains.kotlin.psi.KtDeclarationWithBody import org.jetbrains.kotlin.psi.KtExpression import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtLambdaExpression import org.jetbrains.kotlin.psi.KtLiteralStringTemplateEntry import org.jetbrains.kotlin.psi.KtLoopExpression import org.jetbrains.kotlin.psi.KtOperationReferenceExpression @@ -179,6 +180,7 @@ private fun PsiElement.isAncestorOf(other: PsiElement): Boolean = PsiTreeUtil.is * * Excluded, and why: * - blocks, loops, `return`/`throw`/`break`/`continue` -- no useful value to bind; + * - lambdas, literal and wrapper alike -- outside their call site the parameter types are gone; * - operator tokens and call callees (`foo` in `foo(x)`) -- fragments, not expressions; * - the selector of a qualified expression (`b` in `a.b`) -- only meaningful with its receiver; * - the left side of an assignment -- a write target, not a value; @@ -195,6 +197,9 @@ internal fun KtExpression.isLegalExtractionTarget(): Boolean { if (this is KtOperationReferenceExpression) return false if (this is KtSuperExpression) return false if (this is KtFunctionLiteral) return false + // The wrapper around the literal. A hoisted lambda loses the parameter types its call site was + // supplying, so `{ it.length + 1 }` becomes uncompilable the moment it leaves the call. + if (this is KtLambdaExpression) return false if (isBareLiteral()) return false val parent = parent diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 11aff94443..e6c81822b1 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -386,4 +386,27 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { apply(content, rewrite!!), ) } + + @Test + fun `does not offer the lambda that wraps the expression`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { + it.length + 1 + } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + // `{ it.length + 1 }` must not appear between the two: a hoisted lambda loses the `it` the call + // site was supplying. + assertEquals( + listOf("it.length + 1", "items.map { it.length + 1 }"), + result.candidates.map { it.label }, + ) + } } From 93c0bdb3c10dedf87e053d477305778be7c1ece4 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 14:45:59 +0000 Subject: [PATCH 07/16] ADFA-4826: Label a block rung by the construct that owns it --- docs/features/kotlin-extract-variable.md | 5 ++++ .../lsp/kotlin/utils/refactor/ScopeChain.kt | 17 ++++++++++--- .../ExtractVariablePlanEndToEndTest.kt | 25 +++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index f6fe22d263..005439222f 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -95,6 +95,11 @@ The plan records the document version it was computed against. On confirm, the v | `WrapInBraces` | a braceless statement position: `if (c) foo()`, a `when` entry, a braceless loop body | the statement is replaced by a braced block holding the declaration and the original statement | | `ConvertExpressionBody` | an expression-bodied function or accessor, `fun area(r: Int) = r * r` | `=` and the body become a block body; `return` is added unless the declaration returns `Unit` | +Each rung is labelled with the construct that owns it -- `fun name`, `getter`, `setter`, `init block`, +`lambda`, `if block`, `else block`, `for loop`, `while loop`, `do-while loop`, `when branch` -- so the +`Declare in` list reads as a place rather than as a nesting level. A braced control-structure body is +wrapped in a container node, so the owner is the block's grandparent, not its parent. + The walk stops after the enclosing named function, accessor or `init` body. A class body or file is never an anchor. Lambda boundaries are crossed during the syntactic walk, then **truncated afterwards** by the innermost scope holding a declaration the candidate references - so a candidate using `it` or a lambda parameter can never be hoisted out of that lambda. `it` needs its own case: it has no source PSI, so a value-parameter symbol with no PSI referenced by the name `it` is taken to be the innermost enclosing lambda's implicit parameter. That is a property of the language, not a guess about the text. Braceless control-structure bodies are wrapped in a container node, so the `if`/loop is the grandparent; without unwrapping, no braceless body is ever detected and the declaration silently hoists to the enclosing block instead of braces being added. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt index 79ac67d2fe..64692923bf 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -170,19 +170,30 @@ private fun isCeilingBody(scopeElement: PsiElement): Boolean { } } -private fun blockLabel(block: KtBlockExpression): String = - when (val owner = block.parent) { +/** + * The name shown for a block rung. + * + * A braceless *or* braced control-structure body is wrapped in a container node, so the `if`/loop is + * the block's grandparent; without unwrapping, every braced branch reads as a generic "block". The + * container is also what `then`/`else` point at, so the branch check compares against it. + */ +private fun blockLabel(block: KtBlockExpression): String { + val parent = block.parent + val container = parent as? KtContainerNodeForControlStructureBody + val branch = container ?: block + return when (val owner = container?.parent ?: parent) { is KtNamedFunction -> "fun ${owner.name ?: ""}" is KtPropertyAccessor -> if (owner.isGetter) "getter" else "setter" is KtAnonymousInitializer -> "init block" is KtFunctionLiteral -> "lambda" - is KtIfExpression -> if (owner.then === block) "if block" else "else block" + is KtIfExpression -> if (owner.then === branch || owner.then?.parent === container) "if block" else "else block" is KtForExpression -> "for loop" is KtWhileExpression -> "while loop" is KtDoWhileExpression -> "do-while loop" is KtWhenEntry -> "when branch" else -> "block" } +} private fun declarationLabel(declaration: KtDeclarationWithBody): String = when (declaration) { diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index e6c81822b1..7c29f31919 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -409,4 +409,29 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { result.candidates.map { it.label }, ) } + + @Test + fun `labels a braced if branch by its owner`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b * 2 + } + return 0 + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + assertEquals( + listOf("if block", "fun demo"), + result.candidates + .first() + .scopes + .map { it.label }, + ) + } } From 9554c001af2ed2bb9abacddd9c926e39ee24daf0 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 14:54:58 +0000 Subject: [PATCH 08/16] ADFA-4826: Fix misleading KDoc and add else block test Remove dead code path (owner.then === branch can never be true). Correct the KDoc to accurately describe that getThen()/getElse() return unwrapped body expressions, not containers, so branch identity is checked via owner.then?.parent === container. Add test for braced else branch to prevent regression. --- .../lsp/kotlin/utils/refactor/ScopeChain.kt | 8 +++--- .../ExtractVariablePlanEndToEndTest.kt | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt index 64692923bf..1361d45080 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -174,8 +174,10 @@ private fun isCeilingBody(scopeElement: PsiElement): Boolean { * The name shown for a block rung. * * A braceless *or* braced control-structure body is wrapped in a container node, so the `if`/loop is - * the block's grandparent; without unwrapping, every braced branch reads as a generic "block". The - * container is also what `then`/`else` point at, so the branch check compares against it. + * the block's grandparent; without unwrapping, every braced branch reads as a generic "block". + * `getThen()`/`getElse()` return the unwrapped body expression, never the container, so branch + * identity is decided by checking if the container's parent matches what `then`/`else` point at + * (by comparing `owner.then?.parent === container`). */ private fun blockLabel(block: KtBlockExpression): String { val parent = block.parent @@ -186,7 +188,7 @@ private fun blockLabel(block: KtBlockExpression): String { is KtPropertyAccessor -> if (owner.isGetter) "getter" else "setter" is KtAnonymousInitializer -> "init block" is KtFunctionLiteral -> "lambda" - is KtIfExpression -> if (owner.then === branch || owner.then?.parent === container) "if block" else "else block" + is KtIfExpression -> if (owner.then?.parent === container) "if block" else "else block" is KtForExpression -> "for loop" is KtWhileExpression -> "while loop" is KtDoWhileExpression -> "do-while loop" diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 7c29f31919..0db9c2cbb8 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -434,4 +434,30 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { .map { it.label }, ) } + + @Test + fun `labels a braced else branch by its owner`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return 0 + } else { + return a + b * 2 + } + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + assertEquals( + listOf("else block", "fun demo"), + result.candidates + .first() + .scopes + .map { it.label }, + ) + } } From bd2ace6cfb07a4d87e51b32125ff98a031973ec4 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 15:10:57 +0000 Subject: [PATCH 09/16] ADFA-4826: Write the return type when converting an expression body --- docs/features/kotlin-extract-variable.md | 13 ++- .../utils/refactor/ExtractVariableEdit.kt | 20 +++- .../utils/refactor/ExtractVariablePlanner.kt | 55 ++++++++++- .../kotlin/utils/refactor/ExtractionPlan.kt | 5 + .../lsp/kotlin/utils/refactor/TypeText.kt | 98 +++++++++++++++++++ .../utils/refactor/ExtractVariableEditTest.kt | 26 +++++ .../ExtractVariablePlanEndToEndTest.kt | 98 ++++++++++++++++++- .../utils/refactor/RefactorPrimitivesTest.kt | 45 +++++++++ 8 files changed, 349 insertions(+), 11 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index 005439222f..ffb53df5a6 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -93,7 +93,14 @@ The plan records the document version it was computed against. On confirm, the v |---|---|---| | `ExistingBlock` | the scope already has a `{ ... }` body | a new statement line | | `WrapInBraces` | a braceless statement position: `if (c) foo()`, a `when` entry, a braceless loop body | the statement is replaced by a braced block holding the declaration and the original statement | -| `ConvertExpressionBody` | an expression-bodied function or accessor, `fun area(r: Int) = r * r` | `=` and the body become a block body; `return` is added unless the declaration returns `Unit` | +| `ConvertExpressionBody` | an expression-bodied function or accessor, `fun area(r: Int) = r * r` | `=` and the body become a block body; `return` is added unless the declaration returns `Unit`; the return type is written into the signature when the declaration does not spell one out, because a block body with no declared type returns `Unit` | + +A written-out return type is rendered fully qualified and then shortened to its simple name only where +that name already resolves in the file -- an exact import, a star import of its package, or a +default-imported package such as `kotlin.collections`. Everything else stays qualified: verbose, but it +compiles, and this refactoring adds no imports. When the type cannot be written as source at all +(anonymous, intersection, an unresolved type, or a platform type the renderer cannot reduce) the rung +is declined rather than emitting a block body that does not compile. Each rung is labelled with the construct that owns it -- `fun name`, `getter`, `setter`, `init block`, `lambda`, `if block`, `else block`, `for loop`, `while loop`, `do-while loop`, `when branch` -- so the @@ -162,8 +169,8 @@ The emitted text is **fully indented**: code-action edits bypass the editor's au 7. The same expression with an intervening reassignment of a `var` it reads offers only the contiguous sound run. 8. An expression using `it` inside a lambda offers no anchor outside that lambda. 9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. -10. Extracting from `fun area(r: Int) = r * r` converts it to a block body with `return`. -11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return`. +10. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. +11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. 12. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. 13. Editing the file while the sheet is open, then confirming, reports "The file changed. Try extracting again." and leaves the file untouched. 14. One undo restores the file exactly. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index da41a5e2fa..a4215500e6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -114,14 +114,30 @@ private fun convertExpressionBodyRewrite( val body = replaceOccurrences(fileText, bodySpan, targets, name) val returned = if (form.needsReturn) "return $body" else body + // Writing a type means rewriting from the end of the signature, not from the `=`: starting at the + // `=` would leave the space in front of it and emit `fun area(r: Int) : Int {`. + val spanStart = + if (form.returnTypeText == null) form.assignStart else startOfWhitespaceBefore(fileText, form.assignStart) + val header = form.returnTypeText?.let { ": $it " } ?: "" + val newText = buildString { - append('{').append(newline) + append(header).append('{').append(newline) append(form.innerIndent).append(declaration).append(newline) append(form.innerIndent).append(returned).append(newline) append(form.indent).append('}') } - return RewriteSpan(TextSpan(form.assignStart, form.bodyEnd), newText) + return RewriteSpan(TextSpan(spanStart, form.bodyEnd), newText) +} + +/** The offset where the run of whitespace ending at [offset] begins. */ +private fun startOfWhitespaceBefore( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index > 0 && text[index - 1].isWhitespace()) index-- + return index } /** diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt index bc39bde916..0b4a058c41 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt @@ -11,10 +11,12 @@ import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol import org.jetbrains.kotlin.analysis.api.types.KaType import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.psi.KtCallableDeclaration import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtDeclarationWithBody import org.jetbrains.kotlin.psi.KtExpression import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtPropertyAccessor import org.slf4j.LoggerFactory import java.nio.file.Path @@ -82,7 +84,9 @@ private fun KaSession.candidateFor(expression: KtExpression): CandidateExpressio if (frames.isEmpty()) return null val span = TextSpan(expression.textRange.startOffset, expression.textRange.endOffset) - val scopes = frames.map { scopeOptionFor(expression, span, it) } + val file = expression.containingKtFile + val scopes = frames.mapNotNull { scopeOptionFor(expression, span, it, file) } + if (scopes.isEmpty()) return null val takenNames = visibleNamesAt(expression) return CandidateExpression( @@ -94,25 +98,66 @@ private fun KaSession.candidateFor(expression: KtExpression): CandidateExpressio ) } -/** Builds one scope option, resolving its occurrence set and fixing up expression-body details. */ +/** + * Builds one scope option, resolving its occurrence set and fixing up expression-body details. + * + * Returns null when the rung cannot be honoured: converting an expression body whose return type is + * neither declared nor renderable would emit a block body that does not compile, and declining is + * always safe (ADR 0013). + */ private fun KaSession.scopeOptionFor( expression: KtExpression, span: TextSpan, frame: ScopeFrame, -): ScopeOption { + file: KtFile, +): ScopeOption? { val matches = findOccurrences(expression, frame.scopeElement, frame.searchRange) val writes = writeOffsetsFor(expression, frame.scopeElement) val occurrences = excludeUnsoundOccurrences(matches, span, writes) val anchorForm = when (val form = frame.anchorForm) { - is AnchorForm.ConvertExpressionBody -> form.copy(needsReturn = expressionBodyNeedsReturn(frame.scopeElement)) - else -> form + is AnchorForm.ConvertExpressionBody -> { + val declaration = frame.scopeElement.parent as? KtDeclarationWithBody + val needsReturn = expressionBodyNeedsReturn(frame.scopeElement) + val returnTypeText = + if (needsReturn && declaration != null && !declaration.declaresReturnType()) { + returnTypeTextOf(declaration, file) ?: return null + } else { + null + } + form.copy(needsReturn = needsReturn, returnTypeText = returnTypeText) + } + + else -> { + form + } } return ScopeOption(label = frame.label, anchorForm = anchorForm, occurrences = occurrences) } +/** Whether the declaration spells its return type out, in which case nothing needs writing. */ +private fun KtDeclarationWithBody.declaresReturnType(): Boolean = + when (this) { + // KtPropertyAccessor.returnTypeReference is deprecated in favour of the identical typeReference. + is KtPropertyAccessor -> typeReference != null + + is KtCallableDeclaration -> typeReference != null + + else -> false + } + +/** The declaration's return type as source text, shortened where the file can resolve it. */ +private fun KaSession.returnTypeTextOf( + declaration: KtDeclarationWithBody, + file: KtFile, +): String? { + val type = runCatching { ((declaration as? KtDeclaration)?.symbol as? KaCallableSymbol)?.returnType }.getOrNull() ?: return null + val rendered = renderedTypeTextOrNull(type) ?: return null + return shortenTypeText(rendered, importedNamesOf(file), starImportedPackagesOf(file)) +} + /** * Whether converting an expression body to a block body needs a `return`. * diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt index 47d1f43538..e96c76aefb 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -46,6 +46,10 @@ sealed interface AnchorForm { * An expression-bodied function or property accessor -- `fun area(r: Int) = r * r`. The `=` and * the body are replaced by a block body. [needsReturn] is false only when the declaration * returns `Unit`, where `return` is both unnecessary and wrong for a non-`Unit` expression. + * + * [returnTypeText] is the type to write into the signature, or null when there is nothing to write + * -- the declaration already spells its type out, or the block body infers `Unit` anyway. A block + * body with no declared type returns `Unit`, so `return ` without this would not compile. */ data class ConvertExpressionBody( val assignStart: Int, @@ -54,6 +58,7 @@ sealed interface AnchorForm { val indent: String, val innerIndent: String, val needsReturn: Boolean, + val returnTypeText: String? = null, ) : AnchorForm } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt new file mode 100644 index 0000000000..4db23f4256 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt @@ -0,0 +1,98 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.utils.renderName +import org.jetbrains.kotlin.analysis.api.KaExperimentalApi +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource +import org.jetbrains.kotlin.analysis.api.types.KaFlexibleType +import org.jetbrains.kotlin.analysis.api.types.KaType +import org.jetbrains.kotlin.psi.KtFile + +/** + * Types are rendered **fully qualified** and only then shortened against what the file can resolve. + * + * A short name resolves only when the file imports it or it comes from a default-imported package, and + * a refactoring that adds imports would be a much larger change -- so qualified is the safe starting + * point and [shortenTypeText] gives back readability where it provably costs nothing. + */ +@OptIn(KaExperimentalApi::class) +private val QUALIFIED_TYPE_RENDERER = KaTypeRendererForSource.WITH_QUALIFIED_NAMES + +/** Packages whose simple names resolve with no import at all on the JVM/Android target. */ +private val DEFAULT_IMPORTED_PACKAGES = + setOf( + "kotlin", + "kotlin.annotation", + "kotlin.collections", + "kotlin.comparisons", + "kotlin.io", + "kotlin.jvm", + "kotlin.ranges", + "kotlin.sequences", + "kotlin.text", + "java.lang", + ) + +/** A dotted run of identifiers -- one qualified name inside rendered type text. */ +private val QUALIFIED_NAME = Regex("""[\p{L}_][\p{L}\p{Nd}_]*(?:\.[\p{L}_][\p{L}\p{Nd}_]*)+""") + +/** + * A type that cannot be written out as source -- anonymous, intersection, a resolution error, or a + * platform type the renderer could not reduce (`List`, where the `!` is on a type argument). + * `!` is not Kotlin syntax anywhere, so its presence alone settles it. + */ +internal fun isUnrenderableTypeText(text: String): Boolean = + text.isBlank() || + text.contains("anonymous") || + text.contains("ERROR") || + text.contains(" & ") || + text.contains('!') + +/** + * One type as source text, fully qualified, or null when it cannot be written out. + * + * A platform type is unwrapped to its lower bound first: the renderer prints `String!`, which does not + * parse. Only the outermost bound is unwrapped, so a `!` on a type argument still reaches + * [isUnrenderableTypeText]. + */ +@OptIn(KaExperimentalApi::class) +internal fun KaSession.renderedTypeTextOrNull(type: KaType): String? = + runCatching { renderName((type as? KaFlexibleType)?.lowerBound ?: type, QUALIFIED_TYPE_RENDERER) } + .getOrNull() + ?.takeUnless(::isUnrenderableTypeText) + +/** + * Replaces each qualified name in [rendered] with its simple name when that name already resolves in + * the file -- because the file imports it exactly, star-imports its package, or it comes from a + * default-imported package. Everything else stays qualified: verbose, but it always compiles. + * + * Purely textual, so it needs no analysis session and is unit-testable on its own. A nested class + * (`com.example.Outer.Inner`) is only shortened by an import of the nested name itself; an import of + * the outer class leaves it alone rather than emitting an unresolvable `Inner`. + */ +internal fun shortenTypeText( + rendered: String, + importedNames: Set, + starImportedPackages: Set, +): String = + QUALIFIED_NAME.replace(rendered) { match -> + val qualified = match.value + val container = qualified.substringBeforeLast('.') + val resolvable = + qualified in importedNames || + container in DEFAULT_IMPORTED_PACKAGES || + container in starImportedPackages + if (resolvable) qualified.substringAfterLast('.') else qualified + } + +/** The fully qualified names [file] imports by name. Syntactic: no analysis session needed. */ +internal fun importedNamesOf(file: KtFile): Set = + file.importDirectives + .filterNot { it.isAllUnder } + .mapNotNullTo(mutableSetOf()) { it.importedFqName?.asString() } + +/** The packages [file] star-imports (`import com.example.*`). */ +internal fun starImportedPackagesOf(file: KtFile): Set = + file.importDirectives + .filter { it.isAllUnder } + .mapNotNullTo(mutableSetOf()) { it.importedFqName?.asString() } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt index 334146c19e..5f4c810269 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -245,6 +245,32 @@ class ExtractVariableEditTest { ) } + @Test + fun `writes the return type into the signature when the declaration has none`() { + val text = "fun area(r: Int) = r * r" + val candidate = spanOf(text, "r * r") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = candidate.start, + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = true, + returnTypeText = "Int", + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "squared", replaceAll = false)!! + + assertEquals( + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(text, result), + ) + } + @Test fun `null when there is nothing to replace`() { val text = "fun f() {}" diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 0db9c2cbb8..00a1c2c1b6 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -342,7 +342,7 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { assertEquals( """ package p - fun area(r: Int) { + fun area(r: Int): Int { val square = r * r return square + square } @@ -460,4 +460,100 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { .map { it.label }, ) } + + @Test + fun `converting an inferred-type expression body writes the type out`() { + val content = + """ + package p + fun area(r: Int) = r * r + """.trimIndent() + + val target = "r * r" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "squared", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `a declared return type is not written twice`() { + val content = + """ + package p + fun area(r: Int): Int = r * r + """.trimIndent() + + val target = "r * r" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "squared", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `a Unit-returning expression body gets neither a type nor a return`() { + val content = + """ + package p + fun report(value: Int) { + println(value) + } + fun show(text: String) = report(text.length + 1) + """.trimIndent() + + val target = "text.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun report(value: Int) {\n" + + "\tprintln(value)\n" + + "}\n" + + "fun show(text: String) {\n" + + "\tval length = text.length + 1\n" + + "\treport(length)\n" + + "}", + apply(content, rewrite), + ) + } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt index 1d212b8404..45bc4751ef 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt @@ -1,7 +1,9 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test /** The analysis-free primitives the refactoring is built from: name rules, indentation, soundness. */ @@ -139,4 +141,47 @@ class RefactorPrimitivesTest { excludeUnsoundOccurrences(listOf(TextSpan(10, 20)), TextSpan(70, 80), writeOffsets = emptyList()), ) } + + @Test + fun `shortens types from Kotlin's default-imported packages`() { + assertEquals("Int", shortenTypeText("kotlin.Int", emptySet(), emptySet())) + assertEquals( + "List", + shortenTypeText("kotlin.collections.List", emptySet(), emptySet()), + ) + } + + @Test + fun `keeps a type qualified when its short name would not resolve`() { + assertEquals("java.util.Date", shortenTypeText("java.util.Date", emptySet(), emptySet())) + // An import of the enclosing class is not an import of the nested one. + assertEquals( + "com.example.Outer.Inner", + shortenTypeText("com.example.Outer.Inner", setOf("com.example.Outer"), emptySet()), + ) + } + + @Test + fun `shortens a type the file already imports, by name or by star`() { + assertEquals("Date", shortenTypeText("java.util.Date", setOf("java.util.Date"), emptySet())) + assertEquals("Date", shortenTypeText("java.util.Date", emptySet(), setOf("java.util"))) + assertEquals( + "Flow", + shortenTypeText( + "kotlinx.coroutines.flow.Flow", + setOf("kotlinx.coroutines.flow.Flow", "com.example.Widget"), + emptySet(), + ), + ) + } + + @Test + fun `unrenderable type text is recognised`() { + assertTrue(isUnrenderableTypeText("")) + assertTrue(isUnrenderableTypeText("kotlin.collections.List")) + assertTrue(isUnrenderableTypeText("")) + assertTrue(isUnrenderableTypeText("ERROR CLASS: unresolved")) + assertTrue(isUnrenderableTypeText("kotlin.Any & kotlin.Comparable<*>")) + assertFalse(isUnrenderableTypeText("kotlin.Int")) + } } From 0840d48732188a3ad80721274238e0f07c892bd3 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 15:27:34 +0000 Subject: [PATCH 10/16] ADFA-4826: Anchor the declaration in the scope the user picked --- docs/features/kotlin-extract-variable.md | 8 +- .../utils/refactor/ExtractVariableEdit.kt | 33 ++-- .../kotlin/utils/refactor/ExtractionPlan.kt | 19 ++- .../lsp/kotlin/utils/refactor/ScopeChain.kt | 26 +++- .../ui/ExtractVariableViewModelTest.kt | 2 +- .../utils/refactor/ExtractVariableEditTest.kt | 146 +++++++++++++++++- .../ExtractVariablePlanEndToEndTest.kt | 40 +++++ 7 files changed, 243 insertions(+), 31 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index ffb53df5a6..5be55864f2 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -39,7 +39,9 @@ The chain member the user picked. The `val` is declared inside it. How the declaration is woven into an anchor scope, since not all Kotlin scopes are blocks: `ExistingBlock`, `WrapInBraces`, or `ConvertExpressionBody`. **Anchor point**: -The exact insertion offset - immediately before the first statement *within the anchor scope* that contains a replaced occurrence. +The exact insertion offset - the start of the line holding the first statement *within the anchor +scope* that contains a replaced occurrence. Recorded per rung in the plan (`ExistingBlock`'s +`statementSpans`), because it is the only thing that makes an outer rung differ from an inner one. **Occurrence**: A site inside the anchor scope that is structurally equal to the candidate *and* whose every name reference resolves to the same declaration. Sites made unsound by an intervening write are excluded, so an occurrence set is always safe to replace wholesale. @@ -142,6 +144,9 @@ Each chooser is hidden when it has nothing to ask: the expression chooser when t **R9 - Edit.** Exactly **one** `TextEdit`, built as a `RewriteSpan` covering one contiguous span. `IDELanguageClientImpl.applyActionEdits` applies each edit in its own `runOnUiThread` with no `beginBatchEdit`, and every range is interpreted against the *current* text - so a list of N edits would be applied against positions already shifted by its predecessors and would cost N undo steps with a typing window between each. Occurrences are substituted right-to-left within the span so an earlier substitution cannot shift a later offset. +The span is anchored on the chosen rung's statement, not on the occurrence: for an outer rung the +declaration goes above the whole enclosing statement, at that statement's indentation. + The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. **R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. @@ -169,6 +174,7 @@ The emitted text is **fully indented**: code-action edits bypass the editor's au 7. The same expression with an intervening reassignment of a `var` it reads offers only the contiguous sound run. 8. An expression using `it` inside a lambda offers no anchor outside that lambda. 9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. +9a. With a candidate inside a braced `if` inside a function, picking `fun name` in `Declare in` puts the declaration above the `if`, and picking `if block` puts it inside the branch. 10. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. 11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. 12. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index a4215500e6..712703ddd8 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -44,37 +44,42 @@ fun buildExtractVariableRewrite( val declaration = "val $name = $expression" return when (val form = scope.anchorForm) { - AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, targets, declaration, name) + is AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, form, targets, declaration, name) is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form, targets, declaration, name) is AnchorForm.ConvertExpressionBody -> convertExpressionBodyRewrite(fileText, form, targets, declaration, name) } } /** - * Inserts the declaration as its own line before the first served occurrence's line, and rewrites - * everything from there through the last occurrence. + * Inserts the declaration as its own line before the anchor statement, and rewrites everything from + * there through the last occurrence. * - * The rewritten span starts at that line's start (not at the occurrence) so the declaration lands on - * a line of its own at the right indentation, and ends at the last occurrence so untouched trailing - * code is left alone. + * The anchor is the statement *of this scope* that holds the first served occurrence, so picking an + * outer rung hoists the declaration above the enclosing statement rather than leaving it where the + * inner rung would have put it. The rewritten span starts at that statement's line start so the + * declaration lands on a line of its own at the right indentation, and ends at the last occurrence so + * untouched trailing code is left alone. + * + * Null when no statement of the scope contains the occurrence, which would mean the plan and the text + * disagree; the caller reports that rather than guessing. */ private fun existingBlockRewrite( fileText: String, + form: AnchorForm.ExistingBlock, targets: List, declaration: String, name: String, -): RewriteSpan { +): RewriteSpan? { val first = targets.first() val last = targets.last() - val lineStart = lineStartOffset(fileText, first.start) - val indent = leadingIndentAt(fileText, first.start) + val anchor = form.statementSpans.firstOrNull { it.start <= first.start && first.end <= it.end } ?: return null + val lineStart = lineStartOffset(fileText, anchor.start) + val indent = leadingIndentAt(fileText, anchor.start) val newline = detectNewline(fileText) - val body = replaceOccurrences(fileText, TextSpan(lineStart, last.end), targets, name) - return RewriteSpan( - span = TextSpan(lineStart, last.end), - newText = indent + declaration + newline + body, - ) + val span = TextSpan(lineStart, last.end) + val body = replaceOccurrences(fileText, span, targets, name) + return RewriteSpan(span = span, newText = indent + declaration + newline + body) } /** Wraps a braceless statement in a block containing the declaration and the original statement. */ diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt index e96c76aefb..379fe89960 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -21,14 +21,21 @@ data class TextSpan( sealed interface AnchorForm { /** * The scope already has a `{ ... }` body (function body, `if` block, lambda body, ...), so the - * declaration is simply a new statement line. + * declaration is a new statement line inside it. * - * Deliberately field-free: the insertion offset and indentation are both derived from the first - * occurrence being served, which is the candidate itself when replacing only one site and an - * earlier statement when replacing all. Storing a precomputed anchor would duplicate that and - * let the two drift apart. + * [statementSpans] are the block's direct child statements, ascending. The anchor point is the + * first of them containing the first served occurrence -- which is what makes an outer rung differ + * from an inner one. Anchoring on the occurrence's own line instead would make every rung of a + * chain produce the same edit. + * + * [contentSpan] is the region *inside* the braces. It tells a block written on one line + * (`items.map { it.length + 1 }`) from a multi-line one, where inserting at the statement's line + * start would put the declaration outside the braces. */ - data object ExistingBlock : AnchorForm + data class ExistingBlock( + val contentSpan: TextSpan, + val statementSpans: List, + ) : AnchorForm /** * A braceless statement position -- `if (c) foo()`, a `when` entry, a braceless loop body. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt index 1361d45080..97ec38ddf2 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -112,7 +112,12 @@ private fun frameFor( scopeElement = parent, searchRange = parent.textRange.let { TextSpan(it.startOffset, it.endOffset) }, statementSpan = TextSpan(lineStart, inner.textRange.endOffset), - anchorForm = AnchorForm.ExistingBlock, + anchorForm = + AnchorForm.ExistingBlock( + contentSpan = contentSpanOf(parent), + statementSpans = + parent.statements.map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) }, + ), ) } @@ -241,6 +246,25 @@ private fun bracelessOwnerLabel( } } +/** + * The region inside a block's braces. + * + * A function, `if` or loop body owns its braces, so they are trimmed off. A lambda body block does not + * -- the braces and any `param ->` header belong to the enclosing function literal -- so its own range + * already *is* the content, which is what keeps the header on the brace line when the block is + * expanded. Deriving this from the block's text rather than from brace PSI keeps one code path for + * both shapes. + */ +internal fun contentSpanOf(block: KtBlockExpression): TextSpan { + val range = block.textRange + val text = block.text + return if (text.length >= 2 && text.startsWith("{") && text.endsWith("}")) { + TextSpan(range.startOffset + 1, range.endOffset - 1) + } else { + TextSpan(range.startOffset, range.endOffset) + } +} + /** Offset of the start of the line containing [offset]. */ internal fun lineStartOffset( text: String, diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt index 4f25b9a3aa..1b008a941b 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt @@ -25,7 +25,7 @@ class ExtractVariableViewModelTest { occurrences: Int, ) = ScopeOption( label = label, - anchorForm = AnchorForm.ExistingBlock, + anchorForm = AnchorForm.ExistingBlock(contentSpan = TextSpan(0, 100), statementSpans = emptyList()), occurrences = (0 until occurrences).map { TextSpan(it * 10, it * 10 + 5) }, ) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt index 5f4c810269..afb3b5cecf 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -42,6 +42,18 @@ class ExtractVariableEditTest { return spans } + /** + * The block rung of a single-block fixture: content is everything between the first `{` and the + * last `}`, and [statements] are the block's direct child statements in source order. + */ + private fun existingBlock( + text: String, + vararg statements: String, + ) = AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = statements.map { spanOf(text, it) }, + ) + private fun rewrite( text: String, candidate: TextSpan, @@ -62,7 +74,15 @@ class ExtractVariableEditTest { val text = "fun f(items: List) {\n\tprintln(items.size * 2)\n}" val candidate = spanOf(text, "items.size * 2") - val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! assertEquals( "fun f(items: List) {\n" + @@ -85,7 +105,15 @@ class ExtractVariableEditTest { // The user selected the middle one; the declaration must still hoist above the first. val candidate = occurrences[1] - val result = rewrite(text, candidate, AnchorForm.ExistingBlock, occurrences, "size", replaceAll = true)!! + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)", "log(items.size * 2)", "use(items.size * 2)"), + occurrences, + "size", + replaceAll = true, + )!! assertEquals( "fun f(items: List) {\n" + @@ -107,7 +135,15 @@ class ExtractVariableEditTest { "}" val occurrences = allSpansOf(text, "items.size * 2") - val result = rewrite(text, occurrences[0], AnchorForm.ExistingBlock, occurrences, "size", replaceAll = false)!! + val result = + rewrite( + text, + occurrences[0], + existingBlock(text, "println(items.size * 2)", "log(items.size * 2)"), + occurrences, + "size", + replaceAll = false, + )!! assertEquals( "fun f(items: List) {\n" + @@ -124,7 +160,15 @@ class ExtractVariableEditTest { val text = "fun f(items: List) {\n println(items.size * 2)\n}" val candidate = spanOf(text, "items.size * 2") - val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! assertEquals( "fun f(items: List) {\n" + @@ -140,7 +184,15 @@ class ExtractVariableEditTest { val text = "fun f(items: List) {\r\n\tprintln(items.size * 2)\r\n}" val candidate = spanOf(text, "items.size * 2") - val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! assertEquals( "fun f(items: List) {\r\n" + @@ -156,7 +208,15 @@ class ExtractVariableEditTest { val text = "class C {\n\tfun f(items: List) {\n\t\tprintln(items.size * 2)\n\t}\n}" val candidate = spanOf(text, "items.size * 2") - val result = rewrite(text, candidate, AnchorForm.ExistingBlock, listOf(candidate), "size", replaceAll = false)!! + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! assertEquals( "class C {\n" + @@ -278,7 +338,7 @@ class ExtractVariableEditTest { buildExtractVariableRewrite( fileText = text, candidateSpan = TextSpan(0, 3), - scope = ScopeOption("scope", AnchorForm.ExistingBlock, emptyList()), + scope = ScopeOption("scope", AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), emptyList()), name = "value", replaceAll = true, ), @@ -292,13 +352,83 @@ class ExtractVariableEditTest { buildExtractVariableRewrite( fileText = text, candidateSpan = TextSpan(0, 3), - scope = ScopeOption("scope", AnchorForm.ExistingBlock, listOf(TextSpan(0, text.length + 5))), + scope = + ScopeOption( + "scope", + AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), + listOf(TextSpan(0, text.length + 5)), + ), name = "value", replaceAll = true, ), ) } + @Test + fun `the inner rung declares inside the if block`() { + val text = + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\treturn a + b * 2\n" + + "\t}\n" + + "\treturn 0\n" + + "}" + val candidate = spanOf(text, "a + b * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, "\n\t\treturn a + b * 2\n\t"), + statementSpans = listOf(spanOf(text, "return a + b * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "total", replaceAll = false)!! + + assertEquals( + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\tval total = a + b * 2\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `the outer rung declares above the enclosing statement`() { + val text = + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\treturn a + b * 2\n" + + "\t}\n" + + "\treturn 0\n" + + "}" + val candidate = spanOf(text, "a + b * 2") + // The function block's rung: its statements are the whole `if` and the trailing `return 0`. + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = + listOf( + spanOf(text, "if (flag) {\n\t\treturn a + b * 2\n\t}"), + spanOf(text, "return 0"), + ), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "total", replaceAll = false)!! + + assertEquals( + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tval total = a + b * 2\n" + + "\tif (flag) {\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(text, result), + ) + } + @Test fun `position index line and column all agree`() { val text = "aa\nbbb\nc" diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 00a1c2c1b6..5801109cbc 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -521,6 +521,46 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { ) } + @Test + fun `picking the outer rung hoists the declaration above the enclosing statement`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b * 2 + } + return 0 + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + assertEquals(listOf("if block", "fun demo"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes[1], + name = "total", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tval total = a + b * 2\n" + + "\tif (flag) {\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(content, rewrite), + ) + } + @Test fun `a Unit-returning expression body gets neither a type nor a return`() { val content = From 4b0d94688838e877ce86e6d3146b1eb42eb498d7 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 16:08:05 +0000 Subject: [PATCH 11/16] ADFA-4826: Cover contentSpanOf and fix a nested-block fixture --- .../utils/refactor/ExtractVariableEditTest.kt | 12 +++- .../ExtractVariablePlanEndToEndTest.kt | 66 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt index afb3b5cecf..58fb0e672c 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -45,6 +45,9 @@ class ExtractVariableEditTest { /** * The block rung of a single-block fixture: content is everything between the first `{` and the * last `}`, and [statements] are the block's direct child statements in source order. + * + * Only correct for a fixture with exactly one brace pair -- a nested one (e.g. a class wrapping a + * function) needs its `AnchorForm.ExistingBlock` built by hand instead. */ private fun existingBlock( text: String, @@ -207,12 +210,19 @@ class ExtractVariableEditTest { fun `deeper indentation is preserved`() { val text = "class C {\n\tfun f(items: List) {\n\t\tprintln(items.size * 2)\n\t}\n}" val candidate = spanOf(text, "items.size * 2") + // Two brace pairs are nested here, so `existingBlock`'s "first { .. last }" heuristic would + // grab the class's braces instead of `fun f`'s -- built by hand for the inner pair instead. + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, "\n\t\tprintln(items.size * 2)\n\t"), + statementSpans = listOf(spanOf(text, "println(items.size * 2)")), + ) val result = rewrite( text, candidate, - existingBlock(text, "println(items.size * 2)"), + form, listOf(candidate), "size", replaceAll = false, diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 5801109cbc..d1f9559bec 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -1,6 +1,11 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtIfExpression +import org.jetbrains.kotlin.psi.KtLambdaExpression +import org.jetbrains.kotlin.psi.KtNamedFunction import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull @@ -561,6 +566,67 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { ) } + @Test + fun `contentSpanOf finds the region inside a block's braces`() { + val content = + """ + package p + fun functionBody(a: Int, b: Int): Int { + return a + b + } + fun ifBody(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b + } + return 0 + } + fun lambdaWithHeader(items: List): List { + return items.map { x -> x + 1 } + } + fun lambdaWithoutHeader(items: List): List { + return items.map { it + 1 } + } + fun emptyBody() {} + """.trimIndent() + val ktFile = createSourceFile("Main.kt", content) + val functions = ktFile.declarations.filterIsInstance().associateBy { it.name } + + fun contentOf(block: KtBlockExpression): String { + val span = contentSpanOf(block) + return content.substring(span.start, span.end) + } + + assertEquals("\n\treturn a + b\n", contentOf(functions.getValue("functionBody").bodyBlockExpression!!)) + + val ifBody = functions.getValue("ifBody").bodyBlockExpression!! + val ifThen = PsiTreeUtil.findChildOfType(ifBody, KtIfExpression::class.java)!!.then as KtBlockExpression + assertEquals("\n\tif (flag) {\n\t\treturn a + b\n\t}\n\treturn 0\n", contentOf(ifBody)) + assertEquals("\n\t\treturn a + b\n\t", contentOf(ifThen)) + + val lambdaWithHeaderBody = + PsiTreeUtil + .findChildOfType( + functions.getValue("lambdaWithHeader").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + .bodyExpression!! + val lambdaWithHeaderContent = contentOf(lambdaWithHeaderBody) + // The `x ->` header belongs to the enclosing function literal, not to this block. + assertFalse(lambdaWithHeaderContent.contains("->")) + assertEquals("x + 1", lambdaWithHeaderContent.trim()) + + val lambdaWithoutHeaderBody = + PsiTreeUtil + .findChildOfType( + functions.getValue("lambdaWithoutHeader").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + .bodyExpression!! + assertEquals("it + 1", contentOf(lambdaWithoutHeaderBody).trim()) + + assertEquals("", contentOf(functions.getValue("emptyBody").bodyBlockExpression!!)) + } + @Test fun `a Unit-returning expression body gets neither a type nor a return`() { val content = From bc9e08dd83d899a337b9d03ace41f92ed55a6d7c Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 16:20:30 +0000 Subject: [PATCH 12/16] ADFA-4826: Expand a block written on one line --- docs/features/kotlin-extract-variable.md | 8 +++ .../utils/refactor/ExtractVariableEdit.kt | 52 ++++++++++++++ .../utils/refactor/ExtractVariableEditTest.kt | 68 +++++++++++++++++++ .../ExtractVariablePlanEndToEndTest.kt | 37 ++++++++++ 4 files changed, 165 insertions(+) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index 5be55864f2..d58f7e2eba 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -147,6 +147,13 @@ Each chooser is hidden when it has nothing to ask: the expression chooser when t The span is anchored on the chosen rung's statement, not on the occurrence: for an outer rung the declaration goes above the whole enclosing statement, at that statement's indentation. +A block written on one line -- `items.map { it.length + 1 }`, `fun f(n: Int): Int { return n * 2 }`, +a one-line `if` body -- is expanded instead: the content between the braces moves onto its own line +with the declaration above it and the closing brace below. Anchoring on the statement's line start +there would place the declaration *before* the `{`, outside the scope the value belongs to, which +leaves a lambda's `it` unresolved. The braces themselves and a lambda's `param ->` header are left +where they are. + The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. **R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. @@ -175,6 +182,7 @@ The emitted text is **fully indented**: code-action edits bypass the editor's au 8. An expression using `it` inside a lambda offers no anchor outside that lambda. 9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. 9a. With a candidate inside a braced `if` inside a function, picking `fun name` in `Declare in` puts the declaration above the `if`, and picking `if block` puts it inside the branch. +9b. Extracting from `return items.map { it.length + 1 }` puts the declaration inside the lambda and expands the block over three lines; the same holds for a one-line function body. 10. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. 11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. 12. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index 712703ddd8..e739ba8491 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -74,6 +74,13 @@ private fun existingBlockRewrite( val last = targets.last() val anchor = form.statementSpans.firstOrNull { it.start <= first.start && first.end <= it.end } ?: return null val lineStart = lineStartOffset(fileText, anchor.start) + + // The statement shares its line with the block's opening brace (a one-line lambda or body). The + // line start is then *outside* the block, so the declaration has to go inside the braces instead. + if (lineStart < form.contentSpan.start) { + return oneLineBlockRewrite(fileText, form, targets, declaration, name) + } + val indent = leadingIndentAt(fileText, anchor.start) val newline = detectNewline(fileText) @@ -82,6 +89,41 @@ private fun existingBlockRewrite( return RewriteSpan(span = span, newText = indent + declaration + newline + body) } +/** + * Puts the declaration inside a block written on one line, moving the block's content and its closing + * brace onto their own lines. + * + * Only the content between the braces is rewritten: the braces, and a lambda's `param ->` header, + * stay exactly where they are, so the expansion cannot disturb the call around it. + */ +private fun oneLineBlockRewrite( + fileText: String, + form: AnchorForm.ExistingBlock, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val content = form.contentSpan + val newline = detectNewline(fileText) + val indent = leadingIndentAt(fileText, content.start) + val innerIndent = indent + detectIndentUnit(fileText) + + // A block that does not own its braces (a lambda body) stops short of them, leaving a single + // space between the content span and the brace on each side. Widen the replaced span over that + // gap so it does not survive the rewrite as a stray "{ " or " }". + val span = TextSpan(startOfWhitespaceBefore(fileText, content.start), endOfWhitespaceAfter(fileText, content.end)) + val body = replaceOccurrences(fileText, content, targets, name).trim() + + val newText = + buildString { + append(newline) + append(innerIndent).append(declaration).append(newline) + append(innerIndent).append(body).append(newline) + append(indent) + } + return RewriteSpan(span = span, newText = newText) +} + /** Wraps a braceless statement in a block containing the declaration and the original statement. */ private fun wrapInBracesRewrite( fileText: String, @@ -145,6 +187,16 @@ private fun startOfWhitespaceBefore( return index } +/** The offset where the run of whitespace starting at [offset] ends. */ +private fun endOfWhitespaceAfter( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index < text.length && text[index].isWhitespace()) index++ + return index +} + /** * Returns `[span]`'s text with every occurrence inside it replaced by [name]. Substitutes * right-to-left so an earlier replacement cannot invalidate a later offset. diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt index 58fb0e672c..9214ad9cdd 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -447,4 +447,72 @@ class ExtractVariableEditTest { assertEquals(0, position.column) assertEquals(7, position.index) } + + @Test + fun `expands a one-line lambda so the declaration lands inside the braces`() { + val text = "fun f(items: List): List {\n\treturn items.map { it.length + 1 }\n}" + val candidate = spanOf(text, "it.length + 1") + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, " it.length + 1 "), + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval length = it.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `expanding a one-line lambda keeps its parameter header on the brace line`() { + val text = "fun f(items: List): List {\n\treturn items.map { item -> item.length + 1 }\n}" + val candidate = spanOf(text, "item.length + 1") + // A lambda body block excludes the `item ->` header, so the header is outside the content span. + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, " item.length + 1 "), + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map { item ->\n" + + "\t\tval length = item.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `expands a one-line function body`() { + val text = "fun f(n: Int): Int { return n * 2 }" + val candidate = spanOf(text, "n * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = listOf(spanOf(text, "return n * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "doubled", replaceAll = false)!! + + assertEquals( + "fun f(n: Int): Int {\n" + + "\tval doubled = n * 2\n" + + "\treturn doubled\n" + + "}", + apply(text, result), + ) + } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index d1f9559bec..4e517ff9fe 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -662,4 +662,41 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { apply(content, rewrite), ) } + + @Test + fun `extracting from a one-line lambda stays inside the lambda`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { it.length + 1 } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `it` is lambda-scoped, so the lambda is the ceiling: there is no outer rung to choose. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval length = it.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } } From d15e4d72dbc50f0ffaef1f5baa74b43e97daba30 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 16:38:36 +0000 Subject: [PATCH 13/16] ADFA-4826: Expand only a block that is really written on one line --- docs/features/kotlin-extract-variable.md | 10 ++ .../utils/refactor/ExtractVariableEdit.kt | 14 ++- .../utils/refactor/ExtractVariableEditTest.kt | 44 +++++++ .../ExtractVariablePlanEndToEndTest.kt | 115 ++++++++++++++++++ 4 files changed, 180 insertions(+), 3 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index d58f7e2eba..be4e44bd58 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -154,6 +154,16 @@ there would place the declaration *before* the `{`, outside the scope the value leaves a lambda's `it` unresolved. The braces themselves and a lambda's `param ->` header are left where they are. +Whether a block counts as "one line" takes two conditions, not one. A single check against where the +block's content starts is not enough: a lambda body's block does not own its braces, so its content +span sits at the body's first token even when that token starts its own line, and comparing that alone +against the line start would wrongly expand an ordinary multi-line lambda. Both must hold: something +other than indentation already precedes the statement on its line (the brace, a header, or a prior +semicolon-separated statement), *and* the block's own content contains no newline (so re-emitting it +as a single line loses nothing). A multi-line lambda fails the first and keeps its shape; a multi-line +block with two semicolon-separated statements on one line satisfies the first but fails the second, so +it also keeps its shape, with the declaration hoisted above the whole line instead. + The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. **R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index e739ba8491..8ea546b4e7 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -75,9 +75,17 @@ private fun existingBlockRewrite( val anchor = form.statementSpans.firstOrNull { it.start <= first.start && first.end <= it.end } ?: return null val lineStart = lineStartOffset(fileText, anchor.start) - // The statement shares its line with the block's opening brace (a one-line lambda or body). The - // line start is then *outside* the block, so the declaration has to go inside the braces instead. - if (lineStart < form.contentSpan.start) { + // A block written on one line needs the declaration expanded inside the braces instead of hoisted + // above the line. `contentSpan.start` is not a reliable signal by itself: a lambda body's block + // does not own its braces, so `contentSpan.start` sits at the body's first token even when that + // token starts its own line -- comparing it to `lineStart` alone would misfire on an ordinary + // multi-line lambda. Two conditions together are what actually mean "one line": something other + // than indentation already precedes the statement on its line (the brace, a header, or a prior + // semicolon-separated statement), *and* the block's content itself contains no newline (so + // re-emitting it as a single line loses nothing). + val linePrefix = fileText.substring(lineStart, anchor.start) + val contentIsOneLine = !fileText.substring(form.contentSpan.start, form.contentSpan.end).contains('\n') + if (linePrefix.isNotBlank() && contentIsOneLine) { return oneLineBlockRewrite(fileText, form, targets, declaration, name) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt index 9214ad9cdd..a670badf17 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -515,4 +515,48 @@ class ExtractVariableEditTest { apply(text, result), ) } + + @Test + fun `widening is a no-op when a one-line lambda has no interior spaces`() { + val text = "fun f(items: List): List {\n\treturn items.map {it + 1}\n}" + val candidate = spanOf(text, "it + 1") + val form = + AnchorForm.ExistingBlock( + contentSpan = candidate, + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "value", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval value = it + 1\n" + + "\t\tvalue\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `keeps CRLF line endings when expanding a one-line block`() { + val text = "fun f(n: Int): Int { return n * 2 }\r\nval x = 1" + val candidate = spanOf(text, "n * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = listOf(spanOf(text, "return n * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "doubled", replaceAll = false)!! + + assertEquals( + "fun f(n: Int): Int {\r\n" + + "\tval doubled = n * 2\r\n" + + "\treturn doubled\r\n" + + "}\r\nval x = 1", + apply(text, result), + ) + } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index 4e517ff9fe..c20f219957 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -699,4 +699,119 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { apply(content, rewrite), ) } + + @Test + fun `extracting from a multi-line lambda with a header on its own line is not collapsed`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { x -> + x + 1 + } + } + """.trimIndent() + + val target = "x + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `x` is the lambda's own parameter, so the lambda is still the ceiling. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "next", + replaceAll = false, + )!! + + // The body already starts its own line, so this is the normal path, not the one-line + // expansion: the header and the closing brace are left exactly where they were. + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map { x ->\n" + + "\t\tval next = x + 1\n" + + "\t\tnext\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `extracting from a multi-line lambda without a header is not collapsed`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { + it + 1 + } + } + """.trimIndent() + + val target = "it + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "next", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval next = it + 1\n" + + "\t\tnext\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `extracting from a semicolon-joined statement leaves the block multi-line`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + val x = a + 1; return x + b + } + """.trimIndent() + + val target = "x + b" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "sum", + replaceAll = false, + )!! + + // A statement already precedes the candidate on this line, but the block itself spans several + // lines, so this is not a one-line block: the declaration hoists above the whole line instead + // of expanding it, and the two semicolon-joined statements stay together. + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\tval sum = x + b\n" + + "\tval x = a + 1; return sum\n" + + "}", + apply(content, rewrite), + ) + } } From 2ee08d713d8f5dce4b1746e18e933247c55326e1 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 17:31:46 +0000 Subject: [PATCH 14/16] ADFA-4826: Split the type-text renderer from its catching form --- .../androidide/lsp/kotlin/utils/refactor/TypeText.kt | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt index 4db23f4256..810caa658a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt @@ -54,12 +54,16 @@ internal fun isUnrenderableTypeText(text: String): Boolean = * A platform type is unwrapped to its lower bound first: the renderer prints `String!`, which does not * parse. Only the outermost bound is unwrapped, so a `!` on a type argument still reaches * [isUnrenderableTypeText]. + * + * Lets a failure from the renderer itself propagate, so a caller that must tell "the renderer threw" + * from "the type is unrenderable" can. [renderedTypeTextOrNull] is the catching form most callers want. */ @OptIn(KaExperimentalApi::class) -internal fun KaSession.renderedTypeTextOrNull(type: KaType): String? = - runCatching { renderName((type as? KaFlexibleType)?.lowerBound ?: type, QUALIFIED_TYPE_RENDERER) } - .getOrNull() - ?.takeUnless(::isUnrenderableTypeText) +internal fun KaSession.typeTextOrNull(type: KaType): String? = + renderName((type as? KaFlexibleType)?.lowerBound ?: type, QUALIFIED_TYPE_RENDERER) + .takeUnless(::isUnrenderableTypeText) + +internal fun KaSession.renderedTypeTextOrNull(type: KaType): String? = runCatching { typeTextOrNull(type) }.getOrNull() /** * Replaces each qualified name in [rendered] with its simple name when that name already resolves in From 257a81fab4ba31b622a27502389003d4c92d25e4 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 18:27:35 +0000 Subject: [PATCH 15/16] ADFA-4826: Decline a block whose statement shares the brace line A block whose first served statement shares the opening-brace line but whose content spans several lines fell through the one-line-expansion check into the normal hoist path, anchoring above the block's own opening delimiter -- outside the scope the user picked. For a lambda this put the declaration where `it` is unresolved, emitting Kotlin that does not compile. Also fix contentSpanOf: it decided brace ownership by sniffing the block's own text for a leading `{` and trailing `}`, which misreads a lambda whose sole statement is itself a lambda literal (`{ x -> { x + 1 } }`) as owning its braces, returning the inner lambda's interior instead of the outer body's content. Ownership is now decided structurally, from the block's parent. --- docs/features/kotlin-extract-variable.md | 6 +++ .../utils/refactor/ExtractVariableEdit.kt | 11 ++++ .../lsp/kotlin/utils/refactor/ScopeChain.kt | 13 ++--- .../ExtractVariablePlanEndToEndTest.kt | 50 +++++++++++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index be4e44bd58..12b3753625 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -164,6 +164,12 @@ as a single line loses nothing). A multi-line lambda fails the first and keeps i block with two semicolon-separated statements on one line satisfies the first but fails the second, so it also keeps its shape, with the declaration hoisted above the whole line instead. +A block that fails *both* conditions -- something besides indentation precedes the statement on its +line, but the block's own content spans more than one line, as in `items.forEach { log(x)\n\tlog(y) }` +-- is **declined** rather than hoisted. Hoisting would anchor before the block's own opening delimiter, +outside the scope the user picked, which is unsound whenever anything inside that scope (a lambda's +`it`, say) is not visible there. + The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. **R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index 8ea546b4e7..ba2822a54f 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -89,6 +89,17 @@ private fun existingBlockRewrite( return oneLineBlockRewrite(fileText, form, targets, declaration, name) } + // A lambda body's content starts right at its first token with no owned whitespace, so `lineStart` + // sits before `contentSpan.start` on plain indentation alone -- that gap must not trigger a + // decline. What does mean "outside the block" is *real code* in that gap: the block's own opening + // delimiter (a call and its brace, a header) sharing the anchor's line, which only happens for the + // multi-line case the one-line check above did not catch. Anchoring there would put the + // declaration before that delimiter, outside the scope the user picked. Declining is safe; hoisting + // is not. + if (form.contentSpan.start > lineStart && fileText.substring(lineStart, form.contentSpan.start).isNotBlank()) { + return null + } + val indent = leadingIndentAt(fileText, anchor.start) val newline = detectNewline(fileText) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt index 97ec38ddf2..73c7336284 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -252,16 +252,17 @@ private fun bracelessOwnerLabel( * A function, `if` or loop body owns its braces, so they are trimmed off. A lambda body block does not * -- the braces and any `param ->` header belong to the enclosing function literal -- so its own range * already *is* the content, which is what keeps the header on the brace line when the block is - * expanded. Deriving this from the block's text rather than from brace PSI keeps one code path for - * both shapes. + * expanded. Ownership is decided structurally, by the block's parent, rather than by sniffing the + * block's own text for a leading `{` and trailing `}`: a lambda body whose sole statement is itself a + * lambda literal (`{ x -> { x + 1 } }`) has text that looks brace-owned, and sniffing it would trim off + * that inner lambda's own braces and return its interior instead of the outer body's full content. */ internal fun contentSpanOf(block: KtBlockExpression): TextSpan { val range = block.textRange - val text = block.text - return if (text.length >= 2 && text.startsWith("{") && text.endsWith("}")) { - TextSpan(range.startOffset + 1, range.endOffset - 1) - } else { + return if (block.parent is KtFunctionLiteral) { TextSpan(range.startOffset, range.endOffset) + } else { + TextSpan(range.startOffset + 1, range.endOffset - 1) } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index c20f219957..d118ba1244 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -9,6 +9,7 @@ import org.jetbrains.kotlin.psi.KtNamedFunction import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -587,6 +588,9 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { return items.map { it + 1 } } fun emptyBody() {} + fun nestedLambda(items: List): List<() -> Int> { + return items.map { x -> { x + 1 } } + } """.trimIndent() val ktFile = createSourceFile("Main.kt", content) val functions = ktFile.declarations.filterIsInstance().associateBy { it.name } @@ -625,6 +629,20 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { assertEquals("it + 1", contentOf(lambdaWithoutHeaderBody).trim()) assertEquals("", contentOf(functions.getValue("emptyBody").bodyBlockExpression!!)) + + // The outer lambda's sole statement is itself a lambda literal, so its text alone (`{ x + 1 }`) + // looks brace-owned; the content must still be that whole statement, not the inner lambda's + // interior. + val nestedOuterLambda = + PsiTreeUtil.findChildOfType( + functions.getValue("nestedLambda").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + val nestedOuterBody = nestedOuterLambda.bodyExpression!! + assertEquals("{ x + 1 }", contentOf(nestedOuterBody).trim()) + + val nestedInnerLambda = PsiTreeUtil.findChildOfType(nestedOuterBody, KtLambdaExpression::class.java)!! + assertEquals("x + 1", contentOf(nestedInnerLambda.bodyExpression!!).trim()) } @Test @@ -779,6 +797,38 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { ) } + @Test + fun `declines a lambda whose first statement shares the brace line but the block spans several lines`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(items: List) { + items.forEach { log(it.length + 1) + log(it) } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `it` is lambda-scoped, so the lambda body is the only legal anchor. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + // The statement shares the opening-brace line, but the block itself spans two lines, so this is + // not the one-line expansion case. Anchoring at the line start would put the declaration before + // the lambda's `{`, where `it` is out of scope -- declining is the only safe outcome here. + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + ) + assertNull(rewrite) + } + @Test fun `extracting from a semicolon-joined statement leaves the block multi-line`() { val content = From e5d36c164fee7425e60a66934c38d4c690f08c44 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 18:28:09 +0000 Subject: [PATCH 16/16] ADFA-4826: Tidy the expression-body conversion and its docs Nothing was folded into the Unit case when deciding whether an expression-body conversion needs a `return`, so a Nothing-returning function (`fun boom() = error(...)`) lost both its `return` and its inferred return type, silently narrowing it to Unit and breaking a caller that uses it in a Nothing position (`x ?: boom()`). Only Unit is excluded now; Nothing goes through the normal return-type-writing path. Also: - Dedupe the symbol-to-return-type lookup into one KaSession.returnTypeOf, dropping the always-succeeding `as? KtDeclaration` cast. - ScopeChain: drop the unread ScopeFrame.statementSpan field and the dead `branch` local. - TypeText: document that the "anonymous"/"ERROR" substring checks in isUnrenderableTypeText are ambiguous but fail safe, and stop shortening a star-imported type when the file also imports a different type of the same simple name. - docs/features/kotlin-extract-variable.md: reword the Status line, the "Refactoring plan" glossary entry and a code comment that referenced the RefactoringPlan supertype and ADR 0013 as already landed -- both arrive with extract method (ADFA-5080); fix the "Anchor point" glossary entry to match the current anchoring behaviour; renumber the 9a/9b acceptance criteria into real ordered items. --- docs/features/kotlin-extract-variable.md | 20 +++++------ .../utils/refactor/ExtractVariableEdit.kt | 3 ++ .../utils/refactor/ExtractVariablePlanner.kt | 21 +++++++----- .../kotlin/utils/refactor/ExtractionPlan.kt | 5 +-- .../lsp/kotlin/utils/refactor/ScopeChain.kt | 9 +---- .../lsp/kotlin/utils/refactor/TypeText.kt | 14 ++++++-- .../ExtractVariablePlanEndToEndTest.kt | 34 +++++++++++++++++++ .../utils/refactor/RefactorPrimitivesTest.kt | 15 ++++++++ 8 files changed, 91 insertions(+), 30 deletions(-) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index 12b3753625..44e27107ef 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -1,7 +1,7 @@ # Kotlin extract variable (K2 LSP) - **Ticket:** ADFA-4826 (subtask of ADFA-3317; split out of the closed ADFA-3324 "Refactoring"). Extract method was originally part of this subtask and is now ADFA-5080. -- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename (the sealed `RefactoringPlan` supertype it will sit under has landed). +- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename under a sealed `RefactoringPlan` supertype, which arrives with extract method (ADFA-5080). - **Module:** `lsp/kotlin` Bind the expression at the cursor, or the selected one, to a new local `val`, and replace the occurrences of that expression with the new name. @@ -48,7 +48,7 @@ A site inside the anchor scope that is structurally equal to the candidate *and* _Avoid_: duplicate, match, usage. **Refactoring plan**: -The complete result of the background analysis pass - the sealed `RefactoringPlan`, carrying the analysed `fileText` and its `documentVersion`. Plain data: no PSI, no symbols, no session. `ExtractVariablePlan` is this refactoring's subtype. +The complete result of the background analysis pass, carrying the analysed `fileText` and its `documentVersion`. Plain data: no PSI, no symbols, no session. Currently `ExtractionPlan`; extract method (ADFA-5080) adds a sealed `RefactoringPlan` supertype and renames this to `ExtractVariablePlan`, its subtype. _Avoid_: model, result, context. **Rewrite span**: @@ -197,14 +197,14 @@ The emitted text is **fully indented**: code-action edits bypass the editor's au 7. The same expression with an intervening reassignment of a `var` it reads offers only the contiguous sound run. 8. An expression using `it` inside a lambda offers no anchor outside that lambda. 9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. -9a. With a candidate inside a braced `if` inside a function, picking `fun name` in `Declare in` puts the declaration above the `if`, and picking `if block` puts it inside the branch. -9b. Extracting from `return items.map { it.length + 1 }` puts the declaration inside the lambda and expands the block over three lines; the same holds for a one-line function body. -10. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. -11. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. -12. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. -13. Editing the file while the sheet is open, then confirming, reports "The file changed. Try extracting again." and leaves the file untouched. -14. One undo restores the file exactly. -15. A file indented with spaces receives space-indented output; a CRLF file keeps CRLF. +10. With a candidate inside a braced `if` inside a function, picking `fun name` in `Declare in` puts the declaration above the `if`, and picking `if block` puts it inside the branch. +11. Extracting from `return items.map { it.length + 1 }` puts the declaration inside the lambda and expands the block over three lines; the same holds for a one-line function body. +12. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. +13. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. +14. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. +15. Editing the file while the sheet is open, then confirming, reports "The file changed. Try extracting again." and leaves the file untouched. +16. One undo restores the file exactly. +17. A file indented with spaces receives space-indented output; a CRLF file keeps CRLF. ## Design diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt index ba2822a54f..2eed7a334d 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -38,6 +38,9 @@ fun buildExtractVariableRewrite( (if (replaceAll) scope.occurrences else listOf(candidateSpan)) .sortedBy { it.start } .takeIf { it.isNotEmpty() } ?: return null + // Only targets are bounds-checked against fileText; contentSpan/statementSpans are trusted + // unchecked. That is safe only because fileText is the plan's own text, not the live document -- + // if a caller ever passed live text here instead, those spans would need the same check. if (targets.any { it.end > fileText.length }) return null val expression = fileText.substring(candidateSpan.start, candidateSpan.end) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt index 0b4a058c41..5f25af3e5c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt @@ -12,7 +12,6 @@ import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol import org.jetbrains.kotlin.analysis.api.types.KaType import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.psi.KtCallableDeclaration -import org.jetbrains.kotlin.psi.KtDeclaration import org.jetbrains.kotlin.psi.KtDeclarationWithBody import org.jetbrains.kotlin.psi.KtExpression import org.jetbrains.kotlin.psi.KtFile @@ -103,7 +102,8 @@ private fun KaSession.candidateFor(expression: KtExpression): CandidateExpressio * * Returns null when the rung cannot be honoured: converting an expression body whose return type is * neither declared nor renderable would emit a block body that does not compile, and declining is - * always safe (ADR 0013). + * always safe -- the decline-rather-than-rewrite principle that ADR 0013 records, landing alongside + * extract method (ADFA-5080). */ private fun KaSession.scopeOptionFor( expression: KtExpression, @@ -148,12 +148,16 @@ private fun KtDeclarationWithBody.declaresReturnType(): Boolean = else -> false } +/** The declaration's resolved return type, or null when it cannot be resolved. */ +private fun KaSession.returnTypeOf(declaration: KtDeclarationWithBody): KaType? = + runCatching { (declaration.symbol as? KaCallableSymbol)?.returnType }.getOrNull() + /** The declaration's return type as source text, shortened where the file can resolve it. */ private fun KaSession.returnTypeTextOf( declaration: KtDeclarationWithBody, file: KtFile, ): String? { - val type = runCatching { ((declaration as? KtDeclaration)?.symbol as? KaCallableSymbol)?.returnType }.getOrNull() ?: return null + val type = returnTypeOf(declaration) ?: return null val rendered = renderedTypeTextOrNull(type) ?: return null return shortenTypeText(rendered, importedNamesOf(file), starImportedPackagesOf(file)) } @@ -162,15 +166,16 @@ private fun KaSession.returnTypeTextOf( * Whether converting an expression body to a block body needs a `return`. * * False only for a `Unit`-returning function, where `return expr` on a non-`Unit` expression would - * not compile and is unnecessary anyway. Defaults to true, which is right for everything else + * not compile and is unnecessary anyway. `Nothing` is deliberately not folded in here even though + * [isValuelessType] treats it like `Unit` for the R4 candidate filter -- a `Nothing`-returning + * function needs its `return` and its written-out type kept, or a caller using it in a `Nothing` + * position (`x ?: boom()`) stops compiling. Defaults to true, which is right for everything else * including property accessors. */ private fun KaSession.expressionBodyNeedsReturn(bodyExpression: PsiElement): Boolean { val declaration = bodyExpression.parent as? KtDeclarationWithBody ?: return true - val returnType = - runCatching { ((declaration as? KtDeclaration)?.symbol as? KaCallableSymbol)?.returnType }.getOrNull() - ?: return true - return !isValuelessType(returnType) + val returnType = returnTypeOf(declaration) ?: return true + return !runCatching { returnType.isUnitType }.getOrDefault(false) } /** `Unit` and `Nothing` carry no value worth binding to a `val`. */ diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt index 379fe89960..be948e179b 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -115,8 +115,9 @@ data class CandidateExpression( * candidate's own statement through enclosing blocks, crossing a lambda boundary only when nothing * lambda-scoped is referenced, and stopping at the enclosing method body. * - **Anchor scope** -- the chain member the user picked. The `val` is declared inside it. - * - **Anchor point** -- the exact insertion offset: immediately before the first statement *within the - * anchor scope* that contains a replaced occurrence. + * - **Anchor point** -- the exact insertion offset: the start of the line holding the first statement + * *within the anchor scope* that contains a replaced occurrence, or inside the braces when that + * statement shares its line with a block written on one line. * - **Occurrence** -- a site inside the anchor scope that is structurally equal to the candidate *and* * whose every name reference resolves to the same symbol. Sites made unsound by an intervening * reassignment are excluded, so an occurrence set is always safe to replace wholesale. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt index 73c7336284..d89c570e5a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -22,14 +22,12 @@ import org.jetbrains.kotlin.psi.KtWhileExpression * * [scopeElement] is the PSI node that *is* the scope, used to decide whether a referenced * declaration lives inside it (see [truncateAtCeiling]). [searchRange] bounds the occurrence search - * for this rung. [statementSpan] is the statement within this scope that contains the candidate -- - * the fallback anchor when only the selected occurrence is replaced. + * for this rung. */ data class ScopeFrame( val label: String, val scopeElement: PsiElement, val searchRange: TextSpan, - val statementSpan: TextSpan, val anchorForm: AnchorForm, ) @@ -106,12 +104,10 @@ private fun frameFor( val controlOwner = (parent as? KtContainerNodeForControlStructureBody)?.parent if (parent is KtBlockExpression) { - val lineStart = lineStartOffset(text, inner.textRange.startOffset) return ScopeFrame( label = blockLabel(parent), scopeElement = parent, searchRange = parent.textRange.let { TextSpan(it.startOffset, it.endOffset) }, - statementSpan = TextSpan(lineStart, inner.textRange.endOffset), anchorForm = AnchorForm.ExistingBlock( contentSpan = contentSpanOf(parent), @@ -130,7 +126,6 @@ private fun frameFor( label = bracelessLabel, scopeElement = inner, searchRange = span, - statementSpan = span, anchorForm = AnchorForm.WrapInBraces( bodyStart = span.start, @@ -149,7 +144,6 @@ private fun frameFor( label = declarationLabel(parent), scopeElement = inner, searchRange = span, - statementSpan = span, anchorForm = AnchorForm.ConvertExpressionBody( assignStart = assign.textRange.startOffset, @@ -187,7 +181,6 @@ private fun isCeilingBody(scopeElement: PsiElement): Boolean { private fun blockLabel(block: KtBlockExpression): String { val parent = block.parent val container = parent as? KtContainerNodeForControlStructureBody - val branch = container ?: block return when (val owner = container?.parent ?: parent) { is KtNamedFunction -> "fun ${owner.name ?: ""}" is KtPropertyAccessor -> if (owner.isGetter) "getter" else "setter" diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt index 810caa658a..b2b3efbae1 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt @@ -40,6 +40,11 @@ private val QUALIFIED_NAME = Regex("""[\p{L}_][\p{L}\p{Nd}_]*(?:\.[\p{L}_][\p{L} * A type that cannot be written out as source -- anonymous, intersection, a resolution error, or a * platform type the renderer could not reduce (`List`, where the `!` is on a type argument). * `!` is not Kotlin syntax anywhere, so its presence alone settles it. + * + * The `"anonymous"` and `"ERROR"` substring checks are not unambiguous -- a real type named + * `com.example.AnonymousUser` or `p.ERRORS` would also match. Both fail safe: a false positive only + * declines the rung instead of emitting a block body that does not compile, so the heuristic is left + * as-is rather than made precise. */ internal fun isUnrenderableTypeText(text: String): Boolean = text.isBlank() || @@ -73,6 +78,10 @@ internal fun KaSession.renderedTypeTextOrNull(type: KaType): String? = runCatchi * Purely textual, so it needs no analysis session and is unit-testable on its own. A nested class * (`com.example.Outer.Inner`) is only shortened by an import of the nested name itself; an import of * the outer class leaves it alone rather than emitting an unresolvable `Inner`. + * + * A star import is trusted only when nothing else in the file imports the same simple name from a + * different package -- that explicit import would resolve first, so writing the short name here would + * silently name the wrong type. */ internal fun shortenTypeText( rendered: String, @@ -82,11 +91,12 @@ internal fun shortenTypeText( QUALIFIED_NAME.replace(rendered) { match -> val qualified = match.value val container = qualified.substringBeforeLast('.') + val simpleName = qualified.substringAfterLast('.') val resolvable = qualified in importedNames || container in DEFAULT_IMPORTED_PACKAGES || - container in starImportedPackages - if (resolvable) qualified.substringAfterLast('.') else qualified + (container in starImportedPackages && importedNames.none { it.endsWith(".$simpleName") }) + if (resolvable) simpleName else qualified } /** The fully qualified names [file] imports by name. Syntactic: no analysis session needed. */ diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt index d118ba1244..08430fc2a7 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -681,6 +681,40 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { ) } + @Test + fun `converting a Nothing-returning expression body preserves the signature`() { + val content = + """ + package p + fun boom(name: String) = error("bad " + name) + fun demo(x: Int?): Int = x ?: boom("missing") + """.trimIndent() + + val target = "\"bad \" + name" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "message", + replaceAll = false, + )!! + + // `boom`'s inferred return type is `Nothing`; folding it into the `Unit` case would drop both + // the `return` and the written-out `: Nothing`, and `x ?: boom(...)` would stop compiling. + assertEquals( + "package p\n" + + "fun boom(name: String): Nothing {\n" + + "\tval message = \"bad \" + name\n" + + "\treturn error(message)\n" + + "}\n" + + "fun demo(x: Int?): Int = x ?: boom(\"missing\")", + apply(content, rewrite), + ) + } + @Test fun `extracting from a one-line lambda stays inside the lambda`() { val content = diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt index 45bc4751ef..6303290b14 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt @@ -175,6 +175,21 @@ class RefactorPrimitivesTest { ) } + @Test + fun `a star import is skipped when a colliding name is imported from elsewhere`() { + // An explicit import of a different `Date` shadows the star import, so shortening would + // resolve to the wrong type. + assertEquals( + "java.util.Date", + shortenTypeText("java.util.Date", setOf("com.example.Date"), setOf("java.util")), + ) + // With nothing colliding, the star import still shortens as before. + assertEquals( + "Date", + shortenTypeText("java.util.Date", emptySet(), setOf("java.util")), + ) + } + @Test fun `unrenderable type text is recognised`() { assertTrue(isUnrenderableTypeText(""))