diff --git a/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md b/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md index ca700de553..be525b20d1 100644 --- a/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md +++ b/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md @@ -1,6 +1,6 @@ # 0013. Refactoring UI lives in the owning LSP module -- **Status:** Proposed +- **Status:** Accepted, revised 2026-09-05 (see Revision below) - **Date:** 2026-08-03 - **Deciders:** Code On The Go team @@ -44,6 +44,22 @@ So a refactoring in `lsp/kotlin` either renders its own UI, or a new inversion m - **Render in `app`.** `app` is the integration point and already hosts `BottomSheetDialogFragment`s and `ILanguageClient`. Rejected: same inversion problem, and it puts Kotlin-specific refactoring UI in the module where nothing else language-specific lives. - **A new `lsp/kotlin-ui` module.** Keeps Compose out of `lsp/kotlin` without inverting. Rejected for now: a new Gradle module in a ~80-module build is disproportionate for one sheet. Reconsider once extract-method and inline-variable have landed and the UI surface is known. +## Revision: a shared `:lsp:ui` for UI that serves two languages + +- **Date:** 2026-09-05 +- **Tickets:** ADFA-5047 (extract variable), ADFA-5048 (extract method) + +The decision above anticipated its own revisit: *"If three or more `lsp/*` modules end up with Compose UI, extracting a shared UI module becomes worthwhile"*, and *"Reconsider once extract-method and inline-variable have landed and the UI surface is known"*. Both refactorings have now landed in both languages, and the trigger turned out to be duplication between two modules rather than a third one appearing. + +**The rule is now:** a refactoring sheet used by **more than one** language server lives in **`:lsp:ui`**, behind a plain-data contract of strings, name sets and index positions. A sheet used by exactly one stays in that language's own module. + +- `:lsp:ui` holds `ExtractVariableSheet` and `ExtractMethodSheet` with their `ViewModel`s, states, events and the shared `LabelledSection`/`OptionList`. Neither knows what a `KtExpression` or a javac `Tree` is: each caller maps its own plan into `CandidateView`/`MethodCandidateView` and maps a selection back out. +- Language-specific wording stays with the language. `NameMessages` and the keyword set are passed in, because two of the four name-problem strings name the language ("Not a valid Java name") and a shared lookup would show a Java user Kotlin's wording. +- The extract-method signature preview crosses the boundary as a **prefix and a suffix around the name**, not a rendered string: the preview follows what the user types, and each language keeps one derivation shared with its own edit builder. +- `lsp/kotlin` keeps `InlineVariableSheet`, which has no Java counterpart, under the original decision. + +Everything else in this ADR stands unchanged: the plain-data plan boundary, the `BottomSheetDialogFragment` hosting a `ComposeView`, the `ContextWrapper` walk for a `FragmentActivity`, and ADR 0009's UDF shape. The costs listed above are unchanged too, except that Compose now sits in one shared module rather than being added to each language server in turn. + ## Related - [ADR 0009](0009-jetpack-compose-for-new-ui.md) — Compose for new UI; this ADR answers *where*, not *what*. diff --git a/docs/features/java-extract-method.md b/docs/features/java-extract-method.md new file mode 100644 index 0000000000..1cc99e8bcb --- /dev/null +++ b/docs/features/java-extract-method.md @@ -0,0 +1,324 @@ +# Java extract method + +- **Ticket:** ADFA-5048 (the Java sibling of ADFA-5080; requested in ADFA-4821's parity table) +- **Status:** Implemented +- **Modules:** `lsp/java`, `lsp/ui` +- **Vocabulary:** the term is **method**, matching the ticket and the tooltip tag `editor.codeactions.extractmethod`. + +Move the expression at the cursor, or a selected range of statements, into a new `private` method, and replace it with a call to that method. + +Java parity for the Kotlin refactoring specified in [kotlin-extract-method.md](kotlin-extract-method.md), built on the primitives ADFA-5047 already put in `lsp/java/refactor/` and `:lsp:refactor-core`. It adds no new module and no new dependency. + +The governing principle is [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md): this refactoring **moves** code, never edits the interior of what it moved, and where it cannot do that faithfully it **declines with a specific reason** rather than guessing. + +## Language + +Shared vocabulary - *selection*, *extraction region*, *expression candidate*, *text span*, *occurrence*, *refactoring plan*, *rewrite span* - is defined in [kotlin-extract-variable.md](kotlin-extract-variable.md#language); *statement range*, *captured declaration*, *output*, *exit* and *refusal* in [kotlin-extract-method.md](kotlin-extract-method.md#language). Java replaces one term and adds one. + +**Anchor member**: +The nearest ancestor of the region that is a *direct member of a `ClassTree`* - a method, a constructor, an initializer block, or a field. It is both the boundary that decides what becomes a parameter and the sibling the new method is inserted after. Replaces Kotlin's *enclosing declaration*, which had no field case. +_Avoid_: enclosing method, parent, owner. + +**Thrown checked type**: +A checked exception type the region can throw and does not itself catch. Each one becomes an entry in the new method's `throws` clause. Java's only analogue of Kotlin's `suspend`/`@Composable` rules: the one derived modifier whose omission produces code that does not compile. +_Avoid_: exception, throws clause. + +## Scope + +### In scope + +An expression, or a range of sibling statements, inside any executable body in a Java file: a method or constructor body, an initializer block, a lambda body, or a method of a local or anonymous class. + +### Out of scope + +The positions extract variable already rejects for reasons that hold here too, via the same `isExtractionPosition` check: annotation arguments, `this(...)`/`super(...)` delegation arguments, `case` labels, and anything outside an executable body. The positions it rejects only because *it hoists* are in scope here (R2). + +## Requirements + +**R1 - Trigger.** An "Extract method" item (`action_extract_method`, already in `strings.xml`) in the editor code-actions menu for Java files, id `ide.editor.lsp.java.extractMethod`, tooltip tag `EDITOR_CODE_ACTIONS_EXTRACT_METHOD = "editor.codeactions.extractmethod"` - one new constant in `TooltipTag.kt`. The tag string is fixed by ADFA-4821: tooltip content lives in the out-of-repo tooltips database keyed by tag. + +As with Java extract variable: **no `prepare()` visibility gate** (deciding extractability needs an attributed compile, far too costly on the UI thread), and `requiresUIThread = false` so the selection is read on a background thread. `BaseJavaCodeAction`'s file-type and module gate is all that applies. + +**R2 - Region.** The selection resolves to exactly one extraction region, of one of two kinds. + +*Expression candidate* - reuses `candidateExpressionsAt` including the whitespace trim, the `offset - 1` cursor retry, the innermost-first walk, `MAX_CANDIDATES = 3` and the type/lambda/method-reference exclusions. A bare cursor always takes this path first. + +*Statement range* - a non-empty selection that spans statement boundaries snaps **outward** to whole statements; a touch selection will not land on a boundary. The result must be 1..N statements that are **siblings in one `BlockTree`**. A selection spanning two different blocks, or one whose ends do not both resolve to statements, is declined (`NotASingleRegion`). As in Kotlin, a selection that snaps to a single statement but sits strictly inside it prefers the expression path, falling back to the statement when nothing there is a legal target. + +**Two predicates gain a `hoisted: Boolean = true` parameter**, and extract method passes `false`: + +- `isExtractionPosition` skips `isConditionallyEvaluated`. That check exists because extract *variable* lifts the expression into 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**, so `while (it.hasNext())` becomes `while (extracted(it))` and evaluation order is untouched. Refusing these would cost the feature its most useful candidates in exactly the guarded code where a helper reads best. +- `isLegalExtractionTarget` stops excluding an `ExpressionStatementTree`'s whole expression. Extract variable excludes it because replacing the expression leaves a bare `v;`, which is not a statement; `extracted(a, b);` is one. Without this a bare cursor in `foo(a, b);` - the single commonest place to reach for extract method - offers nothing. + +The default value keeps every shipped extract-variable path byte-identical. + +**R3 - Live offsets and the version guard.** Identical to Java extract variable: the plan is built inside `data.requireCompiler().compile(file).get { ... }`, records the document version from `FileManager.getActiveDocument`, and the confirm path re-reads that version, refusing on a mismatch and refusing outright when it is null. + +**R4 - Target.** One uniform rule, no target picker: **the new method is inserted immediately after the anchor member**, as a member of the class that declares it. + +| The region sits in | The new method becomes | +|---|---| +| a method or constructor | a `private` member of that class, after that method | +| an initializer or static initializer block | a `private` member, after that block | +| a lambda, in any of the above | still a sibling of the anchor member; the lambda's captures become parameters | +| a method of an anonymous or local class | a `private` member **of that class**, since the anchor member is the anonymous class's own method | +| a lambda or anonymous class inside a field initializer | a `private` member, after that field | + +Java has no local-method form, so the insertion is *always after* the anchor and the new method always leads the descending edit list (R15). Kotlin's "no enclosing named declaration" refusal has no Java counterpart: a field is a class member, so a lambda in a field initializer anchors on the field and needs no special case. + +The insertion offset absorbs a `;` sitting immediately after the anchor's end position, because javac's end position for a declaration does not reliably reach past its own semicolon. Absorbing one already inside the span is impossible, so the guard is a no-op where it is not needed; without it a field anchor risks an insertion between the field and its semicolon. + +Unlike extract variable there is no scope chain and no ceiling: anything not visible at the insertion site becomes a parameter instead of constraining the anchor. + +**R5 - Parameters.** A referenced variable needs a parameter exactly when it is a captured declaration - its declaration lies inside the anchor member. Fields need nothing: the new method is a member of the same class. + +- **Order** - first textual appearance in the region, so the signature reads in the order the body uses it. +- **Names** - the original identifier, unchanged. +- **Types** - `VariableElement.asType()` rendered and then shortened by `shortenTypeText` against the file's own imports, exactly as extract variable renders a declared type: a name is shortened only where the file already resolves the short form, because this refactoring adds no imports. A type that cannot be written as source - a capture, an intersection, an anonymous class, an `` resolution failure, detected by the existing `isUnrenderableTypeText` - **declines** (`UnrenderableType`). +- **Not editable.** The derived signature is shown read-only (R11). A wrong parameter *name* is fixable afterwards with rename; a wrong parameter *set* is not something the user could correct by hand anyway. + +No receiver rules: Java has no extension receivers, and `this` resolves unchanged from a method of the same class. + +**R6 - Return type and call-site form.** Determined by the region kind and its output: + +| Case | Extracted body | Return type | Call site | +|---|---|---|---| +| expression candidate | `return ;` | the expression's type | `extracted(args)` in the expression's place | +| expression candidate, `void`-typed call | `;` | `void` | `extracted(args)` | +| statement range, no output | the statements | `void` | `extracted(args);` | +| statement range, one output `x` | the statements, then `return x;` | `x`'s declared type | `T x = extracted(args);` | +| statement range, tail return (R8) | the statements including the `return` | the anchor method's return type | `return extracted(args);` | + +A region that always throws still declares `void`; the exception propagates and the call site behaves identically. + +**R7 - Outputs.** An output is a local declared inside the region and read after it, within the anchor member. Exactly one is supported; **two or more declines** (`MultipleOutputs`, naming them). + +Kotlin's `OutputNotReturnable` has **no Java counterpart** and is not implemented: every Java local can be received back as `T x = extracted(...)`. The destructuring entries and local functions that refusal existed for do not exist in Java. + +A variable declared **outside** the region and **assigned inside** it declines (`ReassignsOuterVar`, naming it): Java has no out parameters, so the assignment would be lost. Deliberately stricter than dataflow requires - a reassignment never read afterwards is still refused, because proving that needs real liveness analysis. + +An **element** write through a captured reference (`arr[i] = x`, `list.add(x)`) is **not** a reassignment and does not decline: the reference is passed by value and the mutation is visible to the caller. `writeOffsetsFor` already draws exactly this distinction for extract variable. + +The call site re-declares the output with its rendered type only; `final` and any annotations on the original declaration are dropped. + +**R8 - Exits.** Every exit declines (`ExitsRegion`), with one syntactic exception. + +An exit is a `return`, `break`, `continue` or `yield` inside the region whose target lies outside it. An unlabelled `break`/`continue` targets the nearest enclosing loop (or, for `break`, `switch`); a labelled one targets its `LabeledStatementTree`; a `yield` targets its switch expression. When that target is itself inside the region, nothing crosses the boundary and it is not an exit. + +**Tail return:** when the region is a statement range whose *last* statement is a `return`, the region contains no other exit, there is no output, and the region's enclosing executable body **is the anchor member's own body** (not a nested lambda), the new method takes the anchor method's return type, keeps the `return`, and the call site becomes `return extracted(args);`. The body restriction is load-bearing: a `return` inside a lambda returns from the lambda, so taking the anchor's return type would emit a method returning something its body never returns. + +A constructor anchor is treated as `void`; `return` is illegal in an initializer, so no tail return can arise there. + +Not an exit: a `return` belonging to a method **declared inside** the region - a local class's method, an anonymous class's method, or a lambda body. It moves with its own declaration and its jump never crosses the region boundary. + +A `throw` is not an exit; it propagates, and R10 declares it. + +**R9 - Receivers.** Deliberately empty, and numbered to keep this document aligned with the Kotlin one. Java has no extension receivers and no `with`/`apply` scoping functions, so all three of Kotlin's receiver rules and the `InnerImplicitReceiver` refusal vanish. The dispatch receiver needs nothing: the new method is a member of the same class, so `this` and every instance member resolve unchanged. + +**R10 - Modifiers and `throws`.** Copy nothing from the anchor member; add only what the body needs to compile in its new home. + +- **Visibility** - always `private`. No annotations copied, no Javadoc generated. +- **`static`** - added exactly when the anchor member is static (a `static` method, a static initializer, or a `static` field). An instance anchor yields an instance method, where `this` and every instance member resolve unchanged. +- **`throws`** - the region's thrown checked types, rendered and shortened like every other type. Both halves of the derivation are load-bearing: under-declaring leaves the new method's body uncompilable, and over-declaring breaks the *call site*, which is only obliged to handle what the region actually threw. + + Collected from, within the region: each `MethodInvocationTree` and `NewClassTree`'s resolved `ExecutableElement.getThrownTypes()`, each `ThrowTree`'s expression type, and the `close()` thrown types of each try-with-resources resource. Then **subtracted**: a type caught by a `try` **inside** the region, where the site sits in that `try`'s block and some catch parameter type `C` satisfies `T <: C` (each alternative of a multi-catch considered separately). Then filtered to checked types - not assignable to `RuntimeException`, not assignable to `Error` - and deduplicated. + + The scan **does not descend into a nested lambda body, anonymous class body or local class**: a checked exception thrown there is constrained by that construct's own signature and never propagates to the anchor member. + + One case **declines** (`UnrenderableType`): a **generic `throws E`**, such as `Optional.orElseThrow(Supplier)`. `ExecutableElement.getThrownTypes()` reports the type variable as *declared on the callee*, and javac's public API does not hand back what it was inferred to at this call site. Writing `E` would emit a name nothing declares, and substituting its bound would over-declare and break the call site, so the region is declined rather than guessed at (ADR 0014). + + Copying the anchor member's own `throws` clause instead was rejected: it is wrong in exactly the commonest case, a region inside a `try` block whose method declares nothing. +- **Type parameters** - a region referencing a type variable **declared on the anchor method** declines (`UsesTypeParameter`, naming it). Class-level type parameters need no rule; they stay in scope for a member. Detection walks the `TypeMirror`s themselves for `TypeKind.TYPEVAR` whose element's generic element is the anchor method, rather than matching names in rendered text. + +**R11 - Sheet.** The Kotlin extract-method sheet is **promoted from `lsp/kotlin` to `:lsp:ui`** and serves both languages, exactly as ADFA-5047 promoted the extract-variable sheet. `ExtractMethodSheet`, `ExtractMethodSheetContent`, `ExtractMethodViewModel`, `ExtractMethodUiState` and `ExtractMethodUiEvent` move unchanged in behaviour; `KOTLIN_NAME_MESSAGES`, hardcoded in the content today, becomes a `NameMessages` parameter as it already is on the extract-variable sheet. + +The data boundary is a new `ExtractMethodContract.kt`, mirroring `ExtractVariableContract.kt`: + +```kotlin +data class MethodCandidateView( + val label: String, + val suggestedName: String, + val takenNames: Set, + val signaturePrefix: String, + val signatureSuffix: String, +) + +data class ExtractMethodSelection(val candidateIndex: Int, val name: String) +``` + +The signature is split around the name rather than pre-rendered, because the preview updates as the user types: Java composes `private static int ` + name + `(int a, int b) throws IOException`, Kotlin `private suspend fun ` + name + `(id: String): User`. One derivation each, so no preview can drift from its emitted declaration. + +Contents, top to bottom: title, expression chooser (only for an expression region with more than one candidate), name field with its `NameProblem` message, signature preview, Cancel/Extract. No scope chooser (R4) and no replace-all checkbox (R13). + +[ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md) deferred the shared-UI question until the extract-method surface was known. It now is, and the surface is identical for both languages, so that ADR is updated rather than left stale. + +**R12 - Name.** Suggestion: for an expression region, `suggestVariableName` unchanged (shape, then rendered type, then `value`); for a statement range, the constant `extracted`, since there is no expression to read a name from. Uniquified as today. + +Validation reuses `validateVariableName`, `NameProblem` and the shipped `JAVA_KEYWORDS`, so no new strings. Taken names are **every method name visible in the insertion class, including inherited ones** (`Elements.getAllMembers` filtered to methods and constructors). + +Java private methods never override, so an inherited name is not an accidental-override hazard as it is in Kotlin; rejecting it anyway keeps one rule across both languages and stops the refactoring silently creating an overload the user did not ask for. + +**R13 - One call site.** The region is the only site rewritten. No duplicate detection and no replace-all toggle: exact-duplicate matching would almost never fire, and near-duplicate matching needs anti-unification plus a per-site parameter mapping. `Occurrences.kt` is expression-granular by construction. + +**R14 - Refusals.** The plan carries a typed `ExtractionRefusal` and `postExec` maps it to a specific message. Java uses **8 of Kotlin's 13 reasons**, all with strings that already exist: + +| Reason | String | +|---|---| +| `NotASingleRegion` | `msg_extract_method_not_single_region` | +| `CouldNotAnalyse` | `msg_extract_method_could_not_analyse` | +| `MultipleOutputs` | `msg_extract_method_multiple_outputs` | +| `ReassignsOuterVar` | `msg_extract_method_reassigns_outer_var` | +| `ExitsRegion` | `msg_extract_method_exits_region` | +| `UsesTypeParameter` | `msg_extract_method_uses_type_parameter` | +| `UnrenderableType` | `msg_extract_method_unrenderable_type` | +| `CapturedLocalDeclaration` | `msg_extract_method_captured_local_declaration` | + +`CapturedLocalDeclaration` covers the region referencing a local class, or a value whose type is a local class, declared inside the anchor member but outside the region: the value survives the move, the type name does not. + +Dropped as inapplicable: `OutputNotReturnable` (R7), `AnonymousExtensionFunction`, `InnerImplicitReceiver`, `UsesBackingField`, `SmartCastParameter` - no Java construct produces any of them. + +`CouldNotAnalyse` exists so the others stay truthful: a failed compile or a thrown analysis error must not be reported as `NotASingleRegion`, which blames a selection nothing ever looked at. Cancellation is not a refusal - `CancellationException` is re-thrown, so a cancelled action ends silently. + +**R15 - Edit.** Two regions change - the region becomes a call, and the new method appears after the anchor member - emitted as **two `TextEdit`s in one `DocumentChange`, sorted by descending start offset**. + +The ordering is mandatory, not stylistic, and was verified against the code rather than assumed: `IDELanguageClientImpl.applyActionEdits` iterates `change.getEdits()` in list order and `editInEditor` applies each with **line/column** ranges against whatever the text is at that moment (`Position.index` is ignored), so an earlier edit must never shift a later one. Java always inserts after the anchor, so the insertion always leads. + +**Known consequence:** nothing on that path calls `beginBatchEdit`, so this is **two undo entries**, and a single undo leaves a half-refactored, non-compiling file. ADFA-5081 fixes it by batching the edit loop in `applyActionEdits`, which benefits every multi-edit action; until it lands, the two-step undo is a stated limitation to be covered in QA. + +The new method is emitted **fully indented** at the anchor member's own indentation, separated by one blank line, reusing `detectIndentUnit`, `detectNewline`, `leadingIndentAt` and `positionAt`. Code-action edits bypass the editor's auto-indent, and running `CMD_FORMAT_CODE` here would reformat the whole file into the same undo step as the extraction. + +One exception to re-indenting every line: lines inside a **text block** (`"""`) are emitted byte-for-byte. Their whitespace is part of the literal's value and the closing delimiter sets the incidental-whitespace margin, so shifting either edits the interior of the moved code (ADR 0014). The candidate carries those literals' spans so the text layer needs no tree. + +**R16 - Responsiveness and failure isolation.** One background pass inside the compiler's own `compile(...).get { }` produces the whole plan; the sheet does pure string and offset arithmetic and never re-enters javac on confirm. Anything thrown in the pipeline degrades to a refusal (`CouldNotAnalyse`) plus a log line, never an uncaught throw: `DefaultActionsRegistry` catches only `IllegalArgumentException` and this runs on a scope with no exception handler. + +`CancellationException` is the one deliberate exception and is re-thrown rather than swallowed, so a cancelled action ends quietly instead of flashing a message at a user who has moved on. The sheet's confirm path runs outside the framework's guards entirely, so it wraps its own body. + +## Non-goals + +- **Duplicate or near-duplicate call sites** (R13). +- **An editable parameter list** - rename, reorder or exclude (R5). +- **Two or more outputs, and a reassigned outer variable** (R7). +- **Mid-region `return`/`break`/`continue`/`yield`** (R8). +- **Type parameters declared on the anchor method** (R10). +- **Preserving `final` or annotations** on a re-declared output (R7). +- **Choosing a different target** - another class, another file, a nested class (R4). Moving a declaration elsewhere is a move refactoring. +- **Adding imports.** Types are shortened only where the file already resolves them, and otherwise written fully qualified (R5). +- **A region calling a method with a generic `throws E`** (R10). Declined, not guessed. +- **Extraction from a field initializer expression, annotation argument or `case` label** - inherited from `isExtractionPosition`. +- **Generated Javadoc** for the new method. +- **Atomic undo** of the two edits - ADFA-5081. +- **Formatting the result.** R15 emits indented text instead. + +## Acceptance criteria + +1. "Extract method" appears in the code-actions menu of a Java file and is absent in a non-Java file. +2. A cursor inside an expression offers the innermost-first candidates; extracting one replaces it with a call and adds a `private` method returning that expression, directly below the enclosing method. +3. A cursor inside `foo(a, b);` offers the call, and extracting it produces `extracted(a, b);` with a `void` method. +4. Selecting two adjacent statements that use two locals produces a method with those two locals as parameters, in first-use order, and a call passing them. +5. A selection with ragged boundaries snaps outward to whole statements before extracting. +6. A selection spanning two different blocks reports "Select an expression, or whole statements inside one block". +7. A range declaring a local that is read afterwards produces `T x = extracted(...);` at the call site. +8. A range declaring two locals that are both read afterwards is declined as producing more than one value. +9. Selecting a loop that accumulates into a local declared outside it is declined, and the message names that variable. +10. Selecting the tail of a method ending in `return x;` produces `return extracted(...);` and a method with the enclosing return type. +11. Selecting a range containing a `return` in the middle is declined; a range containing a whole loop with its own `break` is not. +12. Extracting from a `static` method produces a `private static` method. +13. Extracting a region that calls a method declaring `throws IOException` produces a method declaring `throws IOException`, and the file still compiles. +14. Extracting a region whose `try` catches that same `IOException` produces a method with **no** `throws` clause, and the file still compiles. +15. Extracting from inside an anonymous class's method inserts the new method into that anonymous class. +16. A region using a type parameter of the enclosing method is declined, naming the parameter. +17. A name matching an existing method - including an inherited one - is rejected with "That name is already used". +18. The signature preview matches the emitted declaration exactly, including `static` and `throws`. +19. Editing the file while the sheet is open, then confirming, reports the file-changed message and leaves the file untouched. +20. Undo restores the file; it currently takes **two** undo steps (R15), and the intermediate state is non-compiling. +21. A space-indented file receives space-indented output; a CRLF file keeps CRLF; a text block inside the region keeps its exact interior. +22. The Kotlin extract-method sheet is unchanged in appearance and behaviour after the move to `:lsp:ui`. + +## Design + +Same shape as Java extract variable, and the same data boundary as ADR 0013: one background pass produces a plain-data plan, the sheet holds no trees. + +``` +ExtractMethodAction.execAction (background) lsp/java/actions + data.requireCompiler().compile(file).get { task -> + buildExtractMethodPlan(task, file, start, end, version) refactor/ExtractMethodPlanner.kt + resolveExtractionRegion(root, text, start, end) refactor/ExtractionRegion.kt [R2] + expression -> candidateExpressionsAt(hoisted=false) (reused) + statements -> snap outward, sibling-in-one-block + anchorMemberFor(regionPath) [R4] + analyse against the attributed unit: refactor/MethodSignature.kt [R5-R10] + captured declarations -> parameters + outputs / exits / thrown checked types / static / type parameters + -> ExtractMethodPlan | ExtractionRefusal [R14] + } + <- ExtractMethodPlan (plain data, no trees) + +ExtractMethodAction.postExec (UI thread) + refusal -> flashInfo(message for reason) [R14] + ExtractMethodSheet.show(views, JAVA_KEYWORDS, JAVA_NAME_MESSAGES) lsp/ui [R11] + on confirm -> version re-read; mismatch -> refuse [R3] + buildExtractMethodRewrites -> two RewriteSpans refactor/ExtractMethodEdit.kt [R15] + client.performCodeAction(one DocumentChange, two TextEdits, descending) +``` + +New files in `lsp/java`: + +- **`refactor/ExtractionRegion.kt`** - the region model and its resolution (R2). Purely syntactic apart from the legal-target check, so it tests against a parsed unit alone. +- **`refactor/MethodSignature.kt`** - captures to parameters, outputs, exits, thrown checked types, `static`, type parameters, and the signature's two halves (R5-R10). The only attribution-dependent part. +- **`refactor/ExtractMethodPlan.kt`** - `ExtractMethodCandidate`, `ExtractMethodPlan` and `ExtractionRefusal`. +- **`refactor/ExtractMethodPlanner.kt`** - the single background pass (R3, R16), with both a `CompileTask` and a bare `JavacTask` overload so tests need no project model, exactly as `buildExtractionPlan` has today. +- **`refactor/ExtractMethodEdit.kt`** - the two rewrites and their ordering (R15). Pure text and offsets. +- **`refactor/JavaExtractMethodUi.kt`** - the plan-to-view mapping and the selection-to-candidate lookup, mirroring `JavaExtractVariableUi.kt` (R11). +- **`actions/ExtractMethodAction.kt`** - registered in `JavaCodeActionsMenu`; the only class touching the editor, the document version or the language client. + +Moved into `lsp/ui`, with `ExtractMethodContract.kt` added: `ExtractMethodSheet`, `ExtractMethodSheetContent`, `ExtractMethodViewModel`, `ExtractMethodUiState` (R11). + +Reused unchanged from extract variable: `candidateExpressionsAt` / `CandidateSyntax`, `isExtractionPosition`, `isLegalExtractionTarget`, `enclosingExecutableBody`, `spanOf`, `trimToCode`, `deepestPathAt`, `referencedElements`, `INCREMENT_KINDS`, `shortenTypeText`, `importedNamesOf`, `starImportedPackagesOf`, `isUnrenderableTypeText`, `isValuelessKind`, `suggestVariableName`, `JAVA_KEYWORDS`, `JAVA_NAME_MESSAGES`, `collapseForLabel`, and all of `:lsp:refactor-core` (`TextSpan`, `RewriteSpan`, `toTextEdit`, `positionAt`, `detectIndentUnit`, `detectNewline`, `leadingIndentAt`, `MAX_CANDIDATES`). + +Deliberately **not** reused: `ScopeChain.kt`, `ScopeOption`, `AnchorForm` and `Occurrences.findOccurrences`. Each is shaped by the legal scope chain and the replace-all feature, neither of which this refactoring has (R4, R13). + +Outside these two modules, only `TooltipTag.kt` gains a constant. **No new strings, no new module, no new dependency.** + +### Commits + +One PR, reviewable by commit, mechanical before behavioural: + +1. `refactor: promote the extract-method sheet into :lsp:ui` - move the four files, add the contract, thread `NameMessages` through, adapt `ExtractMethodAction` in `lsp/kotlin`, move its ViewModel test. No behaviour change. +2. `ADFA-5048: Add the Java extraction region` - `ExtractionRegion.kt` plus the two `hoisted` parameters. +3. `ADFA-5048: Add the Java extract-method signature analysis` - `MethodSignature.kt`, `ExtractMethodPlan.kt`, `ExtractMethodPlanner.kt`. +4. `ADFA-5048: Add the Java extract-method rewrite` - `ExtractMethodEdit.kt`. +5. `ADFA-5048: Add the Java extract method code action` - the action, the menu entry, the tooltip tag. +6. `docs: Java extract method, and ADR 0013's shared-UI update`. + +## Verification + +Unit tests in `:lsp:java` and `:lsp:ui`, mirroring the extract-variable split so a failure localises to one layer: + +```bash +flox activate -d flox/local -- ./gradlew \ + :lsp:java:testV7DebugUnitTest --tests "com.itsaky.androidide.lsp.java.refactor.*" \ + :lsp:ui:testV7DebugUnitTest :lsp:refactor-core:testV7DebugUnitTest \ + :lsp:kotlin:testV7DebugUnitTest --tests "com.itsaky.androidide.lsp.kotlin.utils.refactor.*" +``` + +The `--tests` filter is not optional on `:lsp:java`: its unqualified suite includes the Robolectric `JavaLSPTest` harness, which boots the Gradle tooling API in a separate process and exceeds the task's 10-minute timeout on a developer machine. The refactor package needs none of it. `JavacFixture` already gives one hermetic attributed compile of a source string with no project model, and `compiles(source)` already answers whether a rewritten file still compiles. + +- **`ExtractMethodRegionTest`** - parse-only: outward snapping to whole statements, the sibling-in-one-block rule, cross-block rejection, the expression path, and the two `hoisted = false` relaxations (R2). +- **`ExtractMethodPlanTest`** - attribution-backed, one case per rule: the parameter set, order and types (R5), each call-site form (R6), the single output and the `void` case (R7), the tail return and the nested-declaration `return` that is not an exit (R8), `static` (R10), `throws` derivation **and** its subtraction by an inner `catch` (R10), the anonymous-class anchor and the field anchor (R4), and **one case per refusal reason** (R14). +- **`ExtractMethodEditTest`** - pure text: the two edits and their descending order, the call-site forms, indentation, text blocks left verbatim, the blank-line separation, and CRLF preservation (R15). +- **`ExtractMethodViewModelTest`** - moved to `:lsp:ui`: chooser visibility, name validation, and the rendered signature preview for **both** languages' prefix/suffix shapes (R11, R12). + +Every plan test asserts the extracted file **actually compiles** via `compiles(...)`, not merely that the emitted text matches a string. That is the assertion that matters for `throws`, `static` and captured types, where a plausible-looking signature is exactly the failure mode. + +The sheet, `prepare()`/`ActionData`, the two-step undo and the new tooltip row are not unit-testable; they are covered by on-device QA from the acceptance criteria above, recorded in ADFA-5048's "Steps to QA" field, at font scale 1.0 and 2.0. + +## Related + +- [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code; the principle behind R7-R10 +- [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI placement; updated by R11 +- [kotlin-extract-method.md](kotlin-extract-method.md) - ADFA-5080, the specification this achieves parity with +- [kotlin-extract-variable.md](kotlin-extract-variable.md) - ADFA-4826; owns the shared Language section +- ADFA-5047 - Java extract variable, which contributed every primitive reused here +- ADFA-4821 - the code-action parity table this ticket comes from +- ADFA-5081 - code action edits should be a single undo step (fixes R15's consequence) +- [ARCHITECTURE.md](../../ARCHITECTURE.md) 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 7c4e23f0e3..8611b970b4 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -179,6 +179,7 @@ object TooltipTag { 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" + const val EDITOR_CODE_ACTIONS_EXTRACT_METHOD = "editor.codeactions.extractmethod" // 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/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractMethodAction.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractMethodAction.kt new file mode 100644 index 0000000000..8d0e1b0696 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/ExtractMethodAction.kt @@ -0,0 +1,235 @@ +package com.itsaky.androidide.lsp.java.actions + +import android.content.Context +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.ExtractMethodPlan +import com.itsaky.androidide.lsp.java.refactor.ExtractionRefusal +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.buildExtractMethodPlan +import com.itsaky.androidide.lsp.java.refactor.buildExtractMethodRewrites +import com.itsaky.androidide.lsp.java.refactor.candidateFor +import com.itsaky.androidide.lsp.java.refactor.toMethodCandidateViews +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.ExtractMethodSelection +import com.itsaky.androidide.lsp.ui.ExtractMethodSheet +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 + +/** + * Moves the expression at the cursor, or a selected range of statements, into a new `private` method. + * + * [execAction] runs one attributed compile and returns a plain-data [ExtractMethodPlan]; [postExec] + * shows the shared sheet and turns the user's selection into two text edits with pure offset + * arithmetic. Where the region cannot be moved faithfully the plan carries a typed refusal, which + * postExec renders as a specific message rather than a generic failure (ADR 0014). + */ +class ExtractMethodAction : BaseJavaCodeAction() { + companion object { + const val ID = "ide.editor.lsp.java.extractMethod" + + private val log = LoggerFactory.getLogger(ExtractMethodAction::class.java) + } + + override val titleTextRes: Int = R.string.action_extract_method + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_EXTRACT_METHOD + + 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 a refusal instead. + override var requiresUIThread: Boolean = false + + override suspend fun execAction(data: ActionData): ExtractMethodPlan { + 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 -> + buildExtractMethodPlan(task, file, selectionStart, selectionEnd, version) + } + }.getOrElse { error -> + if (error is CancellationException) throw error + log.warn("Could not analyse {} for extract method.", file, error) + ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse, documentVersion = version) + } + } + + override fun postExec( + data: ActionData, + result: Any, + ) { + super.postExec(data, result) + if (result !is ExtractMethodPlan) return + + val context = data.requireContext() + if (result.isEmpty) { + flashInfo(refusalMessage(context, result.refusal ?: ExtractionRefusal.CouldNotAnalyse)) + return + } + + 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 = + ExtractMethodSheet.show( + activity, + result.toMethodCandidateViews(), + 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 the two edits and hands them to the language client. + * + * Runs from the sheet's click handler, outside `execAction` and so outside every guard the action + * framework provides -- nothing here may throw, hence the [runCatching]. + */ + private fun applySelection( + data: ActionData, + plan: ExtractMethodPlan, + selection: ExtractMethodSelection, + ) { + runCatching { performSelection(data, plan, selection) }.onFailure { error -> + log.error("Failed to apply the extract-method selection '{}'", selection.name, error) + flashError(R.string.msg_cannot_perform_fix) + } + } + + /** + * 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 performSelection( + data: ActionData, + plan: ExtractMethodPlan, + selection: ExtractMethodSelection, + ) { + 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_method_file_changed) + return + } + + val candidate = + plan.candidateFor(selection) ?: run { + log.warn("Selection {} does not address the plan it came from.", selection) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val rewrites = + buildExtractMethodRewrites(plan.fileText, candidate, selection.name) ?: run { + log.warn("Could not build an extract-method 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 method.") + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = + listOf( + DocumentChange( + file = file, + // Already in descending document order: applyActionEdits applies these in list + // order with line/column ranges, so the call site must not shift the insertion point. + edits = rewrites.map { it.toTextEdit(plan.fileText) }, + ), + ), + kind = CodeActionKind.QuickFix, + // The rewrites are emitted fully indented. Running google-java-format here would reformat + // the whole file into the same undo step as the extraction. + command = Command("", ""), + ), + ) + } + + /** + * Each refusal names the construct in the way; a generic message reads as a broken feature. + * + * Exhaustive with no `else`: a future variant added without a message here is a compile error rather + * than a silent gap. + */ + private fun refusalMessage( + context: Context, + refusal: ExtractionRefusal, + ): String = + when (refusal) { + ExtractionRefusal.NotASingleRegion -> { + context.getString(R.string.msg_extract_method_not_single_region) + } + + ExtractionRefusal.CouldNotAnalyse -> { + context.getString(R.string.msg_extract_method_could_not_analyse) + } + + is ExtractionRefusal.MultipleOutputs -> { + context.getString(R.string.msg_extract_method_multiple_outputs, refusal.names.joinToString(", ")) + } + + is ExtractionRefusal.ReassignsOuterVar -> { + context.getString(R.string.msg_extract_method_reassigns_outer_var, refusal.name) + } + + ExtractionRefusal.ExitsRegion -> { + context.getString(R.string.msg_extract_method_exits_region) + } + + is ExtractionRefusal.UsesTypeParameter -> { + context.getString(R.string.msg_extract_method_uses_type_parameter, refusal.name) + } + + ExtractionRefusal.UnrenderableType -> { + context.getString(R.string.msg_extract_method_unrenderable_type) + } + + is ExtractionRefusal.CapturedLocalDeclaration -> { + context.getString(R.string.msg_extract_method_captured_local_declaration, refusal.name) + } + } + + /** 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 f08014bcbc..052705d8b2 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 @@ -94,5 +94,6 @@ object JavaCodeActionsMenu : IActionsMenuProvider { TooltipTag.EDITOR_CODE_ACTIONS_TRY_CATCH, ), ExtractVariableAction(), + ExtractMethodAction(), ) } diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEdit.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEdit.kt new file mode 100644 index 0000000000..48db9f7b92 --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEdit.kt @@ -0,0 +1,111 @@ +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.detectIndentUnit +import com.itsaky.androidide.lsp.refactor.detectNewline +import com.itsaky.androidide.lsp.refactor.leadingIndentAt + +/** + * The two replacements an extraction performs: the new method, and the call that replaces the region. + * + * **Descending document order is mandatory, not stylistic.** `IDELanguageClientImpl.applyActionEdits` + * iterates the list and applies each edit with line/column ranges against whatever the text is at that + * moment, so an earlier edit must never shift a later one. In Java the new method always leads: the + * anchor member *contains* the region, so its end is always past the region's. + * + * Nothing on that path calls `beginBatchEdit`, so this costs the user **two** undo steps and the + * intermediate state does not compile. ADFA-5081 fixes that by batching the edit loop; until it lands + * the two-step undo is a stated limitation. + * + * Returns null when the offsets cannot be honoured, which the caller reports rather than applying. + */ +fun buildExtractMethodRewrites( + fileText: String, + candidate: ExtractMethodCandidate, + name: String, +): List? { + val span = candidate.span + if (span.end > fileText.length) return null + if (candidate.insertOffset > fileText.length) return null + // The anchor member contains the region, so anything else means the plan and the text disagree. + if (candidate.insertOffset < span.end) return null + + val newline = detectNewline(fileText) + val indent = candidate.insertIndent + val bodyIndent = indent + detectIndentUnit(fileText) + val regionText = fileText.substring(span.start, span.end) + val baseIndent = leadingIndentAt(fileText, span.start) + + val lines = indentedBodyLines(regionText, span.start, baseIndent, bodyIndent, newline, candidate.textBlockSpans) + val bodyLines = + when (val body = candidate.body) { + is ExtractedBody.ExpressionBody -> { + // An expression carries no `;` of its own -- the source one sits outside its span -- so the + // statement it becomes gets one here. + val returned = + if (body.needsReturn) { + // The first line is never inside a text block's interior -- the region starts at the + // code itself -- so it always carries bodyIndent and `return ` goes straight after it. + listOf(bodyIndent + "return " + lines.first().substring(bodyIndent.length)) + lines.drop(1) + } else { + lines + } + returned.dropLast(1) + (returned.last() + ";") + } + + is ExtractedBody.StatementBody -> { + lines + listOfNotNull(body.trailingReturn?.let { bodyIndent + it }) + } + } + + val declaration = + buildString { + append(indent).append(candidate.signatureText(name)).append(" {").append(newline) + bodyLines.forEach { append(it).append(newline) } + append(indent).append('}') + } + + val call = "$name(${candidate.parameters.joinToString(", ") { it.name }})" + val callText = + when (val form = candidate.callSite) { + CallSiteForm.Call -> call + CallSiteForm.CallStatement -> "$call;" + is CallSiteForm.AssignOutput -> "${form.typeText} ${form.name} = $call;" + CallSiteForm.Return -> "return $call;" + } + + return listOf( + RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), newline + newline + declaration), + RewriteSpan(span, callText), + ).sortedByDescending { it.span.start } +} + +/** + * The region's lines at the new method's body indentation: the original base indentation removed and + * [bodyIndent] put in its place. Lines nested deeper than the base keep the extra depth; the first line + * only gains the indent, since the span starts at the code itself. + * + * A line inside one of [protectedSpans] is emitted byte-for-byte. Those are text block literals, whose + * interior whitespace is part of their value and whose closing delimiter sets the incidental-whitespace + * margin, so moving either edits the interior of the moved code (ADR 0014). + */ +private fun indentedBodyLines( + regionText: String, + regionStart: Int, + baseIndent: String, + bodyIndent: String, + newline: String, + protectedSpans: List, +): List { + var offset = regionStart + return regionText.split(newline).mapIndexed { index, line -> + val lineStart = offset + offset += line.length + newline.length + when { + index == 0 -> bodyIndent + line + protectedSpans.any { lineStart > it.start && lineStart < it.end } -> line + else -> bodyIndent + line.removePrefix(baseIndent) + } + } +} diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractMethodUi.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractMethodUi.kt new file mode 100644 index 0000000000..0cd5ba23fe --- /dev/null +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/refactor/JavaExtractMethodUi.kt @@ -0,0 +1,31 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.itsaky.androidide.lsp.ui.ExtractMethodSelection +import com.itsaky.androidide.lsp.ui.MethodCandidateView + +/** + * The plan as the shared sheet sees it: labels, names and the two halves of the signature, no trees and + * no offsets. + * + * Offsets stay on this side deliberately -- the sheet is a chooser, and resolving a selection back into + * a candidate is [candidateFor]'s job. + */ +fun ExtractMethodPlan.toMethodCandidateViews(): List = + candidates.map { candidate -> + MethodCandidateView( + label = candidate.label, + suggestedName = candidate.suggestedName, + takenNames = candidate.takenNames, + signaturePrefix = candidate.signaturePrefix, + signatureSuffix = candidate.signatureSuffix, + ) + } + +/** + * Resolves a selection's index back to the plan it came from, or null when it does not address it. + * + * A null is a wiring bug rather than a user path -- the sheet only ever reports an index it was given -- + * so the caller reports it as a failed quick fix rather than guessing at a candidate. + */ +fun ExtractMethodPlan.candidateFor(selection: ExtractMethodSelection): ExtractMethodCandidate? = + candidates.getOrNull(selection.candidateIndex) diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEditTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEditTest.kt new file mode 100644 index 0000000000..615adff766 --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodEditTest.kt @@ -0,0 +1,136 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * The emitted text (R15): the two edits and their order, the call-site forms, and the whitespace the + * file already uses. + * + * Code-action edits bypass the editor's auto-indent, so what is emitted here is what the user sees. + */ +@RunWith(JUnit4::class) +class ExtractMethodEditTest { + private val fixtures = mutableListOf() + + @After + fun tearDown() = fixtures.forEach(JavacFixture::close) + + @Test + fun `the two edits are in descending document order`() { + val f = fixture(""" int m(int a, int b) {${'\n'} return a + b;${'\n'} }""") + val plan = f.methodPlanAfter("a +") + + val rewrites = buildExtractMethodRewrites(plan.fileText, plan.candidates.first(), "sum")!! + + assertThat(rewrites).hasSize(2) + assertThat(rewrites[0].span.start).isGreaterThan(rewrites[1].span.start) + // The insertion leads: the anchor member contains the region, so its end is always past it. + assertThat(rewrites[0].span.length).isEqualTo(0) + } + + @Test + fun `the new method is separated by a blank line at the anchor's indentation`() { + val f = fixture(""" int m(int a, int b) {${'\n'} return a + b;${'\n'} }""") + + val out = f.applyMethod(f.methodPlanAfter("a +"), "sum") + + assertThat(out).contains("\t}\n\n\tprivate int sum(int a, int b) {\n\t\treturn a + b;\n\t}") + } + + @Test + fun `a statement range call site is a statement`() { + val f = fixture(""" void m(int a) {${'\n'} use(a);${'\n'} }""") + + val out = f.applyMethod(f.methodPlanOver("use(a);"), "report") + + assertThat(out).contains("\t\treport(a);\n") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a multi-statement body keeps its relative indentation`() { + val f = + fixture( + """ void m(int a) {${'\n'} if (a > 0) {${'\n'} use(a);${'\n'} }${'\n'} }""", + ) + + val out = f.applyMethod(f.methodPlanOver("if (a > 0) {${'\n'} use(a);${'\n'} }"), "report") + + assertThat(out).contains("\tprivate void report(int a) {\n\t\tif (a > 0) {\n\t\t\tuse(a);\n\t\t}\n\t}") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a space-indented file receives space-indented output`() { + val f = + JavacFixture( + "class Fixture {\n int m(int a, int b) {\n return a + b;\n }\n}", + ).also { fixtures += it } + + val out = f.applyMethod(f.methodPlanAfter("a +"), "sum") + + assertThat(out).contains("\n private int sum(int a, int b) {\n return a + b;\n }") + assertThat(out).doesNotContain("\t") + } + + @Test + fun `a CRLF file keeps CRLF`() { + val f = + JavacFixture( + "class Fixture {\r\n\tint m(int a, int b) {\r\n\t\treturn a + b;\r\n\t}\r\n}", + ).also { fixtures += it } + + val out = f.applyMethod(f.methodPlanAfter("a +"), "sum") + + assertThat(out).contains("\r\n\r\n\tprivate int sum(int a, int b) {\r\n") + assertThat(out.replace("\r\n", "")).doesNotContain("\n") + } + + @Test + fun `a text block interior is emitted verbatim`() { + val quotes = "\"\"\"" + val f = + JavacFixture( + "class Fixture {\n" + + "\tString m() {\n" + + "\t\treturn $quotes\n" + + "\t\t\tone\n" + + "\t\t\t two\n" + + "\t\t\t$quotes;\n" + + "\t}\n" + + "}", + ).also { fixtures += it } + + val out = f.applyMethod(f.methodPlanOver("return $quotes\n\t\t\tone\n\t\t\t two\n\t\t\t$quotes;"), "banner") + + // The literal's own lines keep their original columns; only the statements around them move. + assertThat(out).contains("\t\t\tone\n\t\t\t two\n\t\t\t$quotes;") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a rewrite is refused when the insertion point is inside the region`() { + val f = fixture(""" int m(int a, int b) {${'\n'} return a + b;${'\n'} }""") + val candidate = f.methodPlanAfter("a +").candidates.first() + + val broken = candidate.copy(insertOffset = candidate.span.start + 1) + + assertThat(buildExtractMethodRewrites(f.text, broken, "sum")).isNull() + } + + private fun fixture(body: String) = + JavacFixture( + """ + |class Fixture { + |$body + | static void use(int value) {} + | static void use(Object value) {} + |} + """.trimMargin(), + ).also { fixtures += it } +} diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodPlanTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodPlanTest.kt new file mode 100644 index 0000000000..56d00d78ba --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodPlanTest.kt @@ -0,0 +1,382 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * One case per analysis rule (R5-R10) and one per refusal reason (R14). + * + * Where a rule exists so the result compiles -- `throws`, `static`, captured types -- the case feeds + * the rewritten file back through javac. A signature that merely looks plausible is exactly the failure + * mode these rules exist to prevent, and asserting on emitted text alone would not catch it. + */ +@RunWith(JUnit4::class) +class ExtractMethodPlanTest { + private val fixtures = mutableListOf() + + @After + fun tearDown() = fixtures.forEach(JavacFixture::close) + + // --- parameters (R5) --- + + @Test + fun `captured locals become parameters in first-use order`() { + val f = + fixture( + """ void m() {${'\n'} int a = 1;${'\n'} String s = "x";${'\n'} use(a);${'\n'} use(s);${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(a);${'\n'} use(s);") + + val candidate = plan.candidates.single() + assertThat(candidate.parameters.map { "${it.typeText} ${it.name}" }) + .containsExactly("int a", "String s") + .inOrder() + assertWithMessage(f.applyMethod(plan, "report")).that(compiles(f.applyMethod(plan, "report"))).isTrue() + } + + @Test + fun `a field needs no parameter`() { + val f = + fixture( + """ private int count = 1;${'\n'} void m() {${'\n'} use(count);${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(count);") + + assertThat(plan.candidates.single().parameters).isEmpty() + val out = f.applyMethod(plan, "report") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + // --- return type and call site (R6) --- + + @Test + fun `a void expression produces a void method and a bare call`() { + val f = fixture(""" void m(int a) {${'\n'} use(a);${'\n'} }""") + + val candidate = f.methodPlanAfter("use").candidates.first { it.label == "use(a)" } + + assertThat(candidate.returnTypeText).isEqualTo("void") + assertThat(candidate.callSite).isEqualTo(CallSiteForm.Call) + } + + @Test + fun `an expression produces a returning method`() { + val f = fixture(""" int m(int a, int b) {${'\n'} return a + b;${'\n'} }""") + + val plan = f.methodPlanAfter("a +") + + val candidate = plan.candidates.first() + assertThat(candidate.returnTypeText).isEqualTo("int") + assertThat(candidate.signatureText("sum")).isEqualTo("private int sum(int a, int b)") + val out = f.applyMethod(plan, "sum") + assertThat(out).contains("return sum(a, b);") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + // --- outputs (R7) --- + + @Test + fun `a single output is assigned back at the call site`() { + val f = + fixture( + """ int m() {${'\n'} int total = 0;${'\n'} total = total + 1;${'\n'} return total;${'\n'} }""", + ) + + val plan = f.methodPlanOver("int total = 0;${'\n'} total = total + 1;") + + val candidate = plan.candidates.single() + assertThat(candidate.callSite).isEqualTo(CallSiteForm.AssignOutput("int", "total")) + assertThat(candidate.returnTypeText).isEqualTo("int") + val out = f.applyMethod(plan, "runningTotal") + assertThat(out).contains("int total = runningTotal();") + assertThat(out).contains("return total;") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `two outputs are declined and named`() { + val f = + fixture( + """ int m() {${'\n'} int a = 1;${'\n'} int b = 2;${'\n'} return a + b;${'\n'} }""", + ) + + val plan = f.methodPlanOver("int a = 1;${'\n'} int b = 2;") + + assertThat(plan.candidates).isEmpty() + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.MultipleOutputs(listOf("a", "b"))) + } + + @Test + fun `assigning a variable declared outside the region is declined and named`() { + val f = + fixture( + """ int m(int[] values) {${'\n'} int sum = 0;${'\n'} for (int v : values) {${'\n'} sum += v;${'\n'} }${'\n'} return sum;${'\n'} }""", + ) + + val plan = f.methodPlanOver("for (int v : values) {${'\n'} sum += v;${'\n'} }") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.ReassignsOuterVar("sum")) + } + + @Test + fun `writing through a captured array is not a reassignment`() { + val f = + fixture( + """ void m(int[] values) {${'\n'} values[0] = 1;${'\n'} use(values[0]);${'\n'} }""", + ) + + val plan = f.methodPlanOver("values[0] = 1;${'\n'} use(values[0]);") + + assertThat(plan.refusal).isNull() + val out = f.applyMethod(plan, "seed") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + // --- exits (R8) --- + + @Test + fun `a return in the middle of the region is declined`() { + val f = + fixture( + """ int m(int x) {${'\n'} if (x > 0) {${'\n'} return 1;${'\n'} }${'\n'} use(x);${'\n'} return 0;${'\n'} }""", + ) + + val plan = f.methodPlanOver("if (x > 0) {${'\n'} return 1;${'\n'} }${'\n'} use(x);") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.ExitsRegion) + } + + @Test + fun `a break targeting a loop inside the region is not an exit`() { + val f = + fixture( + """ void m(int n) {${'\n'} for (int i = 0; i < n; i++) {${'\n'} if (i == 2) {${'\n'} break;${'\n'} }${'\n'} }${'\n'} use(n);${'\n'} }""", + ) + + val plan = f.methodPlanOver("for (int i = 0; i < n; i++) {${'\n'} if (i == 2) {${'\n'} break;${'\n'} }${'\n'} }") + + assertThat(plan.refusal).isNull() + val out = f.applyMethod(plan, "scan") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a break targeting a loop outside the region is declined`() { + val f = + fixture( + """ void m(int n) {${'\n'} for (int i = 0; i < n; i++) {${'\n'} use(i);${'\n'} break;${'\n'} }${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(i);${'\n'} break;") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.ExitsRegion) + } + + @Test + fun `a tail return moves with the region`() { + val f = + fixture( + """ int m(int a, int b) {${'\n'} use(a);${'\n'} return a + b;${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(a);${'\n'} return a + b;") + + val candidate = plan.candidates.single() + assertThat(candidate.callSite).isEqualTo(CallSiteForm.Return) + assertThat(candidate.returnTypeText).isEqualTo("int") + val out = f.applyMethod(plan, "finish") + assertThat(out).contains("return finish(a, b);") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a return belonging to a lambda inside the region is not an exit`() { + val f = + fixture( + """ void m(java.util.List items) {${'\n'} items.removeIf(s -> {${'\n'} return s.isEmpty();${'\n'} });${'\n'} }""", + ) + + val plan = f.methodPlanOver("items.removeIf(s -> {${'\n'} return s.isEmpty();${'\n'} });") + + assertThat(plan.refusal).isNull() + val out = f.applyMethod(plan, "prune") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + // --- modifiers and throws (R10) --- + + @Test + fun `a static anchor produces a static method`() { + val f = fixture(""" static int m(int a) {${'\n'} return a + 1;${'\n'} }""") + + val plan = f.methodPlanAfter("a +") + + assertThat(plan.candidates.first().modifiers).containsExactly("private", "static").inOrder() + val out = f.applyMethod(plan, "bumped") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a checked exception the region throws is declared`() { + val f = + fixture( + """ void m(java.io.InputStream in) throws java.io.IOException {${'\n'} in.read();${'\n'} }""", + ) + + val plan = f.methodPlanOver("in.read();") + + assertThat(plan.candidates.single().thrownTypes).containsExactly("java.io.IOException") + val out = f.applyMethod(plan, "drain") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a checked exception the region itself catches is not declared`() { + val f = + fixture( + """ void m(java.io.InputStream in) {${'\n'} try {${'\n'} in.read();${'\n'} } catch (java.io.IOException e) {${'\n'} use(e);${'\n'} }${'\n'} }""", + ) + + val plan = f.methodPlanOver("try {${'\n'} in.read();${'\n'} } catch (java.io.IOException e) {${'\n'} use(e);${'\n'} }") + + assertThat(plan.candidates.single().thrownTypes).isEmpty() + val out = f.applyMethod(plan, "drain") + assertThat(out).doesNotContain("throws") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `an unchecked exception is not declared`() { + val f = + fixture( + """ void m(int x) {${'\n'} if (x < 0) {${'\n'} throw new IllegalArgumentException("x");${'\n'} }${'\n'} }""", + ) + + val plan = f.methodPlanOver("if (x < 0) {${'\n'} throw new IllegalArgumentException(\"x\");${'\n'} }") + + assertThat(plan.candidates.single().thrownTypes).isEmpty() + val out = f.applyMethod(plan, "reject") + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a type parameter of the anchor method is declined and named`() { + val f = + fixture( + """ T m(T value) {${'\n'} use(value);${'\n'} return value;${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(value);") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.UsesTypeParameter("T")) + } + + // --- target (R4) --- + + @Test + fun `a region in an anonymous class anchors on that class`() { + val f = + fixture( + """ Runnable m(int a) {${'\n'} return new Runnable() {${'\n'} @Override${'\n'} public void run() {${'\n'} use(a + 1);${'\n'} }${'\n'} };${'\n'} }""", + ) + + val plan = f.methodPlanAfter("a +") + val out = f.applyMethod(plan, "bumped") + + // Inserted after run(), still inside the anonymous class body, so `};` still closes it last. + assertThat(out.indexOf("private int bumped")).isGreaterThan(out.indexOf("public void run()")) + assertThat(out.indexOf("private int bumped")).isLessThan(out.indexOf("};")) + assertWithMessage(out).that(compiles(out)).isTrue() + } + + @Test + fun `a local class the region uses but does not contain is declined`() { + val f = + fixture( + """ void m() {${'\n'} class Helper {${'\n'} int value() { return 1; }${'\n'} }${'\n'} Helper h = new Helper();${'\n'} use(h.value());${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(h.value());") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.CapturedLocalDeclaration("h")) + } + + @Test + fun `a local class the region constructs but does not declare is declined`() { + val f = + fixture( + """ void m() {${'\n'} class Helper {${'\n'} int value() { return 1; }${'\n'} }${'\n'} use(new Helper().value());${'\n'} }""", + ) + + val plan = f.methodPlanOver("use(new Helper().value());") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.CapturedLocalDeclaration("Helper")) + } + + @Test + fun `a local class the region declares and the code after it names is declined`() { + val f = + fixture( + """ void m() {${'\n'} class Helper {${'\n'} int value() { return 1; }${'\n'} }${'\n'} Helper h = new Helper();${'\n'} use(h.value());${'\n'} }""", + ) + + val plan = f.methodPlanOver("class Helper {${'\n'} int value() { return 1; }${'\n'} }") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.CapturedLocalDeclaration("Helper")) + } + + @Test + fun `a constructor delegation cannot be extracted`() { + val f = + fixture( + """ Fixture() {}${'\n'} Fixture(int a) {${'\n'} this();${'\n'} use(a);${'\n'} }""", + ) + + val plan = f.methodPlanOver("this();${'\n'} use(a);") + + assertThat(plan.refusal).isEqualTo(ExtractionRefusal.NotASingleRegion) + } + + // --- names (R12) --- + + @Test + fun `taken names include inherited members`() { + val f = fixture(""" int m(int a) {${'\n'} return a + 1;${'\n'} }""") + + val takenNames = + f + .methodPlanAfter("a +") + .candidates + .first() + .takenNames + + assertThat(takenNames).contains("toString") + assertThat(takenNames).contains("m") + } + + @Test + fun `a statement range is named extracted`() { + val f = fixture(""" void m(int a) {${'\n'} use(a);${'\n'} }""") + + val plan = f.methodPlanOver("use(a);") + + assertThat(plan.candidates.single().suggestedName).isEqualTo("extracted") + } + + private fun fixture(body: String) = + JavacFixture( + """ + |class Fixture { + |$body + | static void use(int value) {} + | static void use(Object value) {} + |} + """.trimMargin(), + ).also { fixtures += it } +} diff --git a/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodRegionTest.kt b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodRegionTest.kt new file mode 100644 index 0000000000..1be3caa937 --- /dev/null +++ b/lsp/java/src/test/java/com/itsaky/androidide/lsp/java/refactor/ExtractMethodRegionTest.kt @@ -0,0 +1,138 @@ +package com.itsaky.androidide.lsp.java.refactor + +import com.google.common.truth.Truth.assertThat +import openjdk.source.util.Trees +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +/** + * Region resolution (R2): which selection becomes which kind of region, and the two positions extract + * method accepts that extract variable must refuse. + * + * Parse-level only -- no capture, output or exit analysis reaches this layer. + */ +@RunWith(JUnit4::class) +class ExtractMethodRegionTest { + private val fixtures = mutableListOf() + + @After + fun tearDown() = fixtures.forEach(JavacFixture::close) + + @Test + fun `a bare cursor resolves to nested expressions innermost first`() { + val f = fixture(""" int m(int a, int b) {${'\n'} return compute(a + b);${'\n'} }${'\n'} int compute(int v) { return v; }""") + + val regions = f.regionsAfter("a +") + + assertThat(regions.map { it.text(f) }).containsExactly("a + b", "compute(a + b)").inOrder() + } + + @Test + fun `a selection over two whole statements is a statement range`() { + val f = fixture(""" void m() {${'\n'} int a = 1;${'\n'} use(a);${'\n'} }""") + + val regions = f.regionsOver("int a = 1;${'\n'} use(a);") + + val only = regions.single() as ExtractionRegion.Statements + assertThat(only.statements).hasSize(2) + assertThat(only.text(f)).isEqualTo("int a = 1;\n\t\tuse(a);") + } + + @Test + fun `a ragged selection snaps outward to whole statements`() { + val f = fixture(""" void m() {${'\n'} int a = 1;${'\n'} use(a);${'\n'} }""") + + // Starts inside `int` and stops inside `use(a)`, as a touch drag would. + val regions = f.regionsOver("nt a = 1;${'\n'} use(") + + val only = regions.single() as ExtractionRegion.Statements + assertThat(only.text(f)).isEqualTo("int a = 1;\n\t\tuse(a);") + } + + @Test + fun `a selection spanning two blocks resolves to nothing`() { + val f = + fixture( + """ void m(int x) {${'\n'} if (x > 0) {${'\n'} use(x);${'\n'} }${'\n'} use(1);${'\n'} }""", + ) + + // From inside the `if` body out to the statement after the `if`: two different blocks. + val regions = f.regionsOver("use(x);${'\n'} }${'\n'} use(1);") + + assertThat(regions).isEmpty() + } + + @Test + fun `a selection inside one statement prefers the expression it points at`() { + val f = fixture(""" void m(int a, int b) {${'\n'} use(a + b);${'\n'} }""") + + val regions = f.regionsOver("a + b") + + assertThat(regions.first()).isInstanceOf(ExtractionRegion.Expression::class.java) + assertThat(regions.first().text(f)).isEqualTo("a + b") + } + + // --- the two positions extract variable refuses and extract method does not (R2) --- + + @Test + fun `a cursor inside an expression statement offers the call`() { + val f = fixture(""" void m(java.util.List items) {${'\n'} items.add("x");${'\n'} }""") + + // Extract variable refuses it: replacing the expression with a name leaves a bare `v;`. + assertThat(f.planAfter("items.add").candidates.map { it.label }).doesNotContain("items.add(\"x\")") + // Extract method replaces it with a call, which is a statement. + assertThat(f.regionsAfter("items.add").map { it.text(f) }).contains("items.add(\"x\")") + } + + @Test + fun `a cursor on a short-circuit right operand offers a candidate`() { + val f = fixture(""" boolean m(String s) {${'\n'} return s != null && s.length() > 0;${'\n'} }""") + + // Hoisting it out of the guard would evaluate it unguarded, so extract variable declines. + assertThat(f.planAfter("s.length()").candidates).isEmpty() + // Substituting a call in place changes nothing about when it runs. + assertThat(f.regionsAfter("s.length()")).isNotEmpty() + } + + @Test + fun `a cursor on a loop condition offers a candidate`() { + val f = + fixture( + """ void m(java.util.Iterator it) {${'\n'} while (it.hasNext()) {${'\n'} use(it.next());${'\n'} }${'\n'} }""", + ) + + assertThat(f.planAfter("it.hasNext()").candidates).isEmpty() + assertThat(f.regionsAfter("it.hasNext()").map { it.text(f) }).contains("it.hasNext()") + } + + private fun ExtractionRegion.text(f: JavacFixture) = f.text.substring(span.start, span.end) + + private fun JavacFixture.regionsAfter(prefix: String): List { + val cursor = cursorAfter(prefix) + return regionsBetween(cursor, cursor) + } + + private fun JavacFixture.regionsOver(selection: String): List { + val start = text.indexOf(selection) + require(start >= 0) { "the fixture contains no '$selection'" } + return regionsBetween(start, start + selection.length) + } + + private fun JavacFixture.regionsBetween( + start: Int, + end: Int, + ): List = resolveExtractionRegions(task, root, Trees.instance(task).sourcePositions, text, start, end) + + private fun fixture(body: String) = + JavacFixture( + """ + |class Fixture { + |$body + | static void use(int value) {} + | static void use(Object value) {} + |} + """.trimMargin(), + ).also { fixtures += it } +} 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 index dbfcb1abda..0131c0bd64 100644 --- 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 @@ -101,6 +101,46 @@ class JavacFixture( ) ?: error("no rewrite for '$prefix' in scope '${option.label}'") return text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) } + + /** The extract-method plan for a bare cursor placed just after [prefix]. */ + fun methodPlanAfter( + prefix: String, + documentVersion: Int = 1, + ): ExtractMethodPlan { + val cursor = cursorAfter(prefix) + return buildExtractMethodPlan(task, root, text, cursor, cursor, documentVersion) + } + + /** + * The extract-method plan for a selection covering [selection] verbatim. + * + * Selecting by text rather than by offsets keeps a case readable and makes a snap-outward test say + * what it means: the selection is written exactly as a finger would have dragged it. + */ + fun methodPlanOver( + selection: String, + documentVersion: Int = 1, + ): ExtractMethodPlan { + val start = text.indexOf(selection) + require(start >= 0) { "the fixture contains no '$selection'" } + return buildExtractMethodPlan(task, root, text, start, start + selection.length, documentVersion) + } + + /** The file as it reads after applying [plan]'s candidate at [index] under [name]. */ + fun applyMethod( + plan: ExtractMethodPlan, + name: String, + index: Int = 0, + ): String { + val candidate = plan.candidates.getOrNull(index) ?: error("no candidate at $index: ${plan.refusal}") + val rewrites = buildExtractMethodRewrites(plan.fileText, candidate, name) ?: error("no rewrite for '$name'") + var result = text + // Descending, as the language client applies them: an earlier edit must not shift a later one. + rewrites.sortedByDescending { it.span.start }.forEach { rewrite -> + result = result.substring(0, rewrite.span.start) + rewrite.newText + result.substring(rewrite.span.end) + } + return result + } } /** Whether [source] compiles on its own, which is what most of these findings are really about. */