diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1002ead379..fda76f27ea 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -59,7 +59,7 @@ Strategy: **layer-and-subsystem based**, not feature-by-feature. The Gradle buil |---|---|---| | Application | `app` | The IDE itself — activities, fragments, services, DI, agent, web server. Wires everything together. | | Build engine | `subprojects:tooling-api*`, `gradle-plugin*`, `subprojects:projects`, `subprojects:builder-model-impl` | Runs a real Gradle build of the user's project out-of-process and streams events back. | -| Language tooling | `lsp:{api,java,kotlin,xml,indexing,…}`, `lexers`, `editor*`, `editor-treesitter` | Language servers, indexing, the Sora-based editor and highlighting. | +| Language tooling | `lsp:{api,java,kotlin,xml,indexing,refactor-core,ui,…}`, `lexers`, `editor*`, `editor-treesitter` | Language servers, indexing, the Sora-based editor and highlighting. `lsp:refactor-core` holds the language-agnostic half of the refactorings (offset spans, block geometry, rewrite composition, name primitives) so `lsp:java` and `lsp:kotlin` share one copy; `lsp:ui` holds the Compose sheets they share. Neither depends on a language server. | | UI design tooling | `layouteditor`, `uidesigner`, `xml-inflater`, `vectormaster`, `compose-preview` | Visual/XML design surfaces for the *user's* app. | | Shell | `termux:{termux-app,termux-shared,termux-view,termux-emulator}` | Embedded Termux shell and terminal. | | Plugin system | `plugin-api`, `plugin-api:plugin-builder`, `plugin-manager` | In-app plugin SDK + manager — `AndroidManifest.xml` `` contract, permissions, extensions. See [plugin-api.md](docs/plugin-api.md) for the API surface & compatibility policy. | 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 1a46ea1775..aa709765a5 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -85,6 +85,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_UNUSED_IMPORTS = "editor.codeactions.unusedimports" const val EDITOR_CODE_ACTIONS_ORGANIZE_IMPORTS = "editor.codeactions.organizeimports" const val EDITOR_CODE_ACTIONS_TRY_CATCH = "editor.codeactions.trycatch" + const val EDITOR_CODE_ACTIONS_EXTRACT_VARIABLE = "editor.codeactions.extractvariable" // Kotlin code actions. Tags are per-language even where the action exists in both languages, // so the tooltip can describe the Kotlin behaviour (see ADFA-4730). diff --git a/lsp/java/build.gradle.kts b/lsp/java/build.gradle.kts index 70b545f02d..a8e177ca6d 100644 --- a/lsp/java/build.gradle.kts +++ b/lsp/java/build.gradle.kts @@ -54,6 +54,8 @@ dependencies { implementation(projects.editorApi) implementation(projects.resources) implementation(projects.lsp.api) + implementation(projects.lsp.refactorCore) + implementation(projects.lsp.ui) implementation(projects.lsp.jvmSymbolIndex) implementation(projects.subprojects.libjdwp) implementation(projects.subprojects.javacServices) diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractVariableAction.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractVariableAction.kt new file mode 100644 index 0000000000..acd8cfc1da --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractVariableAction.kt @@ -0,0 +1,173 @@ +package com.itsaky.androidide.lsp.java.actions + +import com.itsaky.androidide.actions.ActionData +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.java.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.java.refactor.JAVA_KEYWORDS +import com.itsaky.androidide.lsp.java.refactor.JAVA_NAME_MESSAGES +import com.itsaky.androidide.lsp.java.refactor.buildExtractVariableRewrite +import com.itsaky.androidide.lsp.java.refactor.buildExtractionPlan +import com.itsaky.androidide.lsp.java.refactor.candidateAndScopeFor +import com.itsaky.androidide.lsp.java.refactor.toCandidateViews +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.lsp.refactor.toTextEdit +import com.itsaky.androidide.lsp.ui.ExtractVariableSelection +import com.itsaky.androidide.lsp.ui.ExtractVariableSheet +import com.itsaky.androidide.lsp.ui.findFragmentActivity +import com.itsaky.androidide.projects.FileManager +import com.itsaky.androidide.resources.R +import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo +import org.slf4j.LoggerFactory +import java.nio.file.Path +import kotlin.coroutines.cancellation.CancellationException + +/** + * Extracts the expression at the cursor, or the selected one, into a local variable. + * + * The work is split so nothing heavy touches the UI thread: [execAction] runs one attributed compile + * and returns a plain-data [ExtractionPlan] covering every candidate, then [postExec] shows the shared + * sheet and turns the user's selection into a single text edit with pure offset arithmetic. + */ +class ExtractVariableAction : BaseJavaCodeAction() { + companion object { + const val ID = "ide.editor.lsp.java.extractVariable" + + private val log = LoggerFactory.getLogger(ExtractVariableAction::class.java) + } + + override val titleTextRes: Int = R.string.action_extract_variable + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_EXTRACT_VARIABLE + + override val id: String = ID + override var label: String = "" + + // Deciding whether anything is extractable needs an attributed compile, far too costly for + // prepare() on the UI thread. BaseJavaCodeAction's file-type and module gate is all that applies; + // the action stays visible on any Java file and reports "nothing to extract" instead, as + // OrganizeImportsAction does. + override var requiresUIThread: Boolean = false + + override suspend fun execAction(data: ActionData): ExtractionPlan { + val file = data.requireFile().toPath() + val cursor = data.requireEditor().cursor + val selectionStart = minOf(cursor.left, cursor.right) + val selectionEnd = maxOf(cursor.left, cursor.right) + val version = documentVersionOf(file) + + // Resolving the compiler and taking its lock can both throw, and neither is inside the planner's + // own guard. DefaultActionsRegistry catches only IllegalArgumentException and this runs on a scope + // with no exception handler, so anything else would crash the app rather than fail the action. + return runCatching { + data.requireCompiler().compile(file).get { task -> + buildExtractionPlan(task, file, selectionStart, selectionEnd, version) + } + }.getOrElse { error -> + if (error is CancellationException) throw error + log.warn("Could not analyse {} for extract variable.", file, error) + ExtractionPlan.empty() + } + } + + 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 context = data.requireContext() + val activity = + context.findFragmentActivity() + ?: run { + // A wiring problem rather than a user path: the editor is always hosted by one. + log.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.toCandidateViews(context), + JAVA_KEYWORDS, + JAVA_NAME_MESSAGES, + ) { selection -> applySelection(data, result, selection) } + if (!shown) { + log.warn("Fragment manager unavailable. Cannot show the extract sheet.") + } + } + + /** + * Turns the user's selection 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 applySelection( + data: ActionData, + plan: ExtractionPlan, + selection: ExtractVariableSelection, + ) { + val file = data.requireFile().toPath() + // A plan built while the document was closed carries no version to compare, so there is nothing + // to prove the text still matches: refuse rather than apply spans on trust. + if (plan.documentVersion == null || documentVersionOf(file) != plan.documentVersion) { + flashInfo(R.string.msg_extract_variable_file_changed) + return + } + + val (candidate, scope) = + plan.candidateAndScopeFor(selection) ?: run { + log.warn("Selection {} does not address the plan it came from.", selection) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val rewrite = + buildExtractVariableRewrite( + fileText = plan.fileText, + candidateSpan = candidate.span, + declaredType = candidate.declaredType, + scope = scope, + name = selection.name, + replaceAll = selection.replaceAll, + ) ?: run { + log.warn("Could not build an extract-variable rewrite for '{}'", candidate.label) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val client = + data.getLanguageClient() ?: run { + log.warn("No language client set. Cannot extract variable.") + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = listOf(DocumentChange(file = file, edits = listOf(rewrite.toTextEdit(plan.fileText)))), + kind = CodeActionKind.QuickFix, + // The rewrite is emitted fully indented. Running google-java-format here would reformat + // the whole file into the same undo step as the extraction. + command = Command("", ""), + ), + ) + } + + /** Null when the document is not open, which the confirm guard treats as unverifiable and refuses. */ + private fun documentVersionOf(path: Path): Int? = FileManager.getActiveDocument(path)?.version +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionsMenu.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionsMenu.kt index d6bc8421e5..f08014bcbc 100644 --- a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionsMenu.kt +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionsMenu.kt @@ -93,5 +93,6 @@ object JavaCodeActionsMenu : IActionsMenuProvider { CATCH_BODY, TooltipTag.EDITOR_CODE_ACTIONS_TRY_CATCH, ), + ExtractVariableAction(), ) } diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/CandidateExpressions.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/CandidateExpressions.kt new file mode 100644 index 0000000000..313735126e --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/CandidateExpressions.kt @@ -0,0 +1,347 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.MAX_CANDIDATES +import com.itsaky.androidide.lsp.refactor.TextSpan +import jdkx.lang.model.element.ElementKind +import openjdk.source.tree.AnnotatedTypeTree +import openjdk.source.tree.AnnotationTree +import openjdk.source.tree.ArrayTypeTree +import openjdk.source.tree.AssignmentTree +import openjdk.source.tree.BinaryTree +import openjdk.source.tree.BlockTree +import openjdk.source.tree.CaseTree +import openjdk.source.tree.ClassTree +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.CompoundAssignmentTree +import openjdk.source.tree.ConditionalExpressionTree +import openjdk.source.tree.DoWhileLoopTree +import openjdk.source.tree.ExpressionStatementTree +import openjdk.source.tree.ExpressionTree +import openjdk.source.tree.ForLoopTree +import openjdk.source.tree.IdentifierTree +import openjdk.source.tree.IntersectionTypeTree +import openjdk.source.tree.LambdaExpressionTree +import openjdk.source.tree.LiteralTree +import openjdk.source.tree.MemberReferenceTree +import openjdk.source.tree.MemberSelectTree +import openjdk.source.tree.MethodInvocationTree +import openjdk.source.tree.MethodTree +import openjdk.source.tree.NewClassTree +import openjdk.source.tree.ParameterizedTypeTree +import openjdk.source.tree.PrimitiveTypeTree +import openjdk.source.tree.StatementTree +import openjdk.source.tree.Tree +import openjdk.source.tree.UnaryTree +import openjdk.source.tree.UnionTypeTree +import openjdk.source.tree.WhileLoopTree +import openjdk.source.tree.WildcardTree +import openjdk.source.util.JavacTask +import openjdk.source.util.SourcePositions +import openjdk.source.util.TreePath +import openjdk.source.util.TreePathScanner +import openjdk.source.util.Trees + +/** + * [paths] is innermost-first, at most [MAX_CANDIDATES] long. Paths rather than bare trees, because every + * downstream question -- my parent, my type, what this name resolves to -- needs the path. + */ +data class CandidateSyntax( + val paths: List, +) { + companion object { + val NONE = CandidateSyntax(emptyList()) + } +} + +/** + * A cursor is the degenerate selection where the offsets are equal, so callers need one code path. + * + * Trimmed first, because a touch-screen selection routinely carries a leading or trailing space. Illegal + * nodes on the way out are **skipped rather than terminating the walk**, so `c ? a : b` is still offered + * from inside a branch, and a cursor on a bare literal still offers the call around it. + */ +fun candidateExpressionsAt( + task: JavacTask, + root: CompilationUnitTree, + fileText: String, + selectionStart: Int, + selectionEnd: Int, +): CandidateSyntax { + val trees = Trees.instance(task) + val positions = trees.sourcePositions + val (start, end) = trimToCode(fileText, selectionStart, selectionEnd) ?: return CandidateSyntax.NONE + + val anchor = + deepestPathAt(root, positions, start, end) + // A caret resting just past a token still resolves, matching a tap inside it. + ?: (if (start == end && start > 0) deepestPathAt(root, positions, start - 1, start - 1) else null) + ?: return CandidateSyntax.NONE + + if (!isExtractionPosition(anchor)) return CandidateSyntax.NONE + + val ceiling = enclosingExecutableBody(anchor)?.leaf ?: return CandidateSyntax.NONE + val collected = mutableListOf() + val seen = mutableSetOf() + var path: TreePath? = anchor + + while (path != null) { + val leaf = path.leaf + if (leaf is ClassTree || leaf is MethodTree) break + if (isLegalExtractionTarget(path, trees)) { + val span = spanOf(root, positions, leaf) + if (span != null && seen.add(span)) { + collected += path + if (collected.size == MAX_CANDIDATES) break + } + } + if (leaf === ceiling) break + path = path.parentPath + } + + if (collected.isEmpty()) return CandidateSyntax.NONE + return CandidateSyntax(collected) +} + +/** + * A whitespace-only selection collapses to a cursor at [start]: a drag over the gap between two tokens + * carries the same intent as a tap in it. Null only when the range is not valid for [text]. + */ +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) start to start else s to e +} + +/** + * javac has no `findElementAt`, so the whole unit is scanned. Pre-order means a child is seen after its + * parent and is never wider, so keeping the narrowest-so-far with `<=` finds the deepest node without + * counting depth. Nodes with no positions are skipped but still descended into. + */ +internal fun deepestPathAt( + root: CompilationUnitTree, + positions: SourcePositions, + start: Int, + end: Int, +): TreePath? { + var best: TreePath? = null + var bestWidth = Int.MAX_VALUE + + val scanner = + object : TreePathScanner() { + override fun scan( + tree: Tree?, + p: Unit?, + ): Unit? { + if (tree == null) return null + val treeStart = positions.getStartPosition(root, tree).toInt() + val treeEnd = positions.getEndPosition(root, tree).toInt() + if (treeStart >= 0 && treeEnd >= treeStart && treeStart <= start && end <= treeEnd) { + val width = treeEnd - treeStart + if (width <= bestWidth) { + // `currentPath` still holds the parent at this point, so extending it names `tree` + // -- the same path getCurrentPath() reports once super.scan has pushed it. + best = TreePath(currentPath, tree) + bestWidth = width + } + } + return super.scan(tree, p) + } + } + scanner.scan(TreePath(root), null) + return best +} + +/** + * Rejects positions where no local declaration can precede the expression: annotation arguments (must be + * constant), `this(...)`/`super(...)` arguments (nothing can precede them), and anything outside an + * executable body -- notably a field initializer, where an initializer block would change when it runs. + */ +internal fun isExtractionPosition(path: TreePath): Boolean { + var current: TreePath? = path + while (current != null) { + val leaf = current.leaf + if (leaf is AnnotationTree) return false + if (leaf is MethodInvocationTree && isConstructorDelegation(leaf)) return false + current = current.parentPath + } + if (isCaseLabel(path)) return false + if (isConditionallyEvaluated(path)) return false + return enclosingExecutableBody(path) != null +} + +/** + * A `case` label must be a compile-time constant, so a hoisted local can never stand in for one -- and + * an unqualified enum constant only resolves inside the label at all. + */ +private fun isCaseLabel(path: TreePath): Boolean { + var child: Tree = path.leaf + var current: TreePath? = path.parentPath + while (current != null) { + val leaf = current.leaf + if (leaf is StatementTree && leaf !is CaseTree) return false + if (leaf is CaseTree) return leaf.body !== child + child = leaf + current = current.parentPath + } + return false +} + +/** + * Whether the expression is evaluated conditionally or repeatedly, where hoisting it changes *when* it + * runs rather than just naming it. + * + * A loop condition hoisted out of its loop is evaluated once, so `while (it.hasNext())` never + * terminates. A `for` update is the same, one clause along. The right operand of `&&`/`||` hoisted out + * stops being guarded, so `s != null && s.length() > 0` throws. A conditional branch is the same shape. + * None of these has an inner rung to offer instead -- `frameFor` finds no frame for a condition, an + * update or an operand -- so the only placement available is the wrong one, and declining is the honest + * answer. + */ +private fun isConditionallyEvaluated(path: TreePath): Boolean { + var child: Tree = path.leaf + var current: TreePath? = path.parentPath + while (current != null) { + val leaf = current.leaf + when { + leaf is WhileLoopTree && leaf.condition === child -> return true + + leaf is DoWhileLoopTree && leaf.condition === child -> return true + + leaf is ForLoopTree && leaf.condition === child -> return true + + leaf is ConditionalExpressionTree && + (leaf.trueExpression === child || leaf.falseExpression === child) -> return true + + leaf is BinaryTree && + leaf.kind in SHORT_CIRCUIT_KINDS && + leaf.rightOperand === child -> return true + + // A `for` update is parsed as an ExpressionStatementTree, so the statement boundary below + // would otherwise read it as a fixed evaluation point and accept it. + leaf is ExpressionStatementTree && isForUpdate(current.parentPath, leaf) -> return true + + // A statement boundary means the expression is evaluated exactly where it is written. + leaf is StatementTree -> return false + } + child = leaf + current = current.parentPath + } + return false +} + +/** Whether [statement] is one of the update clauses of the `for` loop at [parentPath]. */ +private fun isForUpdate( + parentPath: TreePath?, + statement: Tree, +): Boolean { + val loop = parentPath?.leaf as? ForLoopTree ?: return false + return loop.update.any { it === statement } +} + +private val SHORT_CIRCUIT_KINDS = setOf(Tree.Kind.CONDITIONAL_AND, Tree.Kind.CONDITIONAL_OR) + +/** `this(...)` and `super(...)`, whose method select is the bare keyword. */ +private fun isConstructorDelegation(invocation: MethodInvocationTree): Boolean { + val name = (invocation.methodSelect as? IdentifierTree)?.name?.toString() ?: return false + return name == "this" || name == "super" +} + +/** + * The nearest lambda, method/constructor, or initializer body. An initializer block is a `BlockTree` + * under a `ClassTree`, a method body one under a `MethodTree`; both stop the walk at the right ceiling. + */ +internal fun enclosingExecutableBody(path: TreePath): TreePath? { + var current: TreePath? = path.parentPath + var child: Tree = path.leaf + while (current != null) { + val leaf = current.leaf + if (leaf is LambdaExpressionTree && leaf.body === child) return current + if (leaf is MethodTree && leaf.body === child) return current + if (leaf is ClassTree && child is BlockTree) return TreePath(current, child) + child = leaf + current = current.parentPath + } + return null +} + +/** + * Excluded, and why: lambdas and method references, whose type comes from a target a hoisted declaration + * no longer has; method selects and `new` class names, which are fragments; names resolving to a type or + * package; assignment targets; type trees; and bare literals, where extracting is almost never the + * intent -- the expression *around* a literal is still offered. + */ +internal fun isLegalExtractionTarget( + path: TreePath, + trees: Trees, +): Boolean { + val leaf = path.leaf + if (leaf !is ExpressionTree) return false + if (leaf is LambdaExpressionTree || leaf is MemberReferenceTree) return false + if (leaf is LiteralTree) return false + if (leaf is PrimitiveTypeTree || + leaf is ArrayTypeTree || + leaf is ParameterizedTypeTree || + leaf is WildcardTree || + leaf is AnnotatedTypeTree || + leaf is UnionTypeTree || + leaf is IntersectionTypeTree + ) { + return false + } + + if ((leaf is IdentifierTree || leaf is MemberSelectTree) && namesATypeOrPackage(path, trees)) return false + + val parent = path.parentPath?.leaf ?: return false + if (parent is MethodInvocationTree && parent.methodSelect === leaf) return false + if (parent is NewClassTree && parent.identifier === leaf) return false + if (parent is AssignmentTree && parent.variable === leaf) return false + if (parent is CompoundAssignmentTree && parent.variable === leaf) return false + // `foo(i++)` with the cursor on `i`: binding it would increment the copy and leave `i` alone, and it + // compiles, so nothing would tell the user the behaviour changed. + if (parent is UnaryTree && parent.kind in INCREMENT_KINDS && parent.expression === leaf) return false + // The whole expression of an expression statement: the source `;` sits outside the candidate's span, + // so replacing the expression would leave a bare `v;` behind -- "not a statement". + if (parent is ExpressionStatementTree) return false + return true +} + +/** A resolution failure reads as "not a type", keeping a candidate over broken code. */ +private fun namesATypeOrPackage( + path: TreePath, + trees: Trees, +): Boolean { + val kind = runCatching { trees.getElement(path)?.kind }.getOrNull() ?: return false + return when (kind) { + ElementKind.CLASS, + ElementKind.INTERFACE, + ElementKind.ENUM, + ElementKind.ANNOTATION_TYPE, + ElementKind.RECORD, + ElementKind.PACKAGE, + ElementKind.MODULE, + ElementKind.TYPE_PARAMETER, + -> true + + else -> false + } +} + +/** [tree]'s span, or null when javac has no positions for it. */ +internal fun spanOf( + root: CompilationUnitTree, + positions: SourcePositions, + tree: Tree, +): TextSpan? { + val start = positions.getStartPosition(root, tree).toInt() + val end = positions.getEndPosition(root, tree).toInt() + if (start < 0 || end < start) return null + return TextSpan(start, end) +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableEdit.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableEdit.kt new file mode 100644 index 0000000000..0ccda4b6a7 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableEdit.kt @@ -0,0 +1,64 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.detectNewline +import com.itsaky.androidide.lsp.refactor.existingBlockRewrite +import com.itsaky.androidide.lsp.refactor.replaceOccurrences +import com.itsaky.androidide.lsp.refactor.wrapInBracesRewrite + +/** + * Builds the extraction rewrite, or null when the inputs cannot produce one. [name] is already + * validated. Occurrences are substituted right-to-left so an earlier one cannot shift a later offset. + */ +fun buildExtractVariableRewrite( + fileText: String, + candidateSpan: TextSpan, + declaredType: String, + scope: ScopeOption, + name: String, + replaceAll: Boolean, +): RewriteSpan? { + val targets = + (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) + val declaration = "$declaredType $name = $expression;" + + return when (val form = scope.anchorForm) { + is AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, form.block, targets, declaration, name) + is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form.body, targets, declaration, name) + is AnchorForm.ConvertExpressionBody -> convertExpressionBodyRewrite(fileText, form, targets, declaration, name) + } +} + +/** The returned expression gains a `;` because it becomes a statement; the expression body had none. */ +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) + // A switch rule's span reaches past its own `;` (the parser consumes it separately), so strip it + // before composing or the block ends up with `yield v;;`. + val body = replaceOccurrences(fileText, bodySpan, targets, name).trimEnd().removeSuffix(";") + val returned = if (form.needsReturn) "${form.returnKeyword} $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(bodySpan, newText) +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePlanner.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePlanner.kt new file mode 100644 index 0000000000..b26bcf33a7 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePlanner.kt @@ -0,0 +1,287 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.java.compiler.CompileTask +import com.itsaky.androidide.lsp.refactor.BlockPlacement +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.anchorOf +import com.itsaky.androidide.lsp.refactor.blockPlacementFor +import com.itsaky.androidide.lsp.refactor.detectIndentUnit +import com.itsaky.androidide.lsp.refactor.excludeUnsoundOccurrences +import com.itsaky.androidide.lsp.refactor.servableOccurrences +import jdkx.lang.model.element.Element +import jdkx.lang.model.element.ElementKind +import jdkx.lang.model.element.ExecutableElement +import jdkx.lang.model.element.Modifier +import jdkx.lang.model.type.DeclaredType +import jdkx.lang.model.type.TypeKind +import jdkx.lang.model.util.Elements +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.DoWhileLoopTree +import openjdk.source.tree.EnhancedForLoopTree +import openjdk.source.tree.ForLoopTree +import openjdk.source.tree.WhileLoopTree +import openjdk.source.util.JavacTask +import openjdk.source.util.SourcePositions +import openjdk.source.util.TreePath +import openjdk.source.util.Trees +import org.slf4j.LoggerFactory +import java.nio.file.Path +import kotlin.coroutines.cancellation.CancellationException + +private val logger = LoggerFactory.getLogger("JavaExtractVariablePlanner") + +/** + * The whole plan from one attributed compile. + * + * Returns an empty plan both when there is nothing to extract and whenever anything here throws: the + * action framework catches only `IllegalArgumentException` and this runs on a scope with no exception + * handler, so an uncaught throw would crash the app. "Nothing to extract" is always safe. + */ +fun buildExtractionPlan( + task: CompileTask, + file: Path, + selectionStart: Int, + selectionEnd: Int, + documentVersion: Int?, +): ExtractionPlan = + runCatching { + val root = task.root(file) + val fileText = root.sourceFile.getCharContent(true).toString() + val trees = Trees.instance(task.task) + val positions = trees.sourcePositions + + val syntax = candidateExpressionsAt(task.task, root, fileText, selectionStart, selectionEnd) + if (syntax.paths.isEmpty()) return ExtractionPlan.empty(fileText, documentVersion) + + // A property of the file, not of a rung: derived once here rather than re-scanning the whole + // source for every ancestor of every candidate. + val indentUnit = detectIndentUnit(fileText) + + ExtractionPlan( + fileText = fileText, + documentVersion = documentVersion, + candidates = + syntax.paths.mapNotNull { path -> + candidateFor(path, task.task.elements, root, trees, positions, fileText, indentUnit) + }, + ) + }.getOrElse { error -> + // Cancellation is the coroutine's business, not a failure to degrade from: swallowing it would + // leave the action running after its scope was cancelled. + if (error is CancellationException) throw error + logger.warn("Failed to build a Java extract-variable plan for {}", file, error) + ExtractionPlan.empty() + } + +/** + * The pass itself, over an already-attributed unit. + * + * Split from the [CompileTask] overload so it needs nothing but javac, which is what lets the analysis + * be tested against a source string with no project model and no tooling API. + * + * [fileText] must be the text [root]'s positions were computed against. + */ +fun buildExtractionPlan( + task: JavacTask, + root: CompilationUnitTree, + fileText: String, + selectionStart: Int, + selectionEnd: Int, + documentVersion: Int?, +): ExtractionPlan = + runCatching { + val trees = Trees.instance(task) + val positions = trees.sourcePositions + + val syntax = candidateExpressionsAt(task, root, fileText, selectionStart, selectionEnd) + if (syntax.paths.isEmpty()) return ExtractionPlan.empty(fileText, documentVersion) + + val indentUnit = detectIndentUnit(fileText) + + ExtractionPlan( + fileText = fileText, + documentVersion = documentVersion, + candidates = + syntax.paths.mapNotNull { path -> + candidateFor(path, task.elements, root, trees, positions, fileText, indentUnit) + }, + ) + }.getOrElse { error -> + if (error is CancellationException) throw error + logger.warn("Failed to build a Java extract-variable plan", error) + ExtractionPlan.empty(fileText, documentVersion) + } + +/** Null when the type cannot be written as source, or nothing remains of the legal scope chain. */ +private fun candidateFor( + path: TreePath, + elements: Elements, + root: CompilationUnitTree, + trees: Trees, + positions: SourcePositions, + fileText: String, + indentUnit: String, +): CandidateExpression? { + val declaredType = declaredTypeTextFor(path, trees, root) ?: return null + val span = spanOf(root, positions, path.leaf) ?: return null + + // Resolved once and threaded down: the ceiling, the occurrence search and the write search all ask + // the same question, and each answer costs a scan of the candidate plus a getElement per name. + val candidateElements = referencedElements(path, trees) + + val frames = + truncateAtCeiling( + enclosingScopeFrames(path, root, positions, fileText, indentUnit), + referencedDeclarationCeiling(candidateElements, root, positions, trees), + ) + if (frames.isEmpty()) return null + + val scopes = + frames.mapNotNull { + scopeOptionFor(path, candidateElements, span, it, root, trees, positions, fileText) + } + if (scopes.isEmpty()) return null + + val takenNames = namesInScopeAt(path, root, trees, elements) + + return CandidateExpression( + label = collapseForLabel(fileText.substring(span.start, span.end)), + span = span, + declaredType = declaredType, + suggestedName = suggestVariableName(path.leaf, declaredType, takenNames), + takenNames = takenNames, + scopes = scopes, + ) +} + +/** + * Null when the rung cannot be honoured. Both declines run before the occurrence search, so a refused + * rung costs nothing -- and refusing here turns a sheet whose confirm must fail into an up-front + * "nothing to extract". + */ +private fun scopeOptionFor( + candidatePath: TreePath, + candidateElements: List, + span: TextSpan, + frame: ScopeFrame, + root: CompilationUnitTree, + trees: Trees, + positions: SourcePositions, + fileText: String, +): ScopeOption? { + // javac has no parent pointers, so every getPath is a full-unit walk. One here serves both scans + // below and the lambda-target lookup, and bounds them to this rung's subtree. + val scopePath = TreePath.getPath(root, frame.scopeTree) ?: return null + + val anchorForm = + when (val form = frame.anchorForm) { + is AnchorForm.ExistingBlock -> { + if (blockPlacementFor(fileText, form.block, span) is BlockPlacement.Refused) return null + form + } + + is AnchorForm.ConvertExpressionBody -> { + convertExpressionBodyForm(form, scopePath, trees) ?: return null + } + + is AnchorForm.WrapInBraces -> { + form + } + } + + val matches = + findOccurrences(candidatePath, candidateElements, frame, scopePath, root, positions, fileText, trees) + val writes = writeOffsetsFor(candidateElements, frame, scopePath, root, positions, trees) + if (hoistSkipsWrite(candidatePath, span, frame, anchorForm, root, positions, writes)) return null + val sound = excludeUnsoundOccurrences(matches, span, writes) + val occurrences = + servableOccurrences(fileText, (anchorForm as? AnchorForm.ExistingBlock)?.block, sound, span) + + return ScopeOption(label = frame.label, anchorForm = anchorForm, occurrences = occurrences) +} + +/** + * Whether hoisting to this rung would carry the declaration over a write to something the expression + * reads -- which compiles, and silently freezes the value, so declining the rung is the only signal + * available. The inner rungs survive, so the user is never left with nothing. + * + * Two shapes. A write between the anchor statement and the occurrence is simply skipped: extracting + * `limit + 1` from `if (c) { limit = 5; foo(limit + 1); }` at the method rung anchors on the `if` and + * reads the pre-assignment value. A write inside a loop the occurrence sits in but the anchor does not + * is worse: `while (limit < 10) { foo(limit + 1); limit++; }` hoisted out of the loop evaluates once and + * feeds every iteration the same value. + */ +private fun hoistSkipsWrite( + candidatePath: TreePath, + span: TextSpan, + frame: ScopeFrame, + anchorForm: AnchorForm, + root: CompilationUnitTree, + positions: SourcePositions, + writes: List, +): Boolean { + if (writes.isEmpty()) return false + + if (anchorForm is AnchorForm.ExistingBlock) { + val anchor = anchorOf(anchorForm.block, span) + if (anchor != null && writes.any { it in anchor.start until span.start }) return true + } + + var current: TreePath? = candidatePath.parentPath + while (current != null) { + val loop = current.leaf + if (loop is WhileLoopTree || loop is DoWhileLoopTree || loop is ForLoopTree || loop is EnhancedForLoopTree) { + // No span means the plan and the tree disagree about this loop, so its containment cannot be + // checked either way. + val loopSpan = spanOf(root, positions, loop) ?: return true + // The rung is itself inside the loop, so the declaration re-runs with every iteration. + if (loopSpan.start <= frame.scopeSpan.start && frame.scopeSpan.end <= loopSpan.end) return false + if (writes.any { it in loopSpan.start until loopSpan.end }) return true + } + current = current.parentPath + } + return false +} + +/** + * A lambda's `needsReturn` comes from the **functional interface method's** return type, never the + * body's: `Runnable r = () -> list.add(x);` is legal even though `add` returns `boolean`, and emitting + * `return list.add(x);` there would not compile. Unresolvable target or method declines the rung. + */ +private fun convertExpressionBodyForm( + form: AnchorForm.ConvertExpressionBody, + scopePath: TreePath, + trees: Trees, +): AnchorForm.ConvertExpressionBody? { + if (form.returnKeyword == "yield") return form + + // scopePath's leaf is the body expression, so the lambda is its parent. + val lambdaPath = scopePath.parentPath ?: return null + val target = runCatching { trees.getTypeMirror(lambdaPath) }.getOrNull() ?: return null + if (target !is DeclaredType) return null + val abstractMethod = singleAbstractMethodOf(target) ?: return null + return form.copy(needsReturn = abstractMethod.returnType.kind != TypeKind.VOID) +} + +/** + * `Object`'s methods are re-declarable on a functional interface without counting against its single + * abstract method, so they are discounted by name and arity. + */ +private fun singleAbstractMethodOf(target: DeclaredType): ExecutableElement? { + val element = runCatching { target.asElement() }.getOrNull() ?: return null + return runCatching { element.enclosedElements } + .getOrNull() + ?.filterIsInstance() + ?.filter { it.kind == ElementKind.METHOD } + ?.filter { Modifier.ABSTRACT in it.modifiers } + ?.filterNot(::isObjectMethod) + ?.singleOrNull() +} + +private fun isObjectMethod(method: ExecutableElement): Boolean { + val name = method.simpleName.toString() + val arity = method.parameters.size + return (name == "equals" && arity == 1) || + (name == "hashCode" && arity == 0) || + (name == "toString" && arity == 0) +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionPlan.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionPlan.kt new file mode 100644 index 0000000000..86dcc19607 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionPlan.kt @@ -0,0 +1,124 @@ +package com.itsaky.androidide.lsp.java.refactor + +import androidx.annotation.StringRes +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.TextSpan + +/** Not every Java scope is a block: a lambda and a `->` switch rule can have an expression body. */ +sealed interface AnchorForm { + /** A scope that already has braces, described by [BlockAnchor]. */ + data class ExistingBlock( + val block: BlockAnchor, + ) : AnchorForm + + /** A braceless position: `if (c) foo();`, a braceless loop body, a single-statement switch rule. */ + data class WrapInBraces( + val body: BracelessBody, + ) : AnchorForm + + /** + * An expression-bodied lambda (`x -> x * 2`) or switch rule (`case A -> x * 2;`). + * + * [needsReturn] is false when the target's method returns `void`, where returning a value would not + * compile. [returnKeyword] is `yield` for a switch rule, which produces a value rather than + * returning from the enclosing method. Nothing is written into a signature: a Java lambda takes its + * type from its target, not its body. + */ + data class ConvertExpressionBody( + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, + val needsReturn: Boolean, + val returnKeyword: String = "return", + ) : AnchorForm +} + +/** + * A rung's name for the chooser, as a resource id rather than text. + * + * These render in the sheet, so the copy and its word order belong in `strings.xml` where a translator + * can reach them -- `"method $name"` fixes an English word order in code. [argument] is the one variable + * part, filled positionally. + */ +data class ScopeLabel( + @StringRes val res: Int, + val argument: String? = null, +) + +/** + * A place the declaration may go, with the occurrences that are sound to replace there. + * + * [occurrences] always contains the candidate's own span, so its size is the count shown as "Replace + * all N occurrences". A block rung drops leading occurrences whose anchor statement cannot host the + * declaration: a replace-all anchors on the first served one, so keeping an unhostable site would + * refuse the whole rewrite. Lowering N is the point -- it stays achievable. + */ +data class ScopeOption( + val label: ScopeLabel, + val anchorForm: AnchorForm, + val occurrences: List, +) + +/** + * A legal extraction target and everything needed to act on it. + * + * [declaredType] is always spelled out: Java requires a type on a local, and `var` is Java 10+ while an + * opened project may be on `sourceCompatibility 1.8`. [scopes] is innermost first and never empty -- a + * candidate with no legal anchor is not a candidate. + */ +data class CandidateExpression( + val label: String, + val span: TextSpan, + val declaredType: String, + val suggestedName: String, + val takenNames: Set, + val scopes: List, +) + +/** + * The result of one background analysis pass, covering every candidate, so the confirm path does pure + * offset arithmetic and never re-enters javac. + * + * [fileText] is the *compiled* unit's own content, never the editor's buffer read a moment later, since + * every span here was computed against it. [documentVersion] is re-read on confirm: a plan computed + * against text the user has since edited is discarded rather than applied against shifted offsets. It is + * null when the document was not open at plan time -- nullable rather than a sentinel, because a sentinel + * compares equal to itself and so passes the very guard it exists to fail. + */ +data class ExtractionPlan( + val fileText: String, + val documentVersion: Int?, + val candidates: List, +) { + val isEmpty: Boolean get() = candidates.isEmpty() + + companion object { + fun empty( + fileText: String = "", + documentVersion: Int? = null, + ) = ExtractionPlan(fileText, documentVersion, emptyList()) + } +} + +/** + * Collapses whitespace so a multi-line expression reads as one line in a list item. + * + * The space before a `.` goes too: a plain collapse turns `items\n\t.stream()` into `items .stream()`, + * 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/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractVariableUi.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractVariableUi.kt new file mode 100644 index 0000000000..4537424715 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractVariableUi.kt @@ -0,0 +1,57 @@ +package com.itsaky.androidide.lsp.java.refactor + +import android.content.Context +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.ui.CandidateView +import com.itsaky.androidide.lsp.ui.ExtractVariableSelection +import com.itsaky.androidide.lsp.ui.NameMessages +import com.itsaky.androidide.lsp.ui.ScopeView +import com.itsaky.androidide.resources.R + +/** + * Java's wording for the four name problems the shared sheet can report. + * + * Two of them name the language, which is why the sheet takes them rather than looking them up. + */ +val JAVA_NAME_MESSAGES = + NameMessages( + blank = R.string.msg_extract_variable_name_blank, + invalid = R.string.msg_extract_variable_name_invalid_java, + keyword = R.string.msg_extract_variable_name_keyword_java, + taken = R.string.msg_extract_variable_name_taken, + ) + +/** + * The plan as the shared sheet sees it: labels, names and counts, no trees and no offsets. + * + * Offsets stay on this side deliberately -- the sheet is a chooser, and resolving a selection back into + * spans is [candidateAndScopeFor]'s job. Scope labels arrive as resource ids and are resolved here, the + * first layer that has a [Context]. + */ +fun ExtractionPlan.toCandidateViews(context: Context): List = + candidates.map { candidate -> + CandidateView( + label = candidate.label, + suggestedName = candidate.suggestedName, + takenNames = candidate.takenNames, + scopes = + candidate.scopes.map { scope -> + ScopeView(label = scope.label.resolve(context), occurrenceCount = scope.occurrences.size) + }, + ) + } + +private fun ScopeLabel.resolve(context: Context): String = + if (argument == null) context.getString(res) else context.getString(res, argument) + +/** + * Resolves a selection's indices back to the plan they came from, or null when they do not address it. + * + * A null is a wiring bug rather than a user path -- the sheet only ever reports indices it was given -- + * so the caller reports it as a failed quick fix rather than guessing at a candidate. + */ +fun ExtractionPlan.candidateAndScopeFor(selection: ExtractVariableSelection): Pair? { + val candidate = candidates.getOrNull(selection.candidateIndex) ?: return null + val scope = candidate.scopes.getOrNull(selection.scopeIndex) ?: return null + return candidate to scope +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/NameSuggestion.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/NameSuggestion.kt new file mode 100644 index 0000000000..c70fb7f2e3 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/NameSuggestion.kt @@ -0,0 +1,120 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.FALLBACK_NAME +import com.itsaky.androidide.lsp.refactor.decapitaliseFirst +import com.itsaky.androidide.lsp.refactor.nameFromType +import com.itsaky.androidide.lsp.refactor.stripAccessorPrefix +import com.itsaky.androidide.lsp.refactor.uniqueName +import com.itsaky.androidide.lsp.ui.isIdentifier +import openjdk.source.tree.ArrayAccessTree +import openjdk.source.tree.IdentifierTree +import openjdk.source.tree.MemberSelectTree +import openjdk.source.tree.MethodInvocationTree +import openjdk.source.tree.NewClassTree +import openjdk.source.tree.ParameterizedTypeTree +import openjdk.source.tree.ParenthesizedTree +import openjdk.source.tree.Tree +import openjdk.source.tree.TypeCastTree + +/** + * The restricted identifiers -- `var`, `yield`, `record`, `sealed`, `permits` -- are legal variable names + * and are deliberately absent. `true`, `false` and `null` are literals, so they are present. + */ +val JAVA_KEYWORDS = + setOf( + "abstract", + "assert", + "boolean", + "break", + "byte", + "case", + "catch", + "char", + "class", + "const", + "continue", + "default", + "do", + "double", + "else", + "enum", + "extends", + "final", + "finally", + "float", + "for", + "goto", + "if", + "implements", + "import", + "instanceof", + "int", + "interface", + "long", + "native", + "new", + "package", + "private", + "protected", + "public", + "return", + "short", + "static", + "strictfp", + "super", + "switch", + "synchronized", + "this", + "throw", + "throws", + "transient", + "try", + "void", + "volatile", + "while", + "true", + "false", + "null", + ) + +/** + * Shape (`items.size()` -> `size`), then rendered type (`List` -> `list`), then [FALLBACK_NAME], + * uniquified against [takenNames]. Shape beats type because `size` and `count` are far better names than + * `int` and `string`. A primitive type yields its own keyword, which the sanitise check turns into + * [FALLBACK_NAME] rather than emitting `int int = ...`. + */ +fun suggestVariableName( + expression: Tree, + typeName: String?, + takenNames: Set, +): String { + val base = + nameFromShape(expression) + ?: typeName?.let(::nameFromType) + ?: FALLBACK_NAME + val sanitised = base.takeIf { isIdentifier(it) && it !in JAVA_KEYWORDS } ?: FALLBACK_NAME + return uniqueName(sanitised, takenNames) +} + +internal fun nameFromShape(tree: Tree): String? = + when (tree) { + is ParenthesizedTree -> nameFromShape(tree.expression) + + is TypeCastTree -> nameFromShape(tree.expression) + + is MethodInvocationTree -> nameFromShape(tree.methodSelect) + + // A constructor call is named after a *type*, so it needs decapitalising where an identifier + // reached as a value (`items` -> `items`) must not be touched. + is NewClassTree -> nameFromShape(tree.identifier)?.decapitaliseFirst() + + is ParameterizedTypeTree -> nameFromShape(tree.type) + + is MemberSelectTree -> stripAccessorPrefix(tree.identifier.toString()) + + is IdentifierTree -> stripAccessorPrefix(tree.name.toString()) + + is ArrayAccessTree -> nameFromShape(tree.expression) + + else -> null + }?.takeIf { it.isNotBlank() } diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/Occurrences.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/Occurrences.kt new file mode 100644 index 0000000000..d282b0d805 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/Occurrences.kt @@ -0,0 +1,324 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.TextSpan +import jdkx.lang.model.element.Element +import jdkx.lang.model.element.ElementKind +import jdkx.lang.model.element.Modifier +import jdkx.lang.model.element.TypeElement +import jdkx.lang.model.element.VariableElement +import jdkx.lang.model.util.Elements +import openjdk.source.tree.AssignmentTree +import openjdk.source.tree.BlockTree +import openjdk.source.tree.CatchTree +import openjdk.source.tree.ClassTree +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.CompoundAssignmentTree +import openjdk.source.tree.ExpressionTree +import openjdk.source.tree.IdentifierTree +import openjdk.source.tree.LambdaExpressionTree +import openjdk.source.tree.MemberSelectTree +import openjdk.source.tree.MethodTree +import openjdk.source.tree.Scope +import openjdk.source.tree.Tree +import openjdk.source.tree.UnaryTree +import openjdk.source.tree.VariableTree +import openjdk.source.util.SourcePositions +import openjdk.source.util.TreePath +import openjdk.source.util.TreePathScanner +import openjdk.source.util.TreeScanner +import openjdk.source.util.Trees +import java.util.Collections +import java.util.IdentityHashMap + +/** + * "The same expression" is normalized source text plus an identical ordered sequence of resolved + * elements. The element check is the point: text alone would match `config.timeout` inside a nested + * lambda where `config` is a different `config`, and ADFA-3324 states it outright -- text-based matching + * breaks things. + * + * Matches must themselves be legal targets: in `a.a`, a candidate of `a` matches the selector too, and + * rewriting it would produce `v.v`. Overlaps are dropped so no site is rewritten twice. + * + * [scopePath] is the rung's own subtree, so the walk covers what [ScopeFrame.searchRange] names rather + * than the whole compilation unit -- this runs once per rung per candidate, on the coroutine the user is + * waiting on. [candidateElements] is resolved once by the caller for the same reason. + */ +internal fun findOccurrences( + candidatePath: TreePath, + candidateElements: List, + frame: ScopeFrame, + scopePath: TreePath, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + trees: Trees, +): List { + val candidateSpan = spanOf(root, positions, candidatePath.leaf) ?: return emptyList() + val candidateKind = candidatePath.leaf.kind + val candidateText = normalizeSource(fileText.substring(candidateSpan.start, candidateSpan.end)) + + val matches = mutableListOf() + + fun consider(path: TreePath) { + val tree = path.leaf + if (tree !is ExpressionTree || tree.kind != candidateKind) return + val span = spanOf(root, positions, tree) ?: return + if (span.start < frame.searchRange.start || span.end > frame.searchRange.end) return + if (span == candidateSpan) { + matches += span + return + } + if (isLegalExtractionTarget(path, trees) && + // Shape alone is not enough: a `case` label matches every structural test and then fails to + // compile once the local is substituted in. + isExtractionPosition(path) && + normalizeSource(fileText.substring(span.start, span.end)) == candidateText && + referencedElements(path, trees) == candidateElements + ) { + matches += span + } + } + + val scanner = + object : TreePathScanner() { + override fun scan( + tree: Tree?, + p: Unit?, + ): Unit? { + if (tree == null) return null + consider(TreePath(currentPath, tree)) + return super.scan(tree, p) + } + } + // `TreePathScanner.scan(TreePath, P)` dispatches straight to the leaf's visitor, so the subtree's own + // root never reaches the override above -- and for an expression-bodied rung that root can *be* the + // candidate. + consider(scopePath) + scanner.scan(scopePath, null) + + val accepted = mutableListOf() + for (match in matches.sortedBy { it.start }) { + if (accepted.none { it.overlaps(match) }) accepted += match + } + return accepted +} + +/** + * Same-unit elements are the same `Symbol` instance for the same declaration, so list equality answers + * "the same declarations?" exactly. An unresolvable reference contributes null, making two identical + * expressions compare unequal -- the safe answer, since it cannot be shown to hold the same value. + */ +internal fun referencedElements( + path: TreePath, + trees: Trees, +): List { + val elements = mutableListOf() + val scanner = + object : TreePathScanner() { + override fun visitIdentifier( + node: IdentifierTree, + p: Unit?, + ): Unit? { + elements += runCatching { trees.getElement(currentPath) }.getOrNull() + return super.visitIdentifier(node, p) + } + + override fun visitMemberSelect( + node: MemberSelectTree, + p: Unit?, + ): Unit? { + elements += runCatching { trees.getElement(currentPath) }.getOrNull() + return super.visitMemberSelect(node, p) + } + } + scanner.scan(path, null) + return elements +} + +/** + * Writes to any non-`final` variable the candidate reads, feeding [excludeUnsoundOccurrences]. A `final` + * variable cannot be written, which is the role Kotlin's `val` check plays. Effectively-final locals need + * no special case: a local that is never written has no write to find. + * + * Bounded to [scopePath]'s subtree for the same reason as [findOccurrences], and given the candidate's + * already-resolved elements rather than resolving them again. + */ +internal fun writeOffsetsFor( + candidateElements: List, + frame: ScopeFrame, + scopePath: TreePath, + root: CompilationUnitTree, + positions: SourcePositions, + trees: Trees, +): List { + val mutables = + candidateElements + .filterIsInstance() + .filterNot { Modifier.FINAL in it.modifiers } + .toSet() + if (mutables.isEmpty()) return emptyList() + + val offsets = mutableListOf() + + fun consider(path: TreePath) { + val tree = path.leaf + val target = + when (tree) { + is AssignmentTree -> tree.variable + is CompoundAssignmentTree -> tree.variable + is UnaryTree -> if (tree.kind in INCREMENT_KINDS) tree.expression else null + else -> null + } ?: return + val span = spanOf(root, positions, target) ?: return + if (span.start < frame.searchRange.start || span.end > frame.searchRange.end) return + val element = runCatching { trees.getElement(TreePath(path, target)) }.getOrNull() + if (element in mutables) offsets += span.start + } + + val scanner = + object : TreePathScanner() { + override fun scan( + tree: Tree?, + p: Unit?, + ): Unit? { + if (tree == null) return null + consider(TreePath(currentPath, tree)) + return super.scan(tree, p) + } + } + consider(scopePath) + scanner.scan(scopePath, null) + return offsets.sorted() +} + +internal val INCREMENT_KINDS = + setOf( + Tree.Kind.PREFIX_INCREMENT, + Tree.Kind.PREFIX_DECREMENT, + Tree.Kind.POSTFIX_INCREMENT, + Tree.Kind.POSTFIX_DECREMENT, + ) + +/** + * What stops a hoist escaping a lambda it depends on: a candidate using a lambda parameter gets that + * lambda's body back as its ceiling, and [truncateAtCeiling] drops every outer rung. Only locals, + * parameters, exception parameters and resource variables constrain anything -- a field or a library + * declaration does not. + */ +internal fun referencedDeclarationCeiling( + candidateElements: List, + root: CompilationUnitTree, + positions: SourcePositions, + trees: Trees, +): TextSpan? { + var narrowest: TextSpan? = null + for (element in candidateElements) { + if (element == null || element.kind !in LOCAL_KINDS) continue + val declaration = runCatching { trees.getPath(element) }.getOrNull() ?: continue + if (declaration.compilationUnit !== root) continue + val scope = constrainingScopeFor(declaration) ?: continue + val span = spanOf(root, positions, scope) ?: continue + if (narrowest == null || span.length < narrowest.length) narrowest = span + } + return narrowest +} + +/** + * Deliberately *not* [enclosingExecutableBody]: a parameter is not inside the body it scopes, so asking + * which body encloses the *declaration* walks past the lambda and answers the enclosing method -- which + * would let a lambda-parameter-using expression hoist clean out of the lambda that binds it. + */ +private fun constrainingScopeFor(declaration: TreePath): Tree? = + when (val owner = declaration.parentPath?.leaf) { + is LambdaExpressionTree -> owner.body + + is MethodTree -> owner.body + + is CatchTree -> owner.block + + is BlockTree -> owner + + // A `for`, enhanced-`for`, try-with-resources or `instanceof` pattern variable is scoped to the + // construct that declares it. Returning the construct itself is both correct and the safe default + // for anything unrecognised: a narrower ceiling only removes rungs, it never adds a bad one. + else -> owner + } + +private val LOCAL_KINDS = + setOf( + ElementKind.LOCAL_VARIABLE, + ElementKind.PARAMETER, + ElementKind.EXCEPTION_PARAMETER, + ElementKind.RESOURCE_VARIABLE, + ElementKind.BINDING_VARIABLE, + ) + +/** + * `Trees.getScope` answers this directly, and `Elements.getAllMembers` **includes inherited members**, so + * a generated local cannot silently shadow one -- which Kotlin's syntactic walk cannot manage. A local in + * a sibling method is absent: it is in no enclosing scope, and treating it as taken refuses a legal name. + */ +fun namesInScopeAt( + candidatePath: TreePath, + root: CompilationUnitTree, + trees: Trees, + elements: Elements, +): Set { + val names = mutableSetOf() + + root.typeDecls + .filterIsInstance() + .forEach { names += it.simpleName.toString() } + + /* + * javac's outermost scopes do not reliably terminate the getEnclosingScope() chain -- a star-import + * scope can report itself -- so the walk is guarded by identity rather than trusting a null. Without + * this the loop never ends and the action hangs the compiler's semaphore. + */ + val seenScopes = Collections.newSetFromMap(IdentityHashMap()) + val seenClasses = mutableSetOf() + names += localNamesInEnclosingBodies(candidatePath) + + var scope = runCatching { trees.getScope(candidatePath) }.getOrNull() + while (scope != null && seenScopes.add(scope)) { + val current = scope + runCatching { current.localElements } + .getOrNull() + ?.forEach { element -> names += element.simpleName.toString() } + + val enclosing = runCatching { current.enclosingClass }.getOrNull() + if (enclosing != null && seenClasses.add(enclosing)) { + runCatching { elements.getAllMembers(enclosing) } + .getOrNull() + ?.forEach { member -> names += member.simpleName.toString() } + } + scope = runCatching { current.enclosingScope }.getOrNull() + } + return names +} + +/** + * Every local declared anywhere in the executable bodies enclosing [candidatePath]. + * + * `Trees.getScope` reports only what is in scope *at* the candidate, because javac has attributed the + * method only that far. Java forbids two locals of the same name in a block whatever their order, so a + * name taken by a declaration further down the same body is still taken -- and it gates the sheet's text + * field as well as the suggestion. + */ +private fun localNamesInEnclosingBodies(candidatePath: TreePath): Set { + val names = mutableSetOf() + var body = enclosingExecutableBody(candidatePath) + while (body != null) { + object : TreeScanner() { + override fun visitVariable( + node: VariableTree, + p: Unit?, + ): Unit? { + names += node.name.toString() + return super.visitVariable(node, p) + } + }.scan(body.leaf, null) + body = body.parentPath?.let { enclosingExecutableBody(it) } + } + return names +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ScopeChain.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ScopeChain.kt new file mode 100644 index 0000000000..4786988a59 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ScopeChain.kt @@ -0,0 +1,333 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.leadingIndentAt + +import androidx.annotation.StringRes +import com.itsaky.androidide.resources.R +import openjdk.source.tree.BlockTree +import openjdk.source.tree.CaseTree +import openjdk.source.tree.CatchTree +import openjdk.source.tree.ClassTree +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.DoWhileLoopTree +import openjdk.source.tree.EnhancedForLoopTree +import openjdk.source.tree.ExpressionTree +import openjdk.source.tree.ForLoopTree +import openjdk.source.tree.IfTree +import openjdk.source.tree.LambdaExpressionTree +import openjdk.source.tree.MethodTree +import openjdk.source.tree.StatementTree +import openjdk.source.tree.SynchronizedTree +import openjdk.source.tree.Tree +import openjdk.source.tree.TryTree +import openjdk.source.tree.WhileLoopTree +import openjdk.source.util.SourcePositions +import openjdk.source.util.TreePath + +/** + * javac trees carry no parent pointers, so containment is answered positionally from [scopeSpan] -- see + * [truncateAtCeiling]. [searchRange] bounds the occurrence search for this rung. + */ +data class ScopeFrame( + val label: ScopeLabel, + val scopeTree: Tree, + val scopeSpan: TextSpan, + val searchRange: TextSpan, + val anchorForm: AnchorForm, +) + +/** + * The scopes the candidate could be hoisted into, innermost first. Stops after the enclosing method, + * constructor or initializer body; a class body is never an anchor. + * + * Lambda boundaries are *crossed* here: whether crossing is legal depends on what the candidate + * references, which needs resolution, so [truncateAtCeiling] applies it afterwards. + * + * An old-style `case X:` group is deliberately not a rung -- its statements have no owning braces, so + * the block geometry the rewrite reasons about does not exist. `case X: { ... }` works normally. + * + * [indentUnit] is passed in rather than derived here: it is a property of the whole file, and deriving it + * per rung re-scanned the entire source once for every ancestor of every candidate. + */ +internal fun enclosingScopeFrames( + candidatePath: TreePath, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + indentUnit: String, +): List { + val frames = mutableListOf() + var path: TreePath = candidatePath + + while (true) { + val parentPath = path.parentPath ?: break + val frame = frameFor(path.leaf, parentPath, root, positions, fileText, indentUnit) + if (frame == null) { + // Most nodes are not themselves anchorable -- an argument, an argument list, a member + // select. Keep climbing rather than stopping, otherwise the chain would end at the first + // such node and a candidate inside a lambda could never be hoisted out of it. + path = parentPath + continue + } + + frames += frame + if (isCeilingBlock(frame.scopeTree, parentPath)) break + path = parentPath + } + return frames +} + +/** + * Enforces "crossing a lambda boundary only when nothing lambda-scoped is referenced": if the candidate + * uses a lambda parameter, that lambda's body is the ceiling and every outer rung disappears. Truncating + * to nothing keeps the innermost rung, so this step never empties a chain on its own. + */ +internal fun truncateAtCeiling( + frames: List, + ceiling: TextSpan?, +): List { + if (ceiling == null) return frames + val kept = frames.takeWhile { ceiling.start <= it.scopeSpan.start && it.scopeSpan.end <= ceiling.end } + return kept.ifEmpty { frames.take(1) } +} + +/** Null when [parentPath]'s leaf is not a position this refactoring anchors in. */ +private fun frameFor( + inner: Tree, + parentPath: TreePath, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + indentUnit: String, +): ScopeFrame? { + val parent = parentPath.leaf + + if (parent is BlockTree) { + val blockSpan = spanOf(root, positions, parent) ?: return null + return ScopeFrame( + label = blockLabel(parent, parentPath), + scopeTree = parent, + scopeSpan = blockSpan, + searchRange = blockSpan, + anchorForm = + AnchorForm.ExistingBlock( + BlockAnchor( + contentSpan = contentSpanOf(blockSpan, fileText) ?: return null, + statementSpans = parent.statements.mapNotNull { spanOf(root, positions, it) }, + ), + ), + ) + } + + val innerSpan = spanOf(root, positions, inner) ?: return null + + if (inner is ExpressionTree && parent is LambdaExpressionTree && parent.body === inner) { + return expressionBodyFrame(LAMBDA, inner, innerSpan, parent, root, positions, fileText, indentUnit, "return") + } + + if (parent is CaseTree && parent.caseKind == CaseTree.CaseKind.RULE && parent.body === inner) { + return if (inner is ExpressionTree) { + // `case A -> value;` parses the body as the expression and takes the `;` separately, so the + // span stops short of it. Replacing only the expression would leave `case A -> { ... };`. + val withTerminator = TextSpan(innerSpan.start, semicolonAfter(fileText, innerSpan.end)) + expressionBodyFrame(SWITCH_RULE, inner, withTerminator, parent, root, positions, fileText, indentUnit, "yield") + } else { + bracelessFrame(SWITCH_RULE, innerSpan, parent, root, positions, fileText, indentUnit) + } + } + + if (inner is StatementTree && inner !is BlockTree) { + val label = bracelessOwnerLabel(inner, parent) ?: return null + return bracelessFrame(label, innerSpan, parent, root, positions, fileText, indentUnit) + } + + return null +} + +/** The statement is replaced by a braced block holding both lines. */ +private fun bracelessFrame( + label: ScopeLabel, + innerSpan: TextSpan, + owner: Tree, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + indentUnit: String, +): ScopeFrame? { + val ownerSpan = spanOf(root, positions, owner) ?: return null + val indent = leadingIndentAt(fileText, ownerSpan.start) + return ScopeFrame( + label = label, + scopeTree = owner, + scopeSpan = innerSpan, + searchRange = innerSpan, + anchorForm = + AnchorForm.WrapInBraces( + BracelessBody( + bodyStart = innerSpan.start, + bodyEnd = innerSpan.end, + indent = indent, + innerIndent = indent + indentUnit, + ), + ), + ) +} + +/** + * `needsReturn` is left true here and settled by the planner, the only layer that can resolve the target + * type's abstract method. + */ +private fun expressionBodyFrame( + label: ScopeLabel, + inner: Tree, + innerSpan: TextSpan, + owner: Tree, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + indentUnit: String, + returnKeyword: String, +): ScopeFrame? { + val ownerSpan = spanOf(root, positions, owner) ?: return null + val indent = leadingIndentAt(fileText, ownerSpan.start) + return ScopeFrame( + label = label, + scopeTree = inner, + scopeSpan = innerSpan, + searchRange = innerSpan, + anchorForm = + AnchorForm.ConvertExpressionBody( + bodyStart = innerSpan.start, + bodyEnd = innerSpan.end, + indent = indent, + innerIndent = indent + indentUnit, + needsReturn = true, + returnKeyword = returnKeyword, + ), + ) +} + +/** Where the chain stops. [blockPath] is the block's own path, so its parent is the owner. */ +private fun isCeilingBlock( + scopeTree: Tree, + blockPath: TreePath, +): Boolean { + if (scopeTree !is BlockTree) return false + val owner = blockPath.parentPath?.leaf ?: return true + return owner is MethodTree || owner is ClassTree +} + +/** The name shown for a block rung, taken from what owns the block. */ +private fun blockLabel( + block: BlockTree, + blockPath: TreePath, +): ScopeLabel = + when (val owner = blockPath.parentPath?.leaf) { + is MethodTree -> + if (owner.name.contentEquals("")) { + ScopeLabel(R.string.label_extract_scope_constructor) + } else { + ScopeLabel(R.string.label_extract_scope_method, owner.name.toString()) + } + + is ClassTree -> + ScopeLabel( + if (block.isStatic) { + R.string.label_extract_scope_static_initializer + } else { + R.string.label_extract_scope_initializer + }, + ) + + is LambdaExpressionTree -> LAMBDA + + is IfTree -> + ScopeLabel( + if (owner.thenStatement === block) { + R.string.label_extract_scope_if_block + } else { + R.string.label_extract_scope_else_block + }, + ) + + is ForLoopTree, is EnhancedForLoopTree -> ScopeLabel(R.string.label_extract_scope_for_loop) + + is WhileLoopTree -> ScopeLabel(R.string.label_extract_scope_while_loop) + + is DoWhileLoopTree -> ScopeLabel(R.string.label_extract_scope_do_while_loop) + + is TryTree -> + ScopeLabel( + if (owner.finallyBlock === block) { + R.string.label_extract_scope_finally_block + } else { + R.string.label_extract_scope_try_block + }, + ) + + is CatchTree -> ScopeLabel(R.string.label_extract_scope_catch_block) + + is SynchronizedTree -> ScopeLabel(R.string.label_extract_scope_synchronized_block) + + is CaseTree -> SWITCH_RULE + + else -> ScopeLabel(R.string.label_extract_scope_block) + } + +/** A label when [inner] is a braceless body of [parent], else null. */ +private fun bracelessOwnerLabel( + inner: Tree, + parent: Tree, +): ScopeLabel? = + when (parent) { + is IfTree -> + when { + parent.thenStatement === inner -> ScopeLabel(R.string.label_extract_scope_if_branch) + parent.elseStatement === inner -> ScopeLabel(R.string.label_extract_scope_else_branch) + else -> null + } + + is ForLoopTree -> bodyLabel(parent.statement === inner, R.string.label_extract_scope_for_body) + is EnhancedForLoopTree -> bodyLabel(parent.statement === inner, R.string.label_extract_scope_for_body) + is WhileLoopTree -> bodyLabel(parent.statement === inner, R.string.label_extract_scope_while_body) + is DoWhileLoopTree -> bodyLabel(parent.statement === inner, R.string.label_extract_scope_do_while_body) + else -> null + } + +private fun bodyLabel( + isBody: Boolean, + @StringRes res: Int, +): ScopeLabel? = if (isBody) ScopeLabel(res) else null + +private val LAMBDA = ScopeLabel(R.string.label_extract_scope_lambda) +private val SWITCH_RULE = ScopeLabel(R.string.label_extract_scope_switch_rule) + +/** + * The region inside a block's braces. + * + * Derived from the first `{` rather than from `blockSpan.start + 1`, because javac's `JCBlock.pos` is + * not always the brace: `JavacParser` takes the position before `modifiersOpt()`, so a `static { ... }` + * initializer reports the `s` of `static`. Null when no brace is found, which means the span and the + * text disagree and the rung must be declined. + */ +private fun contentSpanOf( + blockSpan: TextSpan, + fileText: String, +): TextSpan? { + val open = fileText.indexOf('{', blockSpan.start) + if (open < 0 || open >= blockSpan.end - 1) return null + return TextSpan(open + 1, blockSpan.end - 1) +} + +/** The offset just past the `;` following [from], or [from] when there is none. */ +private fun semicolonAfter( + fileText: String, + from: Int, +): Int { + var i = from + while (i < fileText.length && fileText[i].isWhitespace()) i++ + return if (i < fileText.length && fileText[i] == ';') i + 1 else from +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizer.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizer.kt new file mode 100644 index 0000000000..fedd53644f --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizer.kt @@ -0,0 +1,142 @@ +package com.itsaky.androidide.lsp.java.refactor + +/** + * Strips comments and collapses whitespace outside string and character literals. + * + * Deliberately not a kind-by-kind structural comparator: javac's `Tree` exposes no generic child list, so + * a structural walk means one visitor case per tree kind, and a kind left unhandled by accident silently + * answers "not equal". Java has no string interpolation, so a literal is opaque and needs no recursion. + */ +internal fun normalizeSource(text: String): String { + val out = StringBuilder(text.length) + var i = 0 + var pendingSpace = false + + while (i < text.length) { + val c = text[i] + + if (c == '/' && i + 1 < text.length && text[i + 1] == '/') { + while (i < text.length && text[i] != '\n') i++ + // The comment's own trailing newline goes with it; leaving it for the whitespace branch would + // put back the space the operator before the comment already swallowed. + while (i < text.length && text[i].isWhitespace()) i++ + pendingSpace = out.spaceSurvivesComment() + continue + } + + if (c == '/' && i + 1 < text.length && text[i + 1] == '*') { + i += 2 + while (i + 1 < text.length && !(text[i] == '*' && text[i + 1] == '/')) i++ + i = (i + 2).coerceAtMost(text.length) + while (i < text.length && text[i].isWhitespace()) i++ + pendingSpace = out.spaceSurvivesComment() + continue + } + + if (c == '"' || c == '\'') { + if (pendingSpace) { + out.append(' ') + pendingSpace = false + } + i = appendLiteral(text, i, c, out) + continue + } + + if (c.isWhitespace()) { + pendingSpace = out.isNotEmpty() + i++ + continue + } + + /* + * Whitespace around an operator or separator carries no meaning in Java, so `a+1` and `a + 1` must + * normalize alike -- otherwise the occurrence search silently skips the differently-spelled site + * and the user is never told a match was missed. + * + * The exception is two characters that could lex into one token: dropping the space in `a - -b` + * would produce `a--b`, which is a different expression. Only combinable characters need that + * guard, so `items.size() + 1` still closes up to `items.size()+1`. + */ + val previous = out.lastOrNull() + // Two combinable characters must stay apart: closing up `a - -b` would produce `a--b`, a different + // expression. Anything else loses nothing, so the space goes. + val clashesBehind = previous != null && previous in COMBINABLE && c in COMBINABLE + if (c in PUNCTUATION && !clashesBehind) pendingSpace = false + if (pendingSpace) { + out.append(' ') + pendingSpace = false + } + out.append(c) + i++ + if (c in PUNCTUATION) { + var next = i + while (next < text.length && text[next].isWhitespace()) next++ + // A comment's `/` is not an operator: the comment is about to vanish, so it cannot combine. + val startsComment = + next + 1 < text.length && text[next] == '/' && (text[next + 1] == '/' || text[next + 1] == '*') + val clashesAhead = + c in COMBINABLE && next < text.length && text[next] in COMBINABLE && !startsComment + // Dropping it here as well as behind is what makes `a + 1` and `a+1` the same string; doing only + // one side left `a+ 1`, which still failed to match. + if (!clashesAhead) i = next + } + } + return out.toString() +} + +/** + * A backslash escapes whatever follows, so `"a\""` does not end at the middle quote and `"a\\"` does end + * at the last. An unterminated literal, possible mid-edit, consumes to the end rather than looping. + */ +private fun appendLiteral( + text: String, + start: Int, + quote: Char, + out: StringBuilder, +): Int { + // A text block's delimiter is three quotes. Stopping at the first would leave its body outside any + // literal, so its significant whitespace would be collapsed and two different blocks could compare + // equal -- which would let the occurrence search replace a site that does not hold the same value. + if (quote == '"' && text.startsWith(TEXT_BLOCK, start)) { + val close = text.indexOf(TEXT_BLOCK, start + TEXT_BLOCK.length) + val end = if (close < 0) text.length else close + TEXT_BLOCK.length + out.append(text, start, end) + return end + } + + out.append(quote) + var i = start + 1 + while (i < text.length) { + val c = text[i] + out.append(c) + i++ + if (c == '\\') { + if (i < text.length) { + out.append(text[i]) + i++ + } + continue + } + if (c == quote) return i + } + return i +} + +private const val TEXT_BLOCK = "\"\"\"" + +/** Everything that is neither an identifier character nor a literal delimiter. */ +private val PUNCTUATION = "+-*/%=!<>&|^~:.?,;(){}[]".toSet() + +/** + * The punctuation that can lex into a longer token when juxtaposed, so a space between two of them is + * load-bearing: `a - -b` must not collapse into `a--b`. + */ +private val COMBINABLE = "+-*/%=!<>&|^~:.".toSet() + +/** + * Whether a space is still needed where a comment was. + * + * A comment sitting after an operator must not put back the space that operator just swallowed: + * `a + // why` then `b` has to reach `a+b`, the same as `a+b`. + */ +private fun StringBuilder.spaceSurvivesComment(): Boolean = isNotEmpty() && last() !in PUNCTUATION diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/TypeText.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/TypeText.kt new file mode 100644 index 0000000000..0e26598fef --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/TypeText.kt @@ -0,0 +1,117 @@ +package com.itsaky.androidide.lsp.java.refactor + +import jdkx.lang.model.type.DeclaredType +import jdkx.lang.model.type.TypeKind +import jdkx.lang.model.type.TypeMirror +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.ImportTree +import openjdk.source.tree.NewClassTree +import openjdk.source.util.TreePath +import openjdk.source.util.Trees + +/** The only package whose simple names resolve with no import in Java. */ +internal val DEFAULT_IMPORTED_PACKAGES = setOf("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}_$]*)+""") + +/** + * `TYPEVAR` is deliberately absent: a type variable declared on the enclosing method or class is in + * scope at the anchor and writes out as its own name. + */ +internal fun isValuelessKind(kind: TypeKind): Boolean = + when (kind) { + TypeKind.VOID, + TypeKind.NONE, + TypeKind.NULL, + TypeKind.ERROR, + TypeKind.OTHER, + TypeKind.EXECUTABLE, + TypeKind.PACKAGE, + -> true + + else -> false + } + +/** + * javac renders a captured wildcard as `capture#1 of ? extends Foo`, an intersection with `&`, and an + * unresolvable type as ``; none parse. Heuristic, but it fails safe -- a false positive declines a + * candidate rather than emitting a declaration that does not compile. + */ +internal fun isUnrenderableTypeText(text: String): Boolean = + text.isBlank() || + text.contains('#') || + text.contains('&') || + text.contains("capture") || + text.contains("` for `new ArrayList<>()` in a `List` context, so writing it back + * reproduces the inference rather than guessing. The two poly forms where that would not hold, a lambda + * and a method reference, are excluded targets. An anonymous class is declined by construction. + */ +fun declaredTypeTextFor( + path: TreePath, + trees: Trees, + root: CompilationUnitTree, +): String? { + val leaf = path.leaf + if (leaf is NewClassTree && leaf.classBody != null) return null + + val type = runCatching { trees.getTypeMirror(path) }.getOrNull() ?: return null + if (isValuelessKind(type.kind)) return null + if (isAnonymousDeclared(type)) return null + + val rendered = type.toString() + if (isUnrenderableTypeText(rendered)) return null + + return shortenTypeText(rendered, importedNamesOf(root), starImportedPackagesOf(root)) +} + +/** A `DeclaredType` whose element has no simple name is an anonymous class. */ +private fun isAnonymousDeclared(type: TypeMirror): Boolean = + runCatching { type is DeclaredType && type.asElement().simpleName.isEmpty() }.getOrDefault(false) + +/** + * Shortens a qualified name only where the file already resolves the short form. Everything else stays + * qualified: verbose, but it compiles, and this refactoring adds no imports. + * + * A nested class is shortened only by an import of the nested name itself, never of the outer class. A + * star import is trusted only when nothing else imports the same simple name from a different package, + * since that explicit import would resolve first and the short name would silently mean the wrong type. + */ +internal fun shortenTypeText( + rendered: String, + importedNames: Set, + starImportedPackages: Set, +): String = + 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 && importedNames.none { it.endsWith(".$simpleName") }) + if (resolvable) simpleName else qualified + } + +/** The fully qualified names [root] imports by name. Syntactic: no compiler queries needed. */ +internal fun importedNamesOf(root: CompilationUnitTree): Set = + root.imports + .filterNot(ImportTree::isStatic) + .map { it.qualifiedIdentifier.toString() } + .filterNot { it.endsWith(".*") } + .toSet() + +/** The packages [root] star-imports (`import java.util.*;`). */ +internal fun starImportedPackagesOf(root: CompilationUnitTree): Set = + root.imports + .filterNot(ImportTree::isStatic) + .map { it.qualifiedIdentifier.toString() } + .filter { it.endsWith(".*") } + .map { it.removeSuffix(".*") } + .toSet() diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionTooltipTagTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionTooltipTagTest.kt index f8d2cb363f..5d6dfea398 100644 --- a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionTooltipTagTest.kt +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/actions/JavaCodeActionTooltipTagTest.kt @@ -66,6 +66,9 @@ class JavaCodeActionTooltipTagTest { // editor.codeactions.trycatch row, so long-press renders the documentation // fallback. The Kotlin twin editor.codeactions.kotlin.trycatch is authored. "ide.editor.lsp.java.surroundWithTryCatch" to TooltipTag.EDITOR_CODE_ACTIONS_TRY_CATCH, + // Tag is reserved ahead of content, as with try/catch above: ADFA-5047 specifies + // editor.codeactions.extractvariable and the documentation.db row is a hand-off item. + "ide.editor.lsp.java.extractVariable" to TooltipTag.EDITOR_CODE_ACTIONS_EXTRACT_VARIABLE, // No tag pinned. "ide.editor.lsp.java.diagnostics.variableToStatement" to "", "ide.editor.lsp.java.diagnostics.fieldToBlock" to "", diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePrimitivesTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePrimitivesTest.kt new file mode 100644 index 0000000000..24417b07b1 --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariablePrimitivesTest.kt @@ -0,0 +1,218 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.resources.R +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * Java's own compiler-free half: what a candidate label reads as, what a selection trims to, how the + * type text is rendered, and how [buildExtractVariableRewrite] composes Java's three [AnchorForm]s. + * + * The geometry and offset primitives these sit on belong to `:lsp:refactor-core` and are tested there, + * once, rather than again per language. + */ +@RunWith(JUnit4::class) +class ExtractVariablePrimitivesTest { + @Test + fun `a label collapses whitespace and closes up before a dot`() { + assertThat(collapseForLabel("items\n\t.stream()\n\t.count()")).isEqualTo("items.stream().count()") + } + + @Test + fun `a label longer than the limit is elided`() { + val label = collapseForLabel("a".repeat(200), maxLength = 20) + assertThat(label).hasLength(20) + assertThat(label).endsWith("...") + } + + @Test + fun `a whitespace-only selection collapses to a cursor at its start`() { + assertThat(trimToCode("a + b", 1, 3)).isEqualTo(1 to 1) + } + + @Test + fun `a selection is trimmed to the code inside it`() { + assertThat(trimToCode(" a + b ", 0, 9)).isEqualTo(2 to 7) + } + + @Test + fun `an out-of-bounds selection is not a selection`() { + assertThat(trimToCode("abc", 2, 1)).isNull() + assertThat(trimToCode("abc", -1, 2)).isNull() + assertThat(trimToCode("abc", 0, 4)).isNull() + } + + @Test + fun `an existing block gains the declaration on the line above the anchor`() { + val text = "void m() {\n\tfoo(a + b);\n}" + val rewrite = rewriteOf(text, candidate = TextSpan(16, 21), form = existingBlock(text)) + assertThat(applied(text, rewrite)).isEqualTo("void m() {\n\tint v = a + b;\n\tfoo(v);\n}") + } + + @Test + fun `a replace-all rewrites every served occurrence in one edit`() { + val text = "void m() {\n\tfoo(a + b);\n\tbar(a + b);\n}" + val occurrences = listOf(TextSpan(16, 21), TextSpan(29, 34)) + val rewrite = + buildExtractVariableRewrite( + fileText = text, + candidateSpan = occurrences[0], + declaredType = "int", + scope = ScopeOption(BLOCK, existingBlock(text), occurrences), + name = "v", + replaceAll = true, + )!! + assertThat(applied(text, rewrite)).isEqualTo("void m() {\n\tint v = a + b;\n\tfoo(v);\n\tbar(v);\n}") + } + + @Test + fun `a braceless body is wrapped in braces around the declaration`() { + val text = "if (c)\n\tfoo(a + b);" + // The body span starts at the statement, not at its indentation -- javac's own span does the same, + // and starting a character earlier would carry the source indent into the emitted line. + val form = + AnchorForm.WrapInBraces( + BracelessBody(bodyStart = 8, bodyEnd = 19, indent = "", innerIndent = "\t"), + ) + val rewrite = rewriteOf(text, candidate = TextSpan(12, 17), form = form) + assertThat(applied(text, rewrite)).isEqualTo("if (c)\n\t{\n\tint v = a + b;\n\tfoo(v);\n}") + } + + @Test + fun `an expression body becomes a block that returns the named value`() { + val text = "x -> a + b" + val form = + AnchorForm.ConvertExpressionBody( + bodyStart = 5, + bodyEnd = 10, + indent = "", + innerIndent = "\t", + needsReturn = true, + ) + val rewrite = rewriteOf(text, candidate = TextSpan(5, 10), form = form) + assertThat(applied(text, rewrite)).isEqualTo("x -> {\n\tint v = a + b;\n\treturn v;\n}") + } + + @Test + fun `a void expression body becomes a block with a bare statement`() { + val text = "() -> sink(a + b)" + val form = + AnchorForm.ConvertExpressionBody( + bodyStart = 6, + bodyEnd = 17, + indent = "", + innerIndent = "\t", + needsReturn = false, + ) + val rewrite = rewriteOf(text, candidate = TextSpan(11, 16), form = form) + assertThat(applied(text, rewrite)).isEqualTo("() -> {\n\tint v = a + b;\n\tsink(v);\n}") + } + + @Test + fun `a switch rule body does not gain a doubled semicolon`() { + val text = "case A -> a + b;" + val form = + AnchorForm.ConvertExpressionBody( + bodyStart = 10, + bodyEnd = 16, + indent = "", + innerIndent = "\t", + needsReturn = true, + returnKeyword = "yield", + ) + val rewrite = rewriteOf(text, candidate = TextSpan(10, 15), form = form) + assertThat(applied(text, rewrite)).isEqualTo("case A -> {\n\tint v = a + b;\n\tyield v;\n}") + } + + @Test + fun `a rewrite whose targets fall outside the text is refused`() { + val text = "void m() {\n\tfoo(a + b);\n}" + val rewrite = + buildExtractVariableRewrite( + fileText = text, + candidateSpan = TextSpan(16, 21), + declaredType = "int", + scope = ScopeOption(BLOCK, existingBlock(text), listOf(TextSpan(900, 905))), + name = "v", + replaceAll = true, + ) + assertThat(rewrite).isNull() + } + + @Test + fun `an unrenderable type is recognised by its javac spelling`() { + assertThat(isUnrenderableTypeText("capture#1 of ? extends Foo")).isTrue() + assertThat(isUnrenderableTypeText("Foo & Bar")).isTrue() + assertThat(isUnrenderableTypeText("")).isTrue() + assertThat(isUnrenderableTypeText(" ")).isTrue() + assertThat(isUnrenderableTypeText("java.util.List")).isFalse() + } + + @Test + fun `a type is shortened only where the short name resolves`() { + val shortened = + shortenTypeText( + "java.util.Map", + importedNames = setOf("java.util.Map"), + starImportedPackages = emptySet(), + ) + assertThat(shortened).isEqualTo("Map") + } + + @Test + fun `a star import does not shorten a name an explicit import already claims`() { + val shortened = + shortenTypeText( + "java.awt.List", + importedNames = setOf("java.util.List"), + starImportedPackages = setOf("java.awt"), + ) + assertThat(shortened).isEqualTo("java.awt.List") + } + + private fun existingBlock(text: String): AnchorForm.ExistingBlock { + val open = text.indexOf('{') + val close = text.lastIndexOf('}') + val statements = + text + .substring(open + 1, close) + .split('\n') + .filter { it.isNotBlank() } + .map { line -> + val start = text.indexOf(line.trim(), open) + TextSpan(start, start + line.trim().length) + } + return AnchorForm.ExistingBlock( + BlockAnchor(contentSpan = TextSpan(open + 1, close), statementSpans = statements), + ) + } + + private fun rewriteOf( + text: String, + candidate: TextSpan, + form: AnchorForm, + ): RewriteSpan = + buildExtractVariableRewrite( + fileText = text, + candidateSpan = candidate, + declaredType = "int", + scope = ScopeOption(BLOCK, form, listOf(candidate)), + name = "v", + replaceAll = false, + )!! + + private fun applied( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + + private companion object { + val BLOCK = ScopeLabel(R.string.label_extract_scope_block) + } +} diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableSoundnessTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableSoundnessTest.kt new file mode 100644 index 0000000000..0642851b6f --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractVariableSoundnessTest.kt @@ -0,0 +1,296 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.resources.R +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * One case per review finding that turns a working file into a broken one. + * + * Every case asserts on the *emitted source*, and where the finding is "this does not compile" it feeds + * the result back through javac. Comparing a `RewriteSpan` in isolation hides exactly these defects. + */ +@RunWith(JUnit4::class) +class ExtractVariableSoundnessTest { + // --- Akash: emits code that does not compile --- + + @Test + fun `extracting a whole expression statement does not leave a bare name behind`() { + // itsaky, CandidateExpressions.kt:223 -- `sb.append("x");` became `StringBuilder v = ...;` + `v;` + val f = fixture(""" void m(StringBuilder sb) {${'\n'} sb.append("x");${'\n'} }""") + val offered = f.planAfter("sb.append").candidates.map { it.label } + assertThat(offered).doesNotContain("sb.append(\"x\")") + } + + @Test + fun `a static initializer content span starts at its brace`() { + // itsaky, ScopeChain.kt:107 -- javac's JCBlock.pos for `static { }` is the `s` of `static`. + val f = fixture(""" static int a = 1, b = 2;${'\n'} static { use(a + b); }""") + val out = f.applyAfter("a +", "v") + // The old span started at `blockSpan.start + 1`, i.e. inside the keyword, and emitted `s` / `tatic` + // on separate lines. + assertThat(out).contains("static {") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a switch expression rule body conversion does not leave a stray semicolon`() { + // itsaky, ScopeChain.kt:121 -- the rule's `;` is consumed separately by the parser. + val f = + fixture( + """ int m(int x, int a, int b) {${'\n'} return switch (x) {${'\n'} case 1 -> a + b;${'\n'} default -> 0;${'\n'} };${'\n'} }""", + ) + val out = f.applyAfter("case 1 -> a +", "v", scope = SWITCH_RULE) + assertThat(out).doesNotContain("};;") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a rung whose anchor shares a line inside a multi-line block is refused`() { + // itsaky, ExtractVariableEdit.kt:94 -- this used to fall through to LineAbove and hoist the + // declaration above `it.next();`, reading `it` before it was assigned. Declining is the honest + // answer: threading a declaration into a line that also holds unrelated statements is not a move + // this refactoring makes. + val f = + fixture( + """ void m(java.util.Iterator it) {${'\n'} it.next(); use(it.hashCode() + 1);${'\n'} tail();${'\n'} }${'\n'} void tail() {}""", + ) + val plan = f.planAfter("it.hashCode() +") + val rungs = plan.candidates.flatMap { it.scopes }.map { it.label } + assertThat(rungs).doesNotContain(METHOD_M) + } + + @Test + fun `replace-all does not substitute into a case label`() { + // itsaky, Occurrences.kt:70 -- matches are shape-checked but not position-checked. + val f = + fixture( + """ static final int A = 1, B = 2;${'\n'} void m(int x) {${'\n'} use(A + B);${'\n'} switch (x) {${'\n'} case A + B: tail(); break;${'\n'} }${'\n'} }${'\n'} void tail() {}""", + ) + val scope = + f + .planAfter("use(A +") + .candidates + .first() + .scopes + .last() + assertThat(scope.occurrences).hasSize(1) + } + + // --- Hal: emits code that does not compile --- + + @Test + fun `a for-loop variable pins the ceiling inside the loop`() { + // hal, Occurrences.kt:207 -- constrainingScopeFor has no ForLoopTree branch. + val f = + fixture( + """ void m(java.util.List items) {${'\n'} for (String s : items) {${'\n'} use(s.length() + 1);${'\n'} }${'\n'} }""", + ) + val scopes = + f + .planAfter("s.length() +") + .candidates + .first() + .scopes + .map { it.label } + assertThat(scopes).doesNotContain(METHOD_M) + } + + @Test + fun `a try-with-resources variable pins the ceiling inside the try`() { + val f = + fixture( + """ void m() throws Exception {${'\n'} try (java.io.Reader r = null) {${'\n'} use(r.hashCode() + 1);${'\n'} }${'\n'} }""", + ) + val scopes = + f + .planAfter("r.hashCode() +") + .candidates + .first() + .scopes + .map { it.label } + assertThat(scopes).doesNotContain(METHOD_M) + } + + @Test + fun `a one-line block does not hoist the declaration above a preceding statement`() { + // hal, ExtractVariableEdit.kt:185 -- oneLineBlockRewrite always prepends. + val f = fixture(""" void m() { int a = 1; use(a + 2); }""") + val out = f.applyAfter("a +", "v") + assertThat(out.indexOf("int a = 1")).isLessThan(out.indexOf("int v =")) + assertThat(compiles(out)).isTrue() + } + + @Test + fun `a switch case label is not offered for extraction`() { + // hal, CandidateExpressions.kt:156 -- case labels must be constant expressions. + val f = + fixture( + """ static final int FOO = 1;${'\n'} void m(int x) {${'\n'} switch (x) {${'\n'} case FOO + 1: tail(); break;${'\n'} }${'\n'} }${'\n'} void tail() {}""", + ) + assertThat(f.planAfter("case FOO +").isEmpty).isTrue() + } + + @Test + fun `a name colliding with a later local in the same block is rejected`() { + // hal, Occurrences.kt:229 -- Trees.getScope stops at the candidate. + val f = + fixture( + """ void m(java.util.List items) {${'\n'} use(items.size());${'\n'} int size = 3;${'\n'} use(size);${'\n'} }""", + ) + val taken = + f + .planAfter("items.siz") + .candidates + .first { it.label == "items.size()" } + .takenNames + assertThat(taken).contains("size") + } + + // --- Hal: compiles but silently changes behaviour --- + + @Test + fun `the operand of an increment is not offered`() { + // hal, CandidateExpressions.kt:197 -- `foo(i++)` would increment the copy, not `i`. + val f = fixture(""" void m(int i) {${'\n'} use(i++);${'\n'} }""") + assertThat(f.planAfter("use(i").candidates.map { it.label }).doesNotContain("i") + } + + @Test + fun `a loop condition is not offered`() { + // itsaky, CandidateExpressions.kt:164 -- hoisting it out evaluates it once, so the loop never ends. + val f = + fixture( + """ void m(java.util.Iterator it) {${'\n'} while (it.hasNext()) {${'\n'} use(it.next().length());${'\n'} }${'\n'} }""", + ) + assertThat(f.planAfter("while (it.hasNext").candidates.map { it.label }) + .doesNotContain("it.hasNext()") + } + + @Test + fun `the right operand of a short-circuit is not offered`() { + // itsaky -- hoisting it out defeats the guard that made it safe. + val f = + fixture( + """ void m(String s) {${'\n'} if (s != null && s.length() > 0) {${'\n'} tail();${'\n'} }${'\n'} }${'\n'} void tail() {}""", + ) + assertThat(f.planAfter("&& s.length() > ").candidates.map { it.label }) + .doesNotContain("s.length() > 0") + } + + @Test + fun `spacing around an operator does not defeat occurrence matching`() { + // hal, SourceNormalizer.kt:53 -- only `.` had its adjacent space dropped. + assertThat(normalizeSource("items.size()+1")).isEqualTo(normalizeSource("items.size() + 1")) + } + + // --- This round: the second review pass --- + + @Test + fun `a for update expression is not offered`() { + // coderabbit, CandidateExpressions.kt:231 -- a `for` update is an ExpressionStatementTree, so the + // statement boundary read it as a fixed evaluation point and the only rung was outside the loop. + val f = + fixture( + """ void m(int n) {${'\n'} for (int i = 0; i < n; i = step(i + 1)) {${'\n'} tail();${'\n'} }${'\n'} }${'\n'} static int step(int v) { return v; }${'\n'} void tail() {}""", + ) + assertThat(f.planAfter("i = step(i +").candidates.map { it.label }).doesNotContain("i + 1") + } + + @Test + fun `hoisting out of a loop past a write to a read variable is refused`() { + // hal, ExtractVariablePlanner.kt:162 -- the outer rung froze the value at its first iteration. + val f = + fixture( + """ void m() {${'\n'} int limit = 0;${'\n'} while (limit < 10) {${'\n'} use(limit + 1);${'\n'} limit++;${'\n'} }${'\n'} }""", + ) + val rungs = + f + .planAfter("use(limit +") + .candidates + .first() + .scopes + .map { it.label } + assertThat(rungs).doesNotContain(METHOD_M) + // The rung inside the loop is still offered, so the action stays usable. + assertThat(rungs).contains(WHILE_LOOP) + } + + @Test + fun `hoisting over a write on the way to an outer rung is refused`() { + // The same defect one shape along: the anchor is the `if`, so the declaration would land before + // the assignment the expression reads. + val f = + fixture( + """ void m(boolean c) {${'\n'} int limit = 0;${'\n'} if (c) {${'\n'} limit = 5;${'\n'} use(limit + 1);${'\n'} }${'\n'} }""", + ) + val rungs = + f + .planAfter("use(limit +") + .candidates + .first() + .scopes + .map { it.label } + assertThat(rungs).doesNotContain(METHOD_M) + } + + @Test + fun `a plan built with no open document carries no version to compare`() { + // hal, ExtractVariableAction.kt:169 -- a -1 sentinel compared equal to itself and so passed the + // staleness guard it existed to fail. + val f = fixture(""" void m(int a, int b) {${'\n'} use(a + b);${'\n'} }""") + val cursor = f.cursorAfter("a +") + val plan = buildExtractionPlan(f.task, f.root, f.text, cursor, cursor, documentVersion = null) + assertThat(plan.candidates).isNotEmpty() + assertThat(plan.documentVersion).isNull() + } + + @Test + fun `an occurrence search does not reach past the rung it was asked about`() { + // hal, Occurrences.kt:85 -- the walk covered the whole compilation unit per rung per candidate. + val f = + fixture( + """ void m(java.util.List items) {${'\n'} use(items.size());${'\n'} }${'\n'} void other(java.util.List items) {${'\n'} use(items.size());${'\n'} }""", + ) + val scopes = + f + .planAfter("use(items.siz") + .candidates + .first { it.label == "items.size()" } + .scopes + // `other` spells the same expression over a different `items`, and is outside the rung anyway. + assertThat(scopes.map { it.occurrences.size }).containsExactly(1) + } + + @After + fun closeFixtures() { + fixtures.forEach(JavacFixture::close) + fixtures.clear() + } + + private val fixtures = mutableListOf() + + // Registered rather than `use`d so each case still reads as a straight line; the fixture holds a + // JavaFileManager, so it has to be closed either way. + private fun fixture(body: String) = + JavacFixture( + """ + |class Fixture { + |$body + | static void use(int value) {} + | static void use(Object value) {} + |} + """.trimMargin(), + ).also { fixtures += it } + + private companion object { + val METHOD_M = ScopeLabel(R.string.label_extract_scope_method, "m") + val SWITCH_RULE = ScopeLabel(R.string.label_extract_scope_switch_rule) + val WHILE_LOOP = ScopeLabel(R.string.label_extract_scope_while_loop) + } +} diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/JavacFixture.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/JavacFixture.kt new file mode 100644 index 0000000000..dbfcb1abda --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/JavacFixture.kt @@ -0,0 +1,129 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import jdkx.tools.JavaFileManager +import jdkx.tools.JavaFileObject +import jdkx.tools.SimpleJavaFileObject +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.util.JavacTask +import openjdk.source.util.Trees +import openjdk.tools.javac.api.JavacTool +import java.net.URI + +/** + * One attributed compile of a source string, with no project model and no tooling API. + * + * The Robolectric `JavaLSPTest` harness cannot serve this layer: it boots the Gradle tooling API in a + * separate process, which needs a resolved project and does not start at all in some environments. + * Everything the extract-variable analysis needs is a `CompilationUnitTree` plus `Trees`, and + * `JavacTool` supplies both directly, so these tests are hermetic and run in milliseconds. + */ +class JavacFixture( + val text: String, + fileName: String = "Fixture.java", +) : AutoCloseable { + val task: JavacTask + val root: CompilationUnitTree + + // The manager has to outlive the task -- javac reads through it lazily -- so it is held here and + // closed with the fixture rather than around the compile. + private val fileManager: JavaFileManager + + val trees: Trees get() = Trees.instance(task) + + init { + val tool = JavacTool.create() + fileManager = tool.getStandardFileManager(null, null, null) + val source = + object : SimpleJavaFileObject(URI.create("string:///$fileName"), JavaFileObject.Kind.SOURCE) { + override fun getCharContent(ignoreEncodingErrors: Boolean): CharSequence = text + } + + task = tool.getTask(null, fileManager, null, listOf("-proc:none", "-g"), null, listOf(source)) + root = task.parse().first() + // Attribution is what fills in types and elements; without it getTypeMirror answers nothing. + task.analyze() + } + + override fun close() = fileManager.close() + + /** + * The offset immediately after [prefix]'s first occurrence. + * + * Prefer this over `indexOf(x) + n`: one character into `a + b * c` sits inside `a` and resolves to + * that identifier, not to the binary expression, so a delta silently tests a different candidate + * than the case name claims. + */ + fun cursorAfter(prefix: String): Int { + val index = text.indexOf(prefix) + require(index >= 0) { "the fixture contains no '$prefix'" } + return index + prefix.length + } + + /** The plan for a cursor placed just after [prefix]. */ + fun planAfter( + prefix: String, + documentVersion: Int = 1, + ): ExtractionPlan { + val cursor = cursorAfter(prefix) + return buildExtractionPlan(task, root, text, cursor, cursor, documentVersion) + } + + /** + * The file as it reads after extracting at [prefix] into [name], picking the rung labelled [scope]. + * + * This is the assertion that actually matters for the rewrite bugs: a span or ordering error shows up + * as broken source here, where comparing a `RewriteSpan` in isolation hides it. + */ + fun applyAfter( + prefix: String, + name: String, + scope: ScopeLabel? = null, + replaceAll: Boolean = false, + ): String { + val plan = planAfter(prefix) + val candidate = plan.candidates.firstOrNull() ?: error("no candidate after '$prefix'") + val option = + if (scope == null) { + candidate.scopes.first() + } else { + candidate.scopes.firstOrNull { it.label == scope } + ?: error("no scope $scope in ${candidate.scopes.map { it.label }}") + } + val rewrite = + buildExtractVariableRewrite( + fileText = plan.fileText, + candidateSpan = candidate.span, + declaredType = candidate.declaredType, + scope = option, + name = name, + replaceAll = replaceAll, + ) ?: error("no rewrite for '$prefix' in scope '${option.label}'") + return text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + } +} + +/** Whether [source] compiles on its own, which is what most of these findings are really about. */ +fun compiles(source: String): Boolean { + val tool = JavacTool.create() + val file = + object : SimpleJavaFileObject(URI.create("string:///Probe.java"), JavaFileObject.Kind.SOURCE) { + override fun getCharContent(ignoreEncodingErrors: Boolean): CharSequence = source + } + val diagnostics = mutableListOf() + // Nothing here outlives analyze(), so the manager is scoped to the probe rather than leaked per call. + tool.getStandardFileManager(null, null, null).use { fileManager -> + val task = + tool.getTask( + null, + fileManager, + { d -> if (d.kind.name == "ERROR") diagnostics += d.getMessage(null) }, + listOf("-proc:none"), + null, + listOf(file), + ) + task.analyze() + } + if (diagnostics.isNotEmpty()) println(" compile errors: $diagnostics") + return diagnostics.isEmpty() +} diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizerTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizerTest.kt new file mode 100644 index 0000000000..89dd81c6c1 --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/SourceNormalizerTest.kt @@ -0,0 +1,127 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.refactor.detectIndentUnit +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** The occurrence matcher's text half, with no compiler involved. */ +@RunWith(JUnit4::class) +class SourceNormalizerTest { + @Test + fun `whitespace runs collapse to one space`() { + assertThat(normalizeSource("a +\n\tb")).isEqualTo("a+b") + } + + @Test + fun `leading and trailing whitespace is dropped`() { + assertThat(normalizeSource(" a + b ")).isEqualTo("a+b") + } + + @Test + fun `line comments are stripped`() { + assertThat(normalizeSource("a + // why\nb")).isEqualTo("a+b") + } + + @Test + fun `block comments are stripped`() { + assertThat(normalizeSource("a /* note */ + b")).isEqualTo("a+b") + } + + @Test + fun `whitespace inside a string literal is preserved`() { + assertThat(normalizeSource("f(\"a b\")")).isEqualTo("f(\"a b\")") + } + + @Test + fun `a comment marker inside a string literal is preserved`() { + assertThat(normalizeSource("f(\"http://x\")")).isEqualTo("f(\"http://x\")") + } + + @Test + fun `an escaped quote does not end a string literal`() { + assertThat(normalizeSource("f(\"a\\\" b\")")).isEqualTo("f(\"a\\\" b\")") + } + + @Test + fun `a char literal holding a quote is preserved`() { + assertThat(normalizeSource("c == '\"' ")).isEqualTo("c=='\"'") + } + + @Test + fun `an escaped backslash before a quote ends the literal`() { + assertThat(normalizeSource("f(\"a\\\\\") ")).isEqualTo("f(\"a\\\\\")") + } + + @Test + fun `an unterminated literal consumes to the end rather than looping`() { + assertThat(normalizeSource("f(\"abc")).isEqualTo("f(\"abc") + } + + @Test + fun `two spellings of the same expression normalize equal`() { + assertThat(normalizeSource("items.size() + 1")) + .isEqualTo(normalizeSource("items\n\t.size() /* n */ + 1")) + } + + @Test + fun `a text block keeps its significant whitespace`() { + val block = "f(\"\"\"\n a b\n\"\"\")" + assertThat(normalizeSource(block)).isEqualTo(block) + } + + @Test + fun `two text blocks differing only in whitespace do not normalize equal`() { + // If they collapsed, the occurrence search would replace a site holding a different value. + val a = normalizeSource("f(\"\"\"\n a\n\"\"\")") + val b = normalizeSource("f(\"\"\"\n a\n\"\"\")") + assertThat(a).isNotEqualTo(b) + } + + @Test + fun `an unterminated text block consumes to the end rather than looping`() { + assertThat(normalizeSource("f(\"\"\"abc")).isEqualTo("f(\"\"\"abc") + } + + @Test + fun `a javadoc continuation line is not treated as the indent unit`() { + val spaceIndented = + """ + |package qa; + | + |/** + | * Doc. + | */ + |class Qa { + | void f() { + | } + |} + """.trimMargin() + // The ` * ` and ` */` lines are runs of exactly one space; before this was guarded they won the + // minimum on virtually every real Java file. + assertThat(detectIndentUnit(spaceIndented)).isEqualTo(" ") + } + + @Test + fun `a tab-indented file still reports a tab`() { + assertThat(detectIndentUnit("class Qa {\n\tvoid f() {\n\t}\n}")).isEqualTo("\t") + } + + @Test + fun `a file with no indentation at all falls back to a tab`() { + assertThat(detectIndentUnit("class Qa {}")).isEqualTo("\t") + } + + @Test + fun `spacing collapses around every operator, not just the dot`() { + assertThat(normalizeSource("a+1")).isEqualTo(normalizeSource("a + 1")) + assertThat(normalizeSource("m(x,y)")).isEqualTo(normalizeSource("m(x, y)")) + } + + @Test + fun `two combinable characters are kept apart`() { + // Closing `a - -b` up to `a--b` would make it a different expression, and equal to `a-- b`. + assertThat(normalizeSource("a - -b")).isNotEqualTo(normalizeSource("a-- b")) + } +} diff --git a/lsp/kotlin/build.gradle.kts b/lsp/kotlin/build.gradle.kts index d25dd4a40a..d095fa6e2c 100644 --- a/lsp/kotlin/build.gradle.kts +++ b/lsp/kotlin/build.gradle.kts @@ -59,6 +59,8 @@ dependencies { implementation(projects.subprojects.projectModels) implementation(projects.commonCompose) + implementation(projects.lsp.refactorCore) + implementation(projects.lsp.ui) implementation(platform(libs.compose.bom)) implementation(libs.compose.runtime) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt index 64e9984774..7e673c4985 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -11,16 +11,16 @@ import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodChoice import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodSheet -import com.itsaky.androidide.lsp.kotlin.refactor.ui.findFragmentActivity import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionRefusal import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodPlan import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodRewrites -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.lsp.refactor.toTextEdit +import com.itsaky.androidide.lsp.ui.findFragmentActivity import com.itsaky.androidide.projects.FileManager import com.itsaky.androidide.resources.R import com.itsaky.androidide.tasks.createJobCancelChecker 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 index 086c8060f7..d92b2a9afe 100644 --- 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 @@ -8,17 +8,21 @@ 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.refactor.KOTLIN_NAME_MESSAGES +import com.itsaky.androidide.lsp.kotlin.refactor.candidateAndScopeFor +import com.itsaky.androidide.lsp.kotlin.refactor.toCandidateViews import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS 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.lsp.refactor.toTextEdit +import com.itsaky.androidide.lsp.ui.ExtractVariableSelection +import com.itsaky.androidide.lsp.ui.ExtractVariableSheet +import com.itsaky.androidide.lsp.ui.findFragmentActivity import com.itsaky.androidide.projects.FileManager import com.itsaky.androidide.resources.R import com.itsaky.androidide.tasks.createJobCancelChecker @@ -95,23 +99,29 @@ class ExtractVariableAction : BaseKotlinCodeAction() { return } - val shown = ExtractVariableSheet.show(activity, result) { choice -> applyChoice(data, result, choice) } + val shown = + ExtractVariableSheet.show( + activity, + result.toCandidateViews(), + HARD_KEYWORDS, + KOTLIN_NAME_MESSAGES, + ) { selection -> applySelection(data, result, selection) } 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. + * Turns the user's selection 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( + private fun applySelection( data: ActionData, plan: ExtractionPlan, - choice: ExtractionChoice, + selection: ExtractVariableSelection, ) { val file = data.requireFile() val nioPath = file.toPath() @@ -120,15 +130,22 @@ class ExtractVariableAction : BaseKotlinCodeAction() { return } + val (candidate, scope) = + plan.candidateAndScopeFor(selection) ?: run { + logger.warn("Selection {} does not address the plan it came from.", selection) + flashError(R.string.msg_cannot_perform_fix) + return + } + val rewrite = buildExtractVariableRewrite( fileText = plan.fileText, - candidateSpan = choice.candidate.span, - scope = choice.scope, - name = choice.name, - replaceAll = choice.replaceAll, + candidateSpan = candidate.span, + scope = scope, + name = selection.name, + replaceAll = selection.replaceAll, ) ?: run { - logger.warn("Could not build an extract-variable rewrite for '{}'", choice.candidate.label) + logger.warn("Could not build an extract-variable rewrite for '{}'", candidate.label) flashError(R.string.msg_cannot_perform_fix) return } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractVariableUi.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractVariableUi.kt new file mode 100644 index 0000000000..28640cae5f --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/KotlinExtractVariableUi.kt @@ -0,0 +1,54 @@ +package com.itsaky.androidide.lsp.kotlin.refactor + +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.ScopeOption +import com.itsaky.androidide.lsp.ui.CandidateView +import com.itsaky.androidide.lsp.ui.ExtractVariableSelection +import com.itsaky.androidide.lsp.ui.NameMessages +import com.itsaky.androidide.lsp.ui.ScopeView +import com.itsaky.androidide.resources.R + +/** + * Kotlin's wording for the four name problems the shared sheet can report. + * + * Two of them name the language, which is why the sheet takes them rather than looking them up. + */ +val KOTLIN_NAME_MESSAGES = + NameMessages( + blank = R.string.msg_extract_variable_name_blank, + invalid = R.string.msg_extract_variable_name_invalid, + keyword = R.string.msg_extract_variable_name_keyword, + taken = R.string.msg_extract_variable_name_taken, + ) + +/** + * The plan as the shared sheet sees it: labels, names and counts, no PSI and no offsets. + * + * Offsets stay on this side deliberately -- the sheet is a chooser, and resolving a choice back into + * spans is [candidateAndScopeFor]'s job. + */ +fun ExtractionPlan.toCandidateViews(): List = + candidates.map { candidate -> + CandidateView( + label = candidate.label, + suggestedName = candidate.suggestedName, + takenNames = candidate.takenNames, + scopes = + candidate.scopes.map { scope -> + ScopeView(label = scope.label, occurrenceCount = scope.occurrences.size) + }, + ) + } + +/** + * Resolves a selection's indices back to the plan they came from, or null when they do not address it. + * + * A null is a wiring bug rather than a user path -- the sheet only ever reports indices it was given - + * so the caller reports it as a failed quick fix rather than guessing at a candidate. + */ +fun ExtractionPlan.candidateAndScopeFor(selection: ExtractVariableSelection): Pair? { + val candidate = candidates.getOrNull(selection.candidateIndex) ?: return null + val scope = candidate.scopes.getOrNull(selection.scopeIndex) ?: return null + return candidate to scope +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt index cf0e3cdf92..98e0792f23 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt @@ -16,6 +16,9 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.unit.dp +import com.itsaky.androidide.lsp.kotlin.refactor.KOTLIN_NAME_MESSAGES +import com.itsaky.androidide.lsp.ui.LabelledSection +import com.itsaky.androidide.lsp.ui.OptionList import com.itsaky.androidide.resources.R /** @@ -63,7 +66,10 @@ fun ExtractMethodSheetContent( label = { Text(stringResource(R.string.label_extract_variable_name)) }, isError = state.nameProblem != null, singleLine = true, - supportingText = state.nameProblem?.let { problem -> { Text(stringResource(problem.messageRes())) } }, + supportingText = + state.nameProblem?.let { problem -> + { Text(stringResource(KOTLIN_NAME_MESSAGES.resFor(problem))) } + }, modifier = Modifier.fillMaxWidth(), ) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt index 82bf60186f..80f4c916d4 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt @@ -1,7 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.refactor.ui import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate -import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.ui.NameProblem /** * Everything the extract-method sheet renders. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt index b1e0ecfc68..b0969c46d8 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt @@ -3,8 +3,9 @@ package com.itsaky.androidide.lsp.kotlin.refactor.ui import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS import com.itsaky.androidide.lsp.kotlin.utils.refactor.signatureText -import com.itsaky.androidide.lsp.kotlin.utils.refactor.validateVariableName +import com.itsaky.androidide.lsp.ui.validateVariableName import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -64,7 +65,7 @@ class ExtractMethodViewModel( selectedCandidate = bounded, showCandidatePicker = plan.candidates.size > 1, name = resolvedName, - nameProblem = validateVariableName(resolvedName, candidate.takenNames), + nameProblem = validateVariableName(resolvedName, candidate.takenNames, HARD_KEYWORDS), // The same call the edit builder makes, so the preview cannot drift from the declaration. signaturePreview = candidate.signatureText(resolvedName), ) 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 8498e1d1a4..417a661b0a 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 @@ -1,5 +1,6 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.MAX_CANDIDATES 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 diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt index 02dc61fe6c..175d832bd6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt @@ -1,5 +1,11 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.detectIndentUnit +import com.itsaky.androidide.lsp.refactor.detectNewline +import com.itsaky.androidide.lsp.refactor.leadingIndentAt + /** * The two replacements an extraction performs: the new function, and the call that replaces the * region. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 730215ff52..adb180bca9 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -1,5 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.TextSpan + /** One derived parameter of the new function. Names are the originals, unchanged (R5). */ data class MethodParameter( val name: String, 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 205997b921..ff585fc33c 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 @@ -1,22 +1,12 @@ 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, -) +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.detectNewline +import com.itsaky.androidide.lsp.refactor.existingBlockRewrite +import com.itsaky.androidide.lsp.refactor.replaceOccurrences +import com.itsaky.androidide.lsp.refactor.startOfWhitespaceBefore +import com.itsaky.androidide.lsp.refactor.wrapInBracesRewrite /** * Builds the extraction rewrite, or null when the inputs cannot produce one. @@ -47,190 +37,12 @@ fun buildExtractVariableRewrite( val declaration = "val $name = $expression" return when (val form = scope.anchorForm) { - is AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, form, targets, declaration, name) - is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form, targets, declaration, name) + is AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, form.block, targets, declaration, name) + is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form.body, targets, declaration, name) is AnchorForm.ConvertExpressionBody -> convertExpressionBodyRewrite(fileText, form, targets, declaration, name) } } -/** - * What a block rung can do with the anchor statement holding a given target. - * - * Shared by the planner and the rewriter so a rung is never *offered* that the rewrite would then - * refuse: the sheet would open, the user would fill it in, and the confirm would fail with the generic - * quick-fix error instead of the action reporting up front that there is nothing to extract. - */ -internal sealed interface BlockPlacement { - /** The declaration becomes a new line above [anchor], at [anchor]'s indentation. */ - data class LineAbove( - val anchor: TextSpan, - ) : BlockPlacement - - /** The block is written on one line and is expanded, with the declaration inside its braces. */ - data object ExpandOneLine : BlockPlacement - - /** Neither is sound here, so the rung is declined. */ - data object Refused : BlockPlacement -} - -/** - * Decides the placement for the anchor statement of [form] that contains [firstTarget]. - * - * [Refused] covers two shapes. Nothing in the block contains the target, which means the plan and the - * text disagree. Or something other than indentation precedes the anchor statement on its line while - * the block's own content spans several lines, as in `items.forEach { log(x)\n\tlog(y) }` -- anchoring - * at that line start would put the declaration before the block's own opening delimiter, outside the - * scope the user picked, where a lambda's `it` does not exist. - * - * 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 read as "outside the - * block", which is why the second check tests the gap for real code rather than for mere distance. - * - * [form]'s spans are substringed against [fileText] unchecked, so callers must pass the very text those - * spans were computed against -- the plan's own text, never the live document. - */ -internal fun blockPlacementFor( - fileText: String, - form: AnchorForm.ExistingBlock, - firstTarget: TextSpan, -): BlockPlacement { - val anchor = - form.statementSpans.firstOrNull { it.start <= firstTarget.start && firstTarget.end <= it.end } - ?: return BlockPlacement.Refused - val lineStart = lineStartOffset(fileText, anchor.start) - - /* - * Two conditions together are what actually mean "written on 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 BlockPlacement.ExpandOneLine - - if (form.contentSpan.start > lineStart && fileText.substring(lineStart, form.contentSpan.start).isNotBlank()) { - return BlockPlacement.Refused - } - return BlockPlacement.LineAbove(anchor) -} - -/** - * Narrows [occurrences] to the ones a replace-all can actually be anchored on. - * - * A replace-all anchors on the *first* served occurrence, so a leading occurrence whose own statement - * shares the block's opening-brace line would refuse the whole rewrite even though the site the user - * selected is perfectly placeable. Dropping such leading sites keeps "Replace all N occurrences" - * achievable, which is the same guarantee `excludeUnsoundOccurrences` makes about soundness. - * - * [candidateSpan] is never dropped: the site the user selected is always served. Only leading sites - * matter, because a later occurrence never becomes the anchor. - */ -internal fun servableOccurrences( - fileText: String, - form: AnchorForm, - occurrences: List, - candidateSpan: TextSpan, -): List { - if (form !is AnchorForm.ExistingBlock) return occurrences - return occurrences.dropWhile { it != candidateSpan && blockPlacementFor(fileText, form, it) is BlockPlacement.Refused } -} - -/** - * Inserts the declaration as its own line before the anchor statement, and rewrites everything from - * there through the last occurrence. - * - * 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 [blockPlacementFor] refuses the anchor; the caller reports that rather than guessing. - */ -private fun existingBlockRewrite( - fileText: String, - form: AnchorForm.ExistingBlock, - targets: List, - declaration: String, - name: String, -): RewriteSpan? { - val last = targets.last() - val anchor = - when (val placement = blockPlacementFor(fileText, form, targets.first())) { - is BlockPlacement.Refused -> return null - is BlockPlacement.ExpandOneLine -> return oneLineBlockRewrite(fileText, form, targets, declaration, name) - is BlockPlacement.LineAbove -> placement.anchor - } - - val lineStart = lineStartOffset(fileText, anchor.start) - val indent = leadingIndentAt(fileText, anchor.start) - val newline = detectNewline(fileText) - - val span = TextSpan(lineStart, last.end) - val body = replaceOccurrences(fileText, span, targets, name) - 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, - 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, @@ -259,77 +71,3 @@ private fun convertExpressionBodyRewrite( } 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 -} - -/** 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. - */ -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 index 1270709429..16f730cc08 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 @@ -6,6 +6,11 @@ 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 com.itsaky.androidide.lsp.refactor.BlockPlacement +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.blockPlacementFor +import com.itsaky.androidide.lsp.refactor.excludeUnsoundOccurrences +import com.itsaky.androidide.lsp.refactor.servableOccurrences import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol @@ -120,7 +125,7 @@ private fun KaSession.scopeOptionFor( * tested here; servableOccurrences is what makes the first served target placeable when * replace-all is on. */ - if (blockPlacementFor(fileText, form, span) is BlockPlacement.Refused) return null + if (blockPlacementFor(fileText, form.block, span) is BlockPlacement.Refused) return null form } @@ -136,7 +141,8 @@ private fun KaSession.scopeOptionFor( val matches = findOccurrences(expression, frame.scopeElement, frame.searchRange) val writes = writeOffsetsFor(expression, frame.scopeElement) val sound = excludeUnsoundOccurrences(matches, span, writes) - val occurrences = servableOccurrences(fileText, anchorForm, sound, span) + val occurrences = + servableOccurrences(fileText, (anchorForm as? AnchorForm.ExistingBlock)?.block, sound, span) return ScopeOption(label = frame.label, anchorForm = anchorForm, occurrences = occurrences) } 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 6a8e6761e0..1a4a47769f 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 @@ -1,62 +1,38 @@ 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 -} +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.MAX_CANDIDATES +import com.itsaky.androidide.lsp.refactor.TextSpan /** - * 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. + * 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. + * + * The first two carry types from `:lsp:refactor-core`, because a braced scope and a braceless statement + * have the same geometry in both languages and the code that reasons about them is shared. + * [ConvertExpressionBody] is genuinely Kotlin's own -- it replaces an `=` and can write a return type + * into the signature, neither of which a Java lambda does. */ sealed interface AnchorForm { - /** - * The scope already has a `{ ... }` body (function body, `if` block, lambda body, ...), so the - * declaration is a new statement line inside it. - * - * [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. - */ + /** A scope that already has a `{ ... }` body, described by [BlockAnchor]. */ data class ExistingBlock( - val contentSpan: TextSpan, - val statementSpans: List, + val block: BlockAnchor, ) : 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. - */ + /** A braceless statement position: `if (c) foo()`, a `when` entry, a braceless loop body. */ data class WrapInBraces( - val bodyStart: Int, - val bodyEnd: Int, - val indent: String, - val innerIndent: String, + val body: BracelessBody, ) : 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. + * 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. + * [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, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt index f6b2f5d198..a199e1f22d 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt @@ -1,5 +1,6 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.TextSpan import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtExpression diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 2a05b28bee..b5cf24e11a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -1,5 +1,8 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.leadingIndentAt +import com.itsaky.androidide.lsp.refactor.uniqueName import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundArrayAccessCall 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 index c6cc362228..7ed4d9d182 100644 --- 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 @@ -1,5 +1,10 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.FALLBACK_NAME +import com.itsaky.androidide.lsp.refactor.nameFromType +import com.itsaky.androidide.lsp.refactor.stripAccessorPrefix +import com.itsaky.androidide.lsp.refactor.uniqueName +import com.itsaky.androidide.lsp.ui.isIdentifier import org.jetbrains.kotlin.psi.KtArrayAccessExpression import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtExpression @@ -8,14 +13,11 @@ 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 = +internal val HARD_KEYWORDS = setOf( "as", "break", @@ -47,38 +49,6 @@ private val HARD_KEYWORDS = "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. * @@ -116,40 +86,3 @@ private fun nameFromShape(expression: KtExpression): String? = 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. */ -internal fun uniqueName( - 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 index 9936b3cfec..67a46a5b32 100644 --- 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 @@ -1,5 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.excludeUnsoundOccurrences import org.jetbrains.kotlin.analysis.api.KaSession import org.jetbrains.kotlin.analysis.api.symbols.KaSymbol import org.jetbrains.kotlin.analysis.api.symbols.KaValueParameterSymbol 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 d89c570e5a..68d6fa856f 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 @@ -1,5 +1,10 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.detectIndentUnit +import com.itsaky.androidide.lsp.refactor.leadingIndentAt import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil import org.jetbrains.kotlin.psi.KtAnonymousInitializer @@ -110,9 +115,11 @@ private fun frameFor( searchRange = parent.textRange.let { TextSpan(it.startOffset, it.endOffset) }, anchorForm = AnchorForm.ExistingBlock( - contentSpan = contentSpanOf(parent), - statementSpans = - parent.statements.map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) }, + BlockAnchor( + contentSpan = contentSpanOf(parent), + statementSpans = + parent.statements.map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) }, + ), ), ) } @@ -128,10 +135,12 @@ private fun frameFor( searchRange = span, anchorForm = AnchorForm.WrapInBraces( - bodyStart = span.start, - bodyEnd = span.end, - indent = indent, - innerIndent = indent + detectIndentUnit(text), + BracelessBody( + bodyStart = span.start, + bodyEnd = span.end, + indent = indent, + innerIndent = indent + detectIndentUnit(text), + ), ), ) } @@ -258,36 +267,3 @@ internal fun contentSpanOf(block: KtBlockExpression): TextSpan { TextSpan(range.startOffset + 1, range.endOffset - 1) } } - -/** 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/refactor/ui/ExtractMethodViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt index e6e69869da..fa0de3d7b6 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt @@ -5,8 +5,8 @@ import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractedBody import com.itsaky.androidide.lsp.kotlin.utils.refactor.MethodParameter -import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem -import com.itsaky.androidide.lsp.kotlin.utils.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.ui.NameProblem import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull @@ -141,4 +141,25 @@ class ExtractMethodViewModelTest { assertEquals(NameProblem.Blank, model.uiState.value.nameProblem) assertNull(model.choice()) } + + @Test + fun `a hard keyword blocks confirmation`() { + val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("when")) + + assertEquals(NameProblem.Keyword, model.uiState.value.nameProblem) + assertFalse(model.uiState.value.canConfirm) + assertNull(model.choice()) + } + + @Test + fun `a name that only looks like a keyword is accepted`() { + val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("whenever")) + + assertNull(model.uiState.value.nameProblem) + assertTrue(model.uiState.value.canConfirm) + } } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt index 713c2cf2e6..285b7e4538 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt @@ -1,5 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index b0354531de..9da1a5642b 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -2,6 +2,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.refactor.RewriteSpan import com.itsaky.androidide.progress.ICancelChecker import org.junit.Assert.assertEquals import org.junit.Assert.assertNull diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt index 35f572be56..ec7222eb8d 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.lsp.refactor.TextSpan import org.jetbrains.kotlin.psi.KtFile import org.junit.Assert.assertEquals import org.junit.Assert.assertNull 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 3936da29d2..a08d89d0a0 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 @@ -1,5 +1,12 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.refactor.BlockAnchor +import com.itsaky.androidide.lsp.refactor.BlockPlacement +import com.itsaky.androidide.lsp.refactor.BracelessBody +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.blockPlacementFor +import com.itsaky.androidide.lsp.refactor.positionAt import org.junit.Assert.assertEquals import org.junit.Assert.assertNull import org.junit.Test @@ -53,8 +60,10 @@ class ExtractVariableEditTest { text: String, vararg statements: String, ) = AnchorForm.ExistingBlock( - contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), - statementSpans = statements.map { spanOf(text, it) }, + BlockAnchor( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = statements.map { spanOf(text, it) }, + ), ) private fun rewrite( @@ -214,8 +223,10 @@ class ExtractVariableEditTest { // 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)")), + BlockAnchor( + contentSpan = spanOf(text, "\n\t\tprintln(items.size * 2)\n\t"), + statementSpans = listOf(spanOf(text, "println(items.size * 2)")), + ), ) val result = @@ -246,10 +257,12 @@ class ExtractVariableEditTest { val body = spanOf(text, "log(a.b)") val form = AnchorForm.WrapInBraces( - bodyStart = body.start, - bodyEnd = body.end, - indent = "\t", - innerIndent = "\t\t", + BracelessBody( + bodyStart = body.start, + bodyEnd = body.end, + indent = "\t", + innerIndent = "\t\t", + ), ) val result = rewrite(text, candidate, form, listOf(candidate), "b", replaceAll = false)!! @@ -348,7 +361,7 @@ class ExtractVariableEditTest { buildExtractVariableRewrite( fileText = text, candidateSpan = TextSpan(0, 3), - scope = ScopeOption("scope", AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), emptyList()), + scope = ScopeOption("scope", AnchorForm.ExistingBlock(BlockAnchor(TextSpan(9, 9), emptyList())), emptyList()), name = "value", replaceAll = true, ), @@ -365,7 +378,7 @@ class ExtractVariableEditTest { scope = ScopeOption( "scope", - AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), + AnchorForm.ExistingBlock(BlockAnchor(TextSpan(9, 9), emptyList())), listOf(TextSpan(0, text.length + 5)), ), name = "value", @@ -386,8 +399,10 @@ class ExtractVariableEditTest { 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")), + BlockAnchor( + 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)!! @@ -417,12 +432,14 @@ class ExtractVariableEditTest { // 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"), - ), + BlockAnchor( + 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)!! @@ -454,8 +471,10 @@ class ExtractVariableEditTest { val candidate = spanOf(text, "it.length + 1") val form = AnchorForm.ExistingBlock( - contentSpan = spanOf(text, " it.length + 1 "), - statementSpans = listOf(candidate), + BlockAnchor( + contentSpan = spanOf(text, " it.length + 1 "), + statementSpans = listOf(candidate), + ), ) val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! @@ -478,8 +497,10 @@ class ExtractVariableEditTest { // 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), + BlockAnchor( + contentSpan = spanOf(text, " item.length + 1 "), + statementSpans = listOf(candidate), + ), ) val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! @@ -501,8 +522,10 @@ class ExtractVariableEditTest { val candidate = spanOf(text, "n * 2") val form = AnchorForm.ExistingBlock( - contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), - statementSpans = listOf(spanOf(text, "return n * 2")), + BlockAnchor( + 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)!! @@ -522,8 +545,10 @@ class ExtractVariableEditTest { val candidate = spanOf(text, "it + 1") val form = AnchorForm.ExistingBlock( - contentSpan = candidate, - statementSpans = listOf(candidate), + BlockAnchor( + contentSpan = candidate, + statementSpans = listOf(candidate), + ), ) val result = rewrite(text, candidate, form, listOf(candidate), "value", replaceAll = false)!! @@ -545,8 +570,10 @@ class ExtractVariableEditTest { val candidate = spanOf(text, "n * 2") val form = AnchorForm.ExistingBlock( - contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), - statementSpans = listOf(spanOf(text, "return n * 2")), + BlockAnchor( + 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)!! @@ -570,7 +597,7 @@ class ExtractVariableEditTest { BlockPlacement.ExpandOneLine, blockPlacementFor( fileText = text, - form = AnchorForm.ExistingBlock(contentSpan = content, statementSpans = listOf(statement)), + block = BlockAnchor(contentSpan = content, statementSpans = listOf(statement)), firstTarget = statement, ), ) @@ -586,7 +613,7 @@ class ExtractVariableEditTest { BlockPlacement.LineAbove(statement), blockPlacementFor( fileText = text, - form = AnchorForm.ExistingBlock(contentSpan = content, statementSpans = listOf(statement)), + block = BlockAnchor(contentSpan = content, statementSpans = listOf(statement)), firstTarget = statement, ), ) @@ -604,7 +631,7 @@ class ExtractVariableEditTest { BlockPlacement.Refused, blockPlacementFor( fileText = text, - form = AnchorForm.ExistingBlock(contentSpan = content, statementSpans = listOf(first, second)), + block = BlockAnchor(contentSpan = content, statementSpans = listOf(first, second)), firstTarget = first, ), ) @@ -618,7 +645,7 @@ class ExtractVariableEditTest { BlockPlacement.Refused, blockPlacementFor( fileText = text, - form = AnchorForm.ExistingBlock(contentSpan = TextSpan(8, text.length), statementSpans = emptyList()), + block = BlockAnchor(contentSpan = TextSpan(8, text.length), statementSpans = emptyList()), firstTarget = TextSpan(0, 3), ), ) 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 8024a6746c..293b867b7b 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 com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS +import com.itsaky.androidide.lsp.refactor.RewriteSpan +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.ui.NameProblem +import com.itsaky.androidide.lsp.ui.validateVariableName import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtIfExpression @@ -855,7 +860,7 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { } @Test - fun `extracting from a semicolon-joined statement leaves the block multi-line`() { + fun `extracting from a semicolon-joined statement is refused rather than reordered`() { val content = """ package p @@ -866,28 +871,15 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { 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), - ) + /* + * A statement already precedes the candidate on its line, and the block spans several lines, so + * there is nowhere the declaration can go. A new line above the whole line reorders it in front of + * `val x = a + 1`, which the expression reads -- this used to emit exactly that, and it does not + * compile. Expanding is only sound when the whole block is the one line. Declining is the answer, + * and it is what the shared placement in :lsp:refactor-core now gives both languages. + */ + assertTrue(result.isEmpty) } @Test @@ -957,7 +949,7 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { // `val length` lives in another function's lambda: invisible here, so naming this one `length` // is legal and must not be refused. - assertNull(validateVariableName("length", result.candidates.first().takenNames)) + assertNull(validateVariableName("length", result.candidates.first().takenNames, HARD_KEYWORDS)) } @Test @@ -974,8 +966,8 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { val taken = plan(content, content.indexOf("items.size") + 1).candidates.first().takenNames - assertEquals(NameProblem.AlreadyTaken, validateVariableName("items", taken)) - assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", taken)) + assertEquals(NameProblem.AlreadyTaken, validateVariableName("items", taken, HARD_KEYWORDS)) + assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", taken, HARD_KEYWORDS)) } @Test @@ -1001,8 +993,8 @@ class ExtractVariablePlanEndToEndTest : KtLspTest() { // A local `val total` would shadow the member, changing what every other `total` in the block // means, so it stays refused. - assertEquals(NameProblem.AlreadyTaken, validateVariableName("total", taken)) - assertEquals(NameProblem.AlreadyTaken, validateVariableName("demo", taken)) + assertEquals(NameProblem.AlreadyTaken, validateVariableName("total", taken, HARD_KEYWORDS)) + assertEquals(NameProblem.AlreadyTaken, validateVariableName("demo", taken, HARD_KEYWORDS)) } @Test 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 5171c807bf..59e52cb9a8 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,5 +1,13 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor +import com.itsaky.androidide.lsp.kotlin.utils.refactor.HARD_KEYWORDS +import com.itsaky.androidide.lsp.refactor.TextSpan +import com.itsaky.androidide.lsp.refactor.detectIndentUnit +import com.itsaky.androidide.lsp.refactor.excludeUnsoundOccurrences +import com.itsaky.androidide.lsp.refactor.leadingIndentAt +import com.itsaky.androidide.lsp.refactor.lineStartOffset +import com.itsaky.androidide.lsp.ui.NameProblem +import com.itsaky.androidide.lsp.ui.validateVariableName import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull @@ -10,40 +18,40 @@ import org.junit.Test class RefactorPrimitivesTest { @Test fun `rejects blank names`() { - assertEquals(NameProblem.Blank, validateVariableName("", emptySet())) - assertEquals(NameProblem.Blank, validateVariableName(" ", emptySet())) + assertEquals(NameProblem.Blank, validateVariableName("", emptySet(), HARD_KEYWORDS)) + assertEquals(NameProblem.Blank, validateVariableName(" ", emptySet(), HARD_KEYWORDS)) } @Test fun `rejects non-identifiers`() { - assertEquals(NameProblem.NotAnIdentifier, validateVariableName("1size", emptySet())) - assertEquals(NameProblem.NotAnIdentifier, validateVariableName("my size", emptySet())) - assertEquals(NameProblem.NotAnIdentifier, validateVariableName("size!", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("1size", emptySet(), HARD_KEYWORDS)) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("my size", emptySet(), HARD_KEYWORDS)) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("size!", emptySet(), HARD_KEYWORDS)) // Backticked names are legal Kotlin but deliberately unsupported for a generated local. - assertEquals(NameProblem.NotAnIdentifier, validateVariableName("`size`", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("`size`", emptySet(), HARD_KEYWORDS)) } @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())) + assertEquals(NameProblem.Keyword, validateVariableName("val", emptySet(), HARD_KEYWORDS)) + assertEquals(NameProblem.Keyword, validateVariableName("when", emptySet(), HARD_KEYWORDS)) + assertEquals(NameProblem.Keyword, validateVariableName("this", emptySet(), HARD_KEYWORDS)) // `it`, `data` and `by` are soft keywords -- perfectly legal identifiers. - assertNull(validateVariableName("it", emptySet())) - assertNull(validateVariableName("data", emptySet())) - assertNull(validateVariableName("by", emptySet())) + assertNull(validateVariableName("it", emptySet(), HARD_KEYWORDS)) + assertNull(validateVariableName("data", emptySet(), HARD_KEYWORDS)) + assertNull(validateVariableName("by", emptySet(), HARD_KEYWORDS)) } @Test fun `rejects names already in use`() { - assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", setOf("size"))) - assertNull(validateVariableName("size", setOf("count"))) + assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", setOf("size"), HARD_KEYWORDS)) + assertNull(validateVariableName("size", setOf("count"), HARD_KEYWORDS)) } @Test fun `accepts underscores and digits`() { - assertNull(validateVariableName("_size", emptySet())) - assertNull(validateVariableName("size2", emptySet())) + assertNull(validateVariableName("_size", emptySet(), HARD_KEYWORDS)) + assertNull(validateVariableName("size2", emptySet(), HARD_KEYWORDS)) } @Test diff --git a/lsp/refactor-core/build.gradle.kts b/lsp/refactor-core/build.gradle.kts new file mode 100644 index 0000000000..53d3024902 --- /dev/null +++ b/lsp/refactor-core/build.gradle.kts @@ -0,0 +1,40 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +import com.itsaky.androidide.build.config.BuildConfig + +plugins { + id("com.android.library") + id("kotlin-android") +} + +android { + namespace = "${BuildConfig.PACKAGE_NAME}.lsp.refactor" +} + +dependencies { + + // api, not implementation: both language servers build a RewriteSpan here and hand the resulting + // TextEdit to the language client, so the edit model belongs on their compile classpath. + api(projects.lsp.models) + api(projects.shared) + + implementation(libs.common.kotlin) + + testImplementation(libs.tests.junit) + testImplementation(libs.tests.google.truth) +} diff --git a/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/BlockRewrite.kt b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/BlockRewrite.kt new file mode 100644 index 0000000000..b2ad49e596 --- /dev/null +++ b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/BlockRewrite.kt @@ -0,0 +1,245 @@ +package com.itsaky.androidide.lsp.refactor + +/** + * The geometry of a scope that already has a `{ ... }` body, which is where a declaration lands in the + * overwhelming majority of extractions. + * + * [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 from a + * multi-line one, where inserting at the statement's line start would put the declaration outside the + * braces. + * + * Both languages describe a block this way, so the geometry and everything that reasons about it live + * here rather than once per language server. + */ +data class BlockAnchor( + val contentSpan: TextSpan, + val statementSpans: List, +) + +/** A braceless statement position: `if (c) foo();`, a braceless loop body, a single-statement rule. */ +data class BracelessBody( + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, +) + +/** + * Shared by the planner and the rewriter so a rung is never *offered* that the rewrite would refuse -- + * otherwise the sheet opens, the user fills it in, and confirm fails with a generic error. + */ +sealed interface BlockPlacement { + /** The declaration becomes a new line above [anchor], at [anchor]'s indentation. */ + data class LineAbove( + val anchor: TextSpan, + ) : BlockPlacement + + /** The block is written on one line and is expanded, with the declaration inside its braces. */ + data object ExpandOneLine : BlockPlacement + + /** Neither is sound here, so the rung is declined. */ + data object Refused : BlockPlacement +} + +/** The block statement holding [target], or null when the plan and the text disagree. */ +fun anchorOf( + block: BlockAnchor, + target: TextSpan, +): TextSpan? = block.statementSpans.firstOrNull { it.start <= target.start && target.end <= it.end } + +/** + * Refuses two shapes: nothing in the block contains the target (plan and text disagree), or something + * besides indentation precedes the anchor statement while the block's content spans several lines. + * + * The second is the interesting one. `items.forEach(x -> { log(x);\n\tlog(y); })` cannot take a new + * line above `log(x)`, because that line start is before the opening brace -- outside the scope where a + * lambda parameter exists. Neither can `it = src.iterator(); use(it.next());\n\ttail();`, where the + * anchor shares its line with a statement the expression depends on and hoisting above it reorders + * execution. Expanding is sound only when the whole block is that one line, because then re-emitting + * its content loses nothing. + * + * [block]'s spans are substringed against [fileText] unchecked, so callers must pass the text those + * spans were computed against -- the plan's own, never the live document. + */ +fun blockPlacementFor( + fileText: String, + block: BlockAnchor, + firstTarget: TextSpan, +): BlockPlacement { + val anchor = anchorOf(block, firstTarget) ?: return BlockPlacement.Refused + val lineStart = lineStartOffset(fileText, anchor.start) + + val linePrefix = fileText.substring(lineStart, anchor.start) + // Nothing but indentation in front: the declaration can take its own line above, which is the common + // case and the only one where the anchor's line needs no rearranging. + if (linePrefix.isBlank()) return BlockPlacement.LineAbove(anchor) + + val contentIsOneLine = !fileText.substring(block.contentSpan.start, block.contentSpan.end).contains('\n') + return if (contentIsOneLine) BlockPlacement.ExpandOneLine else BlockPlacement.Refused +} + +/** + * A replace-all anchors on the *first* served occurrence, so a leading one whose statement shares the + * opening-brace line would refuse the whole rewrite even though the user's own site is placeable. + * [candidateSpan] is never dropped; only leading sites matter, since a later one never becomes anchor. + * + * [block] is null for a rung that is not a block, where every occurrence is servable. + */ +fun servableOccurrences( + fileText: String, + block: BlockAnchor?, + occurrences: List, + candidateSpan: TextSpan, +): List { + if (block == null) return occurrences + return occurrences.dropWhile { + it != candidateSpan && blockPlacementFor(fileText, block, it) is BlockPlacement.Refused + } +} + +/** + * Restricts [occurrences] to a contiguous run no write to a referenced mutable interrupts, since + * `foo(limit + 1); limit = 5; foo(limit + 1);` is the same expression holding two different values. + * + * Unsound sites are excluded rather than warned about. The walk grows outward from the candidate, never + * dropping the user's own site, and stops in each direction at the first write it would cross. + * + * The guarantee is bounded to **variable writes**, which is all a caller's write scan can see. + * Collapsing repeated evaluations of an effectful expression is left alone deliberately -- it is what + * Extract Variable means, and what every IDE does -- so `foo(items.size()); items.add(x); + * bar(items.size());` does fold to one read. What this rules out is the case where the *same* text + * provably names two different values. + */ +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 +} + +/** + * The anchor is the statement *of this scope* holding the first served occurrence, so picking an outer + * rung hoists the declaration above the enclosing statement. The span ends at the last occurrence, so + * untouched trailing code is left alone. + * + * Null when [blockPlacementFor] refuses the anchor; the caller reports that rather than guessing. + */ +fun existingBlockRewrite( + fileText: String, + block: BlockAnchor, + targets: List, + declaration: String, + name: String, +): RewriteSpan? { + val last = targets.last() + val anchor = + when (val placement = blockPlacementFor(fileText, block, targets.first())) { + is BlockPlacement.Refused -> { + return null + } + + is BlockPlacement.ExpandOneLine -> { + return oneLineBlockRewrite(fileText, block, targets, declaration, name) + } + + is BlockPlacement.LineAbove -> { + placement.anchor + } + } + + val lineStart = lineStartOffset(fileText, anchor.start) + val indent = leadingIndentAt(fileText, anchor.start) + val newline = detectNewline(fileText) + + val span = TextSpan(lineStart, last.end) + val body = replaceOccurrences(fileText, span, targets, name) + return RewriteSpan(span = span, newText = indent + declaration + newline + body) +} + +/** + * Expands a block written on one line. Only the content between the braces is rewritten, so the braces + * and anything before them (a `param ->` header, a `case A ->` label) stay put. + */ +private fun oneLineBlockRewrite( + fileText: String, + block: BlockAnchor, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val content = block.contentSpan + val newline = detectNewline(fileText) + val indent = leadingIndentAt(fileText, content.start) + val innerIndent = indent + detectIndentUnit(fileText) + + // Widen over the whitespace on each side of the content so it does not survive the rewrite as a + // stray "{ " or " }". A block that does not own its braces (a lambda body) stops short of them. + val span = TextSpan(startOfWhitespaceBefore(fileText, content.start), endOfWhitespaceAfter(fileText, content.end)) + + // Anything already in front of the anchor stays in front of it: prepending the declaration to the + // whole block would hoist it above statements the expression depends on. + val anchorStart = anchorOf(block, targets.first())?.start?.coerceIn(content.start, content.end) ?: content.start + val before = fileText.substring(content.start, anchorStart).trim() + val body = replaceOccurrences(fileText, TextSpan(anchorStart, content.end), targets, name).trim() + + val newText = + buildString { + append(newline) + if (before.isNotEmpty()) append(innerIndent).append(before).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. */ +fun wrapInBracesRewrite( + fileText: String, + body: BracelessBody, + 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(body.bodyStart, body.bodyEnd) + val newline = detectNewline(fileText) + val statement = replaceOccurrences(fileText, span, targets, name) + + val newText = + buildString { + append('{').append(newline) + append(body.innerIndent).append(declaration).append(newline) + append(body.innerIndent).append(statement).append(newline) + append(body.indent).append('}') + } + return RewriteSpan(span, newText) +} diff --git a/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/NamePrimitives.kt b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/NamePrimitives.kt new file mode 100644 index 0000000000..b600f771cd --- /dev/null +++ b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/NamePrimitives.kt @@ -0,0 +1,50 @@ +package com.itsaky.androidide.lsp.refactor + +// The language-independent half of name suggestion: what a name looks like, not what the language +// allows. Picking a name from an expression's *shape* stays per-language, since only a language server +// knows what a call or a member access is. + +/** `getFoo` -> `foo`, `isReady` -> `ready`. Leaves anything else alone. */ +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`, `java.time.Duration` -> `duration`, `String[]` -> `string`, `Foo?` -> `foo`. + * + * Both languages' spellings are handled in one pass, which is what let their two copies of this differ + * unnoticed: javac renders an array as `String[]` and never appends a nullability marker, Kotlin renders + * `Foo?` / `Foo!` and never uses brackets, so stripping all of them is a no-op for whichever language + * did not produce them. + */ +fun nameFromType(typeName: String): String? = + typeName + .substringBefore('<') + .removeSuffix("[]") + .substringAfterLast('.') + .trimEnd('[', ']', '?', '!') + .takeIf { it.isNotBlank() } + ?.decapitaliseFirst() + +fun String.decapitaliseFirst(): String = if (isEmpty()) this else this[0].lowercaseChar() + substring(1) + +/** `size` -> `size1` -> `size2` until nothing in [takenNames] matches. */ +fun uniqueName( + 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/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/RewriteSpan.kt b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/RewriteSpan.kt new file mode 100644 index 0000000000..a9bd571889 --- /dev/null +++ b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/RewriteSpan.kt @@ -0,0 +1,44 @@ +package com.itsaky.androidide.lsp.refactor + +import com.itsaky.androidide.lsp.models.TextEdit +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.models.Range + +/** + * Deliberately a single replacement, not a list of edits: `IDELanguageClientImpl.applyActionEdits` runs + * each `TextEdit` in its own `runOnUiThread` with no `beginBatchEdit`, against the *original* offsets -- + * so N edits would land on positions already shifted by their predecessors, and cost N undo steps. + */ +data class RewriteSpan( + val span: TextSpan, + val newText: String, +) + +/** All three of [Position]'s fields are filled, so no consumer of either path sees a stale value. */ +fun RewriteSpan.toTextEdit(fileText: String): TextEdit = + TextEdit( + range = + Range( + positionAt(fileText, span.start), + positionAt(fileText, span.end), + ), + newText = newText, + ) + +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/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/SourceText.kt b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/SourceText.kt new file mode 100644 index 0000000000..32d298eaec --- /dev/null +++ b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/SourceText.kt @@ -0,0 +1,85 @@ +package com.itsaky.androidide.lsp.refactor + +// The offset arithmetic every extract refactoring needs, with no notion of a language in it. These were +// duplicated per language server until ADFA-5047; none of them touches a javac `Tree` or a +// `KtExpression`, and each fix used to have to land twice -- `detectIndentUnit` had already drifted. + +/** Offset of the start of the line containing [offset]. */ +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. */ +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, + * else 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. + */ +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 trimmed = line.trimStart() + // A block-comment continuation (` * text`, ` */`) is alignment, not indentation, and its single + // leading space would otherwise win the minimum on virtually every real file. + if (trimmed.startsWith('*')) continue + val spaces = line.length - trimmed.length + // A one-space indent unit is not a real style, so it can only be a line this scan misread. + if (spaces in 2 until minSpaces) minSpaces = spaces + } + return if (minSpaces == Int.MAX_VALUE) "\t" else " ".repeat(minSpaces) +} + +/** CRLF only when the file already uses it, so the edit does not mix line endings. */ +fun detectNewline(text: String): String = if (text.contains("\r\n")) "\r\n" else "\n" + +/** The offset where the run of whitespace ending at [offset] begins. */ +fun startOfWhitespaceBefore( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index > 0 && text[index - 1].isWhitespace()) index-- + return index +} + +/** The offset where the run of whitespace starting at [offset] ends. */ +fun endOfWhitespaceAfter( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index < text.length && text[index].isWhitespace()) index++ + return index +} + +/** + * Substitutes [name] for every one of [targets] inside [span], right-to-left so an earlier replacement + * cannot invalidate a later offset. Targets outside [span] are ignored. + */ +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() +} diff --git a/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/TextSpan.kt b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/TextSpan.kt new file mode 100644 index 0000000000..19d4d371da --- /dev/null +++ b/lsp/refactor-core/src/main/java/com/itsaky/androidide/lsp/refactor/TextSpan.kt @@ -0,0 +1,21 @@ +package com.itsaky.androidide.lsp.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 many candidate expressions are ever offered. Keeps the chooser scannable on a phone. */ +const val MAX_CANDIDATES = 3 + +/** Used when neither the expression's shape nor its type suggests anything better. */ +const val FALLBACK_NAME = "value" diff --git a/lsp/refactor-core/src/test/java/com/itsaky/androidide/lsp/refactor/RefactorCoreTest.kt b/lsp/refactor-core/src/test/java/com/itsaky/androidide/lsp/refactor/RefactorCoreTest.kt new file mode 100644 index 0000000000..489bacd39f --- /dev/null +++ b/lsp/refactor-core/src/test/java/com/itsaky/androidide/lsp/refactor/RefactorCoreTest.kt @@ -0,0 +1,256 @@ +package com.itsaky.androidide.lsp.refactor + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * The language-agnostic half of extract-variable, tested once instead of once per language server. + * + * The rewrite functions produce the text written into the user's file, so they are asserted on their + * emitted source rather than on a [RewriteSpan], which is where an off-by-one hides. + */ +@RunWith(JUnit4::class) +class RefactorCoreTest { + @Test + fun `spans overlap only when they share a character`() { + assertThat(TextSpan(0, 5).overlaps(TextSpan(4, 8))).isTrue() + assertThat(TextSpan(0, 5).overlaps(TextSpan(5, 8))).isFalse() + assertThat(TextSpan(3, 3).overlaps(TextSpan(3, 3))).isFalse() + assertThat(TextSpan(2, 9).overlaps(TextSpan(4, 5))).isTrue() + } + + @Test + fun `a span cannot end before it starts`() { + runCatching { TextSpan(5, 2) }.let { assertThat(it.isFailure).isTrue() } + } + + @Test + fun `an indented anchor on its own line takes the line above`() { + val text = "{\n\tfoo(a + b);\n}" + val block = BlockAnchor(contentSpan = TextSpan(1, 15), statementSpans = listOf(TextSpan(3, 14))) + assertThat(blockPlacementFor(text, block, TextSpan(7, 12))) + .isInstanceOf(BlockPlacement.LineAbove::class.java) + } + + @Test + fun `a one-line block is expanded rather than refused`() { + val text = "{ foo(a + b); }" + val block = BlockAnchor(contentSpan = TextSpan(1, 14), statementSpans = listOf(TextSpan(2, 13))) + assertThat(blockPlacementFor(text, block, TextSpan(6, 11))).isEqualTo(BlockPlacement.ExpandOneLine) + } + + @Test + fun `an anchor sharing a line inside a multi-line block is refused`() { + // Threading a declaration into a line that also holds unrelated statements would reorder them, and + // hoisting it above the line can land outside the block. Both languages used to reorder here. + val text = "{\n\tbar(); foo(a + b);\n\ttail();\n}" + val block = + BlockAnchor( + contentSpan = TextSpan(1, 30), + statementSpans = listOf(TextSpan(3, 9), TextSpan(10, 21)), + ) + assertThat(blockPlacementFor(text, block, TextSpan(14, 19))).isEqualTo(BlockPlacement.Refused) + } + + @Test + fun `a target no statement contains is refused`() { + val block = BlockAnchor(contentSpan = TextSpan(1, 10), statementSpans = emptyList()) + assertThat(blockPlacementFor("{ foo(); }", block, TextSpan(2, 8))).isEqualTo(BlockPlacement.Refused) + } + + @Test + fun `occurrences stop at the first write in each direction`() { + val occurrences = listOf(TextSpan(0, 5), TextSpan(20, 25), TextSpan(40, 45), TextSpan(60, 65)) + val sound = excludeUnsoundOccurrences(occurrences, candidateSpan = TextSpan(20, 25), writeOffsets = listOf(50)) + assertThat(sound).containsExactly(TextSpan(0, 5), TextSpan(20, 25), TextSpan(40, 45)).inOrder() + } + + @Test + fun `a write before the candidate drops the earlier occurrences`() { + val occurrences = listOf(TextSpan(0, 5), TextSpan(20, 25), TextSpan(40, 45)) + val sound = excludeUnsoundOccurrences(occurrences, candidateSpan = TextSpan(20, 25), writeOffsets = listOf(10)) + assertThat(sound).containsExactly(TextSpan(20, 25), TextSpan(40, 45)).inOrder() + } + + @Test + fun `the candidate is never dropped even when it is not in the list`() { + val sound = excludeUnsoundOccurrences(listOf(TextSpan(0, 5)), TextSpan(90, 95), writeOffsets = emptyList()) + assertThat(sound).containsExactly(TextSpan(90, 95)) + } + + @Test + fun `leading unplaceable occurrences are dropped and the candidate survives`() { + // The leading site shares the opening-brace line, so anchoring a replace-all there would refuse + // the whole rewrite; dropping it keeps the count achievable. + val text = "{ foo(a + b);\n\tbar(a + b);\n}" + val block = + BlockAnchor( + contentSpan = TextSpan(1, 26), + statementSpans = listOf(TextSpan(2, 13), TextSpan(15, 26)), + ) + val candidate = TextSpan(20, 25) + assertThat(servableOccurrences(text, block, listOf(TextSpan(6, 11), candidate), candidate)) + .containsExactly(candidate) + } + + @Test + fun `a rung that is not a block serves every occurrence it was given`() { + val occurrences = listOf(TextSpan(0, 2), TextSpan(4, 6)) + assertThat(servableOccurrences("foo(a + b)", null, occurrences, TextSpan(4, 6))).isEqualTo(occurrences) + } + + @Test + fun `a tab anywhere makes the indent unit a tab`() { + assertThat(detectIndentUnit("class A {\n\tint a;\n}")).isEqualTo("\t") + } + + @Test + fun `the smallest real run of spaces is the indent unit`() { + assertThat(detectIndentUnit("class A {\n int a;\n int b;\n}")).isEqualTo(" ") + } + + @Test + fun `a block comment continuation does not win the indent unit`() { + // ` * text` is alignment, not indentation. The Kotlin copy of this had no such guard, so a single + // leading space beat every real indent on virtually any documented file. + assertThat(detectIndentUnit("/**\n * doc\n */\nclass A {\n int a;\n}")).isEqualTo(" ") + } + + @Test + fun `a file with no indentation at all falls back to a tab`() { + assertThat(detectIndentUnit("class A {\nint a;\n}")).isEqualTo("\t") + } + + @Test + fun `CRLF is only emitted for a file that already uses it`() { + assertThat(detectNewline("a\r\nb")).isEqualTo("\r\n") + assertThat(detectNewline("a\nb")).isEqualTo("\n") + assertThat(detectNewline("a")).isEqualTo("\n") + } + + @Test + fun `a line start is found from anywhere on the line`() { + val text = "one\ntwo\nthree" + assertThat(lineStartOffset(text, 0)).isEqualTo(0) + assertThat(lineStartOffset(text, 5)).isEqualTo(4) + assertThat(lineStartOffset(text, 8)).isEqualTo(8) + } + + @Test + fun `leading indent stops at the first non-blank`() { + assertThat(leadingIndentAt("a\n\t\t foo();", 8)).isEqualTo("\t\t ") + assertThat(leadingIndentAt("foo();", 3)).isEmpty() + } + + @Test + fun `whitespace runs are widened over from either side`() { + assertThat(startOfWhitespaceBefore("a \tb", 4)).isEqualTo(1) + assertThat(endOfWhitespaceAfter("a \tb", 1)).isEqualTo(4) + } + + @Test + fun `a position carries line, column and index`() { + val position = positionAt("one\ntwo\nthree", 9) + assertThat(position.line).isEqualTo(2) + assertThat(position.column).isEqualTo(1) + assertThat(position.index).isEqualTo(9) + } + + @Test + fun `a position past the end clamps to the end`() { + assertThat(positionAt("ab", 99).index).isEqualTo(2) + } + + @Test + fun `occurrences are replaced right-to-left and outside targets ignored`() { + val text = "foo(a + b) + bar(a + b)" + val replaced = replaceOccurrences(text, TextSpan(0, text.length), listOf(TextSpan(4, 9), TextSpan(17, 22)), "v") + assertThat(replaced).isEqualTo("foo(v) + bar(v)") + } + + @Test + fun `an existing block gains the declaration on the line above the anchor`() { + val text = "void m() {\n\tfoo(a + b);\n}" + val block = BlockAnchor(contentSpan = TextSpan(10, 24), statementSpans = listOf(TextSpan(12, 23))) + val rewrite = existingBlockRewrite(text, block, listOf(TextSpan(16, 21)), "int v = a + b;", "v")!! + assertThat(applied(text, rewrite)).isEqualTo("void m() {\n\tint v = a + b;\n\tfoo(v);\n}") + } + + @Test + fun `a replace-all rewrites every target in one edit`() { + val text = "void m() {\n\tfoo(a + b);\n\tbar(a + b);\n}" + val block = + BlockAnchor( + contentSpan = TextSpan(10, 37), + statementSpans = listOf(TextSpan(12, 23), TextSpan(25, 36)), + ) + val targets = listOf(TextSpan(16, 21), TextSpan(29, 34)) + val rewrite = existingBlockRewrite(text, block, targets, "int v = a + b;", "v")!! + assertThat(applied(text, rewrite)).isEqualTo("void m() {\n\tint v = a + b;\n\tfoo(v);\n\tbar(v);\n}") + } + + @Test + fun `a refused anchor produces no rewrite`() { + val block = BlockAnchor(contentSpan = TextSpan(1, 10), statementSpans = emptyList()) + assertThat(existingBlockRewrite("{ foo(); }", block, listOf(TextSpan(2, 8)), "int v = 1;", "v")).isNull() + } + + @Test + fun `expanding a one-line block keeps what precedes the anchor in front of it`() { + // Prepending the declaration to the whole block would hoist it above `int a = 1;`, which the + // expression depends on. Both languages did exactly that before this moved here. + val text = "void m() { int a = 1; foo(a + 2); }" + val block = + BlockAnchor( + contentSpan = TextSpan(11, 33), + statementSpans = listOf(TextSpan(11, 21), TextSpan(22, 33)), + ) + val rewrite = existingBlockRewrite(text, block, listOf(TextSpan(26, 31)), "int v = a + 2;", "v")!! + val out = applied(text, rewrite) + assertThat(out.indexOf("int a = 1")).isLessThan(out.indexOf("int v =")) + } + + @Test + fun `a braceless body is wrapped in braces around the declaration`() { + val text = "if (c)\n\tfoo(a + b);" + val body = BracelessBody(bodyStart = 8, bodyEnd = 19, indent = "", innerIndent = "\t") + val rewrite = wrapInBracesRewrite(text, body, listOf(TextSpan(12, 17)), "int v = a + b;", "v") + assertThat(applied(text, rewrite)).isEqualTo("if (c)\n\t{\n\tint v = a + b;\n\tfoo(v);\n}") + } + + @Test + fun `an accessor prefix is stripped only in front of a capital`() { + assertThat(stripAccessorPrefix("getFoo")).isEqualTo("foo") + assertThat(stripAccessorPrefix("isReady")).isEqualTo("ready") + assertThat(stripAccessorPrefix("hasNext")).isEqualTo("next") + assertThat(stripAccessorPrefix("getter")).isEqualTo("getter") + assertThat(stripAccessorPrefix("is")).isEqualTo("is") + } + + @Test + fun `a name from a type handles both languages' spellings`() { + // The two copies of this had drifted: Java's stripped `[]` and Kotlin's stripped `?`/`!`, so each + // mishandled the other's. Neither language produces the other's spelling, so one pass covers both. + assertThat(nameFromType("java.util.List")).isEqualTo("list") + assertThat(nameFromType("java.time.Duration")).isEqualTo("duration") + assertThat(nameFromType("String[]")).isEqualTo("string") + assertThat(nameFromType("kotlin.time.Duration?")).isEqualTo("duration") + assertThat(nameFromType("Foo!")).isEqualTo("foo") + assertThat(nameFromType("int")).isEqualTo("int") + assertThat(nameFromType(" ")).isNull() + } + + @Test + fun `a taken name gains the first free suffix`() { + assertThat(uniqueName("size", emptySet())).isEqualTo("size") + assertThat(uniqueName("size", setOf("size"))).isEqualTo("size1") + assertThat(uniqueName("size", setOf("size", "size1", "size2"))).isEqualTo("size3") + } + + private fun applied( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) +} diff --git a/lsp/ui/build.gradle.kts b/lsp/ui/build.gradle.kts new file mode 100644 index 0000000000..6268a03f8b --- /dev/null +++ b/lsp/ui/build.gradle.kts @@ -0,0 +1,54 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +import com.itsaky.androidide.build.config.BuildConfig + +plugins { + id("com.android.library") + id("kotlin-android") + alias(libs.plugins.kotlin.compose) +} + +android { + namespace = "${BuildConfig.PACKAGE_NAME}.lsp.ui" + + buildFeatures { + compose = true + } +} + +dependencies { + + // api, not implementation: a language server's action calls show(activity, ...) and receives a + // selection, so it needs FragmentActivity and the contract types on its own compile classpath. + api(libs.androidx.fragment.ktx) + api(projects.resources) + + 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.androidx.lifecycle.runtime.ktx) + implementation(libs.androidx.lifecycle.viewmodel.ktx) + implementation(libs.androidx.lifecycle.runtime.compose) + implementation(libs.google.material) + + testImplementation(libs.tests.junit) + testImplementation(libs.tests.google.truth) +} diff --git a/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableContract.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableContract.kt new file mode 100644 index 0000000000..db1b1993c4 --- /dev/null +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableContract.kt @@ -0,0 +1,73 @@ +package com.itsaky.androidide.lsp.ui + +/** + * What the extract-variable sheet needs to know about one candidate expression. + * + * Deliberately a *view* of a language's plan rather than the plan itself: strings, counts and index + * positions only. That is what lets one sheet serve both language servers without either of them + * depending on the other, and without this module knowing what a `KtExpression` or an `ExpressionTree` + * is. Each caller maps its own plan into these and maps an [ExtractVariableSelection] back out. + * + * [takenNames] is what a new declaration at this candidate would collide with or shadow, used to + * reject a typed name. + * + * [scopes] is the legal scope chain, innermost first, and is never empty -- a candidate with no legal + * anchor is not offered. + */ +data class CandidateView( + val label: String, + val suggestedName: String, + val takenNames: Set, + val scopes: List, +) { + init { + require(scopes.isNotEmpty()) { "candidate '$label' has no scopes" } + } +} + +/** + * One place the declaration may go. + * + * [occurrenceCount] counts every site this scope would rewrite, **including** the one the user + * selected, so "Replace all 3 occurrences" means three sites in total. + */ +data class ScopeView( + val label: String, + val occurrenceCount: Int, +) + +/** + * The user's finished decision. + * + * Positional rather than resolved: the caller knows which candidate and scope these indices name, and + * turning them back into offsets and an edit is its job. Keeping the sheet free of offsets is what + * makes it a pure chooser. + */ +data class ExtractVariableSelection( + val candidateIndex: Int, + val scopeIndex: Int, + val name: String, + val replaceAll: Boolean, +) + +/** + * The four name-problem strings, supplied per language. + * + * Two of them name the language ("Not a valid Java name"), so a shared res-id lookup would put + * Kotlin's wording in front of a Java user. Passing the ids keeps this module language-agnostic + * without genericising the copy into something less useful. + */ +data class NameMessages( + val blank: Int, + val invalid: Int, + val keyword: Int, + val taken: Int, +) { + fun resFor(problem: NameProblem): Int = + when (problem) { + NameProblem.Blank -> blank + NameProblem.NotAnIdentifier -> invalid + NameProblem.Keyword -> keyword + NameProblem.AlreadyTaken -> taken + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheet.kt similarity index 59% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheet.kt index 17ffdf7dba..43c58b45c6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheet.kt @@ -1,4 +1,4 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.ui import android.content.Context import android.content.ContextWrapper @@ -14,22 +14,27 @@ 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]. + * Hosts [ExtractVariableSheetContent], for whichever language server showed it. * - * 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. + * The candidates are handed in directly rather than through fragment arguments: they are a view of an + * analysis result whose offsets refer to one snapshot of one document, which is neither `Parcelable` + * nor meaningful to restore -- after process death the document may be entirely different. So + * [candidates] is null on a recreated instance and the sheet dismisses itself, which is the same + * outcome the caller's document-version guard would reach anyway. */ class ExtractVariableSheet : BottomSheetDialogFragment() { - private var plan: ExtractionPlan? = null - private var onChoice: ((ExtractionChoice) -> Unit)? = null + private var candidates: List? = null + private var keywords: Set = emptySet() + private var nameMessages: NameMessages? = null + private var onSelected: ((ExtractVariableSelection) -> Unit)? = null private val viewModel: ExtractVariableViewModel by viewModels { - ExtractVariableViewModel.factory(requireNotNull(plan) { "sheet shown without a plan" }) + ExtractVariableViewModel.factory( + requireNotNull(candidates) { "sheet shown without candidates" }, + keywords, + ) } override fun onCreateView( @@ -37,7 +42,8 @@ class ExtractVariableSheet : BottomSheetDialogFragment() { container: ViewGroup?, savedInstanceState: Bundle?, ): View? { - if (plan == null) { + val messages = nameMessages + if (candidates == null || messages == null) { dismissAllowingStateLoss() return null } @@ -50,6 +56,7 @@ class ExtractVariableSheet : BottomSheetDialogFragment() { val state by viewModel.uiState.collectAsStateWithLifecycle() ExtractVariableSheetContent( state = state, + nameMessages = messages, onEvent = ::handleEvent, ) } @@ -60,7 +67,7 @@ class ExtractVariableSheet : BottomSheetDialogFragment() { private fun handleEvent(event: ExtractVariableUiEvent) { when (event) { ExtractVariableUiEvent.Confirmed -> { - viewModel.choice()?.let { choice -> onChoice?.invoke(choice) } + viewModel.selection()?.let { selection -> onSelected?.invoke(selection) } dismiss() } @@ -78,22 +85,27 @@ class ExtractVariableSheet : BottomSheetDialogFragment() { private const val TAG = "extract_variable_sheet" /** - * Shows the sheet on [activity], calling [onChoice] once if the user confirms. + * Shows the sheet on [activity], calling [onSelected] 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. + * [keywords] is the language's reserved-word set and [nameMessages] its name-problem strings, so + * a Java user is never shown Kotlin's wording. 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, + candidates: List, + keywords: Set, + nameMessages: NameMessages, + onSelected: (ExtractVariableSelection) -> Unit, ): Boolean { val manager = activity.supportFragmentManager if (manager.isStateSaved || manager.isDestroyed) return false ExtractVariableSheet() .apply { - this.plan = plan - this.onChoice = onChoice + this.candidates = candidates + this.keywords = keywords + this.nameMessages = nameMessages + this.onSelected = onSelected }.show(manager, TAG) return true } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheetContent.kt similarity index 95% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheetContent.kt index 5d193ac223..a244a2deff 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableSheetContent.kt @@ -1,4 +1,4 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.ui import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column @@ -34,6 +34,7 @@ import com.itsaky.androidide.resources.R @Composable fun ExtractVariableSheetContent( state: ExtractVariableUiState, + nameMessages: NameMessages, onEvent: (ExtractVariableUiEvent) -> Unit, modifier: Modifier = Modifier, ) { @@ -67,7 +68,10 @@ fun ExtractVariableSheetContent( label = { Text(stringResource(R.string.label_extract_variable_name)) }, isError = state.nameProblem != null, singleLine = true, - supportingText = state.nameProblem?.let { problem -> { Text(stringResource(problem.messageRes())) } }, + supportingText = + state.nameProblem?.let { problem -> + { Text(stringResource(nameMessages.resFor(problem))) } + }, modifier = Modifier.fillMaxWidth(), ) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableUiState.kt similarity index 56% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableUiState.kt index 7a54322405..f5100663fa 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableUiState.kt @@ -1,16 +1,12 @@ -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 +package com.itsaky.androidide.lsp.ui /** - * Everything the extract-variable sheet renders, derived entirely from the - * [com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan]. + * Everything the extract-variable sheet renders, derived entirely from the [CandidateView]s it was + * shown with. * - * [showCandidatePicker] is false only when the plan holds a single candidate. It stays visible for an - * exact selection: long-press is the natural gesture and selects exactly one token, so hiding the list - * there leaves no way to widen to an enclosing expression short of cancelling and re-selecting. + * [showCandidatePicker] is false only when there is a single candidate. It stays visible for an exact + * selection: long-press is the natural gesture and selects exactly one token, so hiding the list there + * leaves no way to widen to an enclosing expression short of cancelling and re-selecting. * * [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 @@ -56,16 +52,3 @@ sealed interface 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/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModel.kt similarity index 65% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModel.kt index d646c21d5c..6ac01e095c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModel.kt @@ -1,26 +1,26 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.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. + * Derives the sheet's state from the [CandidateView]s it was given 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. + * Each candidate already carries its scope chain and per-scope occurrence count, so switching + * expression or scope is pure recomputation -- no analysis, no syntax tree, no I/O. That is what lets + * this class hold all the sheet's logic while remaining a plain unit test, and what lets both language + * servers share it without either depending on the other. * * 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, + * through Koin): this one is sheet-scoped, injects nothing, and takes its inputs as runtime arguments, * so a Koin definition would add indirection without providing anything. */ class ExtractVariableViewModel( - private val plan: ExtractionPlan, + private val candidates: List, + private val keywords: Set, ) : ViewModel() { private val _uiState = MutableStateFlow(initialState()) val uiState: StateFlow = _uiState.asStateFlow() @@ -39,15 +39,19 @@ class ExtractVariableViewModel( is ExtractVariableUiEvent.ScopeSelected -> { if (event.index == current.selectedScope) return - _uiState.value = - stateFor(current.selectedCandidate, event.index, current.replaceAll, current.name) + _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), + nameProblem = + validateVariableName( + event.name, + candidate(current.selectedCandidate).takenNames, + keywords, + ), ) } @@ -62,22 +66,20 @@ class ExtractVariableViewModel( } /** The user's decision, or null when the name is unusable. */ - fun choice(): ExtractionChoice? { + fun selection(): ExtractVariableSelection? { 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, + return ExtractVariableSelection( + candidateIndex = state.selectedCandidate, + scopeIndex = state.selectedScope, 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. + // stale `true` from a previous candidate cannot leak into the selection. replaceAll = state.replaceAll && state.occurrenceCount > 1, ) } - private fun candidate(index: Int) = plan.candidates[index.coerceIn(plan.candidates.indices)] + private fun candidate(index: Int) = candidates[index.coerceIn(candidates.indices)] /** * Recomputes the whole state for a (candidate, scope) pair. [name] carries the user's typed name @@ -93,26 +95,28 @@ class ExtractVariableViewModel( 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, + candidateLabels = candidates.map { it.label }, + selectedCandidate = candidateIndex.coerceIn(candidates.indices), + showCandidatePicker = candidates.size > 1, name = resolvedName, - nameProblem = validateVariableName(resolvedName, candidate.takenNames), + nameProblem = validateVariableName(resolvedName, candidate.takenNames, keywords), scopeLabels = candidate.scopes.map { it.label }, selectedScope = boundedScope, - occurrenceCount = occurrenceCount, - replaceAll = replaceAll && occurrenceCount > 1, + occurrenceCount = scope.occurrenceCount, + replaceAll = replaceAll && scope.occurrenceCount > 1, ) } companion object { - fun factory(plan: ExtractionPlan): ViewModelProvider.Factory = + fun factory( + candidates: List, + keywords: Set, + ): ViewModelProvider.Factory = object : ViewModelProvider.Factory { @Suppress("UNCHECKED_CAST") - override fun create(modelClass: Class): T = ExtractVariableViewModel(plan) as T + override fun create(modelClass: Class): T = ExtractVariableViewModel(candidates, keywords) as T } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/SheetComponents.kt similarity index 68% rename from lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt rename to lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/SheetComponents.kt index 6a746e4634..73dfff3dd5 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/SheetComponents.kt @@ -1,4 +1,4 @@ -package com.itsaky.androidide.lsp.kotlin.refactor.ui +package com.itsaky.androidide.lsp.ui import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column @@ -16,16 +16,18 @@ import androidx.compose.ui.Modifier 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 -/** Shared by the extract-variable and extract-method sheets; neither owns them. */ +/** Shared by every refactoring sheet in either language; none of them owns these. */ @Composable -internal fun LabelledSection( +fun LabelledSection( label: String, + modifier: Modifier = Modifier, content: @Composable () -> Unit, ) { - Column(verticalArrangement = Arrangement.spacedBy(4.dp)) { + Column( + modifier = modifier, + verticalArrangement = Arrangement.spacedBy(4.dp), + ) { Text(text = label, style = MaterialTheme.typography.labelLarge) content() } @@ -33,14 +35,15 @@ internal fun LabelledSection( /** A radio group. Expression text is monospaced so a candidate reads as the code it is. */ @Composable -internal fun OptionList( +fun OptionList( options: List, selected: Int, monospace: Boolean, onSelect: (Int) -> Unit, + modifier: Modifier = Modifier, ) { Column( - modifier = Modifier.selectableGroup(), + modifier = modifier.selectableGroup(), verticalArrangement = Arrangement.spacedBy(8.dp), ) { options.forEachIndexed { index, option -> @@ -74,12 +77,3 @@ internal fun OptionList( } } } - -/** The message shown under a 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/ui/src/main/java/com/itsaky/androidide/lsp/ui/VariableName.kt b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/VariableName.kt new file mode 100644 index 0000000000..dc36e96e03 --- /dev/null +++ b/lsp/ui/src/main/java/com/itsaky/androidide/lsp/ui/VariableName.kt @@ -0,0 +1,44 @@ +package com.itsaky.androidide.lsp.ui + +/** 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 the language's identifier rules and the names already + * visible at the anchor point. Returns null when the name is usable. + * + * Lives here rather than in a language module because the sheet's Extract button is gated on it and + * its result selects the message shown under the text field. [keywords] is the set the language + * rejects outright: both callers pass only the words that are never legal identifiers -- Kotlin's hard + * keywords, Java's reserved words -- since a language's soft or restricted keywords are legal names. + */ +fun validateVariableName( + name: String, + takenNames: Set, + keywords: Set, +): NameProblem? { + if (name.isBlank()) return NameProblem.Blank + if (!isIdentifier(name)) return NameProblem.NotAnIdentifier + if (name in keywords) return NameProblem.Keyword + if (name in takenNames) return NameProblem.AlreadyTaken + return null +} + +/** + * The identifier shape both languages share: a letter or underscore, then letters, digits and + * underscores. + * + * Deliberately the intersection rather than either language's full grammar. A generated local has no + * reason to need more, and it is what rejects Kotlin's backtick-quoted names -- legal Kotlin, but a + * poor generated name, and accepting them would mean validating the quoted form too. + */ +fun isIdentifier(name: String): Boolean { + if (name.isEmpty()) return false + if (!(name[0].isLetter() || name[0] == '_')) return false + return name.all { it.isLetterOrDigit() || it == '_' } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt b/lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModelTest.kt similarity index 64% rename from lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt rename to lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModelTest.kt index 2712342ca7..32c1a936b5 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt +++ b/lsp/ui/src/test/java/com/itsaky/androidide/lsp/ui/ExtractVariableViewModelTest.kt @@ -1,11 +1,5 @@ -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 +package com.itsaky.androidide.lsp.ui + import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull @@ -16,38 +10,29 @@ 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. + * Every choice the sheet offers is recomputed from the candidate views it was given, so all of this is + * exercisable as plain state transitions -- which is the point of the sheet taking a view of a plan + * rather than the plan itself. */ class ExtractVariableViewModelTest { private fun scope( label: String, occurrences: Int, - ) = ScopeOption( - label = label, - anchorForm = AnchorForm.ExistingBlock(contentSpan = TextSpan(0, 100), statementSpans = emptyList()), - occurrences = (0 until occurrences).map { TextSpan(it * 10, it * 10 + 5) }, - ) + ) = ScopeView(label = label, occurrenceCount = occurrences) private fun candidate( label: String, suggestedName: String, - scopes: List, + scopes: List, takenNames: Set = emptySet(), - ) = CandidateExpression( + ) = CandidateView( label = label, - span = TextSpan(0, 5), suggestedName = suggestedName, takenNames = takenNames, scopes = scopes, ) - private fun plan(candidates: List) = - ExtractionPlan( - fileText = "unused", - documentVersion = 1, - candidates = candidates, - ) + private fun plan(candidates: List) = candidates private val threeCandidatePlan = plan( @@ -60,7 +45,7 @@ class ExtractVariableViewModelTest { @Test fun `starts on the innermost candidate, innermost scope, replace-all off`() { - val state = ExtractVariableViewModel(threeCandidatePlan).uiState.value + val state = viewModelFor(threeCandidatePlan).uiState.value assertEquals(0, state.selectedCandidate) assertEquals(0, state.selectedScope) @@ -71,15 +56,15 @@ class ExtractVariableViewModelTest { @Test fun `shows the candidate picker only when there is a real choice`() { - assertTrue(ExtractVariableViewModel(threeCandidatePlan).uiState.value.showCandidatePicker) + assertTrue(viewModelFor(threeCandidatePlan).uiState.value.showCandidatePicker) val single = plan(listOf(candidate("items.size", "size", listOf(scope("fun demo", 1))))) - assertFalse(ExtractVariableViewModel(single).uiState.value.showCandidatePicker) + assertFalse(viewModelFor(single).uiState.value.showCandidatePicker) } @Test fun `changing the expression re-derives name, scopes and count`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) viewModel.onEvent(ExtractVariableUiEvent.CandidateSelected(1)) val state = viewModel.uiState.value @@ -92,7 +77,7 @@ class ExtractVariableViewModelTest { @Test fun `changing the scope changes the occurrence count`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) assertEquals(1, viewModel.uiState.value.occurrenceCount) viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) @@ -103,7 +88,7 @@ class ExtractVariableViewModelTest { @Test fun `a scope change keeps the name the user typed`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) viewModel.onEvent(ExtractVariableUiEvent.NameChanged("mySize")) viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) @@ -113,7 +98,7 @@ class ExtractVariableViewModelTest { @Test fun `the replace-all toggle is hidden at a single occurrence`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) assertFalse(viewModel.uiState.value.showReplaceAll) viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) @@ -123,20 +108,20 @@ class ExtractVariableViewModelTest { @Test fun `an invalid name blocks confirming`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) viewModel.onEvent(ExtractVariableUiEvent.NameChanged("val")) assertEquals(NameProblem.Keyword, viewModel.uiState.value.nameProblem) assertFalse(viewModel.uiState.value.canConfirm) - assertNull(viewModel.choice()) + assertNull(viewModel.selection()) } @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) + val viewModel = viewModelFor(colliding) viewModel.onEvent(ExtractVariableUiEvent.NameChanged("size")) @@ -144,23 +129,24 @@ class ExtractVariableViewModelTest { } @Test - fun `the choice carries the selected expression, scope, name and toggle`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + fun `the selection carries the chosen expression, scope, name and toggle`() { + val viewModel = viewModelFor(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) + val selection = viewModel.selection() + assertNotNull(selection) + // Indices, not resolved objects: mapping them back to a candidate is the caller's job. + assertEquals(0, selection!!.candidateIndex) + assertEquals(1, selection.scopeIndex) + assertEquals("total", selection.name) + assertTrue(selection.replaceAll) } @Test fun `replace-all cannot leak from a wider scope into a single-occurrence one`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) assertTrue(viewModel.uiState.value.replaceAll) @@ -169,12 +155,12 @@ class ExtractVariableViewModelTest { viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(0)) assertFalse(viewModel.uiState.value.replaceAll) - assertFalse(viewModel.choice()!!.replaceAll) + assertFalse(viewModel.selection()!!.replaceAll) } @Test fun `switching expression resets replace-all`() { - val viewModel = ExtractVariableViewModel(threeCandidatePlan) + val viewModel = viewModelFor(threeCandidatePlan) viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) @@ -182,4 +168,14 @@ class ExtractVariableViewModelTest { assertFalse(viewModel.uiState.value.replaceAll) } + + /** + * The keyword set is language-specific and irrelevant to state derivation, so every case here uses + * a small Kotlin-shaped one; each language's own suite covers its real set. + */ + private fun viewModelFor(candidates: List) = ExtractVariableViewModel(candidates, KEYWORDS) + + private companion object { + private val KEYWORDS = setOf("val", "var", "fun", "when", "this") + } } diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index d02d5683c9..b3e81eb81c 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -542,6 +542,33 @@ No expression to extract here The file changed. Try extracting again. + + Not a valid Java name + That is a Java keyword + + + method %1$s + constructor + static initializer + initializer + lambda + if block + else block + for loop + while loop + do-while loop + try block + finally block + catch block + synchronized block + switch rule + block + if branch + else branch + for body + while body + do-while body + Extract method Extract method diff --git a/settings.gradle.kts b/settings.gradle.kts index 7ce1b50938..3d781b0f9e 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -137,6 +137,8 @@ include( ":lsp:jvm-symbol-index", ":lsp:jvm-symbol-models", ":lsp:kotlin", + ":lsp:refactor-core", + ":lsp:ui", ":lsp:xml", ":profiler", ":subprojects:aapt2-proto",