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,