diff --git a/docs/adr/0013-refactorings-decline-rather-than-rewrite.md b/docs/adr/0013-refactorings-decline-rather-than-rewrite.md new file mode 100644 index 0000000000..d4b8898a04 --- /dev/null +++ b/docs/adr/0013-refactorings-decline-rather-than-rewrite.md @@ -0,0 +1,63 @@ +# 0013. Interactive refactorings decline rather than rewrite unselected code + +- **Status:** Proposed +- **Date:** 2026-08-10 +- **Deciders:** Code On The Go team + +## Context + +The K2 Kotlin LSP is growing a family of interactive refactorings: extract variable (ADFA-4826), extract method (ADFA-5080), inline variable (ADFA-4827), semantic rename (ADFA-4825). [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) settles where their UI lives and that analysis produces plain data. It says nothing about how capable they should be. + +That question turns out to dominate the requirements. Designing extract method surfaced a run of cases where the transformation the user asked for cannot be performed by *moving* their code - it also needs the moved code's interior edited, or a guess about intent: + +- A `var` declared outside the selection and reassigned inside it. Kotlin has no `out` parameters, so the faithful emission is a parameter plus `var x = x` at the top of the body - which compiles, with a name-shadowing warning. +- Two or more values flowing out of the selection. There is no tuple to return that the user would have written themselves. +- A `return` in the middle of the selection. Real IDEs encode the exit in a nullable or sentinel return and re-test it at the call site. +- Members of an enclosing `with`/`apply`/`run` receiver used unqualified. They can only survive as a parameter if every unqualified access inside the body is qualified. +- A type parameter declared on the enclosing function. It needs a filtered copy of the type-parameter list with its bounds. + +Desktop IDEs handle most of these, and their users accept the result because they can read a multi-file diff, undo granularly, and fix up whatever the refactoring got slightly wrong. Code On The Go's users are on a phone: a small screen, no side-by-side diff, imprecise touch selection, and - per ADFA-5081 - a code-action edit history that is not even reliably one undo step yet. Many are also students, for whom generated code carrying a fresh compiler warning is indistinguishable from a broken tool. + +## Decision + +**An interactive refactoring moves the user's code. It does not edit the interior of what it moved, and where it cannot transform faithfully it declines with a specific, actionable reason.** + +Concretely: + +- **Refusal is a designed outcome, not an error.** Each refactoring's plan carries a typed reason (extract method: `ExtractionRefusal`), and each reason has its own user-facing message naming the construct in the way - "the selection assigns to `total`, which is declared outside it", not "cannot extract". +- **Prefer excluding a case by construction over filtering it later.** Extract method accepts only sibling statements in one block; extract variable rejects bare literals and expression fragments up front. Both remove whole classes of hard case before any analysis runs. +- **Prefer a stricter rule to a cleverer one** when strictness costs capability and cleverness costs certainty. Extract method refuses a reassigned outer `var` even when the write is provably dead, because proving it needs liveness analysis. +- **Never emit code that does not compile, and avoid emitting code that warns.** The two modifiers extract method *does* add - `suspend` and `@Composable` - are required precisely because omitting them breaks compilation. +- **A refusal is a backlog item, not a dead end.** Where the refused case is common, file it: ADFA-5082 tracks the reassigned-`var` output. + +This applies to the whole refactoring family, not just extract method. Inline variable and rename inherit it. + +## Consequences + +**Positive** + +- Every applied refactoring produces code the user could have written, so the feature earns trust on a device where verifying the result is expensive. +- Refusal reasons are cheap to specify, cheap to test (one case each) and cheap to QA, where a clever transformation needs its own test matrix and its own failure modes. +- The rules are stateable in a sentence each, which is what makes the feature docs reviewable by someone who has not read the implementation. +- Excluding cases by construction keeps the analysis pass small, which matters when it runs on a phone. + +**Negative / costs** + +- The refactorings are visibly less capable than a desktop IDE's. Two of extract method's refusals - a reassigned outer `var` (the accumulator loop) and an enclosing `with`/`apply` receiver (pervasive in Android code) - will be hit routinely. +- The quality of the *messages* becomes load-bearing. A generic refusal reads as a broken feature, so this decision spends translated strings: roughly seven for extract method alone. +- Users arriving from IntelliJ will read some refusals as regressions rather than as design. +- The line is a judgement, not a formalism. "Editing the interior of the moved code" is clear in the cases above but will need re-application, case by case, in each future refactoring. + +## Alternatives considered + +- **Match desktop IDE capability.** Handle multiple outputs, mid-selection returns, receiver capture and type parameters, as IntelliJ does. Rejected: each requires rewriting the body's interior or inventing a signature the user did not ask for, and the cost of getting it subtly wrong is paid on a device where the user can least easily see it. +- **Transform, but warn.** Apply the refactoring and flash a caveat ("check the result"). Rejected: it puts the verification burden on the person least equipped to do it, and a warning shown once is gone before the user reads the code. +- **Transform behind a setting**, off by default. Rejected: it doubles the behaviour to test and support for a feature whose hard cases are exactly the ones a setting's users would hit first. Revisit only if specific refusals prove to be common complaints - which is what ADFA-5082 exists to measure. +- **One generic refusal message.** Cheapest, and consistent with extract variable's single "nothing to extract". Rejected as a direct consequence of this decision: if declining is the primary answer in hard cases, the decline has to teach. + +## Related + +- [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - where refactoring UI lives; this ADR answers *how capable it is* +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth +- [kotlin-extract-method.md](../features/kotlin-extract-method.md) - R7 to R10 and R14 are this decision applied case by case +- [kotlin-extract-variable.md](../features/kotlin-extract-variable.md) - the shared vocabulary and primitives diff --git a/docs/adr/README.md b/docs/adr/README.md index 554f429bee..26767bbd3f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -26,3 +26,4 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed | | [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed | | [0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | +| [0013](0013-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | diff --git a/docs/features/kotlin-extract-method.md b/docs/features/kotlin-extract-method.md new file mode 100644 index 0000000000..041c51c66c --- /dev/null +++ b/docs/features/kotlin-extract-method.md @@ -0,0 +1,285 @@ +# Kotlin extract method (K2 LSP) + +- **Ticket:** ADFA-5080 (subtask of ADFA-3317; split out of ADFA-4826, which now covers extract variable only) +- **Status:** Implemented +- **Module:** `lsp/kotlin` +- **Vocabulary:** the term is **method**, matching the ticket and the already-fixed tooltip tag `editor.codeactions.kotlin.extractmethod`, even though the refactoring's output is a Kotlin `fun`. + +Move the expression at the cursor, or a selected range of statements, into a new function, and replace it with a call to that function. + +Ships as the top of a three-PR stack: `common-compose` theming, then extract variable (ADFA-4826), then this. It reuses that PR's primitives - offsets, naming, indentation, edit emission - and adds no new module, no new dependency and no new UI mechanism. + +The governing principle is [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md): this refactoring **moves** code, it never edits the interior of what it moved, and where it cannot do that faithfully it **declines with a specific reason** rather than guessing. Most of the requirements below are that principle applied to one case each. + +## Language + +Shared vocabulary - *selection*, *extraction region*, *expression candidate*, *text span*, *occurrence*, *refactoring plan*, *rewrite span* - is defined once in [kotlin-extract-variable.md](kotlin-extract-variable.md#language). This feature adds: + +**Statement range**: +One or more *sibling* statements inside a single `KtBlockExpression`, snapped outward from the selection to whole statement boundaries. The second kind of extraction region; the first is an expression candidate. +_Avoid_: statement list, block, selection. + +**Enclosing declaration**: +The named function, property accessor or `init` block whose body contains the extraction region. It is both the boundary that decides what becomes a parameter and the sibling anchor the new function is inserted after. +_Avoid_: parent function, host, owner. + +**Captured declaration**: +A declaration the region references whose PSI lies *inside* the enclosing declaration - a local, a function or lambda parameter, `it`, a destructuring entry, a loop variable. Each becomes a **parameter**. Anything else (class members, top-level declarations, imports) resolves unchanged from the new function body and needs no parameter. +_Avoid_: free variable, capture, dependency. + +**Output**: +The single value that flows out of the region and is still needed after it - a local declared inside the region and read after it. Zero outputs means the extracted function returns `Unit`; two or more is declined. +_Avoid_: result, return value (that's the extracted function's `return`, which an output is only one cause of). + +**Exit**: +A `return`, `break`, `continue` or non-local return inside the region whose target lies outside it. Declined, except the tail return (R8). +_Avoid_: jump, control flow, early return. + +**Refusal**: +A typed reason (`ExtractionRefusal`) the region could not be extracted, carried on the plan and rendered as a specific message. A refusal is a designed outcome, not an error. +_Avoid_: failure, error, invalid. + +## Scope + +### In scope + +An expression, or a range of sibling statements, inside any executable body - a function body, an accessor, an `init` block, a constructor, or a lambda - in a Kotlin file. + +### Out of scope + +The positions extract variable already rejects, for the same reasons and via the same `isExtractionPosition` check: annotation arguments, default parameter values, super-constructor delegation arguments, and anything outside an executable body (notably a class-body property initializer). + +## Requirements + +**R1 - Trigger.** An "Extract method" item (`action_extract_method`) in the editor code-actions menu for Kotlin files, id `ide.editor.lsp.kt.extractMethod`, tooltip tag `EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD = "editor.codeactions.kotlin.extractmethod"` - a new constant in `TooltipTag.kt`. The tag string is fixed: tooltip *content* lives in the out-of-repo tooltips database keyed by tag, so it cannot be renamed here. + +As with extract variable: **no `prepare()` visibility gate** (deciding extractability needs an analysis session, far too costly for the UI thread), and `requiresUIThread = false` so the selection is read on a background thread. + +**R2 - Region.** The selection resolves to exactly one extraction region, of one of two kinds. + +*Expression candidate* - reuses `candidateExpressionsAt` unchanged, including whitespace trimming, the `offset - 1` cursor retry, the innermost-first walk, `MAX_CANDIDATES = 3`, the legal-target rules and `selectionMatchedCandidate`. A bare cursor always takes this path. + +*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 `KtBlockExpression`**. A selection spanning two different blocks, or partially covering a statement that cannot be snapped, is declined (`NotASingleRegion`). + +Restricting to siblings in one block excludes every hard case - a selection covering half an `if` and half its `else`, a range straddling a lambda boundary - by construction rather than by later filtering, exactly as `isLegalExtractionTarget` excludes expression fragments today. + +**R3 - Live offsets and the version guard.** Identical to extract variable: analysis runs against `getCurrentKtFile(path)` fetched *before* entering `project.read`, the plan records the document version (on the `RefactoringPlan` supertype), and each action re-reads the version on confirm with a mismatch refusing the edit. + +**R4 - Target.** One uniform rule, no target picker: **the new function is inserted as a sibling of the enclosing declaration** - immediately after it, except for a local `fun` target, where it goes immediately *before* it. A local function is only visible from its declaration onward, so it has to be declared above the code that calls it; every other target has no such constraint. That one rule produces the conventional answer in every context: + +| The region sits in | The new function becomes | +|---|---| +| a member function, accessor or `init` of a class | a `private fun` member of that class | +| a top-level function or property | a `private` top-level `fun` | +| a lambda inside either of the above | still a sibling of the enclosing *named* declaration; the lambda's captures become parameters | +| a local `fun` or local class | a local `fun` in the enclosing block, since the sibling *is* a statement there | +| a companion object body | a member of the companion | + +Unlike extract variable there is no scope chain and no ceiling, because anything not visible at the insertion site becomes a parameter instead of constraining the anchor. + +**R5 - Parameters.** A referenced declaration needs a parameter exactly when it is a captured declaration - its PSI lies inside the enclosing declaration. Members of the enclosing class need nothing, because the new function is a member of that 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. `it` becomes a parameter literally named `it`, which is legal Kotlin, and the call site passes `it`. +- **Types** - the resolved type rendered **fully qualified** (`KaTypeRendererForSource.WITH_QUALIFIED_NAMES`), so `java.util.Date` rather than `Date`. Verbose, but a short name resolves only when the file already imports it, and a local's type usually comes from inference rather than a spelled-out type reference - this refactoring adds no imports. A **platform type** is emitted as its lower bound: the renderer prints `String!`, which does not parse, and the lower bound is both what IntelliJ writes and what the moved body already assumes. A type that cannot be rendered - anonymous, intersection, a resolution failure, or a `!` the lower bound did not remove (a platform type on a type *argument*) - **declines the extraction** (`UnrenderableType`) rather than emitting uncompilable text. A value whose type is a class declared inside the enclosing declaration declines too (`CapturedLocalDeclaration`): the value survives the move, its type name does not. +- **Not editable.** The derived signature is shown read-only (R11). Renaming, reordering or excluding parameters is a desktop-sized dialog; a wrong parameter *name* is fixable afterwards with rename (ADFA-4825), and a wrong parameter *set* is not something the user could correct by hand anyway. + +**R6 - Return type and call-site form.** Determined by the region kind and its output: + +| Case | Extracted body | Call site | +|---|---|---| +| expression candidate | `return ` | `extracted(args)` in the expression's place | +| statement range, no output | the statements; returns `Unit` | `extracted(args)` as a statement | +| statement range, one output `x` | the statements, then `return x` | `val x = extracted(args)` | +| statement range, tail return (R8) | the statements including the `return` | `return extracted(args)` | + +A region that always throws still declares `Unit`; the exception propagates and the call site behaves identically, so `throw` needs no rule of its own. + +**R7 - Outputs.** An output is a local declared inside the region and read after it. Exactly one plain `val`/`var` is supported; **two or more declines** (`MultipleOutputs`, naming them), and a single output the call site cannot receive back declines separately (`OutputNotReturnable`, naming it) - a destructuring entry or a local `fun`, which a `val` cannot stand in for, or a local the following code reassigns, which a `val` cannot be. The two are distinct refusals because "produces more than one value" is simply untrue of the second, and a refusal that misdescribes the situation teaches nothing. + +A `var` declared outside the region and **reassigned inside it declines** (`ReassignsOuterVar`, naming the variable), because Kotlin has no `out` parameters and the faithful emission - a parameter plus `var x = x` at the top of the body - carries a name-shadowing warning into generated code. This is deliberately stricter than dataflow requires: a reassignment whose result is never read afterwards is still refused, because proving that needs real liveness analysis. ADFA-5082 tracks supporting it. + +The refused case is the accumulator loop, which is a genuinely common extraction, so its message must name the variable and read as a limitation rather than a malfunction. + +**R8 - Exits.** Every exit declines (`ExitsRegion`), with one syntactic exception. + +**Tail return:** when the region's *last* statement is a `return`, the region contains no other `return`, `break` or `continue`, and there is no other output, the extracted function takes the enclosing function's return type, keeps the `return`, and the call site becomes `return extracted(args)`. "Extract the rest of this function into a helper" is one of the most common real extractions and the enabling check is purely syntactic - last-child kind plus a recursive absence check - so it costs a predicate and one call-site form, not an analysis. + +One exception to "the enclosing function's return type": a **secondary constructor** is treated as `Unit`. Its symbol's return type is the constructed class, but its `return` carries no value, so taking that type would emit both a bare `return` in a value-returning function and a call site returning the wrong thing. `return extracted(args)` on a `Unit`-valued call is legal inside a constructor. An `init` block needs no rule - `return` is illegal there, so no tail return can arise. + +Declined: a `return` anywhere but the tail position, a `break`/`continue` whose target loop is outside the region, a labelled `return@` whose target is outside it, and a non-local return from an inlined lambda. Each would silently change meaning, since a `return` in the extracted body returns from *it*. + +**R9 - Receivers.** + +- **Class dispatch receiver** - nothing to do; the new function is a member of the same class. +- **The enclosing declaration's extension receiver** - the new function is generated as an extension on the **same receiver type**, copied syntactically from the enclosing declaration's receiver type reference. The call site needs no change at all: inside `fun Foo.original()`, `this` is a `Foo`, so `extracted(args)` resolves to `private fun Foo.extracted(args)`. +- **An implicit receiver introduced inside the enclosing declaration** - the `with(x) { ... }` / `apply` / `run` / `buildString` case - **declines** (`InnerImplicitReceiver`). Turning that receiver into a parameter would require qualifying every unqualified member access inside the extracted body, which is editing the interior of the moved code. Android code uses these scoping functions heavily, so this refusal will be common and its message must say which construct is in the way. + +**R10 - Modifiers.** Copy nothing from the enclosing declaration; add only what the body needs in order to compile in its new home. + +- **Visibility** - always `private`, whether a class member or top-level. Never `internal`, never `open`, no annotations copied, no KDoc generated. +- **`suspend`** - added when any call in the region resolves to a suspend function, or the region references `coroutineContext`. The call site is necessarily already a suspend context. **Not** added for a suspension the region only performs inside a *nested* suspend-typed lambda - `scope.launch { }`, `runBlocking { }`, any `suspend () -> T` parameter: the region carries that lambda with it, so the new function needs no `suspend`, and adding it breaks a call site that is not itself a suspend context. An ordinary inline lambda (`forEach`, `let`, `run`) is not one of these and still propagates `suspend` outwards. +- **`@Composable`** - added when any call in the region resolves to a `@Composable`-annotated function. This is not polish: CoGo users write Compose apps on the device, and an extracted composable without the annotation does not compile. +- **Function-level type parameters** - a region referencing a type parameter declared on the *enclosing function* **declines** (`UsesTypeParameter`, naming it). Class-level type parameters need no rule; they stay in scope for a member. A filtered copy of the enclosing type-parameter list with its bounds would mean deciding "is `T` referenced" from rendered type text, which is fragile. + +`suspend` and `@Composable` are the two cases where omitting a modifier produces non-compiling code, which is why they are requirements while everything else is left off. + +**R11 - Sheet.** A sibling of the extract-variable sheet, not a generalisation of it: `ExtractMethodSheet` (a `BottomSheetDialogFragment` hosting a `ComposeView`), a stateless `ExtractMethodSheetContent`, `ExtractMethodViewModel` + `ExtractMethodUiState` + a sealed `ExtractMethodUiEvent`. `LabelledSection` and `OptionList` are promoted to a shared internal file in `refactor/ui/`. + +Contents, top to bottom: title -> expression chooser (only for an expression region with more than one candidate and no exact selection match) -> name field with its `NameProblem` message -> signature preview -> Cancel/Extract. There is **no scope chooser** (R4) and **no replace-all checkbox** (R13). + +The preview is **one monospace line: the signature exactly as it will be emitted** - modifiers, receiver, parameters, return type. Types render fully qualified (R5), so a real preview reads `private suspend fun loadUser(id: kotlin.String): com.example.User`. It wraps rather than truncating. No body preview: the body is the code the user selected and can see behind the sheet, so it moves verbatim and previewing it says nothing new, while the signature is the one derived artefact and the one place the derivation can surprise them. + +ADR 0012 defers the shared-UI question until the extract-method surface is known; a single generalised sheet would need a state class where half the fields are meaningless to either caller, so that question stays open rather than being settled from one data point. + +**R12 - Name.** Suggestion: for an expression region, the existing shape/type derivation unchanged; for a statement range, the constant `extracted`, since there is no expression to read a name from and inventing a verb from statement shapes is guesswork. Uniquified as today. + +Validation reuses `validateVariableName` and `NameProblem` unchanged - so no new error strings - with taken names being **every callable name visible in the insertion container, including inherited members** (the container's `memberScope`, not just its declared members) for a class target; every top-level declaration name in the file for a top-level target; enclosing-block declarations for a local target. + +Including inherited names is a correctness requirement, not a nicety: a private function accidentally matching a supertype member is an accidental-override compile error. Rejecting *any* name match rather than only a signature match also means the refactoring never creates an overload the user did not ask for. + +**R13 - One call site.** The region is the only site rewritten. No duplicate detection, no replace-all toggle: exact-duplicate matching would almost never fire, and near-duplicate matching needs anti-unification plus a per-site parameter mapping - a feature in its own right. `Occurrences.kt` is expression-granular by construction. + +**R14 - Refusals.** The plan carries a typed `ExtractionRefusal` rather than merely being empty, and `postExec` maps it to a specific message: + +| Reason | Message intent | +|---|---| +| `NotASingleRegion` | select an expression, or whole statements inside one block | +| `CouldNotAnalyse` | the analysis could not run - deliberately neutral, since the selection may have been fine | +| `MultipleOutputs` | the selection produces more than one value | +| `OutputNotReturnable` | the selection produces ``, which cannot be handed back as a return value | +| `ReassignsOuterVar` | the selection assigns to ``, declared outside it | +| `ExitsRegion` | the selection jumps out of itself (`return`/`break`/`continue`) | +| `InnerImplicitReceiver` | the selection uses members of an enclosing `with`/`apply` receiver | +| `UsesTypeParameter` | the selection uses type parameter `` | +| `UnrenderableType` | a type in the selection cannot be written out | +| `UsesBackingField` | the selection uses the property's backing field, only reachable inside this accessor | +| `SmartCastParameter` | the selection uses `` under a smart cast that does not hold outside it | +| `CapturedLocalDeclaration` | the selection uses ``, which goes out of scope once the selection moves | + +All but `CouldNotAnalyse` are actionable - they tell the user what to change - and several (`ReassignsOuterVar`, `InnerImplicitReceiver`, `UsesBackingField`) are common enough that a generic message would read as the feature being broken. `CouldNotAnalyse` exists precisely so the others stay truthful: a missing compilation environment, an unreachable `KtFile` or a thrown analysis error must not be reported as `NotASingleRegion`, which blames a selection nothing ever looked at. Given how much of this design is "decline cleanly", the refusal text is a first-class part of the feature. New entries in `resources/.../values/strings.xml`, picked up by the next translation batch. + +Cancellation is not a refusal at all: `buildExtractMethodPlan` re-throws `CancellationException` (which `AnalysisPreemptedException` is), so a cancelled action ends silently rather than flashing at a user who has moved on. + +The refusal lives on `ExtractMethodPlan` only; extract variable keeps its single "nothing to extract" behaviour unchanged. + +**R15 - Edit.** Two regions change - the region becomes a call, and the new function appears next to the enclosing declaration - emitted as **two `TextEdit`s in one `DocumentChange`, sorted by descending start offset**. + +The ordering is mandatory, not stylistic. `IDELanguageClientImpl.applyActionEdits` iterates the edit list in order and `editInEditor` applies each with **line/column** ranges against whatever the text is at that moment (the `index` in `Position` is ignored), so an earlier edit must never shift a later one. Which edit leads follows from R4 rather than being fixed: a member or top-level target is inserted *after* its anchor, so the new function leads; a **local `fun` target is inserted before** its anchor, so the call site 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. This knowingly diverges from `RewriteSpan`'s single-replacement rule, which extract variable relies on. **ADFA-5081** fixes it properly 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 function is emitted **fully indented** at the enclosing declaration's own indentation, separated by one blank line, reusing `detectIndentUnit`, `detectNewline`, `leadingIndentAt` and `positionAt`. Code-action edits bypass the editor's auto-indent and `CMD_FORMAT_CODE` is a no-op for Kotlin. + +**R16 - Responsiveness and failure isolation.** As extract variable: one background pass at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine produces the whole plan; the sheet does pure string and offset arithmetic and re-enters no analysis on confirm. Anything thrown in the pipeline degrades to a refusal (`CouldNotAnalyse`) plus a log line, never an uncaught throw - the action framework catches only `IllegalArgumentException` and this runs on a scope with no exception handler. + +**`CancellationException` is the one deliberate exception**, and it is re-thrown rather than swallowed - `AnalysisPreemptedException` is one. A cancelled action has no result worth reporting, and `DefaultActionsRegistry.executeAction` launches into a scope whose `invokeOnCompletion` already treats a `CancellationException` as an ordinary cancel, so re-throwing ends the action quietly instead of flashing a message at a user who has moved on. Swallowing it would also break structured concurrency for whatever cancelled the job. The sheet's confirm path is outside the framework's guards entirely, so `ExtractMethodAction.applyChoice` 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 `var`** (R7). The latter is ADFA-5082. +- **Mid-region `return`/`break`/`continue`** (R8). +- **Inner `with`/`apply`/`run` receivers** (R9). +- **Function-level type parameters** (R10). +- **Choosing a different target** - another class, another file, a local `fun` when a member is possible, or a property instead of a function (R4). Moving a declaration elsewhere is a move refactoring. +- **Extraction from a property initializer or annotation argument** - inherited from `isExtractionPosition`. +- **Generated KDoc** for the new function. +- **Post-extract inline rename** of the new name in the editor - ADFA-4825. +- **Atomic undo** of the two edits - ADFA-5081. +- **Formatting the result.** R15 emits indented text instead. +- **Java extract method** - ADFA-5048. + +## Acceptance criteria + +1. "Extract method" appears in the code-actions menu of a Kotlin file and is absent in a non-Kotlin file. +2. A cursor inside an expression offers the innermost-first candidates; extracting one replaces it with a call and adds a `private fun` returning that expression, directly below the enclosing function. +3. Selecting two adjacent statements that use two locals produces a function with those two locals as parameters, in first-use order, and a call passing them. +4. A selection with ragged boundaries snaps outward to whole statements before extracting. +5. A selection spanning two different blocks reports "select an expression, or whole statements inside one block". +6. A range declaring a local that is read afterwards produces `val x = extracted(...)` at the call site. +7. A range declaring two locals that are both read afterwards is declined as producing more than one value. +8. Selecting a loop that accumulates into an outer `var` is declined, and the message names that variable. +9. Selecting the tail of a function ending in `return x` produces `return extracted(...)` and a function with the enclosing return type. +10. Selecting a range containing a `return` in the middle is declined. +11. Selecting a range with a `break` targeting a loop outside it is declined. +12. Extracting from inside `fun Foo.bar()` when the region touches `Foo`'s members produces `private fun Foo.extracted(...)`, and the call site is unchanged. +13. Extracting from inside a `with(x) { ... }` block whose region uses `x`'s members is declined, and the message names the construct. +14. A region calling a suspend function produces a `suspend fun`. +15. A region calling a `@Composable` produces a `@Composable` function that compiles. +16. A region using a type parameter of the enclosing function is declined, naming the parameter. +17. A name matching an existing member - including an inherited one - is rejected with "That name is already used". +18. The signature preview matches the emitted declaration exactly, including modifiers and receiver. +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. + +## Design + +Same shape as extract variable, and the same data boundary from [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md): one background pass produces a plain-data plan, the sheet holds no PSI. + +``` +ExtractMethodAction.execAction (background) lsp/kotlin/actions + server.compilationEnvironmentFor(path) ?: refusal + -> buildExtractMethodPlan(...) utils/refactor/ExtractMethodPlanner.kt + ktFile = env.ktSymbolIndex.getCurrentKtFile(path).get() [R3: before project.read] + env.project.read { + resolveRegion(ktFile, start, end) utils/refactor/ExtractionRegion.kt [R2] + expression -> candidateExpressionsAt(...) (reused unchanged) + statements -> snap outward, sibling check + analyzeMaybeDangling(INTERACTIVE, cancelChecker) { [R16] + captured declarations -> parameters utils/refactor/MethodSignature.kt [R5] + outputs / exits / receivers / modifiers [R6-R10] + -> ExtractMethodPlan | ExtractionRefusal [R14] + } + } + <- ExtractMethodPlan (plain data, no PSI) + +ExtractMethodAction.postExec (UI thread) + refusal -> flashInfo(message for reason) [R14] + ExtractMethodSheet.show refactor/ui [R11] + on confirm -> version re-read; mismatch -> refuse [R3] + buildExtractMethodRewrite -> two RewriteSpans utils/refactor/ExtractMethodEdit.kt [R15] + client.performCodeAction(one DocumentChange, two TextEdits, descending) +``` + +New files, all in `lsp/kotlin`: + +- **`utils/refactor/ExtractionRegion.kt`** - the region model and its resolution (R2). Purely syntactic, so unit-testable with no analysis session, exactly as `CandidateExpressions.kt` is. +- **`utils/refactor/MethodSignature.kt`** - captured declarations to parameters, outputs, exits, receivers, modifiers, and the rendered signature string (R5-R10). The only analysis-dependent part. +- **`utils/refactor/ExtractMethodPlan.kt`** - `ExtractMethodPlan` (a `RefactoringPlan` subtype) and `ExtractionRefusal`. +- **`utils/refactor/ExtractMethodPlanner.kt`** - the single background pass (R3, R16). +- **`utils/refactor/ExtractMethodEdit.kt`** - the two rewrites and their ordering (R15). Pure text and offsets. +- **`refactor/ui/ExtractMethod*.kt`** - sheet, content, ViewModel, state, events (R11). +- **`actions/ExtractMethodAction.kt`** - registered in `KotlinCodeActionsMenu`; the only class touching the editor, the document version or the language client. +- **`TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD`** - one new constant (R1). + +Reused from extract variable unchanged: `TextSpan`, `collapseForLabel`, `candidateExpressionsAt` / `CandidateSyntax`, `isExtractionPosition`, `enclosingExecutableBody`, `NameProblem` + `validateVariableName`, `suggestVariableName`, `detectIndentUnit`, `detectNewline`, `leadingIndentAt`, `lineStartOffset`, `RewriteSpan` + `toTextEdit`, `positionAt`, `renderName`. + +Deliberately **not** reused: `ScopeOption`, `AnchorForm` and `CandidateExpression`. Each is shaped by the legal scope chain, which this refactoring does not have (R4) - so the two refactorings share primitives, not the aggregate. What they do share is hoisted into the sealed `RefactoringPlan` (`fileText` and `documentVersion`), introduced in the extract-variable PR so this one is purely additive. The version *guard* itself - reading the live version and comparing - stays in each action rather than on the supertype, since it needs the `ActionData` and the action's own "file changed" string; hoisting it is a small cleanup, not a shared primitive today. + +Nothing outside `lsp/kotlin` changes except `TooltipTag.kt` and `values/strings.xml`. No new module, no new dependency. + +## Verification + +Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), mirroring the extract-variable split so a failure localises to one layer: + +- **`ExtractMethodRegionTest`** - no analysis session, PSI only: outward snapping to whole statements, the sibling-in-one-block rule, cross-block rejection, and the expression path (R2). +- **`ExtractMethodPlanEndToEndTest`** - analysis-backed, one case per rule: the parameter set, order and types (R5), the single output and the `Unit` case (R6, R7), the tail return (R8), the extension receiver (R9), `suspend` and `@Composable` (R10), and **one case per refusal reason** (R14). +- **`ExtractMethodEditTest`** - pure text: the two edits and their descending order, the three call-site forms, indentation, the blank-line separation, and CRLF preservation (R15). +- **`ExtractMethodViewModelTest`** - state derivation: chooser visibility, name validation against inherited names, and the rendered signature preview (R11, R12). + +`lsp/kotlin` has **no `androidTest`** source set, and none is added: `@Composable` detection is tested by declaring `package androidx.compose.runtime; annotation class Composable` in a test source module, and `suspend` is a language modifier, so both need **no new dependency** (`KtLspTestEnvironment` supports `extraLibraryJars`, but not for this). + +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-5080's "Steps to QA" field. + +## Related + +- [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code; the principle behind R7-R10 +- [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module +- [kotlin-extract-variable.md](kotlin-extract-variable.md) - ADFA-4826; owns the shared Language section and every primitive reused here +- ADFA-5081 - code action edits should be a single undo step (fixes R15's consequence) +- ADFA-5082 - support a reassigned outer `var` as the single output (lifts R7's refusal) +- ADFA-5048 - Java extract method, the sibling in `lsp/java` +- [ARCHITECTURE.md](../../ARCHITECTURE.md) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index 44e27107ef..f4592973f1 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -6,7 +6,7 @@ Bind the expression at the cursor, or the selected one, to a new local `val`, and replace the occurrences of that expression with the new name. -This is the first *interactive* Kotlin code action: the user chooses an expression, a name, a target scope and whether to replace other occurrences, so it needs a real UI surface rather than a fire-and-forget edit. Where that UI lives is [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do is the decline-rather-than-rewrite principle, recorded as ADR 0013 alongside extract method (ADFA-5080). +This is the first *interactive* Kotlin code action: the user chooses an expression, a name, a target scope and whether to replace other occurrences, so it needs a real UI surface rather than a fire-and-forget edit. Where that UI lives is [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do is [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md). ## Language @@ -264,8 +264,8 @@ Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotl ## Related - [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module -- ADR 0013 - refactorings decline rather than rewrite unselected code (lands with extract method, ADFA-5080) +- [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code - [ADR 0009](../adr/0009-jetpack-compose-for-new-ui.md) - Compose, UDF, `ViewModel` + `StateFlow` - [ADR 0010](../adr/0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth -- ADFA-5080 - extract method, the sibling refactoring; it reuses this vocabulary and these primitives +- [kotlin-extract-method.md](kotlin-extract-method.md) - ADFA-5080, the sibling refactoring - [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 ac8fd24d98..2295b925be 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -94,6 +94,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_KT_GOTO_DEF = "editor.codeactions.kotlin.gotodef" const val EDITOR_CODE_ACTIONS_KT_FIND_REFS = "editor.codeactions.kotlin.findrefs" const val EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE = "editor.codeactions.kotlin.extractvariable" + const val EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD = "editor.codeactions.kotlin.extractmethod" const val EXIT_TO_MAIN = "exit.to.main" diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt index 311d0dadd6..e9be630f20 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.kt @@ -6,6 +6,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.IActionsMenuProvider import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ExtractMethodAction import com.itsaky.androidide.lsp.kotlin.actions.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction @@ -41,5 +42,6 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { NullSafetyAction(), ImplementMembersAction(), ExtractVariableAction(), + ExtractMethodAction(), ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt new file mode 100644 index 0000000000..5dd2d95f65 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -0,0 +1,235 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import android.content.Context +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.get +import com.itsaky.androidide.actions.requireContext +import com.itsaky.androidide.actions.requireEditor +import com.itsaky.androidide.actions.requireFile +import com.itsaky.androidide.idetooltips.TooltipTag +import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodChoice +import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractMethodSheet +import com.itsaky.androidide.lsp.kotlin.refactor.ui.findFragmentActivity +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionRefusal +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractMethodRewrites +import com.itsaky.androidide.lsp.kotlin.utils.refactor.toTextEdit +import com.itsaky.androidide.lsp.models.CodeActionItem +import com.itsaky.androidide.lsp.models.CodeActionKind +import com.itsaky.androidide.lsp.models.Command +import com.itsaky.androidide.lsp.models.DocumentChange +import com.itsaky.androidide.projects.FileManager +import com.itsaky.androidide.resources.R +import com.itsaky.androidide.tasks.createJobCancelChecker +import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo +import java.nio.file.Path + +/** + * Moves the expression at the cursor, or a selected range of statements, into a new `private fun`. + * + * [execAction] runs one background analysis pass and returns a plain-data [ExtractMethodPlan]; + * [postExec] shows the sheet and turns the user's choice 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 0013). + */ +class ExtractMethodAction : BaseKotlinCodeAction() { + companion object { + const val ID = "ide.editor.lsp.kt.extractMethod" + } + + override var titleTextRes: Int = R.string.action_extract_method + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD + + override val id: String = ID + override var label: String = "" + + // Analysis must not run on the UI thread, so the selection is read at the top of execAction on a + // background thread. A torn read while the user is mid-edit can only produce a plan the + // document-version guard then refuses to apply. + override var requiresUIThread: Boolean = false + + // Intentionally no prepare() visibility gate: deciding whether anything is extractable needs a K2 + // analysis session, far too costly for prepare(). The action stays visible on any Kotlin file and + // reports a refusal instead. + + override suspend fun execAction(data: ActionData): ExtractMethodPlan { + val server = + data.get() + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + val nioPath = data.requireFile().toPath() + val env = + server.compilationEnvironmentFor(nioPath) + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + + val cursor = data.requireEditor().cursor + return buildExtractMethodPlan( + env = env, + nioPath = nioPath, + selectionStart = minOf(cursor.left, cursor.right), + selectionEnd = maxOf(cursor.left, cursor.right), + documentVersion = documentVersionOf(nioPath), + // Ties the analysis to this action's coroutine: cancelling the action aborts the analysis. + cancelChecker = ScheduledCancelChecker(createJobCancelChecker()), + ) + } + + override fun postExec( + data: ActionData, + result: Any, + ) { + super.postExec(data, result) + if (result !is 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. + logger.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) { choice -> applyChoice(data, result, choice) } + if (!shown) { + logger.warn("Fragment manager unavailable. Cannot show the extract sheet.") + } + } + + /** + * Turns the user's choice into the two edits and hands them to the language client. + * + * The document version is re-read here rather than trusted from the plan: the editor stays + * reachable while the sheet is open, and applying spans computed against older text would corrupt + * the file. Refusing is always safe; the user can invoke the action again. + * + * Runs from the sheet's click handler, outside `execAction` and so outside every guard the action + * framework provides -- nothing here may throw (R16), hence the [runCatching]. + */ + private fun applyChoice( + data: ActionData, + plan: ExtractMethodPlan, + choice: ExtractMethodChoice, + ) { + runCatching { performChoice(data, plan, choice) }.onFailure { error -> + logger.error("Failed to apply the extract-method choice '{}'", choice.name, error) + flashError(R.string.msg_cannot_perform_fix) + } + } + + private fun performChoice( + data: ActionData, + plan: ExtractMethodPlan, + choice: ExtractMethodChoice, + ) { + val file = data.requireFile() + val nioPath = file.toPath() + if (documentVersionOf(nioPath) != plan.documentVersion) { + flashInfo(R.string.msg_extract_method_file_changed) + return + } + + val rewrites = + buildExtractMethodRewrites(plan.fileText, choice.candidate, choice.name) ?: run { + logger.warn("Could not build an extract-method rewrite for '{}'", choice.candidate.label) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val client = + data.languageClient ?: run { + logger.warn("No language client set. Cannot extract method.") + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = + listOf( + DocumentChange( + file = nioPath, + // 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; CMD_FORMAT_CODE is a no-op for Kotlin anyway. + 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.OutputNotReturnable -> { + context.getString(R.string.msg_extract_method_output_not_returnable, refusal.name) + } + + 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.InnerImplicitReceiver -> { + context.getString(R.string.msg_extract_method_inner_implicit_receiver, refusal.construct) + } + + 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) + } + + ExtractionRefusal.UsesBackingField -> { + context.getString(R.string.msg_extract_method_uses_backing_field) + } + + is ExtractionRefusal.SmartCastParameter -> { + context.getString(R.string.msg_extract_method_smart_cast_parameter, refusal.name) + } + + is ExtractionRefusal.CapturedLocalDeclaration -> { + context.getString(R.string.msg_extract_method_captured_local_declaration, refusal.name) + } + } + + /** -1 when the document is not open, which never matches a real version and so fails the guard. */ + private fun documentVersionOf(path: Path): Int = FileManager.getActiveDocument(path)?.version ?: -1 +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt new file mode 100644 index 0000000000..ef72fe6a78 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt @@ -0,0 +1,96 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.compose.runtime.getValue +import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.ViewCompositionStrategy +import androidx.fragment.app.FragmentActivity +import androidx.fragment.app.viewModels +import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.google.android.material.bottomsheet.BottomSheetDialogFragment +import com.itsaky.androidide.common.compose.IdeTheme +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan + +/** + * Hosts [ExtractMethodSheetContent]. + * + * The plan is handed in directly rather than through fragment arguments: it carries the file's text + * and offset spans, which is neither `Parcelable` nor meaningful to restore -- after process death + * the document may be entirely different. So [plan] is null on a recreated instance and the sheet + * dismisses itself, the same outcome the action's document-version guard would reach anyway. + */ +class ExtractMethodSheet : BottomSheetDialogFragment() { + private var plan: ExtractMethodPlan? = null + private var onChoice: ((ExtractMethodChoice) -> Unit)? = null + + private val viewModel: ExtractMethodViewModel by viewModels { + ExtractMethodViewModel.factory(requireNotNull(plan) { "sheet shown without a plan" }) + } + + override fun onCreateView( + inflater: LayoutInflater, + container: ViewGroup?, + savedInstanceState: Bundle?, + ): View? { + if (plan == null) { + dismissAllowingStateLoss() + return null + } + + return ComposeView(requireContext()).apply { + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { + IdeTheme { + val state by viewModel.uiState.collectAsStateWithLifecycle() + ExtractMethodSheetContent( + state = state, + onEvent = ::handleEvent, + ) + } + } + } + } + + private fun handleEvent(event: ExtractMethodUiEvent) { + when (event) { + ExtractMethodUiEvent.Confirmed -> { + viewModel.choice()?.let { choice -> onChoice?.invoke(choice) } + dismiss() + } + + ExtractMethodUiEvent.Dismissed -> { + dismiss() + } + + else -> { + viewModel.onEvent(event) + } + } + } + + companion object { + private const val TAG = "extract_method_sheet" + + /** + * Shows the sheet on [activity], calling [onChoice] once if the user confirms. Returns false + * when it could not be shown, so the caller can report a failure rather than doing nothing. + */ + fun show( + activity: FragmentActivity, + plan: ExtractMethodPlan, + onChoice: (ExtractMethodChoice) -> Unit, + ): Boolean { + val manager = activity.supportFragmentManager + if (manager.isStateSaved || manager.isDestroyed) return false + ExtractMethodSheet() + .apply { + this.plan = plan + this.onChoice = onChoice + }.show(manager, TAG) + return true + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt new file mode 100644 index 0000000000..cf0e3cdf92 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt @@ -0,0 +1,94 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.navigationBarsPadding +import androidx.compose.foundation.layout.padding +import androidx.compose.material3.Button +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedTextField +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.font.FontFamily +import androidx.compose.ui.unit.dp +import com.itsaky.androidide.resources.R + +/** + * The extract-method sheet: the expression chooser (when there is a choice), the name, and the + * signature exactly as it will be emitted. + * + * A sibling of the extract-variable sheet rather than a generalisation of it: a single shared sheet + * would need a state class where half the fields are meaningless to either caller (ADR 0012). + * + * Stateless: all state arrives in [state] and every interaction leaves as an [ExtractMethodUiEvent]. + */ +@Composable +fun ExtractMethodSheetContent( + state: ExtractMethodUiState, + onEvent: (ExtractMethodUiEvent) -> Unit, + modifier: Modifier = Modifier, +) { + Column( + modifier = + modifier + .fillMaxWidth() + .navigationBarsPadding() + .padding(horizontal = 24.dp, vertical = 16.dp), + verticalArrangement = Arrangement.spacedBy(16.dp), + ) { + Text( + text = stringResource(R.string.title_extract_method), + style = MaterialTheme.typography.titleLarge, + ) + + if (state.showCandidatePicker) { + LabelledSection(stringResource(R.string.label_extract_variable_expression)) { + OptionList( + options = state.candidateLabels, + selected = state.selectedCandidate, + monospace = true, + onSelect = { onEvent(ExtractMethodUiEvent.CandidateSelected(it)) }, + ) + } + } + + OutlinedTextField( + value = state.name, + onValueChange = { onEvent(ExtractMethodUiEvent.NameChanged(it)) }, + label = { Text(stringResource(R.string.label_extract_variable_name)) }, + isError = state.nameProblem != null, + singleLine = true, + supportingText = state.nameProblem?.let { problem -> { Text(stringResource(problem.messageRes())) } }, + modifier = Modifier.fillMaxWidth(), + ) + + LabelledSection(stringResource(R.string.label_extract_method_signature)) { + Text( + text = state.signaturePreview, + style = MaterialTheme.typography.bodyMedium.copy(fontFamily = FontFamily.Monospace), + modifier = Modifier.fillMaxWidth(), + ) + } + + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.End, + ) { + TextButton(onClick = { onEvent(ExtractMethodUiEvent.Dismissed) }) { + Text(stringResource(android.R.string.cancel)) + } + Button( + onClick = { onEvent(ExtractMethodUiEvent.Confirmed) }, + enabled = state.canConfirm, + modifier = Modifier.padding(start = 8.dp), + ) { + Text(stringResource(R.string.action_extract)) + } + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt new file mode 100644 index 0000000000..82bf60186f --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt @@ -0,0 +1,50 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem + +/** + * Everything the extract-method sheet renders. + * + * There is no scope chooser (the new function is always a sibling of the enclosing declaration) and + * no replace-all checkbox (the region is the only site rewritten), so the sheet is a chooser, a name + * field and a preview. + * + * [signaturePreview] is the signature exactly as it will be emitted -- the one derived artefact, and + * the one place the derivation can surprise the user. The body is the code they selected and can see + * behind the sheet, so previewing it says nothing new. + */ +data class ExtractMethodUiState( + val candidateLabels: List, + val selectedCandidate: Int, + val showCandidatePicker: Boolean, + val name: String, + val nameProblem: NameProblem?, + val signaturePreview: String, +) { + val canConfirm: Boolean get() = nameProblem == null +} + +/** What the sheet reports back up; the ViewModel never touches the document itself. */ +sealed interface ExtractMethodUiEvent { + data class CandidateSelected( + val index: Int, + ) : ExtractMethodUiEvent + + data class NameChanged( + val name: String, + ) : ExtractMethodUiEvent + + data object Confirmed : ExtractMethodUiEvent + + data object Dismissed : ExtractMethodUiEvent +} + +/** + * The user's finished decision, handed to the action to turn into edits. Free of offsets and text so + * the sheet stays a pure chooser. + */ +data class ExtractMethodChoice( + val candidate: ExtractMethodCandidate, + val name: String, +) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt new file mode 100644 index 0000000000..4148307531 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt @@ -0,0 +1,80 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.lifecycle.ViewModel +import androidx.lifecycle.ViewModelProvider +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.signatureText +import com.itsaky.androidide.lsp.kotlin.utils.refactor.validateVariableName +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow + +/** + * Derives the sheet's state from an [ExtractMethodPlan] and nothing else -- no analysis, no PSI, no + * I/O -- which is what lets it hold all the sheet's logic and still be a plain unit test. + * + * A plain [ViewModelProvider.Factory] rather than a Koin definition, for the same reason as + * `ExtractVariableViewModel`: sheet-scoped, injects nothing, takes the plan as a runtime argument. + */ +class ExtractMethodViewModel( + private val plan: ExtractMethodPlan, +) : ViewModel() { + private val _uiState = MutableStateFlow(stateFor(candidateIndex = 0, name = null)) + val uiState: StateFlow = _uiState.asStateFlow() + + fun onEvent(event: ExtractMethodUiEvent) { + val current = _uiState.value + when (event) { + is ExtractMethodUiEvent.CandidateSelected -> { + if (event.index == current.selectedCandidate) return + // A different expression means a different signature and suggested name, so the name is + // re-suggested rather than carried over -- the old one described the old expression. + _uiState.value = stateFor(event.index, name = null) + } + + is ExtractMethodUiEvent.NameChanged -> { + _uiState.value = stateFor(current.selectedCandidate, name = event.name) + } + + ExtractMethodUiEvent.Confirmed, ExtractMethodUiEvent.Dismissed -> { + Unit + } + } + } + + /** The user's decision, or null when the name is unusable. */ + fun choice(): ExtractMethodChoice? { + val state = _uiState.value + if (!state.canConfirm) return null + return ExtractMethodChoice(candidate(state.selectedCandidate), state.name) + } + + private fun candidate(index: Int) = plan.candidates[index.coerceIn(plan.candidates.indices)] + + private fun stateFor( + candidateIndex: Int, + name: String?, + ): ExtractMethodUiState { + val bounded = candidateIndex.coerceIn(plan.candidates.indices) + val candidate = candidate(bounded) + val resolvedName = name ?: candidate.suggestedName + + return ExtractMethodUiState( + candidateLabels = plan.candidates.map { it.label }, + selectedCandidate = bounded, + showCandidatePicker = plan.candidates.size > 1 && !plan.selectionMatchedCandidate, + name = resolvedName, + nameProblem = validateVariableName(resolvedName, candidate.takenNames), + // The same call the edit builder makes, so the preview cannot drift from the declaration. + signaturePreview = candidate.signatureText(resolvedName), + ) + } + + companion object { + fun factory(plan: ExtractMethodPlan): ViewModelProvider.Factory = + object : ViewModelProvider.Factory { + @Suppress("UNCHECKED_CAST") + override fun create(modelClass: Class): T = ExtractMethodViewModel(plan) as T + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt index 25409974ee..5d193ac223 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt @@ -6,14 +6,11 @@ import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.navigationBarsPadding import androidx.compose.foundation.layout.padding -import androidx.compose.foundation.selection.selectable -import androidx.compose.foundation.selection.selectableGroup import androidx.compose.foundation.selection.toggleable import androidx.compose.material3.Button import androidx.compose.material3.Checkbox import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedTextField -import androidx.compose.material3.RadioButton import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable @@ -22,9 +19,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource import androidx.compose.ui.semantics.Role -import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.unit.dp -import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem import com.itsaky.androidide.resources.R /** @@ -134,67 +129,3 @@ fun ExtractVariableSheetContent( } } } - -@Composable -private fun LabelledSection( - label: String, - content: @Composable () -> Unit, -) { - Column(verticalArrangement = Arrangement.spacedBy(4.dp)) { - Text(text = label, style = MaterialTheme.typography.labelLarge) - content() - } -} - -/** A radio group. Expression text is monospaced so a candidate reads as the code it is. */ -@Composable -private fun OptionList( - options: List, - selected: Int, - monospace: Boolean, - onSelect: (Int) -> Unit, -) { - Column( - modifier = Modifier.selectableGroup(), - verticalArrangement = Arrangement.spacedBy(8.dp), - ) { - options.forEachIndexed { index, option -> - Row( - verticalAlignment = Alignment.CenterVertically, - modifier = - Modifier - .fillMaxWidth() - .selectable( - selected = index == selected, - role = Role.RadioButton, - onClick = { onSelect(index) }, - ), - ) { - RadioButton( - selected = index == selected, - onClick = null, - ) - - Text( - text = option, - style = - if (monospace) { - MaterialTheme.typography.bodyMedium.copy(fontFamily = FontFamily.Monospace) - } else { - MaterialTheme.typography.bodyMedium - }, - modifier = Modifier.padding(start = 8.dp), - ) - } - } - } -} - -/** The message shown under the name field for each way a name can be unusable. */ -internal fun NameProblem.messageRes(): Int = - when (this) { - NameProblem.Blank -> R.string.msg_extract_variable_name_blank - NameProblem.NotAnIdentifier -> R.string.msg_extract_variable_name_invalid - NameProblem.Keyword -> R.string.msg_extract_variable_name_keyword - NameProblem.AlreadyTaken -> R.string.msg_extract_variable_name_taken - } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt new file mode 100644 index 0000000000..6a746e4634 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt @@ -0,0 +1,85 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.selection.selectable +import androidx.compose.foundation.selection.selectableGroup +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.RadioButton +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.semantics.Role +import androidx.compose.ui.text.font.FontFamily +import androidx.compose.ui.unit.dp +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.resources.R + +/** Shared by the extract-variable and extract-method sheets; neither owns them. */ +@Composable +internal fun LabelledSection( + label: String, + content: @Composable () -> Unit, +) { + Column(verticalArrangement = Arrangement.spacedBy(4.dp)) { + Text(text = label, style = MaterialTheme.typography.labelLarge) + content() + } +} + +/** A radio group. Expression text is monospaced so a candidate reads as the code it is. */ +@Composable +internal fun OptionList( + options: List, + selected: Int, + monospace: Boolean, + onSelect: (Int) -> Unit, +) { + Column( + modifier = Modifier.selectableGroup(), + verticalArrangement = Arrangement.spacedBy(8.dp), + ) { + options.forEachIndexed { index, option -> + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = + Modifier + .fillMaxWidth() + .selectable( + selected = index == selected, + role = Role.RadioButton, + onClick = { onSelect(index) }, + ), + ) { + RadioButton( + selected = index == selected, + onClick = null, + ) + + Text( + text = option, + style = + if (monospace) { + MaterialTheme.typography.bodyMedium.copy(fontFamily = FontFamily.Monospace) + } else { + MaterialTheme.typography.bodyMedium + }, + modifier = Modifier.padding(start = 8.dp), + ) + } + } + } +} + +/** The message shown under a name field for each way a name can be unusable. */ +internal fun NameProblem.messageRes(): Int = + when (this) { + NameProblem.Blank -> R.string.msg_extract_variable_name_blank + NameProblem.NotAnIdentifier -> R.string.msg_extract_variable_name_invalid + NameProblem.Keyword -> R.string.msg_extract_variable_name_keyword + NameProblem.AlreadyTaken -> R.string.msg_extract_variable_name_taken + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt new file mode 100644 index 0000000000..4d8f9c4f1e --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt @@ -0,0 +1,95 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** + * The two replacements an extraction performs: the new function, 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. The result is therefore sorted by + * descending start offset rather than assuming which comes first: a member or top-level target is + * inserted *after* its anchor and leads the list, but a **local function must be declared before it + * is called**, so that insertion precedes the region and the call site leads instead. + * + * 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. + * + * The region is the only site rewritten (R13). Exact-duplicate matching would almost never fire, and + * near-duplicate matching needs anti-unification plus a per-site parameter mapping. + * + * 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 + // Either side of the region is fine; inside it is incoherent -- the two edits would overlap. + if (candidate.insertOffset > span.start && 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 bodyLines = + when (val body = candidate.body) { + is ExtractedBody.ExpressionBody -> { + val lines = reindent(regionText, baseIndent, newline) + if (body.needsReturn) listOf("return " + lines.first()) + lines.drop(1) else lines + } + + is ExtractedBody.StatementBody -> { + reindent(regionText, baseIndent, newline) + listOfNotNull(body.trailingReturn) + } + } + + val declaration = + buildString { + append(indent).append(candidate.signatureText(name)).append(" {").append(newline) + bodyLines.forEach { append(bodyIndent).append(it).append(newline) } + append(indent).append('}') + } + + // A blank line separates the new function from its neighbour either way. Inserting before the + // anchor starts at the anchor's own line, whose indentation is already in the file ahead of the + // insertion point -- so that first indent is dropped here and put back in front of the anchor. + val insertionText = + if (candidate.insertOffset <= span.start) { + declaration.removePrefix(indent) + newline + newline + indent + } else { + newline + newline + declaration + } + + val call = "$name(${candidate.parameters.joinToString(", ") { it.name }})" + val callText = + when (val form = candidate.callSite) { + CallSiteForm.Call -> call + is CallSiteForm.AssignOutput -> "val ${form.name} = $call" + CallSiteForm.Return -> "return $call" + } + + return listOf( + RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), insertionText), + RewriteSpan(span, callText), + ).sortedByDescending { it.span.start } +} + +/** + * Splits the region into lines with its original base indentation removed, so the caller can prefix + * each with the new function's body indentation. Lines nested deeper than the base keep the extra + * depth; the first line never carries indentation, since the span starts at the code itself. + */ +private fun reindent( + text: String, + baseIndent: String, + newline: String, +): List = + text.split(newline).mapIndexed { index, line -> + if (index == 0) line else line.removePrefix(baseIndent) + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt new file mode 100644 index 0000000000..afe51cfab3 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -0,0 +1,187 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** One derived parameter of the new function. Names are the originals, unchanged (R5). */ +data class MethodParameter( + val name: String, + val typeText: String, +) + +/** What goes inside the new function's braces. */ +sealed interface ExtractedBody { + /** + * The region's expression text. [needsReturn] is false only for a `Unit`-valued expression, where + * the function returns `Unit` and a bare statement reads better than `return println(x)`. + */ + data class ExpressionBody( + val needsReturn: Boolean, + ) : ExtractedBody + + /** + * The statements verbatim. [trailingReturn] is the `return ` line appended for the + * single-output case, and null otherwise -- including the tail-return case, where the region + * already ends in a `return`. + */ + data class StatementBody( + val trailingReturn: String?, + ) : ExtractedBody +} + +/** How the region's own text is replaced (R6). */ +sealed interface CallSiteForm { + /** `extracted(args)` -- an expression in place, or a statement. */ + data object Call : CallSiteForm + + /** `val x = extracted(args)` for the single output [name]. */ + data class AssignOutput( + val name: String, + ) : CallSiteForm + + /** `return extracted(args)` for the tail-return case (R8). */ + data object Return : CallSiteForm +} + +/** + * One extractable region, fully derived: everything the sheet renders and the edit builder emits, + * with no PSI left in it. + * + * [span] is what the call site replaces. [insertOffset] is the end of the enclosing declaration -- + * the new function goes immediately after it (R4) -- and [insertIndent] is that declaration's own + * indentation, since nothing re-indents a code-action edit after it is applied. + * + * [returnTypeText] is null for a `Unit` function, where the `: Unit` is left off. + */ +data class ExtractMethodCandidate( + val label: String, + val span: TextSpan, + val suggestedName: String, + val takenNames: Set, + val annotations: List, + val modifiers: List, + val receiverTypeText: String?, + val parameters: List, + val returnTypeText: String?, + val body: ExtractedBody, + val callSite: CallSiteForm, + val insertOffset: Int, + val insertIndent: String, +) + +/** + * Why a region could not be extracted. A refusal is a designed outcome, not an error (ADR 0013): + * each reason gets its own message naming the construct in the way, because a generic one reads as + * the feature being broken. + */ +sealed interface ExtractionRefusal { + /** The selection is neither one expression nor whole statements inside one block (R2). */ + data object NotASingleRegion : ExtractionRefusal + + /** + * The analysis could not run at all -- no compilation environment, no `KtFile`, or something threw. + * Deliberately neutral: the selection may have been perfectly good, so it must not be blamed the way + * [NotASingleRegion] blames it. + */ + data object CouldNotAnalyse : ExtractionRefusal + + /** + * The region declares two or more values the code after it still needs, and one return cannot carry + * them (R7). [names] is what is in the way, so the message can name them. + */ + data class MultipleOutputs( + val names: List, + ) : ExtractionRefusal + + /** + * The region declares exactly one thing the code after it still needs, but the call site cannot + * receive it back (R7): a destructuring entry or a local `fun`, which a `val` cannot stand in for, + * or a local the following code reassigns, which a `val` cannot be. + */ + data class OutputNotReturnable( + val name: String, + ) : ExtractionRefusal + + /** A `var` declared outside the region is assigned inside it. ADFA-5082 lifts this (R7). */ + data class ReassignsOuterVar( + val name: String, + ) : ExtractionRefusal + + /** A `return`, `break` or `continue` whose target is outside the region (R8). */ + data object ExitsRegion : ExtractionRefusal + + /** Members of a `with`/`apply`/`run` receiver introduced inside the enclosing declaration (R9). */ + data class InnerImplicitReceiver( + val construct: String, + ) : ExtractionRefusal + + /** A type parameter declared on the enclosing function (R10). */ + data class UsesTypeParameter( + val name: String, + ) : ExtractionRefusal + + /** A parameter or return type that cannot be written out as source (R5). */ + data object UnrenderableType : ExtractionRefusal + + /** + * A property accessor's `field` (R4). The backing field is reachable only from inside the + * accessor, so the reference would move verbatim into the new function and stop resolving. + */ + data object UsesBackingField : ExtractionRefusal + + /** + * A captured value the region uses through a smart cast (R5). Its declared type does not compile + * in the new body and its narrowed type does not compile at the call site, so neither emission is + * faithful (ADR 0013). + */ + data class SmartCastParameter( + val name: String, + ) : ExtractionRefusal + + /** + * A local `fun`, class or object the region uses but does not contain (R5). It goes out of scope + * once the region moves, and only values can be handed over as parameters. + */ + data class CapturedLocalDeclaration( + val name: String, + ) : ExtractionRefusal +} + +/** + * The complete result of the background pass. + * + * Unlike extract variable's plan this carries a [refusal] rather than merely being empty, because + * "why not" is most of what this refactoring has to say (ADR 0013). [candidates] and [refusal] are + * mutually exclusive in practice: a non-empty candidate list means at least one region survived. + */ +data class ExtractMethodPlan( + override val fileText: String, + override val documentVersion: Int, + val candidates: List, + val selectionMatchedCandidate: Boolean, + val refusal: ExtractionRefusal?, +) : RefactoringPlan { + val isEmpty: Boolean get() = candidates.isEmpty() + + companion object { + fun refused( + refusal: ExtractionRefusal, + fileText: String = "", + documentVersion: Int = -1, + ) = ExtractMethodPlan(fileText, documentVersion, emptyList(), selectionMatchedCandidate = false, refusal = refusal) + } +} + +/** + * The signature exactly as [buildExtractMethodRewrites] emits it. The sheet's preview calls this, so + * there is one derivation and the preview cannot drift from the declaration (R11). + */ +fun ExtractMethodCandidate.signatureText(name: String): String = + buildString { + annotations.forEach { append(it).append(' ') } + modifiers.forEach { append(it).append(' ') } + append("fun ") + receiverTypeText?.let { append(it).append('.') } + append(name) + append('(') + append(parameters.joinToString(", ") { "${it.name}: ${it.typeText}" }) + append(')') + returnTypeText?.let { append(": ").append(it) } + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt new file mode 100644 index 0000000000..ebc20d4eab --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt @@ -0,0 +1,88 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.read +import org.slf4j.LoggerFactory +import java.nio.file.Path +import kotlin.coroutines.cancellation.CancellationException + +private val logger = LoggerFactory.getLogger("ExtractMethodPlanner") + +/** + * Computes the whole [ExtractMethodPlan] in one background analysis pass. + * + * The current `KtFile` is fetched *before* entering [read] -- blocking on `getCurrentKtFile(...).get()` + * inside `project.read` deadlocks. + * + * Anything thrown in this pipeline degrades to a refusal plus a log line: the action framework + * catches only `IllegalArgumentException` and this runs on a scope with no exception handler, so an + * uncaught throw would crash the app (R16). Cancellation is the exception -- it is re-thrown, since a + * cancelled action has no result to report and the coroutine machinery already handles it. + * + * Everything that is not "your selection is not one region" refuses with [ExtractionRefusal.CouldNotAnalyse]: + * blaming a selection that may have been fine is worse than saying nothing useful. + */ +internal fun buildExtractMethodPlan( + env: AbstractCompilationEnvironment, + nioPath: Path, + selectionStart: Int, + selectionEnd: Int, + documentVersion: Int, + cancelChecker: ScheduledCancelChecker, +): ExtractMethodPlan = + runCatching { + val ktFile = + env.ktSymbolIndex.getCurrentKtFile(nioPath).get() + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + + env.project.read { + val fileText = ktFile.text + val region = + resolveExtractionRegion(ktFile, selectionStart, selectionEnd) + ?: return@read ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion, fileText, documentVersion) + + analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { + val results = + when (region) { + is ExtractionRegion.Expressions -> { + region.candidates.map { buildCandidate(listOf(it), isExpression = true, fileText = fileText) } + } + + is ExtractionRegion.Statements -> { + listOf(buildCandidate(region.statements, isExpression = false, fileText = fileText)) + } + } + + val candidates = results.filterIsInstance().map { it.candidate } + if (candidates.isEmpty()) { + // The innermost region is the one the user pointed at, so its reason is the one to show. + // A region with no reason at all cannot happen; if it does, saying nothing useful beats + // blaming the selection. + val refusal = + results.filterIsInstance().firstOrNull()?.refusal + ?: ExtractionRefusal.CouldNotAnalyse + return@analyzeMaybeDangling ExtractMethodPlan.refused(refusal, fileText, documentVersion) + } + + ExtractMethodPlan( + fileText = fileText, + documentVersion = documentVersion, + candidates = candidates, + // Only meaningful while the innermost candidate survived: otherwise the selection no + // longer corresponds to the first option shown. + selectionMatchedCandidate = + region is ExtractionRegion.Expressions && + region.selectionMatchedInnermost && + candidates.first().span == region.span, + refusal = null, + ) + } + } + }.getOrElse { error -> + if (error is CancellationException) throw error + logger.warn("Failed to build extract-method plan for {}", nioPath, error) + ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt index be948e179b..d90fb2bb2a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -139,11 +139,11 @@ data class CandidateExpression( * candidate, meaning they already expressed which expression they want and the UI should not ask. */ data class ExtractionPlan( - val fileText: String, - val documentVersion: Int, + override val fileText: String, + override val documentVersion: Int, val candidates: List, val selectionMatchedCandidate: Boolean, -) { +) : RefactoringPlan { val isEmpty: Boolean get() = candidates.isEmpty() companion object { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt new file mode 100644 index 0000000000..1783322d94 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt @@ -0,0 +1,129 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile + +/** + * 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 + + /** + * One or more nested expressions at the cursor, innermost first. The user picks between them in + * the sheet unless [selectionMatchedInnermost] says they already have. + */ + data class Expressions( + val candidates: List, + val selectionMatchedInnermost: Boolean, + ) : ExtractionRegion { + override val span: TextSpan + get() = candidates.first().textRange.let { TextSpan(it.startOffset, it.endOffset) } + } + + /** One or more sibling statements in a single [block]. */ + data class Statements( + val statements: List, + val block: KtBlockExpression, + ) : ExtractionRegion { + override val span: TextSpan + get() = + TextSpan( + statements.first().textRange.startOffset, + statements.last().textRange.endOffset, + ) + } +} + +/** + * Resolves `[selectionStart, selectionEnd)` to the one region the refactoring will act on, or null + * when it is neither kind. + * + * A bare cursor is always the expression path. A non-empty selection snaps **outward** to whole + * statements -- a touch selection will not land on a boundary. When the snapped range is a single + * statement and the selection sits strictly inside it, the expression path is preferred instead: + * that is what the user's selection actually points at, not the enclosing statement. But if nothing + * there is a legal expression target, the snapped statement is used anyway -- a near-miss drag + * (e.g. selecting `sum = a + b` and missing the leading `val`) should still extract something, + * rather than being refused for landing a few characters short. + */ +fun resolveExtractionRegion( + file: KtFile, + selectionStart: Int, + selectionEnd: Int, +): ExtractionRegion? { + val (start, end) = trimToCode(file.text, selectionStart, selectionEnd) ?: return null + if (start == end) return expressionRegion(file, selectionStart, selectionEnd) + + val range = snapToStatements(file, start, end) ?: return expressionRegion(file, selectionStart, selectionEnd) + + val only = range.statements.singleOrNull() + if (only != null && (start > only.textRange.startOffset || end < only.textRange.endOffset)) { + expressionRegion(file, selectionStart, selectionEnd)?.let { return it } + } + + return ExtractionRegion.Statements(range.statements, range.block) +} + +private fun expressionRegion( + file: KtFile, + selectionStart: Int, + selectionEnd: Int, +): ExtractionRegion.Expressions? { + val syntax = candidateExpressionsAt(file, selectionStart, selectionEnd) + if (syntax.expressions.isEmpty()) return null + return ExtractionRegion.Expressions(syntax.expressions, syntax.selectionMatchedInnermost) +} + +/** A run of sibling statements together with the [KtBlockExpression] that holds them. */ +private class StatementRange( + val statements: List, + val block: KtBlockExpression, +) + +/** + * The whole statements `[start, end)` touches, when they are siblings in one [KtBlockExpression]. + * + * 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( + file: KtFile, + start: Int, + end: Int, +): StatementRange? { + // end > start is guaranteed by the start == end early-return in resolveExtractionRegion. + val first = statementContaining(file, start) ?: return null + val last = statementContaining(file, end - 1) ?: return null + + val block = first.parent as? KtBlockExpression ?: return null + if (last.parent !== block) return null + if (!isExtractionPosition(first)) return null + + val statements = block.statements + val from = statements.indexOfFirst { it === first } + val to = statements.indexOfFirst { it === last } + if (from < 0 || to < from) return null + return StatementRange(statements.subList(from, to + 1).toList(), block) +} + +/** + * The statement containing [offset]: the nearest ancestor that is a direct expression child of a + * block. Null for a position that is not inside one, such as a comment or a class body. + */ +private fun statementContaining( + file: KtFile, + offset: Int, +): KtExpression? { + var current: PsiElement? = file.findElementAt(offset) ?: return null + while (current != null && current !is KtFile) { + if (current is KtExpression && current.parent is KtBlockExpression) return current + current = current.parent + } + return null +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt new file mode 100644 index 0000000000..987f98bbde --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -0,0 +1,882 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall +import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundArrayAccessCall +import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundVariableAccessCall +import org.jetbrains.kotlin.analysis.api.resolution.KaImplicitReceiverValue +import org.jetbrains.kotlin.analysis.api.resolution.KaReceiverValue +import org.jetbrains.kotlin.analysis.api.resolution.successfulCallOrNull +import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull +import org.jetbrains.kotlin.analysis.api.resolution.symbol +import org.jetbrains.kotlin.analysis.api.symbols.KaBackingFieldSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaClassSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaNamedFunctionSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaReceiverParameterSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaValueParameterSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaVariableSymbol +import org.jetbrains.kotlin.analysis.api.symbols.markers.KaNamedSymbol +import org.jetbrains.kotlin.analysis.api.types.KaClassType +import org.jetbrains.kotlin.analysis.api.types.KaFlexibleType +import org.jetbrains.kotlin.analysis.api.types.KaFunctionType +import org.jetbrains.kotlin.analysis.api.types.KaType +import org.jetbrains.kotlin.builtins.StandardNames +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.idea.references.mainReference +import org.jetbrains.kotlin.psi.KtAnonymousInitializer +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtBreakExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtContinueExpression +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtExpressionWithLabel +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtLabeledExpression +import org.jetbrains.kotlin.psi.KtLambdaArgument +import org.jetbrains.kotlin.psi.KtLambdaExpression +import org.jetbrains.kotlin.psi.KtLoopExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.KtNamedDeclaration +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtProperty +import org.jetbrains.kotlin.psi.KtPropertyAccessor +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtReturnExpression +import org.jetbrains.kotlin.psi.KtSecondaryConstructor +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.KtThisExpression +import org.jetbrains.kotlin.psi.KtTypeReference +import org.jetbrains.kotlin.psi.KtValueArgument +import org.jetbrains.kotlin.psi.KtValueArgumentList + +/** The name of the statement-range suggestion; there is no expression to read a name from (R12). */ +private const val STATEMENT_RANGE_NAME = "extracted" + +private const val COMPOSABLE_FQ_NAME = "androidx.compose.runtime.Composable" + +/** What a receiver-binding lambda is called in the refusal when it is not a call argument. */ +private const val UNNAMED_SCOPING_CONSTRUCT = "lambda" + +private const val BACKING_FIELD_NAME = "field" + +private const val COROUTINE_CONTEXT_NAME = "coroutineContext" + +/** As [renderedTypeTextOrNull] prints it. A `Unit` return type is left off the signature entirely. */ +private const val UNIT_TYPE_TEXT = "kotlin.Unit" + +/** Either a derived candidate or the reason there is not one. */ +internal sealed interface SignatureResult { + data class Success( + val candidate: ExtractMethodCandidate, + ) : SignatureResult + + data class Refused( + val refusal: ExtractionRefusal, + ) : SignatureResult +} + +/** + * Derives one candidate from [elements] -- a single expression, or the statement range. + * + * Ordered so the cheapest refusals come first and nothing expensive runs for a region that is going + * to be declined anyway. MUST be called inside an analysis session. + */ +internal fun KaSession.buildCandidate( + elements: List, + isExpression: Boolean, + fileText: String, +): SignatureResult { + val first = elements.first() + val last = elements.last() + val span = TextSpan(first.textRange.startOffset, last.textRange.endOffset) + val enclosing = enclosingDeclaration(first) ?: return refuse(ExtractionRefusal.NotASingleRegion) + + val typeParameterNames = typeParameterNamesOf(enclosing) + typeParameterIn(typeParameterNames, elements)?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + if (usesBackingField(enclosing, elements)) return refuse(ExtractionRefusal.UsesBackingField) + innerImplicitReceiver(enclosing, elements, span)?.let { return refuse(ExtractionRefusal.InnerImplicitReceiver(it)) } + reassignedOuterVar(enclosing, elements, span)?.let { return refuse(ExtractionRefusal.ReassignsOuterVar(it)) } + + val tailReturn = !isExpression && isTailReturn(elements, span) + if (!tailReturn && hasExit(elements, span)) return refuse(ExtractionRefusal.ExitsRegion) + + val outputs = if (isExpression) RegionOutputs.NONE else outputsOf(enclosing, elements, span) + // Only a single plain `val`/`var` can come back as the return value. Everything else the region + // declares and the following code still needs is refused rather than silently dropped (R7), split + // by which situation it is: two values genuinely cannot fit in one return, while a lone + // destructuring entry, local `fun` or reassigned local is one value the call site cannot receive. + if (outputs.declarations.size > 1) { + return refuse(ExtractionRefusal.MultipleOutputs(outputs.declarations.mapNotNull { it.name })) + } + val declared = outputs.declarations.singleOrNull() + if (declared != null && (declared !is KtProperty || outputs.writtenAfter.isNotEmpty())) { + return refuse(ExtractionRefusal.OutputNotReturnable(declared.name.orEmpty())) + } + val output = declared as? KtProperty + // The tail-return exception holds only when nothing else flows out (R8). + if (tailReturn && output != null) return refuse(ExtractionRefusal.ExitsRegion) + + val parameters = + when (val captured = capturedParameters(enclosing, elements, span)) { + is CaptureResult.Captured -> captured.parameters + is CaptureResult.Refused -> return refuse(captured.refusal) + } + + val returnTypeText = + when { + isExpression -> { + renderedTypeOrNull(first) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + + tailReturn -> { + // A secondary constructor's symbol returns the constructed class, but its `return` + // carries no value -- so the extracted tail is `Unit`, and `return extracted(...)` on a + // `Unit` call is legal inside a constructor. (`init` needs no rule: `return` is illegal + // there, so no tail return can reach here.) + when (enclosing) { + is KtSecondaryConstructor -> UNIT_TYPE_TEXT + else -> enclosingReturnType(enclosing) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + } + + output != null -> { + renderedDeclarationType(output) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + + else -> { + null + } + }.takeUnless { it == UNIT_TYPE_TEXT } + + val receiverTypeText = receiverTypeTextOf(enclosing) + + // The syntactic check above misses an inferred type argument, which names no type anywhere in the + // region. The rendered signature is the last place to catch it before it is emitted (R10), and it + // has to cover every slot the signature prints -- the receiver included. + renderedTypeParameterIn( + typeParameterNames, + parameters.map { it.typeText } + listOfNotNull(returnTypeText, receiverTypeText), + )?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + + val body = + when { + isExpression -> ExtractedBody.ExpressionBody(needsReturn = returnTypeText != null) + output != null -> ExtractedBody.StatementBody(trailingReturn = "return ${output.name.orEmpty()}") + else -> ExtractedBody.StatementBody(trailingReturn = null) + } + + val callSite = + when { + tailReturn -> CallSiteForm.Return + output != null -> CallSiteForm.AssignOutput(output.name.orEmpty()) + else -> CallSiteForm.Call + } + + // A getter is not a place a function can follow -- inserting there lands between the accessors of + // a `var` and does not parse -- so the new member goes after the whole property (R4). The accessor + // itself stays the capture boundary everywhere else. + val anchor = (enclosing as? KtPropertyAccessor)?.property ?: enclosing + val isLocalTarget = anchor.parent is KtBlockExpression + val takenNames = takenNamesFor(enclosing, anchor, isLocalTarget) + val modifiers = + buildList { + // A local function joins a block, and a visibility modifier on one does not compile. + if (!isLocalTarget) add("private") + if (usesSuspend(elements)) add("suspend") + } + + return SignatureResult.Success( + ExtractMethodCandidate( + label = collapseForLabel(fileText.substring(span.start, span.end)), + span = span, + suggestedName = + if (isExpression) { + suggestVariableName(first, renderedTypeOrNull(first), takenNames) + } else { + uniqueName(STATEMENT_RANGE_NAME, takenNames) + }, + takenNames = takenNames, + annotations = if (usesComposable(elements)) listOf("@Composable") else emptyList(), + modifiers = modifiers, + receiverTypeText = receiverTypeText, + parameters = parameters, + returnTypeText = returnTypeText, + body = body, + callSite = callSite, + // A local function is only visible from its declaration onward, so it has to go *before* the + // anchor that calls it. Sound in general: everything the anchor's body can reach is already + // declared above the anchor. Every other target keeps the new member after its anchor (R4). + insertOffset = if (isLocalTarget) anchor.textRange.startOffset else anchor.textRange.endOffset, + insertIndent = leadingIndentAt(fileText, anchor.textRange.startOffset), + ), + ) +} + +private fun refuse(refusal: ExtractionRefusal): SignatureResult = SignatureResult.Refused(refusal) + +/** + * The named function, accessor, `init` block or constructor whose body holds [element]. Lambdas are + * skipped: the new function is a sibling of the enclosing *named* declaration (R4), and the lambda's + * captures become parameters. + */ +private fun enclosingDeclaration(element: PsiElement): KtDeclaration? { + var current: PsiElement? = element.parent + while (current != null) { + when (current) { + is KtNamedFunction, is KtPropertyAccessor, is KtAnonymousInitializer, is KtSecondaryConstructor -> { + return current + } + + is KtClassOrObject -> { + return null + } + } + current = current.parent + } + return null +} + +/** Whether [element] is inside the region's span. */ +private fun inRegion( + element: PsiElement, + span: TextSpan, +): Boolean = element.textRange.startOffset >= span.start && element.textRange.endOffset <= span.end + +private fun simpleNamesIn(elements: List): List = + elements.flatMap { PsiTreeUtil.collectElementsOfType(it, KtSimpleNameExpression::class.java) } + +private fun descendantsOf( + elements: List, + type: Class, +): List = elements.flatMap { PsiTreeUtil.collectElementsOfType(it, type) } + +/** + * The name of a class declared inside [enclosing] that [type] is written in terms of, or null. + * + * A value of such a type survives the move, but its type name does not resolve at the insertion + * point, so no parameter can be written for it. Type arguments are searched too: `List` is + * just as unwritable as `Holder`. + */ +private fun KaSession.localTypeNameIn( + type: KaType?, + enclosing: KtDeclaration, +): String? { + val classType = ((type as? KaFlexibleType)?.lowerBound ?: type) as? KaClassType ?: return null + val psi = runCatching { classType.symbol.psi }.getOrNull() + if (psi != null && PsiTreeUtil.isAncestor(enclosing, psi, true)) { + return (classType.symbol as? KaNamedSymbol)?.name?.asString() + } + return classType.typeArguments.firstNotNullOfOrNull { localTypeNameIn(it.type, enclosing) } +} + +/** + * A captured declaration is one the region references whose PSI lies inside the enclosing + * declaration but outside the region itself. Anything else -- a class member, a top-level + * declaration, an import -- resolves unchanged from the new function's body (R5). + * + * Declines rather than emitting text that will not compile: a type that cannot be written out as + * source, or a value the region only uses through a smart cast. + */ +private fun KaSession.capturedParameters( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): CaptureResult { + val parameters = mutableListOf() + val seen = mutableSetOf() + + for (reference in simpleNamesIn(elements).sortedBy { it.textRange.startOffset }) { + // Deliberately no "skip a qualified selector" guard here. A selector can still resolve to a + // declaration inside the enclosing declaration -- a local extension `fun` called as `h.twice()` + // -- which goes out of scope once the region moves, and skipping it emits a body that no longer + // resolves. The ancestor test below already lets every selector resolving to a non-local member + // through, which is what a guard would have bought. + val resolved = + runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() ?: continue + val name = reference.getReferencedName() + + // A local class or object is not a callable, so it used to fail the cast below and be silently + // dropped -- emitting a body that names a type the new function cannot see. It is refused here + // for the same reason a local `fun` is: only values can be handed over as parameters (R5). + if (resolved is KaClassSymbol) { + val classPsi = runCatching { resolved.psi }.getOrNull() + if (classPsi != null && PsiTreeUtil.isAncestor(enclosing, classPsi, true) && !inRegion(classPsi, span)) { + return CaptureResult.Refused(ExtractionRefusal.CapturedLocalDeclaration(name)) + } + } + + val symbol = resolved as? KaCallableSymbol ?: continue + val declarationPsi = runCatching { symbol.psi }.getOrNull() + + val key: Any = + when { + declarationPsi != null -> { + if (!PsiTreeUtil.isAncestor(enclosing, declarationPsi, true)) continue + if (inRegion(declarationPsi, span)) continue + declarationPsi + } + + // `it` has no source PSI, so it would otherwise read as "not captured" and be dropped. + // Its binding lambda stands in for the missing declaration: captured only when that + // lambda is outside the region, and keyed on the lambda so that an `it` bound inside the + // region cannot evict a genuinely captured outer one. + symbol is KaValueParameterSymbol && + name == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString() -> { + val lambda = + PsiTreeUtil.getParentOfType(reference, KtFunctionLiteral::class.java, true) ?: continue + if (inRegion(lambda, span)) continue + lambda + } + + else -> { + continue + } + } + if (!seen.add(key)) continue + + // Only a value can be passed. A local `fun`, class or object declared outside the region goes + // out of scope once the region moves, and handing it over as a parameter of its own return type + // is not the same program (R5). + if (symbol !is KaVariableSymbol) { + return CaptureResult.Refused(ExtractionRefusal.CapturedLocalDeclaration(name)) + } + + // The value survives the move but its type may not: a local class declared inside the enclosing + // declaration is out of scope at the insertion point, so the parameter could not be written. + localTypeNameIn(runCatching { symbol.returnType }.getOrNull(), enclosing)?.let { + return CaptureResult.Refused(ExtractionRefusal.CapturedLocalDeclaration(it)) + } + + val typeText = + renderedSymbolType(symbol) ?: return CaptureResult.Refused(ExtractionRefusal.UnrenderableType) + // The signature must print the declared type, but the region may be leaning on a smart cast to + // something narrower: the declared type breaks the moved body, the narrowed one breaks the call + // site. + when (val used = usedTypeOf(reference)) { + // An intersection (`A & B`) cannot be printed at all, but the declared type just rendered + // fine, so the two differ and this is a smart cast however it would have been spelled. + UsedType.Unrenderable -> { + return CaptureResult.Refused(ExtractionRefusal.SmartCastParameter(name)) + } + + is UsedType.Rendered -> { + if (used.text != typeText) { + return CaptureResult.Refused(ExtractionRefusal.SmartCastParameter(name)) + } + } + + UsedType.Absent -> { + Unit + } + } + parameters += MethodParameter(name = name, typeText = typeText) + } + return CaptureResult.Captured(parameters) +} + +/** + * The type of a reference as the region uses it. + * + * [Unrenderable] is kept apart from [Absent] on purpose: folding them together is what let a smart + * cast to an intersection type pass as "no information" and emit the declared type. + */ +private sealed interface UsedType { + data object Absent : UsedType + + data object Unrenderable : UsedType + + data class Rendered( + val text: String, + ) : UsedType +} + +private fun KaSession.usedTypeOf(expression: KtExpression): UsedType { + val type = runCatching { expression.expressionType }.getOrNull() ?: return UsedType.Absent + return runCatching { typeTextOrNull(type) }.fold( + onSuccess = { rendered -> rendered?.let { UsedType.Rendered(it) } ?: UsedType.Unrenderable }, + onFailure = { UsedType.Absent }, + ) +} + +/** Either the derived parameter list or the reason there cannot be one. */ +private sealed interface CaptureResult { + data class Captured( + val parameters: List, + ) : CaptureResult + + data class Refused( + val refusal: ExtractionRefusal, + ) : CaptureResult +} + +private fun KaSession.renderedSymbolType(symbol: KaCallableSymbol): String? = + runCatching { symbol.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } + +private fun KaSession.renderedTypeOrNull(expression: KtExpression): String? = + runCatching { expression.expressionType }.getOrNull()?.let { renderedTypeTextOrNull(it) } + +private fun KaSession.renderedDeclarationType(property: KtProperty): String? = + runCatching { (property.symbol as? KaCallableSymbol)?.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } + +private fun KaSession.enclosingReturnType(enclosing: KtDeclaration): String? = + runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } + +/** + * What the region declares that the code after it still uses (R7). + * + * Every named declaration counts, not just [KtProperty]: a destructuring entry, a local `fun` and a + * local class are all things the following code can reference, and none of them can be returned. + * They are collected so [buildCandidate] can refuse them -- omitting them is what produced a call + * site referring to names that no longer exist. + * + * [writtenAfter] is the subset the following code assigns to. The call site emits a `val`, so even a + * single such output cannot be honoured. + */ +private class RegionOutputs( + val declarations: List, + val writtenAfter: List, +) { + companion object { + val NONE = RegionOutputs(emptyList(), emptyList()) + } +} + +/** + * "Used after the region" is a textual-offset test inside the enclosing declaration, which is sound + * because a local is only in scope after its own declaration in the same block. + */ +private fun KaSession.outputsOf( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): RegionOutputs { + // Lambdas and parameters are named declarations too, and neither can be referenced after the + // region. Dropping them keeps the short-circuit below meaningful for any region holding a lambda, + // and keeps a lambda's "" out of a refusal message. + val declared = + descendantsOf(elements, KtNamedDeclaration::class.java) + .filterNot { it is KtFunctionLiteral || it is KtParameter } + if (declared.isEmpty()) return RegionOutputs.NONE + + val laterReferences = + PsiTreeUtil + .collectElementsOfType(enclosing, KtSimpleNameExpression::class.java) + .filter { it.textRange.startOffset >= span.end } + val read = laterReferences.filterNot { it.isWriteTarget() }.mapNotNullTo(mutableSetOf()) { resolvedPsi(it) } + val written = laterReferences.filter { it.isWriteTarget() }.mapNotNullTo(mutableSetOf()) { resolvedPsi(it) } + + return RegionOutputs( + declarations = declared.filter { it in read || it in written }, + writtenAfter = declared.filter { it in written }, + ) +} + +private fun KaSession.resolvedPsi(reference: KtSimpleNameExpression): PsiElement? = + runCatching { + reference.mainReference + ?.resolveToSymbols() + ?.firstOrNull() + ?.psi + }.getOrNull() + +/** + * A `var` declared inside the enclosing declaration but outside the region, assigned inside it. + * Kotlin has no `out` parameters, so the faithful emission would shadow a name (R7, ADR 0013). + */ +private fun KaSession.reassignedOuterVar( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): String? { + for (reference in simpleNamesIn(elements)) { + if (!reference.isWriteTarget()) continue + val symbol = + runCatching { + (reference.mainReference?.resolveToSymbols()?.firstOrNull() as? KaVariableSymbol)?.takeIf { !it.isVal } + }.getOrNull() ?: continue + val declarationPsi = runCatching { symbol.psi }.getOrNull() ?: continue + if (!PsiTreeUtil.isAncestor(enclosing, declarationPsi, true)) continue + if (inRegion(declarationPsi, span)) continue + return reference.getReferencedName() + } + return null +} + +/** + * The tail-return exception (R8): the region's last statement is a `return`, and it is the region's + * only `return`, `break` or `continue`. Purely syntactic, which is why it is worth having. + */ +private fun isTailReturn( + elements: List, + span: TextSpan, +): Boolean { + if (elements.last() !is KtReturnExpression) return false + val returns = descendantsOf(elements, KtReturnExpression::class.java) + if (returns.size != 1 || returns.single() !== elements.last()) return false + return !hasLoopExit(elements, span) +} + +/** Any `return`, `break` or `continue` whose target lies outside the region (R8). */ +private fun hasExit( + elements: List, + span: TextSpan, +): Boolean { + for (returnExpression in descendantsOf(elements, KtReturnExpression::class.java)) { + // An unlabelled `return` always targets the enclosing named declaration, which is outside the + // region by construction. A labelled one targets the lambda carrying that label, which is not + // necessarily the nearest one -- `return@outer` from a nested lambda still leaves the region. + val label = returnExpression.getLabelName() ?: return true + val target = labelledLambdaFor(returnExpression, label) ?: return true + if (!inRegion(target, span)) return true + } + return hasLoopExit(elements, span) +} + +/** The lambda `return@[label]` targets: the innermost enclosing one carrying that label. */ +private fun labelledLambdaFor( + returnExpression: KtReturnExpression, + label: String, +): KtFunctionLiteral? { + var lambda = PsiTreeUtil.getParentOfType(returnExpression, KtFunctionLiteral::class.java, true) + while (lambda != null) { + if (lambdaLabel(lambda) == label) return lambda + lambda = PsiTreeUtil.getParentOfType(lambda, KtFunctionLiteral::class.java, true) + } + return null +} + +/** + * The label a `return@` can name this lambda by: its explicit `label@` if it has one, otherwise the + * name of the function it is an argument to. + */ +private fun lambdaLabel(lambda: KtFunctionLiteral): String? { + val lambdaExpression = lambda.parent as? KtLambdaExpression ?: return null + (lambdaExpression.parent as? KtLabeledExpression)?.getLabelName()?.let { return it } + return callOwning(lambdaExpression)?.calleeName() +} + +/** The call [lambdaExpression] is an argument of, trailing or parenthesised. */ +private fun callOwning(lambdaExpression: KtLambdaExpression): KtCallExpression? = + when (val argument = lambdaExpression.parent) { + is KtLambdaArgument -> argument.parent as? KtCallExpression + is KtValueArgument -> (argument.parent as? KtValueArgumentList)?.parent as? KtCallExpression + else -> null + } + +private fun KtCallExpression.calleeName(): String? = (calleeExpression as? KtNameReferenceExpression)?.getReferencedName() + +private fun hasLoopExit( + elements: List, + span: TextSpan, +): Boolean { + val jumps: List = + descendantsOf(elements, KtBreakExpression::class.java) + + descendantsOf(elements, KtContinueExpression::class.java) + return jumps.any { jump -> + val loop = targetLoopFor(jump) + loop == null || !inRegion(loop, span) + } +} + +/** + * The loop a `break`/`continue` leaves: the innermost enclosing one, or the one its label names. + * + * Reading the label matters for the same reason it does for a labelled `return` -- `break@outer` from + * a nested loop inside the region leaves the region, however local the nearest loop looks. + */ +private fun targetLoopFor(jump: KtExpressionWithLabel): KtLoopExpression? { + var loop = PsiTreeUtil.getParentOfType(jump, KtLoopExpression::class.java, true) + val label = jump.getLabelName() ?: return loop + while (loop != null) { + if ((loop.parent as? KtLabeledExpression)?.getLabelName() == label) return loop + loop = PsiTreeUtil.getParentOfType(loop, KtLoopExpression::class.java, true) + } + return null +} + +/** An accessor's type parameters live on its property, the same place its receiver does. */ +private fun typeParameterNamesOf(enclosing: KtDeclaration): List = + when (enclosing) { + is KtNamedFunction -> enclosing.typeParameters.mapNotNull { it.name } + is KtPropertyAccessor -> enclosing.property.typeParameters.mapNotNull { it.name } + else -> emptyList() + } + +/** + * The name of the enclosing function's type parameter the region *writes out*, or null. A filtered + * copy of the type-parameter list with its bounds is the alternative, and deciding "is `T` + * referenced" from rendered type text is exactly the fragility that rules it out (R10). + * + * This catches only a type the region names. A type argument the region gets by inference names + * nothing at all, and is caught by [renderedTypeParameterIn] once the signature exists. + */ +private fun typeParameterIn( + names: List, + elements: List, +): String? { + if (names.isEmpty()) return null + + val typeTexts = + descendantsOf(elements, KtTypeReference::class.java).map { it.text } + + simpleNamesIn(elements).map { it.getReferencedName() } + return names.firstOrNull { name -> typeTexts.any { it == name || it.containsWord(name) } } +} + +/** + * The type parameter that leaked into the derived signature, or null. + * + * `fun demo(a: T, b: T) { pick(a, b) }` names `T` nowhere in the region, but the parameters + * render as `T` -- and the new function has no type-parameter list to bind it. Checking the rendered + * strings is the only place that shows up before the text is emitted. + */ +private fun renderedTypeParameterIn( + names: List, + renderedTypes: List, +): String? { + if (names.isEmpty()) return null + return names.firstOrNull { name -> renderedTypes.any { it == name || it.containsWord(name) } } +} + +/** Whole-word containment, so `T` does not match `Type`. */ +private fun String.containsWord(word: String): Boolean = + Regex("(^|[^A-Za-z0-9_])" + Regex.escape(word) + "($|[^A-Za-z0-9_])").containsMatchIn(this) + +/** + * Whether the region reads or writes a property accessor's backing field (R4). + * + * `field` is in scope only inside the accessor, so it would move verbatim into the new function and + * stop resolving. Gated on the enclosing declaration being an accessor, which costs nothing + * everywhere else, and confirmed against the resolved symbol so a local that happens to be called + * `field` is not mistaken for it. + */ +private fun KaSession.usesBackingField( + enclosing: KtDeclaration, + elements: List, +): Boolean { + if (enclosing !is KtPropertyAccessor) return false + return simpleNamesIn(elements).any { reference -> + reference.getReferencedName() == BACKING_FIELD_NAME && + runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() is KaBackingFieldSymbol + } +} + +/** + * The scoping construct whose implicit receiver the region uses unqualified, or null (R9). + * + * Turning that receiver into a parameter would mean qualifying every unqualified member access + * inside the extracted body -- editing the interior of the moved code, which this refactoring does + * not do. Android code leans on `with`/`apply` heavily, so the message names the construct. + * + * The question is asked of the resolved call rather than of a list of known scoping-function names: + * a name list both over-refuses (an inherited member or an outer-class member reached with no + * qualifier is not the receiver's) and under-refuses (it cannot know about `coroutineScope`, + * `buildAnnotatedString`, or any Compose scope). A receiver that is implicit and belongs to a lambda + * between the region and the enclosing declaration is exactly what does not survive the move. + */ +private fun KaSession.innerImplicitReceiver( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): String? { + for (reference in simpleNamesIn(elements)) { + // A qualified selector already has its receiver written out next to it. Deliberately syntactic + // and deliberately shallow: a *call* selector (`h.doubled()`) must NOT be skipped, because its + // dispatch receiver can still be an implicit one -- a member extension invoked on a `with` + // receiver is the pervasive Compose shape (`with(density) { size.toPx() }`). + val parent = reference.parent + if (parent is KtQualifiedExpression && parent.selectorExpression === reference) continue + + val lambda = implicitReceiverLambdaFor(reference) ?: continue + if (isBoundOutsideRegion(enclosing, lambda, span)) return constructNameFor(lambda) + } + + // A bare `this` names the receiver without going through a call, so no resolved call reports it. + // Left undetected it does not fail to compile -- it silently becomes the enclosing class instance, + // which is worse. + for (thisExpression in descendantsOf(elements, KtThisExpression::class.java)) { + val symbol = + runCatching { + thisExpression.instanceReference.mainReference + ?.resolveToSymbols() + ?.firstOrNull() + }.getOrNull() + val lambda = lambdaOwning(symbol) ?: continue + if (isBoundOutsideRegion(enclosing, lambda, span)) return constructNameFor(lambda) + } + return null +} + +/** Whether [lambda] binds its receiver between the region and [enclosing], so the move loses it. */ +private fun isBoundOutsideRegion( + enclosing: KtDeclaration, + lambda: KtFunctionLiteral, + span: TextSpan, +): Boolean = !inRegion(lambda, span) && PsiTreeUtil.isAncestor(enclosing, lambda, true) + +private fun constructNameFor(lambda: KtFunctionLiteral): String = + (lambda.parent as? KtLambdaExpression)?.let { callOwning(it)?.calleeName() } ?: UNNAMED_SCOPING_CONSTRUCT + +/** + * The lambda supplying [reference]'s implicit receiver, or null when it has none or the receiver + * comes from somewhere that survives the move (a class, the enclosing function's own receiver). + */ +private fun KaSession.implicitReceiverLambdaFor(reference: KtSimpleNameExpression): KtFunctionLiteral? = + runCatching { + // A callee name does not resolve to a call on its own; its call expression does. + val callSource = + (reference.parent as? KtCallExpression)?.takeIf { it.calleeExpression === reference } ?: reference + val call = callSource.resolveToCall() + // Defensive only. A compound assignment (`n += 1` inside `apply { }`) redirects to the whole + // compound access, but the resolver flags that redirect and still hands back a plain variable + // access, so the branch above already catches it in this version. + val applied = + call?.successfulCallOrNull>()?.partiallyAppliedSymbol + ?: call?.successfulCallOrNull()?.variableCall?.partiallyAppliedSymbol + ?: call?.successfulCallOrNull()?.getterCall?.partiallyAppliedSymbol + receiverLambda(applied?.dispatchReceiver) ?: receiverLambda(applied?.extensionReceiver) + }.getOrNull() + +private fun receiverLambda(receiver: KaReceiverValue?): KtFunctionLiteral? = lambdaOwning((receiver as? KaImplicitReceiverValue)?.symbol) + +/** The lambda [symbol] belongs to, when it is a lambda's receiver rather than a class's. */ +private fun lambdaOwning(symbol: KaSymbol?): KtFunctionLiteral? { + if (symbol == null) return null + // A lambda's receiver reports itself either as the anonymous function or as that function's + // receiver parameter, and only the former carries the PSI. + val psi = + runCatching { symbol.psi }.getOrNull() + ?: runCatching { (symbol as? KaReceiverParameterSymbol)?.owningCallableSymbol?.psi }.getOrNull() + ?: return null + return psi as? KtFunctionLiteral ?: (psi as? KtLambdaExpression)?.functionLiteral +} + +/** + * The receiver the new function must repeat, or null (R4). + * + * An accessor's receiver is declared on its property (`val Foo.x get() = ...`), not on the accessor, + * so reading only the accessor drops it and the moved body's unqualified members stop resolving. + */ +private fun receiverTypeTextOf(enclosing: KtDeclaration): String? = + when (enclosing) { + is KtNamedFunction -> enclosing.receiverTypeReference?.text + is KtPropertyAccessor -> enclosing.property.receiverTypeReference?.text + else -> null + } + +/** + * `suspend` is added when the region calls one, or touches `coroutineContext` (R10). + * + * A suspension the region only performs inside a *nested* suspend-typed lambda does not count: the + * region carries that lambda with it, so the new function needs no `suspend`, and adding it breaks a + * call site that is not itself a suspend context. `scope.launch { }` and `runBlocking { }` are that + * shape, and "extract this whole launch block" is an everyday request. + */ +private fun KaSession.usesSuspend(elements: List): Boolean = + elements.any { root -> + PsiTreeUtil + .collectElementsOfType(root, KtSimpleNameExpression::class.java) + .any { it.getReferencedName() == COROUTINE_CONTEXT_NAME && !inNestedSuspendLambda(it, root) } || + PsiTreeUtil + .collectElementsOfType(root, KtCallExpression::class.java) + .any { isSuspendCall(it) && !inNestedSuspendLambda(it, root) } + } + +private fun KaSession.isSuspendCall(call: KtCallExpression): Boolean = + runCatching { + (call.resolveToCall()?.successfulFunctionCallOrNull()?.symbol as? KaNamedFunctionSymbol)?.isSuspend + }.getOrNull() == true + +/** + * Whether [element] sits inside a suspend-typed lambda that is itself inside [root]. + * + * An ordinary inline lambda -- `forEach`, `let`, `run` -- is not one, so a suspension inside it still + * propagates `suspend` outwards, which is correct: those bodies run in the caller's context. + */ +private fun KaSession.inNestedSuspendLambda( + element: PsiElement, + root: PsiElement, +): Boolean { + // Strict ancestors of [element] that are strict descendants of [root]. A lambda *containing* the + // region is not one of these: the region moves out of it, so the suspension is the new function's. + var current: PsiElement? = element.takeIf { it !== root }?.parent + while (current != null && current !== root) { + if (current is KtFunctionLiteral && isSuspendLambda(current)) return true + current = current.parent + } + return false +} + +/** + * Read off the lambda expression's own functional type rather than its symbol: the anonymous-function + * symbol in this Analysis API build carries no `suspend`, while the type inferred from the parameter + * it is passed to does. + */ +private fun KaSession.isSuspendLambda(lambda: KtFunctionLiteral): Boolean = + runCatching { + ((lambda.parent as? KtLambdaExpression)?.expressionType as? KaFunctionType)?.isSuspend + }.getOrNull() == true + +/** + * `@Composable` is added when the region calls one. Not polish: CoGo users write Compose apps on the + * device, and an extracted composable without the annotation does not compile (R10). + */ +private fun KaSession.usesComposable(elements: List): Boolean = + descendantsOf(elements, KtCallExpression::class.java).any { call -> + runCatching { + call + .resolveToCall() + ?.successfulFunctionCallOrNull() + ?.symbol + ?.annotations + ?.any { it.classId?.asFqNameString() == COMPOSABLE_FQ_NAME } + }.getOrNull() == true + } + +/** + * Names the new function must avoid (R12). + * + * [isLocalTarget] is tested first, and must be: a local `fun` inside a class member competes with the + * enclosing block's declarations, not with the class's members, and validating against the class + * instead lets the new local collide with a sibling local -- a redeclaration error. + * + * For a class target this is the whole member scope, **including inherited members**: a private + * function accidentally matching a supertype member is an accidental-override compile error. + * Rejecting any name match rather than only a signature match also means the refactoring never + * creates an overload the user did not ask for. + */ +private fun KaSession.takenNamesFor( + enclosing: KtDeclaration, + anchor: KtDeclaration, + isLocalTarget: Boolean, +): Set { + if (isLocalTarget) { + return PsiTreeUtil + .collectElementsOfType(anchor.parent, KtDeclaration::class.java) + .mapNotNull { it.name } + .toSet() + } + + val containingClass = PsiTreeUtil.getParentOfType(enclosing, KtClassOrObject::class.java, true) + if (containingClass != null) { + val fromScope = + runCatching { + (containingClass.symbol as? KaClassSymbol) + ?.memberScope + ?.callables + ?.mapNotNull { (it as? KaNamedSymbol)?.name?.asString() } + ?.toSet() + }.getOrNull().orEmpty() + val declared = containingClass.declarations.mapNotNull { it.name } + return fromScope + declared + } + + return enclosing.containingKtFile.declarations + .mapNotNull { it.name } + .toSet() +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt index 3427571a18..c6cc362228 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt @@ -103,7 +103,7 @@ fun suggestVariableName( ?: typeName?.let(::nameFromType) ?: FALLBACK_NAME val sanitised = base.takeIf { isIdentifier(it) && it !in HARD_KEYWORDS } ?: FALLBACK_NAME - return makeUnique(sanitised, takenNames) + return uniqueName(sanitised, takenNames) } private fun nameFromShape(expression: KtExpression): String? = @@ -144,7 +144,7 @@ private fun nameFromType(typeName: String): String? = private fun String.decapitaliseFirst(): String = if (isEmpty()) this else this[0].lowercaseChar() + substring(1) /** `size` -> `size1` -> `size2` until nothing in [takenNames] matches. */ -private fun makeUnique( +internal fun uniqueName( base: String, takenNames: Set, ): String { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt index 61eea683ae..8e0da41493 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt @@ -135,11 +135,14 @@ internal fun KaSession.referencedDeclarationCeiling(candidate: KtExpression): Ps * A declaration outside the candidate's own scopes -- a class member, a top-level property, anything * from a library -- constrains nothing; only locals and parameters do. * - * The implicit lambda parameter needs its own case: `it` has **no source PSI**, so the ordinary - * psi-based lookup finds nothing and would report "unconstrained", happily hoisting `it.length` clean - * out of its lambda into code that does not compile. A value-parameter symbol with no PSI, referenced - * by the name `it`, *is* by definition the implicit parameter of the innermost enclosing lambda -- a - * property of the language, not a guess about the text. + * The implicit-lambda-parameter branch below is **defensive, and unreachable in this Kotlin + * version**: `it` resolves to a value-parameter symbol whose PSI is the enclosing + * [KtFunctionLiteral] (`KtFakeSourceElementKind.ItLambdaParameter` is an allowed fake element kind), + * so the ordinary psi-based lookup already constrains it to that lambda. It is kept because a + * value-parameter symbol with no PSI referenced by the name `it` *is* by definition the implicit + * parameter of the innermost enclosing lambda -- a property of the language, not a guess about the + * text -- and without it a future version that stops supplying the PSI would silently hoist + * `it.length` clean out of its lambda into code that does not compile. */ private fun constrainingBodyFor( reference: KtSimpleNameExpression, @@ -246,7 +249,7 @@ internal fun KaSession.writeOffsetsFor( } /** Whether this reference is being written to rather than read. */ -private fun KtSimpleNameExpression.isWriteTarget(): Boolean { +internal fun KtSimpleNameExpression.isWriteTarget(): Boolean { val parent = parent if (parent is KtBinaryExpression && parent.left === this && parent.operationToken in ASSIGNMENT_TOKENS) return true if (parent is KtUnaryExpression && parent.operationToken in INCREMENT_TOKENS) return true diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactoringPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactoringPlan.kt new file mode 100644 index 0000000000..b58d6137a7 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactoringPlan.kt @@ -0,0 +1,13 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** + * What every interactive refactoring's background pass returns. + * + * The two fields are what makes applying a plan safe long after it was computed: [fileText] is the + * text its offsets refer to, and [documentVersion] is re-read on confirm so a plan computed against + * text the user has since edited is discarded rather than applied against shifted offsets. + */ +sealed interface RefactoringPlan { + val fileText: String + val documentVersion: Int +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt index 504b4e50de..f93d1bc842 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionTooltipTagTest.kt @@ -5,6 +5,7 @@ import com.itsaky.androidide.lsp.actions.CommentLineAction import com.itsaky.androidide.lsp.actions.UncommentLineAction import com.itsaky.androidide.lsp.kotlin.KotlinCodeActionsMenu.KT_LANG import com.itsaky.androidide.lsp.kotlin.actions.AddImportAction +import com.itsaky.androidide.lsp.kotlin.actions.ExtractMethodAction import com.itsaky.androidide.lsp.kotlin.actions.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction @@ -43,6 +44,7 @@ class KotlinCodeActionTooltipTagTest { ImplementMembersAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_IMPLEMENT_MEMBERS, SurroundWithTryCatchAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH, ExtractVariableAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE, + ExtractMethodAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD, ) assertEquals(expected, actualTags) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt new file mode 100644 index 0000000000..755dbf054c --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt @@ -0,0 +1,147 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.CallSiteForm +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodCandidate +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractMethodPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractedBody +import com.itsaky.androidide.lsp.kotlin.utils.refactor.MethodParameter +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.kotlin.utils.refactor.TextSpan +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** The sheet's derivation logic, tested without Compose, a fragment or an activity. */ +class ExtractMethodViewModelTest { + private fun candidate( + label: String, + suggestedName: String, + parameters: List = listOf(MethodParameter("a", "Int")), + returnTypeText: String? = "Int", + modifiers: List = listOf("private"), + takenNames: Set = emptySet(), + ) = ExtractMethodCandidate( + label = label, + span = TextSpan(0, 5), + suggestedName = suggestedName, + takenNames = takenNames, + annotations = emptyList(), + modifiers = modifiers, + receiverTypeText = null, + parameters = parameters, + returnTypeText = returnTypeText, + body = ExtractedBody.ExpressionBody(needsReturn = true), + callSite = CallSiteForm.Call, + insertOffset = 100, + insertIndent = "\t", + ) + + private fun plan( + candidates: List, + selectionMatched: Boolean = false, + ) = ExtractMethodPlan( + fileText = "unused", + documentVersion = 1, + candidates = candidates, + selectionMatchedCandidate = selectionMatched, + refusal = null, + ) + + @Test + fun `the initial state takes the first candidate's suggestion`() { + val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) + + assertEquals("total", model.uiState.value.name) + assertEquals(0, model.uiState.value.selectedCandidate) + assertNull(model.uiState.value.nameProblem) + } + + @Test + fun `the chooser is hidden for one candidate and for an exact selection match`() { + val single = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) + assertFalse(single.uiState.value.showCandidatePicker) + + val many = listOf(candidate("a + b", "total"), candidate("a + b + c", "total1")) + assertTrue(ExtractMethodViewModel(plan(many)).uiState.value.showCandidatePicker) + assertFalse(ExtractMethodViewModel(plan(many, selectionMatched = true)).uiState.value.showCandidatePicker) + } + + @Test + fun `the preview is the signature as it will be emitted`() { + val model = + ExtractMethodViewModel( + plan( + listOf( + candidate( + "load() + 1", + "total", + parameters = listOf(MethodParameter("id", "String")), + returnTypeText = "User", + modifiers = listOf("private", "suspend"), + ), + ), + ), + ) + + assertEquals("private suspend fun total(id: String): User", model.uiState.value.signaturePreview) + + model.onEvent(ExtractMethodUiEvent.NameChanged("loadUser")) + + assertEquals("private suspend fun loadUser(id: String): User", model.uiState.value.signaturePreview) + } + + @Test + fun `a name matching an inherited member is rejected`() { + val model = + ExtractMethodViewModel(plan(listOf(candidate("a + b", "total", takenNames = setOf("helper"))))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("helper")) + + assertEquals(NameProblem.AlreadyTaken, model.uiState.value.nameProblem) + assertFalse(model.uiState.value.canConfirm) + assertNull(model.choice()) + } + + @Test + fun `switching candidate re-suggests the name`() { + val model = + ExtractMethodViewModel( + plan(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))), + ) + model.onEvent(ExtractMethodUiEvent.NameChanged("mine")) + + model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) + + assertEquals("sum", model.uiState.value.name) + assertEquals(1, model.uiState.value.selectedCandidate) + } + + @Test + fun `the choice carries the selected candidate and the typed name`() { + val model = + ExtractMethodViewModel( + plan(listOf(candidate("a + b", "total"), candidate("a + b + c", "sum"))), + ) + model.onEvent(ExtractMethodUiEvent.CandidateSelected(1)) + model.onEvent(ExtractMethodUiEvent.NameChanged("combined")) + + val choice = model.choice() + + assertNotNull(choice) + assertEquals("a + b + c", choice!!.candidate.label) + assertEquals("combined", choice.name) + } + + @Test + fun `a blank name blocks confirmation`() { + val model = ExtractMethodViewModel(plan(listOf(candidate("a + b", "total")))) + + model.onEvent(ExtractMethodUiEvent.NameChanged("")) + + assertEquals(NameProblem.Blank, model.uiState.value.nameProblem) + assertNull(model.choice()) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt new file mode 100644 index 0000000000..d722edabcd --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt @@ -0,0 +1,418 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The emitted text, with every candidate built by hand -- no PSI, no analysis. Assertions are on the + * resulting file text, the only kind that catches an indentation or off-by-one error. + */ +class ExtractMethodEditTest { + private val file = + "package p\n" + + "class C {\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = a + b\n" + + "\t\treturn sum\n" + + "\t}\n" + + "}\n" + + private val enclosingStart = file.indexOf("fun demo") + private val enclosingEnd = file.indexOf("\t}\n}") + 2 + + private fun candidate( + span: TextSpan, + body: ExtractedBody, + callSite: CallSiteForm, + parameters: List = emptyList(), + returnTypeText: String? = null, + modifiers: List = listOf("private"), + annotations: List = emptyList(), + receiverTypeText: String? = null, + ) = ExtractMethodCandidate( + label = "region", + span = span, + suggestedName = "extracted", + takenNames = emptySet(), + annotations = annotations, + modifiers = modifiers, + receiverTypeText = receiverTypeText, + parameters = parameters, + returnTypeText = returnTypeText, + body = body, + callSite = callSite, + insertOffset = enclosingEnd, + insertIndent = "\t", + ) + + /** Applies the rewrites in the order they are returned, exactly as the language client does. */ + private fun apply( + text: String, + rewrites: List, + ): String = + rewrites.fold(text) { current, rewrite -> + current.substring(0, rewrite.span.start) + rewrite.newText + current.substring(rewrite.span.end) + } + + @Test + fun `the function insertion comes before the call site`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + parameters = listOf(MethodParameter("a", "Int"), MethodParameter("b", "Int")), + returnTypeText = "Int", + ), + "total", + ) + + assertNotNull(rewrites) + assertEquals(2, rewrites!!.size) + assertTrue( + "the insertion must be at a higher offset than the call site", + rewrites[0].span.start > rewrites[1].span.start, + ) + } + + @Test + fun `an insertion before the region puts the call site first`() { + // A local-function target: the new function is declared ahead of the one that calls it, so the + // descending-order invariant now puts the call site at the head of the list. + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + parameters = listOf(MethodParameter("a", "Int"), MethodParameter("b", "Int")), + returnTypeText = "Int", + ).copy(insertOffset = enclosingStart, modifiers = emptyList()), + "total", + ) + + assertNotNull(rewrites) + assertEquals(2, rewrites!!.size) + assertTrue( + "the call site must come first when the insertion precedes the region", + rewrites[0].span.start > rewrites[1].span.start, + ) + assertEquals(span, rewrites[0].span) + assertEquals(enclosingStart, rewrites[1].span.start) + } + + @Test + fun `an insertion before the region declares the function ahead of its anchor`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + parameters = listOf(MethodParameter("a", "Int"), MethodParameter("b", "Int")), + returnTypeText = "Int", + ).copy(insertOffset = enclosingStart, modifiers = emptyList()), + "total", + ) + + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun total(a: Int, b: Int): Int {\n" + + "\t\treturn a + b\n" + + "\t}\n" + + "\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = total(a, b)\n" + + "\t\treturn sum\n" + + "\t}\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `an insertion inside the region is rejected`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + ).copy(insertOffset = span.start + 1), + "total", + ) + + assertNull(rewrites) + } + + @Test + fun `an expression region becomes a call and a returning function`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + parameters = listOf(MethodParameter("a", "Int"), MethodParameter("b", "Int")), + returnTypeText = "Int", + ), + "total", + )!! + + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = total(a, b)\n" + + "\t\treturn sum\n" + + "\t}\n" + + "\n" + + "\tprivate fun total(a: Int, b: Int): Int {\n" + + "\t\treturn a + b\n" + + "\t}\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `a statement range with one output assigns at the call site`() { + val span = TextSpan(file.indexOf("val sum"), file.indexOf("val sum") + "val sum = a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.StatementBody(trailingReturn = "return sum"), + CallSiteForm.AssignOutput("sum"), + parameters = listOf(MethodParameter("a", "Int"), MethodParameter("b", "Int")), + returnTypeText = "Int", + ), + "total", + )!! + + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = total(a, b)\n" + + "\t\treturn sum\n" + + "\t}\n" + + "\n" + + "\tprivate fun total(a: Int, b: Int): Int {\n" + + "\t\tval sum = a + b\n" + + "\t\treturn sum\n" + + "\t}\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `a tail return region returns the call`() { + val span = TextSpan(file.indexOf("return sum"), file.indexOf("return sum") + "return sum".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.StatementBody(trailingReturn = null), + CallSiteForm.Return, + parameters = listOf(MethodParameter("sum", "Int")), + returnTypeText = "Int", + ), + "finish", + )!! + + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = a + b\n" + + "\t\treturn finish(sum)\n" + + "\t}\n" + + "\n" + + "\tprivate fun finish(sum: Int): Int {\n" + + "\t\treturn sum\n" + + "\t}\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `a multi-line statement range is reindented under the new function`() { + val text = + "package p\n" + + "fun demo(a: Int) {\n" + + "\tif (a > 0) {\n" + + "\t\tprintln(a)\n" + + "\t}\n" + + "}\n" + val start = text.indexOf("if (a > 0)") + val rewrites = + buildExtractMethodRewrites( + text, + ExtractMethodCandidate( + label = "region", + span = TextSpan(start, text.indexOf("\t}\n}") + 2), + suggestedName = "extracted", + takenNames = emptySet(), + annotations = emptyList(), + modifiers = listOf("private"), + receiverTypeText = null, + parameters = listOf(MethodParameter("a", "Int")), + returnTypeText = null, + body = ExtractedBody.StatementBody(trailingReturn = null), + callSite = CallSiteForm.Call, + insertOffset = text.length - 1, + insertIndent = "", + ), + "report", + )!! + + assertEquals( + "package p\n" + + "fun demo(a: Int) {\n" + + "\treport(a)\n" + + "}\n" + + "\n" + + "private fun report(a: Int) {\n" + + "\tif (a > 0) {\n" + + "\t\tprintln(a)\n" + + "\t}\n" + + "}\n", + apply(text, rewrites), + ) + } + + @Test + fun `a multi-line CRLF region is reindented and keeps CRLF throughout`() { + // Mirrors "a multi-line statement range is reindented under the new function" with \r\n in + // place of every \n, so reindent's split(newline) path -- the CRLF-sensitive code -- actually + // runs, not just the declaration builder's own append(newline) calls. + val text = + "package p\r\n" + + "fun demo(a: Int) {\r\n" + + "\tif (a > 0) {\r\n" + + "\t\tprintln(a)\r\n" + + "\t}\r\n" + + "}\r\n" + val start = text.indexOf("if (a > 0)") + val rewrites = + buildExtractMethodRewrites( + text, + ExtractMethodCandidate( + label = "region", + span = TextSpan(start, text.indexOf("\t}\r\n}") + 2), + suggestedName = "extracted", + takenNames = emptySet(), + annotations = emptyList(), + modifiers = listOf("private"), + receiverTypeText = null, + parameters = listOf(MethodParameter("a", "Int")), + returnTypeText = null, + body = ExtractedBody.StatementBody(trailingReturn = null), + callSite = CallSiteForm.Call, + insertOffset = text.length - 2, + insertIndent = "", + ), + "report", + )!! + + assertEquals( + "package p\r\n" + + "fun demo(a: Int) {\r\n" + + "\treport(a)\r\n" + + "}\r\n" + + "\r\n" + + "private fun report(a: Int) {\r\n" + + "\tif (a > 0) {\r\n" + + "\t\tprintln(a)\r\n" + + "\t}\r\n" + + "}\r\n", + apply(text, rewrites), + ) + } + + @Test + fun `a Unit-valued expression omits the return type and the return keyword`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val rewrites = + buildExtractMethodRewrites( + file, + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = false), + CallSiteForm.Call, + returnTypeText = null, + ), + "log", + )!! + + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun demo(a: Int, b: Int): Int {\n" + + "\t\tval sum = log()\n" + + "\t\treturn sum\n" + + "\t}\n" + + "\n" + + "\tprivate fun log() {\n" + + "\t\ta + b\n" + + "\t}\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `the signature preview matches what is emitted`() { + val span = TextSpan(file.indexOf("a + b"), file.indexOf("a + b") + "a + b".length) + val subject = + candidate( + span, + ExtractedBody.ExpressionBody(needsReturn = true), + CallSiteForm.Call, + parameters = listOf(MethodParameter("a", "Int")), + returnTypeText = "Int", + modifiers = listOf("private", "suspend"), + annotations = listOf("@Composable"), + receiverTypeText = "Foo", + ) + + assertEquals("@Composable private suspend fun Foo.total(a: Int): Int", subject.signatureText("total")) + assertTrue( + buildExtractMethodRewrites(file, subject, "total")!![0] + .newText + .contains("@Composable private suspend fun Foo.total(a: Int): Int {"), + ) + } + + @Test + fun `a span past the end of the text produces nothing`() { + val subject = + candidate( + TextSpan(file.length - 1, file.length + 10), + ExtractedBody.StatementBody(trailingReturn = null), + CallSiteForm.Call, + ) + + assertNull(buildExtractMethodRewrites(file, subject, "total")) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt new file mode 100644 index 0000000000..1209829719 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -0,0 +1,1182 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.progress.ICancelChecker +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.CancellationException + +/** + * The parts of the plan that need real resolution: the parameter set, the return type and call-site + * form, the modifiers, and one case per refusal reason. + * + * Where a rewrite is produced the assertion is on the resulting file text, which is the only + * assertion that catches an indentation or off-by-one error. + */ +class ExtractMethodPlanEndToEndTest : KtLspTest() { + private fun plan( + content: String, + start: Int, + end: Int = start, + ): ExtractMethodPlan { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + return buildExtractMethodPlan(env, path, start, end, documentVersion = 1, cancelChecker = noopCancelChecker()) + } + + private fun apply( + text: String, + rewrites: List, + ): String = + rewrites.fold(text) { current, rewrite -> + current.substring(0, rewrite.span.start) + rewrite.newText + current.substring(rewrite.span.end) + } + + private fun selection( + content: String, + from: String, + to: String, + ): Pair = content.indexOf(from) to (content.indexOf(to) + to.length) + + @Test + fun `an expression region parameterises the locals it uses, in first-use order`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + return b * a + a + } + """.trimIndent() + + val result = plan(content, content.indexOf("b * a") + 1) + val candidate = result.candidates.first { it.label == "b * a" } + + // Types are emitted fully qualified so they resolve without an import the file may not have. + assertEquals(listOf("b" to "kotlin.Int", "a" to "kotlin.Int"), candidate.parameters.map { it.name to it.typeText }) + assertEquals("kotlin.Int", candidate.returnTypeText) + assertEquals(listOf("private"), candidate.modifiers) + } + + @Test + fun `a statement range with no output returns Unit and calls as a statement`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(a: Int) { + log(a) + log(a + 1) + } + """.trimIndent() + val (start, end) = selection(content, "log(a)", "log(a + 1)") + + val result = plan(content, start, end) + val candidate = result.candidates.single() + + assertNull(candidate.returnTypeText) + assertEquals(CallSiteForm.Call, candidate.callSite) + assertEquals(listOf("a"), candidate.parameters.map { it.name }) + assertEquals("extracted", candidate.suggestedName) + } + + @Test + fun `a single output becomes the return value and a val at the call site`() { + val content = + """ + package p + fun demo(a: Int): Int { + val doubled = a * 2 + return doubled + 1 + } + """.trimIndent() + val (start, end) = selection(content, "val doubled", "val doubled = a * 2") + + val result = plan(content, start, end) + val candidate = result.candidates.single() + + assertEquals(CallSiteForm.AssignOutput("doubled"), candidate.callSite) + assertEquals("kotlin.Int", candidate.returnTypeText) + } + + @Test + fun `two outputs are declined`() { + val content = + """ + package p + fun demo(a: Int): Int { + val x = a * 2 + val y = a * 3 + return x + y + } + """.trimIndent() + val (start, end) = selection(content, "val x", "val y = a * 3") + + val refusal = plan(content, start, end).refusal + + assertTrue(refusal is ExtractionRefusal.MultipleOutputs) + assertEquals(listOf("x", "y"), (refusal as ExtractionRefusal.MultipleOutputs).names) + } + + @Test + fun `a reassigned outer var is declined and names the variable`() { + val content = + """ + package p + fun demo(items: List): Int { + var total = 0 + for (item in items) { + total += item + } + return total + } + """.trimIndent() + val (start, end) = selection(content, "for (item in items)", "\t}") + + val refusal = plan(content, start, end).refusal + + assertEquals(ExtractionRefusal.ReassignsOuterVar("total"), refusal) + } + + @Test + fun `a tail return keeps the return and returns the call`() { + val content = + """ + package p + fun demo(a: Int): Int { + val doubled = a * 2 + return doubled + 1 + } + """.trimIndent() + val (start, end) = selection(content, "return doubled", "return doubled + 1") + + val result = plan(content, start, end) + val candidate = result.candidates.single() + + assertEquals(CallSiteForm.Return, candidate.callSite) + assertEquals("kotlin.Int", candidate.returnTypeText) + assertEquals( + """ + package p + fun demo(a: Int): Int { + val doubled = a * 2 + return finish(doubled) + } + + private fun finish(doubled: kotlin.Int): kotlin.Int { + return doubled + 1 + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "finish")!!), + ) + } + + @Test + fun `a return in the middle of the range is declined`() { + val content = + """ + package p + fun demo(a: Int): Int { + if (a > 0) return a + val b = a * 2 + return b + } + """.trimIndent() + val (start, end) = selection(content, "if (a > 0) return a", "val b = a * 2") + + assertEquals(ExtractionRefusal.ExitsRegion, plan(content, start, end).refusal) + } + + @Test + fun `a break targeting an outer loop is declined`() { + val content = + """ + package p + fun demo(items: List) { + for (item in items) { + if (item < 0) break + println(item) + } + } + """.trimIndent() + val (start, end) = selection(content, "if (item < 0) break", "println(item)") + + assertEquals(ExtractionRefusal.ExitsRegion, plan(content, start, end).refusal) + } + + @Test + fun `an extension receiver is copied onto the new function`() { + val content = + """ + package p + class Foo(val n: Int) + fun Foo.bar(): Int { + return n * 2 + } + """.trimIndent() + + val result = plan(content, content.indexOf("n * 2") + 1) + val candidate = result.candidates.first { it.label == "n * 2" } + + assertEquals("Foo", candidate.receiverTypeText) + // `this` is a Foo at the call site, so nothing is passed and nothing is captured. + assertEquals(emptyList(), candidate.parameters) + } + + @Test + fun `an inner with receiver is declined and names the construct`() { + val content = + """ + package p + class Foo { val n: Int = 1 } + fun demo(f: Foo): Int { + with(f) { + return n * 2 + } + } + """.trimIndent() + + val refusal = plan(content, content.indexOf("n * 2") + 1).refusal + + assertEquals(ExtractionRefusal.InnerImplicitReceiver("with"), refusal) + } + + @Test + fun `a suspend call adds the suspend modifier`() { + val content = + """ + package p + suspend fun load(): Int = 1 + suspend fun demo(): Int { + return load() + 1 + } + """.trimIndent() + + val result = plan(content, content.indexOf("load() + 1") + 1) + val candidate = result.candidates.first { it.label == "load() + 1" } + + assertEquals(listOf("private", "suspend"), candidate.modifiers) + } + + @Test + fun `a Composable call adds the Composable annotation`() { + createSourceFile( + "Composable.kt", + """ + package androidx.compose.runtime + annotation class Composable + """.trimIndent(), + ) + val content = + """ + package p + import androidx.compose.runtime.Composable + @Composable fun Label(text: String) {} + @Composable fun Demo(name: String) { + Label(name) + } + """.trimIndent() + val (start, end) = selection(content, "Label(name)", "Label(name)") + + val candidate = plan(content, start, end).candidates.single() + + assertEquals(listOf("@Composable"), candidate.annotations) + } + + @Test + fun `a function-level type parameter is declined and names it`() { + val content = + """ + package p + fun demo(value: T): String { + val held: T = value + return held.toString() + } + """.trimIndent() + val (start, end) = selection(content, "val held", "val held: T = value") + + assertEquals(ExtractionRefusal.UsesTypeParameter("T"), plan(content, start, end).refusal) + } + + @Test + fun `taken names include inherited members`() { + val content = + """ + package p + open class Base { fun helper(): Int = 1 } + class Child : Base() { + fun demo(a: Int): Int { + return a * 2 + } + } + """.trimIndent() + + val candidate = plan(content, content.indexOf("a * 2") + 1).candidates.first { it.label == "a * 2" } + + // A private member matching an inherited name is an accidental-override compile error. + assertTrue("helper" in candidate.takenNames) + assertTrue("demo" in candidate.takenNames) + } + + @Test + fun `a selection spanning two blocks is declined as not a single region`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(c: Boolean, a: Int) { + if (c) { + log(a) + } + log(a + 1) + } + """.trimIndent() + val (start, end) = selection(content, "log(a)", "log(a + 1)") + + assertEquals(ExtractionRefusal.NotASingleRegion, plan(content, start, end).refusal) + } + + @Test + fun `an expression extraction rewrites the call site and adds a member function`() { + val content = + """ + package p + class C { + fun demo(a: Int, b: Int): Int { + return a + b + } + } + """.trimIndent() + + val result = plan(content, content.indexOf("a + b") + 1) + val candidate = result.candidates.first { it.label == "a + b" } + + assertEquals( + """ + package p + class C { + fun demo(a: Int, b: Int): Int { + return total(a, b) + } + + private fun total(a: kotlin.Int, b: kotlin.Int): kotlin.Int { + return a + b + } + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "total")!!), + ) + } + + @Test + fun `an it bound by a lambda inside the region is not turned into a parameter`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(names: List, extra: Int) { + names.forEach { log(it + extra) } + } + """.trimIndent() + val (start, end) = selection(content, "names.forEach", "names.forEach { log(it + extra) }") + + val candidate = plan(content, start, end).candidates.single() + + // `it` belongs to a lambda the region carries with it, so it is not captured from outside. + assertEquals(listOf("names", "extra"), candidate.parameters.map { it.name }) + } + + @Test + fun `a destructuring declaration read after the region is declined`() { + val content = + """ + package p + data class Point(val a: Int, val b: Int) + fun demo(p: Point): Int { + val (x, y) = p + return x + y + } + """.trimIndent() + val (start, end) = selection(content, "val (x, y)", "val (x, y) = p") + + val refusal = plan(content, start, end).refusal + + assertTrue(refusal is ExtractionRefusal.MultipleOutputs) + assertEquals(listOf("x", "y"), (refusal as ExtractionRefusal.MultipleOutputs).names) + } + + @Test + fun `an output reassigned after the region is declined`() { + val content = + """ + package p + fun compute(): Int = 1 + fun demo(flag: Boolean): Int { + var result = compute() + if (flag) result = 0 + return result + } + """.trimIndent() + val (start, end) = selection(content, "var result", "var result = compute()") + + // A `val` at the call site cannot carry an output the following code assigns to -- which is one + // value the call site cannot receive, not "more than one value". + assertEquals(ExtractionRefusal.OutputNotReturnable("result"), plan(content, start, end).refusal) + } + + @Test + fun `an inferred type parameter is declined even though the region names no type`() { + val content = + """ + package p + fun pick(a: T, b: T): T = a + fun demo(a: T, b: T): T { + return pick(a, b) + } + """.trimIndent() + + assertEquals( + ExtractionRefusal.UsesTypeParameter("T"), + plan(content, content.indexOf("pick(a, b)") + 1).refusal, + ) + } + + @Test + fun `a labelled return targeting an outer lambda is declined`() { + val content = + """ + package p + fun demo(items: List) { + items.forEach outer@{ item -> + listOf(item).forEach { + if (it < 0) return@outer + println(it) + } + } + } + """.trimIndent() + val (start, end) = selection(content, "listOf(item).forEach {", "\t\t}") + + // The nearest lambda is in the region, but `outer@` is not. + assertEquals(ExtractionRefusal.ExitsRegion, plan(content, start, end).refusal) + } + + @Test + fun `an inherited member used inside a with block is not mistaken for the receiver`() { + val content = + """ + package p + open class Base { fun helper(): Int = 1 } + class Child : Base() { + fun demo(n: Int): Int = + with(n) { + helper() + 1 + } + } + """.trimIndent() + + val result = plan(content, content.indexOf("helper() + 1") + 1) + + // `helper()` comes from the supertype, not from `with`'s receiver. + assertNull(result.refusal) + assertEquals("kotlin.Int", result.candidates.first { it.label == "helper() + 1" }.returnTypeText) + } + + @Test + fun `a scope receiver outside the stdlib scoping names is still declined`() { + val content = + """ + package p + class Scope { fun item(n: Int) {} } + fun column(body: Scope.() -> Unit) {} + fun demo() { + column { + item(1) + } + } + """.trimIndent() + val (start, end) = selection(content, "item(1)", "item(1)") + + assertEquals(ExtractionRefusal.InnerImplicitReceiver("column"), plan(content, start, end).refusal) + } + + @Test + fun `extracting from a getter inserts the new function after the whole property`() { + val content = + """ + package p + class C { + var backing: Int = 0 + var total: Int + get() { + return backing + 1 + } + set(value) { + backing = value + } + } + """.trimIndent() + + val result = plan(content, content.indexOf("backing + 1") + 1) + val candidate = result.candidates.first { it.label == "backing + 1" } + + assertEquals( + """ + package p + class C { + var backing: Int = 0 + var total: Int + get() { + return next() + } + set(value) { + backing = value + } + + private fun next(): kotlin.Int { + return backing + 1 + } + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "next")!!), + ) + } + + @Test + fun `a region using the backing field is declined`() { + val content = + """ + package p + class C { + var n: Int = 0 + get() { + return field + 1 + } + } + """.trimIndent() + + assertEquals( + ExtractionRefusal.UsesBackingField, + plan(content, content.indexOf("field + 1") + 1).refusal, + ) + } + + @Test + fun `a compound assignment through a receiver lambda is declined`() { + val content = + """ + package p + class Counter { var n = 0 } + fun demo(c: Counter) { + c.apply { + n += 1 + } + } + """.trimIndent() + val (start, end) = selection(content, "n += 1", "n += 1") + + // The assignment resolves to a compound access, not a member call, and used to slip through. + assertEquals(ExtractionRefusal.InnerImplicitReceiver("apply"), plan(content, start, end).refusal) + } + + @Test + fun `an increment through a receiver lambda is declined`() { + val content = + """ + package p + class Counter { var n = 0 } + fun demo(c: Counter) { + c.apply { + n++ + } + } + """.trimIndent() + val (start, end) = selection(content, "n++", "n++") + + assertEquals(ExtractionRefusal.InnerImplicitReceiver("apply"), plan(content, start, end).refusal) + } + + @Test + fun `a bare this inside a receiver lambda is declined`() { + val content = + """ + package p + class Foo(val n: Int) + fun log(f: Foo) {} + fun demo(f: Foo) { + f.apply { + log(this) + } + } + """.trimIndent() + val (start, end) = selection(content, "log(this)", "log(this)") + + assertEquals(ExtractionRefusal.InnerImplicitReceiver("apply"), plan(content, start, end).refusal) + } + + @Test + fun `a this inside a lambda that does not rebind it is not declined`() { + val content = + """ + package p + class Foo { + fun log(f: Foo) {} + fun demo(items: List) { + items.forEach { + log(this) + } + } + } + """.trimIndent() + val (start, end) = selection(content, "log(this)", "log(this)") + + // `forEach` binds `it`, not `this`, so `this` still means the Foo instance after the move. + assertNull(plan(content, start, end).refusal) + } + + @Test + fun `a type parameter reaching only the receiver is declined`() { + val content = + """ + package p + fun log(s: String) {} + fun List.summarize() { + log("size=" + size) + } + """.trimIndent() + val (start, end) = selection(content, "log(\"size=\" + size)", "log(\"size=\" + size)") + + // Nothing in the region names `T`; only the copied receiver does. + assertEquals(ExtractionRefusal.UsesTypeParameter("T"), plan(content, start, end).refusal) + } + + @Test + fun `a labelled break targeting an outer loop is declined`() { + val content = + """ + package p + fun demo(rows: List>) { + outer@ for (row in rows) { + for (cell in row) { + if (cell < 0) break@outer + println(cell) + } + } + } + """.trimIndent() + val (start, end) = selection(content, "for (cell in row)", "\t\t}") + + assertEquals(ExtractionRefusal.ExitsRegion, plan(content, start, end).refusal) + } + + @Test + fun `a labelled continue targeting an outer loop is declined`() { + val content = + """ + package p + fun demo(rows: List>) { + outer@ for (row in rows) { + for (cell in row) { + if (cell < 0) continue@outer + println(cell) + } + } + } + """.trimIndent() + val (start, end) = selection(content, "for (cell in row)", "\t\t}") + + assertEquals(ExtractionRefusal.ExitsRegion, plan(content, start, end).refusal) + } + + @Test + fun `a local function target gets no visibility modifier`() { + val content = + """ + package p + fun demo(a: Int): Int { + fun inner(b: Int): Int { + return b * 2 + } + return inner(a) + } + """.trimIndent() + + val result = plan(content, content.indexOf("b * 2") + 1) + val candidate = result.candidates.first { it.label == "b * 2" } + + // `private fun` inside a block does not compile, and a local function is only visible from its + // declaration onward -- so it must land *before* the function that calls it. + assertEquals(emptyList(), candidate.modifiers) + assertEquals( + """ + package p + fun demo(a: Int): Int { + fun doubled(b: kotlin.Int): kotlin.Int { + return b * 2 + } + + fun inner(b: Int): Int { + return doubled(b) + } + return inner(a) + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "doubled")!!), + ) + } + + @Test + fun `a local target inserts the new function before the call site`() { + val content = + """ + package p + fun demo(a: Int): Int { + fun inner(b: Int): Int { + return b * 2 + } + return inner(a) + } + """.trimIndent() + + val result = plan(content, content.indexOf("b * 2") + 1) + val candidate = result.candidates.first { it.label == "b * 2" } + + assertTrue(candidate.insertOffset < candidate.span.start) + } + + @Test + fun `a type parameter on an extension property is declined`() { + val content = + """ + package p + val List.doubled: Int + get() { + return size * 2 + } + """.trimIndent() + val (start, end) = selection(content, "return size * 2", "return size * 2") + + // The accessor's type parameters live on its property, the same place its receiver does. + assertEquals(ExtractionRefusal.UsesTypeParameter("T"), plan(content, start, end).refusal) + } + + @Test + fun `a smart cast to an intersection type is declined`() { + val content = + """ + package p + interface A { fun a(): Int } + interface B { fun b(): Int } + fun demo(x: Any): Int { + if (x is A && x is B) { + return x.a() + x.b() + } + return 0 + } + """.trimIndent() + val (start, end) = selection(content, "return x.a() + x.b()", "return x.a() + x.b()") + + // The narrowed type cannot be written out at all, which is not the same as not knowing it. + assertEquals(ExtractionRefusal.SmartCastParameter("x"), plan(content, start, end).refusal) + } + + @Test + fun `a captured local function is declined`() { + val content = + """ + package p + fun demo(a: Int): Int { + fun helper(): Int = 1 + return helper() + a + } + """.trimIndent() + val (start, end) = selection(content, "return helper() + a", "return helper() + a") + + assertEquals( + ExtractionRefusal.CapturedLocalDeclaration("helper"), + plan(content, start, end).refusal, + ) + } + + @Test + fun `a captured local class is declined`() { + val content = + """ + package p + fun demo(a: Int): Int { + class Holder(val n: Int) + return Holder(a).n + } + """.trimIndent() + val (start, end) = selection(content, "return Holder(a).n", "return Holder(a).n") + + assertEquals( + ExtractionRefusal.CapturedLocalDeclaration("Holder"), + plan(content, start, end).refusal, + ) + } + + @Test + fun `an extension property accessor keeps its receiver`() { + val content = + """ + package p + class Foo(val n: Int) + fun Foo.bar(): Int = n * 2 + val Foo.doubled: Int + get() { + return bar() + 1 + } + """.trimIndent() + + val result = plan(content, content.indexOf("bar() + 1") + 1) + val candidate = result.candidates.first { it.label == "bar() + 1" } + + assertEquals("Foo", candidate.receiverTypeText) + } + + @Test + fun `a smart-cast parameter is declined`() { + val content = + """ + package p + fun demo(value: Any): Int { + if (value is String) { + return value.length + 1 + } + return 0 + } + """.trimIndent() + + // `value: Any` breaks the moved body; `value: String` breaks the call site. + assertEquals( + ExtractionRefusal.SmartCastParameter("value"), + plan(content, content.indexOf("value.length + 1") + 1).refusal, + ) + } + + @Test + fun `a file the analysis cannot reach is declined as not analysable, not as a bad selection`() { + createSourceFile("Main.kt", "package p\n") + val missing = env.sourceRoots.first().resolve("Absent.kt") + + // "Select an expression, or whole statements inside one block" would blame a selection that + // never got looked at. + assertEquals( + ExtractionRefusal.CouldNotAnalyse, + buildExtractMethodPlan(env, missing, 0, 0, documentVersion = 1, cancelChecker = noopCancelChecker()).refusal, + ) + } + + @Test + fun `cancellation propagates instead of being reported as a refusal`() { + val content = + """ + package p + fun demo(a: Int): Int { + return a * 2 + } + """.trimIndent() + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + val cancelled = ScheduledCancelChecker(ICancelChecker.CANCELLED) + + // A cancelled action has no result to report; swallowing this would flash a message at a user + // who already moved on. + assertThrows(CancellationException::class.java) { + buildExtractMethodPlan( + env, + path, + content.indexOf("a * 2"), + content.indexOf("a * 2") + 5, + documentVersion = 1, + cancelChecker = cancelled, + ) + } + } + + @Test + fun `a type the file does not import is emitted fully qualified`() { + val content = + """ + package p + fun demo() { + val d = java.util.Date() + println(d.time) + } + """.trimIndent() + val (start, end) = selection(content, "println(d.time)", "println(d.time)") + + val result = plan(content, start, end) + val candidate = result.candidates.single() + + // `Date` came from inference, so the file names it nowhere and a short name would not resolve. + assertEquals(listOf("d" to "java.util.Date"), candidate.parameters.map { it.name to it.typeText }) + assertEquals( + """ + package p + fun demo() { + val d = java.util.Date() + extracted(d) + } + + private fun extracted(d: java.util.Date) { + println(d.time) + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "extracted")!!), + ) + } + + @Test + fun `a platform type is emitted as its lower bound rather than as String bang`() { + val content = + """ + package p + fun demo() { + val v = System.getProperty("k") + println(v.length) + } + """.trimIndent() + val (start, end) = selection(content, "println(v.length)", "println(v.length)") + + val candidate = plan(content, start, end).candidates.single() + + // `String!` is not Kotlin syntax; the lower bound is what the moved body already assumes. + assertEquals(listOf("v" to "kotlin.String"), candidate.parameters.map { it.name to it.typeText }) + } + + @Test + fun `a suspend call inside a nested suspend lambda does not add the suspend modifier`() { + val content = + """ + package p + suspend fun work() {} + fun launchIt(block: suspend () -> Unit) {} + fun demo() { + launchIt { work() } + println("x") + } + """.trimIndent() + val (start, end) = selection(content, "launchIt { work() }", "launchIt { work() }") + + val candidate = plan(content, start, end).candidates.single() + + // `demo` is not a suspend context, so a `suspend fun` here would not compile at the call site. + assertEquals(listOf("private"), candidate.modifiers) + } + + @Test + fun `a suspend call inside an ordinary inline lambda still adds the suspend modifier`() { + val content = + """ + package p + suspend fun work(n: Int) {} + suspend fun demo(items: List) { + items.forEach { work(it) } + println("x") + } + """.trimIndent() + val (start, end) = selection(content, "items.forEach", "items.forEach { work(it) }") + + val candidate = plan(content, start, end).candidates.single() + + // `forEach`'s lambda runs in the caller's context, so the suspension is the new function's. + assertEquals(listOf("private", "suspend"), candidate.modifiers) + } + + @Test + fun `statements inside a suspend lambda still add the suspend modifier`() { + val content = + """ + package p + suspend fun work() {} + fun launchIt(block: suspend () -> Unit) {} + fun demo() { + launchIt { + work() + } + } + """.trimIndent() + val start = content.indexOf("work()", content.indexOf("launchIt {")) + + val candidate = plan(content, start, start + "work()".length).candidates.single() + + // The region is *inside* the suspend lambda, so its own call site is a suspend context. + assertEquals(listOf("private", "suspend"), candidate.modifiers) + } + + @Test + fun `a local extension member reached through a qualified call is declined`() { + val content = + """ + package p + class Holder(val n: Int) + fun demo(h: Holder): Int { + fun Holder.twice(): Int = n * 2 + return h.twice() + 1 + } + """.trimIndent() + val (start, end) = selection(content, "return h.twice() + 1", "return h.twice() + 1") + + // A qualified selector is NOT skipped: `twice` is local, so it goes out of scope with the move. + assertEquals( + ExtractionRefusal.CapturedLocalDeclaration("twice"), + plan(content, start, end).refusal, + ) + } + + @Test + fun `a member extension invoked on a with receiver is declined`() { + val content = + """ + package p + class Holder(val n: Int) + class Scope { fun Holder.doubled(): Int = n * 2 } + fun demo(h: Holder): Int = with(Scope()) { h.doubled() + 1 } + """.trimIndent() + val (start, end) = selection(content, "h.doubled() + 1", "h.doubled() + 1") + + // `h.doubled()` reads as fully qualified but its *dispatch* receiver is `with`'s. This is the + // pervasive Compose shape (`with(density) { size.toPx() }`), so the selector guard in + // `innerImplicitReceiver` must stay shallow enough not to skip a call selector. + assertEquals(ExtractionRefusal.InnerImplicitReceiver("with"), plan(content, start, end).refusal) + } + + @Test + fun `a value typed by a local class is declined rather than emitted`() { + val content = + """ + package p + fun demo(): Int { + class Holder(val n: Int) + val h = Holder(1) + return h.n + 1 + } + """.trimIndent() + val (start, end) = selection(content, "return h.n + 1", "return h.n + 1") + + // `Holder` is out of scope at the insertion point, so no parameter for `h` can be written. + assertEquals( + ExtractionRefusal.CapturedLocalDeclaration("Holder"), + plan(content, start, end).refusal, + ) + } + + @Test + fun `a local object used as a qualifier is declined rather than dropped`() { + val content = + """ + package p + fun demo(): Int { + object Cfg { val n = 1 } + return Cfg.n + 1 + } + """.trimIndent() + val (start, end) = selection(content, "return Cfg.n + 1", "return Cfg.n + 1") + + // A class symbol is not callable, so it used to fail the capture cast and vanish silently. + assertEquals( + ExtractionRefusal.CapturedLocalDeclaration("Cfg"), + plan(content, start, end).refusal, + ) + } + + @Test + fun `a tail return in a secondary constructor extracts a Unit function`() { + val content = + """ + package p + class Foo { + constructor(x: Int) { + println(x) + return + } + } + """.trimIndent() + val (start, end) = selection(content, "println(x)", "return") + + val result = plan(content, start, end) + val candidate = result.candidates.single() + + // A constructor's symbol returns the constructed class, but its `return` carries no value. + assertNull(candidate.returnTypeText) + assertEquals( + """ + package p + class Foo { + constructor(x: Int) { + return tail(x) + } + + private fun tail(x: kotlin.Int) { + println(x) + return + } + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "tail")!!), + ) + } + + @Test + fun `a single destructuring entry read after the region is not reported as more than one value`() { + val content = + """ + package p + data class Point(val a: Int, val b: Int) + fun demo(p: Point): Int { + val (x, y) = p + return x + 1 + } + """.trimIndent() + val (start, end) = selection(content, "val (x, y)", "val (x, y) = p") + + // One value, in a form the call site cannot receive -- not "more than one value". + assertEquals(ExtractionRefusal.OutputNotReturnable("x"), plan(content, start, end).refusal) + } + + @Test + fun `a local fun target validates its name against the enclosing block, not the class`() { + val content = + """ + package p + class C { + fun demo(a: Int): Int { + fun inner(b: Int): Int { + return b * 2 + } + return inner(a) + } + } + """.trimIndent() + + val candidate = plan(content, content.indexOf("b * 2") + 1).candidates.first { it.label == "b * 2" } + + // A sibling local named `inner` is what the new local `fun` would redeclare; `demo` is not. + assertTrue("inner" in candidate.takenNames) + assertTrue("demo" !in candidate.takenNames) + } + + @Test + fun `a parameter whose type cannot be written out is declined`() { + val content = + """ + package p + fun demo(): Int { + val helper = object { + fun value(): Int = 1 + } + return helper.value() + } + """.trimIndent() + + assertEquals( + ExtractionRefusal.UnrenderableType, + plan(content, content.indexOf("helper.value()") + 1).refusal, + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt new file mode 100644 index 0000000000..60fdf04ead --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt @@ -0,0 +1,149 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.jetbrains.kotlin.psi.KtFile +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Region resolution is purely syntactic, so it is tested with no analysis session at all -- the same + * split `CandidateExpressions.kt` already has. + */ +class ExtractMethodRegionTest : KtLspTest() { + private fun file(content: String): KtFile = createSourceFile("Main.kt", content) + + private fun region( + content: String, + start: Int, + end: Int = start, + ): ExtractionRegion? = resolveExtractionRegion(file(content), start, end) + + private val twoStatements = + """ + package p + fun log(n: Int) {} + fun demo(a: Int, b: Int) { + val sum = a + b + log(sum) + } + """.trimIndent() + + @Test + fun `a bare cursor resolves to expression candidates`() { + // On the `+`, not `+ 1`: that lands between `a` and the space, which resolves to the `a` + // identifier itself (also a legal candidate) rather than the binary expression. + val region = region(twoStatements, twoStatements.indexOf("a + b") + 2) + + assertTrue(region is ExtractionRegion.Expressions) + assertEquals("a + b", (region as ExtractionRegion.Expressions).candidates.first().text) + } + + @Test + fun `a selection over two whole statements resolves to a statement range`() { + val start = twoStatements.indexOf("val sum") + val end = twoStatements.indexOf("log(sum)") + "log(sum)".length + + val region = region(twoStatements, start, end) + + assertTrue(region is ExtractionRegion.Statements) + assertEquals( + listOf("val sum = a + b", "log(sum)"), + (region as ExtractionRegion.Statements).statements.map { it.text }, + ) + } + + @Test + fun `ragged boundaries snap outward to whole statements`() { + // Starts mid-`sum` and stops mid-`log(sum)`, as a touch drag routinely does. + val start = twoStatements.indexOf("sum = a + b") + val end = twoStatements.indexOf("log(sum)") + 3 + + val region = region(twoStatements, start, end) + + assertTrue(region is ExtractionRegion.Statements) + assertEquals( + listOf("val sum = a + b", "log(sum)"), + (region as ExtractionRegion.Statements).statements.map { it.text }, + ) + } + + @Test + fun `a selection inside a single statement stays an expression selection`() { + val start = twoStatements.indexOf("a + b") + + val region = region(twoStatements, start, start + "a + b".length) + + assertTrue(region is ExtractionRegion.Expressions) + assertEquals("a + b", (region as ExtractionRegion.Expressions).candidates.first().text) + assertTrue(region.selectionMatchedInnermost) + } + + @Test + fun `a partial selection with no expression candidate still snaps to the statement`() { + // Skips the leading `val`, as a touch drag that starts a little late routinely does. Both + // ends land inside the same KtProperty, which is a declaration, not a legal expression + // target, so the expression path has nothing to offer and the snapped statement wins. + val start = twoStatements.indexOf("sum") + val end = twoStatements.indexOf("a + b") + "a + b".length + + val region = region(twoStatements, start, end) + + assertTrue(region is ExtractionRegion.Statements) + assertEquals( + listOf("val sum = a + b"), + (region as ExtractionRegion.Statements).statements.map { it.text }, + ) + } + + @Test + fun `a selection spanning two different blocks resolves to nothing`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(c: Boolean, a: Int) { + if (c) { + log(a) + } + log(a + 1) + } + """.trimIndent() + val start = content.indexOf("log(a)") + val end = content.indexOf("log(a + 1)") + "log(a + 1)".length + + assertNull(region(content, start, end)) + } + + @Test + fun `the statement range span covers first to last statement`() { + val start = twoStatements.indexOf("val sum") + val end = twoStatements.indexOf("log(sum)") + "log(sum)".length + + val region = region(twoStatements, start, end) as ExtractionRegion.Statements + + assertEquals(TextSpan(start, end), region.span) + } + + @Test + fun `a whitespace-only selection resolves to nothing`() { + val start = twoStatements.indexOf("val sum") - 1 + + assertNull(region(twoStatements, start, start + 1)) + } + + @Test + fun `a property initializer outside an executable body resolves to nothing`() { + val content = + """ + package p + fun compute(): Int = 1 + class C { + val x = compute() + compute() + } + """.trimIndent() + + assertNull(region(content, content.indexOf("compute() + compute()") + 1)) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 3ebed7e55b..22fa37ae8c 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -539,6 +539,24 @@ That name is already used No expression to extract here The file changed. Try extracting again. + + + Extract method + Extract method + Signature + The file changed. Try extracting again. + Select an expression, or whole statements inside one block + Could not analyse the selection. Try again. + The selection produces more than one value: %1$s + The selection produces %1$s, which cannot be handed back as a return value + The selection assigns to %1$s, which is declared outside it + The selection jumps out of itself with return, break or continue + The selection uses members of the enclosing %1$s receiver + The selection uses type parameter %1$s + A type in the selection cannot be written out + The selection uses the property\'s backing field, which only exists inside this accessor + The selection uses %1$s under a smart cast that does not hold outside the selection + The selection uses %1$s, which goes out of scope once the selection moves Select fields No fields selected No fields found