From b095c4f1b52c9a2e96afb55dd44b4000853de329 Mon Sep 17 00:00:00 2001 From: Daniel Alome Date: Sat, 5 Sep 2026 13:55:48 +0100 Subject: [PATCH] ADFA-5048: Add the Java extraction region Resolves a selection to the one region extract method acts on: an expression candidate at the cursor, or a run of sibling statements in one BlockTree snapped outward to whole statements. Restricting a range to siblings in one block excludes the hard cases -- half an if and half its else, a range straddling a lambda -- by construction rather than by later filtering. isExtractionPosition and isLegalExtractionTarget gain a defaulted `hoisted: Boolean = true`, and extract method passes false: - The conditional-evaluation refusal exists because extract *variable* lifts a declaration above the enclosing statement, so hoisting a ternary branch, a && right operand or a loop condition changes when it runs. Extract method substitutes a call in place: `while (extracted(it))` evaluates exactly as `while (it.hasNext())` did. - Extract variable also refuses an expression statement's whole expression, because replacing it with a *name* leaves a bare `v;`. A call is a statement, so extract method keeps that target; without it a bare cursor in `foo(a, b);` offers nothing. The default keeps every shipped extract-variable path byte-identical. statementContaining deliberately does not walk up from deepestPathAt: analyze() synthesises a default constructor whose generated super() carries the class declaration's own start position with zero width, and that synthetic node is narrower than everything real at that offset. Requiring positive width excludes it without having to recognise it. --- .../lsp/java/refactor/CandidateExpressions.kt | 27 ++- .../lsp/java/refactor/ExtractionRegion.kt | 183 ++++++++++++++++++ .../lsp/java/refactor/Occurrences.kt | 2 +- 3 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionRegion.kt 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 index d53b3c00d7..83454a30b5 100644 --- 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 @@ -68,6 +68,7 @@ fun candidateExpressionsAt( fileText: String, selectionStart: Int, selectionEnd: Int, + hoisted: Boolean = true, ): CandidateSyntax { val trees = Trees.instance(task) val positions = trees.sourcePositions @@ -79,7 +80,7 @@ fun candidateExpressionsAt( ?: (if (start == end && start > 0) deepestPathAt(root, positions, start - 1, start - 1) else null) ?: return CandidateSyntax.NONE - if (!isExtractionPosition(anchor)) return CandidateSyntax.NONE + if (!isExtractionPosition(anchor, hoisted)) return CandidateSyntax.NONE val ceiling = enclosingExecutableBody(anchor)?.leaf ?: return CandidateSyntax.NONE val collected = mutableListOf() @@ -89,7 +90,7 @@ fun candidateExpressionsAt( while (path != null) { val leaf = path.leaf if (leaf is ClassTree || leaf is MethodTree) break - if (isLegalExtractionTarget(path, trees)) { + if (isLegalExtractionTarget(path, trees, hoisted)) { val span = spanOf(root, positions, leaf) if (span != null && seen.add(span)) { collected += path @@ -165,8 +166,17 @@ internal fun deepestPathAt( * 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. + * + * [hoisted] is false for a refactoring that substitutes **in place** rather than lifting a declaration + * above the enclosing statement -- extract method. The conditional-evaluation rejection exists only + * because hoisting moves *when* the expression runs; replacing `it.hasNext()` with `extracted(it)` + * inside `while (...)` does not, so refusing it would cost extract method its best candidates in + * exactly the guarded code a helper reads best in. */ -internal fun isExtractionPosition(path: TreePath): Boolean { +internal fun isExtractionPosition( + path: TreePath, + hoisted: Boolean = true, +): Boolean { var current: TreePath? = path while (current != null) { val leaf = current.leaf @@ -175,7 +185,7 @@ internal fun isExtractionPosition(path: TreePath): Boolean { current = current.parentPath } if (isCaseLabel(path)) return false - if (isConditionallyEvaluated(path)) return false + if (hoisted && isConditionallyEvaluated(path)) return false return enclosingExecutableBody(path) != null } @@ -258,7 +268,7 @@ private fun isForUpdate( 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 { +internal fun isConstructorDelegation(invocation: MethodInvocationTree): Boolean { val name = (invocation.methodSelect as? IdentifierTree)?.name?.toString() ?: return false return name == "this" || name == "super" } @@ -294,6 +304,7 @@ internal fun enclosingExecutableBody(path: TreePath): TreePath? { internal fun isLegalExtractionTarget( path: TreePath, trees: Trees, + hoisted: Boolean = true, ): Boolean { val leaf = path.leaf if (leaf !is ExpressionTree) return false @@ -321,8 +332,10 @@ internal fun isLegalExtractionTarget( // 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 + // so replacing the expression with a *name* leaves a bare `v;` behind -- "not a statement". A call is + // a statement, so extract method (hoisted = false) keeps this target; without it a bare cursor in + // `foo(a, b);` -- the commonest place to reach for extract method -- would offer nothing. + if (hoisted && parent is ExpressionStatementTree) return false return true } diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionRegion.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionRegion.kt new file mode 100644 index 0000000000..04bb9e52e7 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractionRegion.kt @@ -0,0 +1,183 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.refactor.TextSpan +import openjdk.source.tree.BlockTree +import openjdk.source.tree.CompilationUnitTree +import openjdk.source.tree.ExpressionStatementTree +import openjdk.source.tree.ExpressionTree +import openjdk.source.tree.MethodInvocationTree +import openjdk.source.tree.StatementTree +import openjdk.source.tree.Tree +import openjdk.source.util.JavacTask +import openjdk.source.util.SourcePositions +import openjdk.source.util.TreePath +import openjdk.source.util.TreePathScanner + +/** + * What a selection resolved to. Exactly two kinds, which is the whole reason the hard cases never + * arise: a selection covering half an `if` and half its `else`, or straddling a lambda boundary, is + * neither, and is declined by construction rather than filtered out later. + */ +sealed interface ExtractionRegion { + /** The region's covering span in the file's text. */ + val span: TextSpan + + /** The path the analysis walks up from to find the anchor member. */ + val path: TreePath + + /** + * One expression at the cursor. A cursor resolves to several nested ones, innermost first, each a + * region in its own right, and the user picks between them in the sheet. + */ + data class Expression( + override val path: TreePath, + override val span: TextSpan, + ) : ExtractionRegion + + /** One or more sibling statements in a single [BlockTree]. */ + data class Statements( + val statements: List, + override val path: TreePath, + override val span: TextSpan, + ) : ExtractionRegion +} + +/** Every region a selection offers: 1..MAX_CANDIDATES expressions, or exactly one statement range. */ +fun resolveExtractionRegions( + task: JavacTask, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + selectionStart: Int, + selectionEnd: Int, +): List { + val (start, end) = trimToCode(fileText, selectionStart, selectionEnd) ?: return emptyList() + if (start == end) return expressionRegions(task, root, positions, fileText, selectionStart, selectionEnd) + + val statements = + snapToStatements(root, positions, fileText, start, end) + ?: return expressionRegions(task, root, positions, fileText, selectionStart, selectionEnd) + + // A selection sitting strictly inside one statement points at something narrower than the statement, + // so the expression path answers what the user actually selected. A near-miss drag that finds no + // legal expression there still gets the statement, rather than being refused for landing short. + val only = statements.statements.singleOrNull() + if (only != null) { + val statementSpan = spanOf(root, positions, only) + if (statementSpan != null && (start > statementSpan.start || end < statementSpan.end)) { + expressionRegions(task, root, positions, fileText, selectionStart, selectionEnd) + .takeIf { it.isNotEmpty() } + ?.let { return it } + } + } + + return listOf(statements) +} + +private fun expressionRegions( + task: JavacTask, + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + selectionStart: Int, + selectionEnd: Int, +): List = + candidateExpressionsAt(task, root, fileText, selectionStart, selectionEnd, hoisted = false) + .paths + .mapNotNull { path -> + spanOf(root, positions, path.leaf)?.let { ExtractionRegion.Expression(path, it) } + } + +/** + * The whole statements `[start, end)` touches, when they are siblings in one [BlockTree]. + * + * Null when the two ends land in different blocks, which is what rejects a selection spanning an `if` + * body and the code after it without needing to reason about the constructs involved. + */ +private fun snapToStatements( + root: CompilationUnitTree, + positions: SourcePositions, + fileText: String, + start: Int, + end: Int, +): ExtractionRegion.Statements? { + // end > start is guaranteed by the start == end early-return in resolveExtractionRegions. + val first = statementContaining(root, positions, start) ?: return null + val last = statementContaining(root, positions, end - 1) ?: return null + + val block = first.parentPath?.leaf as? BlockTree ?: return null + if (last.parentPath?.leaf !== block) return null + if (!isExtractionPosition(first, hoisted = false)) return null + + val statements = block.statements + val from = statements.indexOfFirst { it === first.leaf } + val to = statements.indexOfFirst { it === last.leaf } + if (from < 0 || to < from) return null + + val selected = statements.subList(from, to + 1).toList() + // `this(...)` / `super(...)` cannot move into a method: nothing may precede a delegation, and a + // constructor call is legal only in a constructor. The expression path refuses these through + // isExtractionPosition, which walks *ancestors* and so never sees a delegation that is the statement. + if (selected.any { it is ExpressionStatementTree && it.expression.isConstructorDelegation() }) return null + val firstSpan = spanOf(root, positions, selected.first()) ?: return null + val lastSpan = spanOf(root, positions, selected.last()) ?: return null + + return ExtractionRegion.Statements( + statements = selected, + path = first, + span = TextSpan(firstSpan.start, absorbTrailingSemicolon(fileText, lastSpan.end)), + ) +} + +private fun ExpressionTree.isConstructorDelegation(): Boolean = this is MethodInvocationTree && isConstructorDelegation(this) + +/** + * javac's end position for a statement does not reliably reach past its own `;`, and a region that + * stops short of one leaves a stray `;` behind at the call site. Absorbing a `;` that is already inside + * the span is impossible, so this is a no-op wherever it is not needed. + */ +internal fun absorbTrailingSemicolon( + fileText: String, + end: Int, +): Int = if (end < fileText.length && fileText[end] == ';') end + 1 else end + +/** + * The narrowest statement containing [offset] that is a direct statement child of a block. Null for a + * position that is not inside one, such as a comment or a class body. + * + * Deliberately *not* a walk up from [deepestPathAt]: `analyze()` synthesises a default constructor for + * a class that declares none, and its generated `super()` carries the class declaration's own start + * position with **zero width**. That synthetic node is narrower than everything real at that offset, so + * the walk up from it lands inside the synthetic constructor's block -- and a local class declaration + * then reported a different block from its own closing brace, which read as a cross-block selection. + * Requiring positive width excludes every synthetic node without needing to recognise one. + */ +private fun statementContaining( + root: CompilationUnitTree, + positions: SourcePositions, + offset: 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 + // currentPath still holds the parent here, which is exactly the block being asked about. + if (tree is StatementTree && currentPath?.leaf is BlockTree) { + val span = spanOf(root, positions, tree) + if (span != null && span.length > 0 && offset >= span.start && offset < span.end && span.length < bestWidth) { + best = TreePath(currentPath, tree) + bestWidth = span.length + } + } + return super.scan(tree, p) + } + } + scanner.scan(TreePath(root), null) + return best +} 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 index 587d057afc..4d768d28bf 100644 --- 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 @@ -264,7 +264,7 @@ private fun constrainingScopeFor(declaration: TreePath): Tree? = else -> owner } -private val LOCAL_KINDS = +internal val LOCAL_KINDS = setOf( ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER,