From bb09111e8dd5e43fd8c102816dc2017c52a70acb Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 11 Aug 2026 14:29:48 +0000 Subject: [PATCH 01/17] ADFA-5080: Add extract-method requirements and ADR 0013 Requirements only - no implementation yet. R1 to R16 plus non-goals, 21 acceptance criteria, the design and the test split; shared vocabulary and primitives come from kotlin-extract-variable.md rather than being restated. ADR 0013 records the principle most of those requirements are an application of: the refactoring moves code, never edits the interior of what it moved, and declines with a specific reason where it cannot transform faithfully. Two limitations it creates are tracked separately - ADFA-5081 (multi-edit undo) and ADFA-5082 (reassigned outer var as the single output). --- ...efactorings-decline-rather-than-rewrite.md | 63 ++++ docs/adr/README.md | 1 + docs/features/kotlin-extract-method.md | 274 ++++++++++++++++++ docs/features/kotlin-extract-variable.md | 6 +- 4 files changed, 341 insertions(+), 3 deletions(-) create mode 100644 docs/adr/0013-refactorings-decline-rather-than-rewrite.md create mode 100644 docs/features/kotlin-extract-method.md 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..1b8c065c5c --- /dev/null +++ b/docs/features/kotlin-extract-method.md @@ -0,0 +1,274 @@ +# Kotlin extract method (K2 LSP) + +- **Ticket:** ADFA-5080 (subtask of ADFA-3317; split out of ADFA-4826, which now covers extract variable only) +- **Status:** Requirements only - not 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, and the version is re-read on confirm with a mismatch refusing the edit. Shared via the `RefactoringPlan` supertype. + +**R4 - Target.** One uniform rule, no target picker: **the new function is inserted as a sibling of the enclosing declaration**, immediately after it. 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 with the existing `renderName(KaType)`. A type that cannot be rendered - an anonymous or intersection type, or a resolution failure - **declines the extraction** (`UnrenderableType`) rather than emitting uncompilable text. +- **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 is supported; **two or more declines** (`MultipleOutputs`, naming them). + +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. + +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. +- **`@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, e.g. `private suspend fun loadUser(id: String): 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 | +| `MultipleOutputs` | the selection produces more than one 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 | + +Five of the seven are actionable - they tell the user what to change - and two of them (`ReassignsOuterVar`, `InnerImplicitReceiver`) are common enough that a generic message would read as the feature being broken. 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. + +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 after the enclosing declaration - emitted as **two `TextEdit`s in one `DocumentChange`, ordered new-function-first (descending document order)**. + +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). Emitting the call site first would shift the insertion point and corrupt the file. + +**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 plus a log line, never an uncaught throw - the action framework catches only `IllegalArgumentException` and this runs on a scope with no exception handler. + +## 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`, `documentVersion`, the version guard), introduced in the extract-variable PR so this one is purely additive. + +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) From bb9dab1ac1e1a624455be48f511eefb998cdd8d8 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 15:42:47 +0000 Subject: [PATCH 02/17] ADFA-5080: Hoist the shared refactoring plan supertype --- .../lsp/kotlin/utils/refactor/ExtractionPlan.kt | 6 +++--- .../lsp/kotlin/utils/refactor/RefactoringPlan.kt | 13 +++++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactoringPlan.kt 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/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 +} From 60220d2cca8b4b4b0df0cb22f532ef02fe3401ec Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 15:50:37 +0000 Subject: [PATCH 03/17] ADFA-5080: Resolve a selection to an extraction region --- .../kotlin/utils/refactor/ExtractionRegion.kt | 120 ++++++++++++++++ .../utils/refactor/ExtractMethodRegionTest.kt | 132 ++++++++++++++++++ 2 files changed, 252 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt 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..38a848233e --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt @@ -0,0 +1,120 @@ +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 -- but a selection that lies strictly + * inside one statement is still an expression selection: widening it to the whole statement would + * silently extract more than the user picked. + */ +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 statements = snapToStatements(file, start, end) ?: return expressionRegion(file, selectionStart, selectionEnd) + + val only = statements.singleOrNull() + if (only != null && (start > only.textRange.startOffset || end < only.textRange.endOffset)) { + expressionRegion(file, selectionStart, selectionEnd)?.let { return it } + } + + val block = statements.first().parent as? KtBlockExpression ?: return null + return ExtractionRegion.Statements(statements, 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) +} + +/** + * 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, +): List? { + val first = statementContaining(file, start) ?: return null + val last = statementContaining(file, (end - 1).coerceAtLeast(start)) ?: 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 statements.subList(from, to + 1).toList() +} + +/** + * 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/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..54413341e0 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt @@ -0,0 +1,132 @@ +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 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)) + } +} From 513b2d4d3080e53368b4200715eaccff620449d1 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 16:03:38 +0000 Subject: [PATCH 04/17] ADFA-5080: Fix KDoc and pin fallthrough for extraction region --- .../kotlin/utils/refactor/ExtractionRegion.kt | 29 ++++++++++++------- .../utils/refactor/ExtractMethodRegionTest.kt | 17 +++++++++++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt index 38a848233e..1783322d94 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionRegion.kt @@ -45,9 +45,12 @@ sealed interface ExtractionRegion { * 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 -- but a selection that lies strictly - * inside one statement is still an expression selection: widening it to the whole statement would - * silently extract more than the user picked. + * 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, @@ -57,15 +60,14 @@ fun resolveExtractionRegion( val (start, end) = trimToCode(file.text, selectionStart, selectionEnd) ?: return null if (start == end) return expressionRegion(file, selectionStart, selectionEnd) - val statements = snapToStatements(file, start, end) ?: return expressionRegion(file, selectionStart, selectionEnd) + val range = snapToStatements(file, start, end) ?: return expressionRegion(file, selectionStart, selectionEnd) - val only = statements.singleOrNull() + val only = range.statements.singleOrNull() if (only != null && (start > only.textRange.startOffset || end < only.textRange.endOffset)) { expressionRegion(file, selectionStart, selectionEnd)?.let { return it } } - val block = statements.first().parent as? KtBlockExpression ?: return null - return ExtractionRegion.Statements(statements, block) + return ExtractionRegion.Statements(range.statements, range.block) } private fun expressionRegion( @@ -78,6 +80,12 @@ private fun expressionRegion( 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]. * @@ -88,9 +96,10 @@ private fun snapToStatements( file: KtFile, start: Int, end: Int, -): List? { +): 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).coerceAtLeast(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 @@ -100,7 +109,7 @@ private fun snapToStatements( val from = statements.indexOfFirst { it === first } val to = statements.indexOfFirst { it === last } if (from < 0 || to < from) return null - return statements.subList(from, to + 1).toList() + return StatementRange(statements.subList(from, to + 1).toList(), block) } /** diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt index 54413341e0..60fdf04ead 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodRegionTest.kt @@ -80,6 +80,23 @@ class ExtractMethodRegionTest : KtLspTest() { 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 = From c1ff95b0ac7fb65b606a54862a1cbfadc615bb4b Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 16:11:07 +0000 Subject: [PATCH 05/17] ADFA-5080: Add the extract-method plan model and its two rewrites --- .../utils/refactor/ExtractMethodEdit.kt | 83 +++++ .../utils/refactor/ExtractMethodPlan.kt | 145 +++++++++ .../utils/refactor/ExtractMethodEditTest.kt | 294 ++++++++++++++++++ 3 files changed, 522 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt 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..11a441c5f2 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt @@ -0,0 +1,83 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** + * The two replacements an extraction performs: the new function, then the call that replaces the + * region. + * + * **The 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. + * The insertion point sits after the region, so emitting the call first would shift it and corrupt + * the file. Descending document order is the only safe order. + * + * 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 || 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 { + // A blank line separates the new function from the declaration it follows. + append(newline).append(newline) + append(indent).append(candidate.signatureText(name)).append(" {").append(newline) + bodyLines.forEach { append(bodyIndent).append(it).append(newline) } + append(indent).append('}') + } + + val call = "$name(${candidate.parameters.joinToString(", ") { it.name }})" + val callText = + when (val form = candidate.callSite) { + CallSiteForm.Call -> call + is CallSiteForm.AssignOutput -> "val ${form.name} = $call" + CallSiteForm.Return -> "return $call" + } + + return listOf( + RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), declaration), + RewriteSpan(span, callText), + ) +} + +/** + * 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..47ba274d3f --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -0,0 +1,145 @@ +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 + + /** Two or more locals declared inside the region are read after it (R7). */ + data class MultipleOutputs( + val names: List, + ) : 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 +} + +/** + * 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/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..7ada10d58d --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt @@ -0,0 +1,294 @@ +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 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 CRLF file keeps CRLF`() { + val text = + "package p\r\n" + + "fun demo(a: Int) {\r\n" + + "\tprintln(a)\r\n" + + "}\r\n" + val start = text.indexOf("println(a)") + val rewrites = + buildExtractMethodRewrites( + text, + ExtractMethodCandidate( + label = "region", + span = TextSpan(start, start + "println(a)".length), + 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", + )!! + + assertTrue(rewrites.all { !it.newText.contains("\n") || it.newText.contains("\r\n") }) + assertTrue(apply(text, rewrites).contains("\r\nprivate fun report(a: Int) {\r\n")) + } + + @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")) + } +} From a8177c1c18cc94d4a6326058555f4fe293451625 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 16:19:34 +0000 Subject: [PATCH 06/17] ADFA-5080: Strengthen the CRLF test and cover the Unit-expression case --- .../utils/refactor/ExtractMethodEditTest.kt | 59 +++++++++++++++++-- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt index 7ada10d58d..32b4baa504 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt @@ -225,19 +225,24 @@ class ExtractMethodEditTest { } @Test - fun `a CRLF file keeps CRLF`() { + 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" + - "\tprintln(a)\r\n" + + "\tif (a > 0) {\r\n" + + "\t\tprintln(a)\r\n" + + "\t}\r\n" + "}\r\n" - val start = text.indexOf("println(a)") + val start = text.indexOf("if (a > 0)") val rewrites = buildExtractMethodRewrites( text, ExtractMethodCandidate( label = "region", - span = TextSpan(start, start + "println(a)".length), + span = TextSpan(start, text.indexOf("\t}\r\n}") + 2), suggestedName = "extracted", takenNames = emptySet(), annotations = emptyList(), @@ -253,8 +258,50 @@ class ExtractMethodEditTest { "report", )!! - assertTrue(rewrites.all { !it.newText.contains("\n") || it.newText.contains("\r\n") }) - assertTrue(apply(text, rewrites).contains("\r\nprivate fun report(a: Int) {\r\n")) + 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 From 83eef142648f4a0a1bea041b029a383c7b5acd94 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 16:33:54 +0000 Subject: [PATCH 07/17] ADFA-5080: Derive the extracted signature, or a typed refusal --- .../utils/refactor/ExtractMethodPlanner.kt | 80 +++ .../kotlin/utils/refactor/MethodSignature.kt | 497 ++++++++++++++++++ .../kotlin/utils/refactor/NameSuggestion.kt | 4 +- .../lsp/kotlin/utils/refactor/Occurrences.kt | 2 +- .../refactor/ExtractMethodPlanEndToEndTest.kt | 368 +++++++++++++ 5 files changed, 948 insertions(+), 3 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt 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..55143ab0c1 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanner.kt @@ -0,0 +1,80 @@ +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 + +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). + */ +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.NotASingleRegion) + + 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. + val refusal = + results.filterIsInstance().firstOrNull()?.refusal + ?: ExtractionRefusal.NotASingleRegion + 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 -> + logger.warn("Failed to build extract-method plan for {}", nioPath, error) + ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + } 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..8e0b2e4b9f --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -0,0 +1,497 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.utils.renderName +import org.jetbrains.kotlin.analysis.api.KaExperimentalApi +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.resolution.successfulFunctionCallOrNull +import org.jetbrains.kotlin.analysis.api.resolution.symbol +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.KaValueParameterSymbol +import org.jetbrains.kotlin.analysis.api.symbols.KaVariableSymbol +import org.jetbrains.kotlin.analysis.api.symbols.markers.KaNamedSymbol +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.KtFile +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtLoopExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +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.KtTypeReference + +/** 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" + +/** + * Receiver-binding scoping functions. `let`, `also` and `forEach` are absent on purpose: they bind + * `it`, which is a captured declaration and becomes an ordinary parameter (R5). + */ +private val RECEIVER_SCOPING_FUNCTIONS = + setOf("with", "apply", "run", "buildString", "buildList", "buildMap", "buildSet") + +/** 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) + + typeParameterIn(enclosing, elements)?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + 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) emptyList() else outputsOf(enclosing, elements, span) + if (outputs.size > 1) { + return refuse(ExtractionRefusal.MultipleOutputs(outputs.mapNotNull { it.name })) + } + // The tail-return exception holds only when nothing else flows out (R8). + if (tailReturn && outputs.isNotEmpty()) return refuse(ExtractionRefusal.ExitsRegion) + + val parameters = capturedParameters(enclosing, elements, span) ?: return refuse(ExtractionRefusal.UnrenderableType) + + val returnTypeText = + when { + isExpression -> { + renderedTypeOrNull(first) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + + tailReturn -> { + enclosingReturnType(enclosing) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + + outputs.size == 1 -> { + renderedDeclarationType(outputs.single()) ?: return refuse(ExtractionRefusal.UnrenderableType) + } + + else -> { + null + } + }.takeUnless { it == "Unit" } + + val body = + when { + isExpression -> ExtractedBody.ExpressionBody(needsReturn = returnTypeText != null) + outputs.size == 1 -> ExtractedBody.StatementBody(trailingReturn = "return ${outputs.single().name.orEmpty()}") + else -> ExtractedBody.StatementBody(trailingReturn = null) + } + + val callSite = + when { + tailReturn -> CallSiteForm.Return + outputs.size == 1 -> CallSiteForm.AssignOutput(outputs.single().name.orEmpty()) + else -> CallSiteForm.Call + } + + val takenNames = takenNamesFor(enclosing) + + 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 = if (usesSuspend(elements)) listOf("private", "suspend") else listOf("private"), + receiverTypeText = (enclosing as? KtNamedFunction)?.receiverTypeReference?.text, + parameters = parameters, + returnTypeText = returnTypeText, + body = body, + callSite = callSite, + insertOffset = enclosing.textRange.endOffset, + insertIndent = leadingIndentAt(fileText, enclosing.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) } + +/** + * 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). + * + * Returns null when a type cannot be rendered as source, which declines the extraction rather than + * emitting text that will not compile. + */ +private fun KaSession.capturedParameters( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): List? { + val parameters = mutableListOf() + val seen = mutableSetOf() + + for (reference in simpleNamesIn(elements).sortedBy { it.textRange.startOffset }) { + val symbol = + runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() 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. + symbol is KaValueParameterSymbol && + reference.getReferencedName() == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString() -> { + "it" + } + + else -> { + continue + } + } + if (!seen.add(key)) continue + + val typeText = renderedSymbolType(symbol) ?: return null + parameters += MethodParameter(name = reference.getReferencedName(), typeText = typeText) + } + return parameters +} + +/** A type that cannot be written out as source -- anonymous, intersection, or a resolution error. */ +private fun isUnrenderable(text: String): Boolean = + text.isBlank() || + text.contains("anonymous") || + text.contains("ERROR") || + text.contains(" & ") + +@OptIn(KaExperimentalApi::class) +private fun KaSession.renderedSymbolType(symbol: KaCallableSymbol): String? = + runCatching { renderName(symbol.returnType) }.getOrNull()?.takeUnless(::isUnrenderable) + +@OptIn(KaExperimentalApi::class) +private fun KaSession.renderedTypeOrNull(expression: KtExpression): String? = + runCatching { expression.expressionType?.let { renderName(it) } }.getOrNull()?.takeUnless(::isUnrenderable) + +@OptIn(KaExperimentalApi::class) +private fun KaSession.renderedDeclarationType(property: KtProperty): String? = + runCatching { (property.symbol as? KaCallableSymbol)?.returnType?.let { renderName(it) } } + .getOrNull() + ?.takeUnless(::isUnrenderable) + +@OptIn(KaExperimentalApi::class) +private fun KaSession.enclosingReturnType(enclosing: KtDeclaration): String? = + runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType?.let { renderName(it) } } + .getOrNull() + ?.takeUnless(::isUnrenderable) + +/** + * Locals declared inside the region and read after it (R7). Exactly one is supported. + * + * "Read after it" 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, +): List { + val declared = descendantsOf(elements, KtProperty::class.java) + if (declared.isEmpty()) return emptyList() + + val laterReads = + PsiTreeUtil + .collectElementsOfType(enclosing, KtSimpleNameExpression::class.java) + .filter { it.textRange.startOffset >= span.end } + .mapNotNull { + runCatching { + it.mainReference + ?.resolveToSymbols() + ?.firstOrNull() + ?.psi + }.getOrNull() + }.toSet() + + return declared.filter { it in laterReads } +} + +/** + * 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() }.getOrNull() as? KaVariableSymbol + ?: continue + if (symbol.isVal) 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 is fine only when its lambda is inside the region. + if (returnExpression.getLabelName() == null) return true + val lambda = PsiTreeUtil.getParentOfType(returnExpression, KtFunctionLiteral::class.java, true) + if (lambda == null || !inRegion(lambda, span)) return true + } + return hasLoopExit(elements, span) +} + +private fun hasLoopExit( + elements: List, + span: TextSpan, +): Boolean { + val jumps = + descendantsOf(elements, KtBreakExpression::class.java) + + descendantsOf(elements, KtContinueExpression::class.java) + return jumps.any { jump -> + val loop = PsiTreeUtil.getParentOfType(jump, KtLoopExpression::class.java, true) + loop == null || !inRegion(loop, span) + } +} + +/** + * The name of the enclosing function's type parameter the region uses, 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). + */ +private fun typeParameterIn( + enclosing: KtDeclaration, + elements: List, +): String? { + val names = (enclosing as? KtNamedFunction)?.typeParameters?.mapNotNull { it.name }.orEmpty() + 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) } } +} + +/** 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) + +/** + * 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. + */ +private fun KaSession.innerImplicitReceiver( + enclosing: KtDeclaration, + elements: List, + span: TextSpan, +): String? { + val construct = enclosingScopingCall(elements.first(), enclosing) ?: return null + val enclosingClass = PsiTreeUtil.getParentOfType(enclosing, KtClassOrObject::class.java, true) + + for (reference in simpleNamesIn(elements)) { + val parent = reference.parent + if (parent is KtQualifiedExpression && parent.selectorExpression === reference) continue + if (parent is KtCallExpression && parent.calleeExpression !== reference) continue + + val symbol = + runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() as? KaCallableSymbol + ?: continue + val declarationPsi = runCatching { symbol.psi }.getOrNull() ?: continue + + // A local or a member of the class the new function joins needs nothing. + if (PsiTreeUtil.isAncestor(enclosing, declarationPsi, true)) continue + if (enclosingClass != null && PsiTreeUtil.isAncestor(enclosingClass, declarationPsi, true)) continue + // A top-level declaration resolves unchanged from anywhere in the file. + if (declarationPsi.parent is KtFile) continue + // Anything else reached without a qualifier came in through the scoping receiver. + if (inRegion(declarationPsi, span)) continue + return construct + } + return null +} + +/** The callee name of the nearest receiver-binding scoping call between [element] and [enclosing]. */ +private fun enclosingScopingCall( + element: PsiElement, + enclosing: KtDeclaration, +): String? { + var current: PsiElement? = element + while (current != null && current !== enclosing) { + if (current is KtFunctionLiteral) { + val call = PsiTreeUtil.getParentOfType(current, KtCallExpression::class.java, true) + val callee = (call?.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() + if (callee != null && callee in RECEIVER_SCOPING_FUNCTIONS) return callee + } + current = current.parent + } + return null +} + +/** `suspend` is added when the region calls one, or touches `coroutineContext` (R10). */ +private fun KaSession.usesSuspend(elements: List): Boolean { + if (simpleNamesIn(elements).any { it.getReferencedName() == "coroutineContext" }) return true + return descendantsOf(elements, KtCallExpression::class.java).any { call -> + runCatching { + (call.resolveToCall()?.successfulFunctionCallOrNull()?.symbol as? KaNamedFunctionSymbol)?.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). + * + * 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): Set { + 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 + } + + // A local `fun` target: the enclosing block's own declarations. Otherwise the file's top level. + val block = enclosing.parent + if (block is KtBlockExpression) { + return PsiTreeUtil + .collectElementsOfType(block, KtDeclaration::class.java) + .mapNotNull { it.name } + .toSet() + } + 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..1c365f0482 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 @@ -246,7 +246,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/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..6e31cd89f6 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -0,0 +1,368 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * 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" } + + assertEquals(listOf("b" to "Int", "a" to "Int"), candidate.parameters.map { it.name to it.typeText }) + assertEquals("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("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("Int", candidate.returnTypeText) + assertEquals( + """ + package p + fun demo(a: Int): Int { + val doubled = a * 2 + return finish(doubled) + } + + private fun finish(doubled: Int): 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: Int, b: Int): Int { + return a + b + } + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "total")!!), + ) + } +} From 21a19241844ea63394c5df4743afa71752c67ac7 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 17:08:11 +0000 Subject: [PATCH 08/17] ADFA-5080: Close the extract-method refusal gaps that emit broken Kotlin --- .../utils/refactor/ExtractMethodPlan.kt | 12 +- .../kotlin/utils/refactor/MethodSignature.kt | 294 +++++++++++++----- .../refactor/ExtractMethodPlanEndToEndTest.kt | 214 +++++++++++++ 3 files changed, 439 insertions(+), 81 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 47ba274d3f..ec8dc179a9 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -75,7 +75,11 @@ sealed interface ExtractionRefusal { /** The selection is neither one expression nor whole statements inside one block (R2). */ data object NotASingleRegion : ExtractionRefusal - /** Two or more locals declared inside the region are read after it (R7). */ + /** + * The region declares something the code after it still needs, and a single returned value cannot + * carry it (R7): two or more locals, a destructuring declaration, a local `fun` or class, or a + * local the following code reassigns. [names] is what is in the way, so the message can name it. + */ data class MultipleOutputs( val names: List, ) : ExtractionRefusal @@ -100,6 +104,12 @@ sealed interface 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 } /** diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 8e0b2e4b9f..2685c0db16 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -3,11 +3,17 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.utils.renderName import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall +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.KaValueParameterSymbol import org.jetbrains.kotlin.analysis.api.symbols.KaVariableSymbol import org.jetbrains.kotlin.analysis.api.symbols.markers.KaNamedSymbol @@ -23,10 +29,13 @@ 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.KtFile 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.KtProperty import org.jetbrains.kotlin.psi.KtPropertyAccessor @@ -35,18 +44,18 @@ import org.jetbrains.kotlin.psi.KtReturnExpression import org.jetbrains.kotlin.psi.KtSecondaryConstructor import org.jetbrains.kotlin.psi.KtSimpleNameExpression 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" -/** - * Receiver-binding scoping functions. `let`, `also` and `forEach` are absent on purpose: they bind - * `it`, which is a captured declaration and becomes an ordinary parameter (R5). - */ -private val RECEIVER_SCOPING_FUNCTIONS = - setOf("with", "apply", "run", "buildString", "buildList", "buildMap", "buildSet") +/** 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" /** Either a derived candidate or the reason there is not one. */ internal sealed interface SignatureResult { @@ -75,19 +84,28 @@ internal fun KaSession.buildCandidate( val span = TextSpan(first.textRange.startOffset, last.textRange.endOffset) val enclosing = enclosingDeclaration(first) ?: return refuse(ExtractionRefusal.NotASingleRegion) - typeParameterIn(enclosing, elements)?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + 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) emptyList() else outputsOf(enclosing, elements, span) - if (outputs.size > 1) { - return refuse(ExtractionRefusal.MultipleOutputs(outputs.mapNotNull { it.name })) + 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 -- a second local, a destructuring entry, a local + // `fun`, or a local reassigned afterwards -- is refused rather than silently dropped (R7). + if (outputs.declarations.size > 1 || + outputs.declarations.any { it !is KtProperty } || + outputs.writtenAfter.isNotEmpty() + ) { + return refuse(ExtractionRefusal.MultipleOutputs(outputs.declarations.mapNotNull { it.name })) } + val output = outputs.declarations.singleOrNull() as? KtProperty // The tail-return exception holds only when nothing else flows out (R8). - if (tailReturn && outputs.isNotEmpty()) return refuse(ExtractionRefusal.ExitsRegion) + if (tailReturn && output != null) return refuse(ExtractionRefusal.ExitsRegion) val parameters = capturedParameters(enclosing, elements, span) ?: return refuse(ExtractionRefusal.UnrenderableType) @@ -101,8 +119,8 @@ internal fun KaSession.buildCandidate( enclosingReturnType(enclosing) ?: return refuse(ExtractionRefusal.UnrenderableType) } - outputs.size == 1 -> { - renderedDeclarationType(outputs.single()) ?: return refuse(ExtractionRefusal.UnrenderableType) + output != null -> { + renderedDeclarationType(output) ?: return refuse(ExtractionRefusal.UnrenderableType) } else -> { @@ -110,21 +128,30 @@ internal fun KaSession.buildCandidate( } }.takeUnless { it == "Unit" } + // 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). + renderedTypeParameterIn(typeParameterNames, parameters.map { it.typeText } + listOfNotNull(returnTypeText)) + ?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + val body = when { isExpression -> ExtractedBody.ExpressionBody(needsReturn = returnTypeText != null) - outputs.size == 1 -> ExtractedBody.StatementBody(trailingReturn = "return ${outputs.single().name.orEmpty()}") + output != null -> ExtractedBody.StatementBody(trailingReturn = "return ${output.name.orEmpty()}") else -> ExtractedBody.StatementBody(trailingReturn = null) } val callSite = when { tailReturn -> CallSiteForm.Return - outputs.size == 1 -> CallSiteForm.AssignOutput(outputs.single().name.orEmpty()) + output != null -> CallSiteForm.AssignOutput(output.name.orEmpty()) else -> CallSiteForm.Call } val takenNames = takenNamesFor(enclosing) + // 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 return SignatureResult.Success( ExtractMethodCandidate( @@ -144,8 +171,8 @@ internal fun KaSession.buildCandidate( returnTypeText = returnTypeText, body = body, callSite = callSite, - insertOffset = enclosing.textRange.endOffset, - insertIndent = leadingIndentAt(fileText, enclosing.textRange.startOffset), + insertOffset = anchor.textRange.endOffset, + insertIndent = leadingIndentAt(fileText, anchor.textRange.startOffset), ), ) } @@ -219,9 +246,15 @@ private fun KaSession.capturedParameters( } // `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 && reference.getReferencedName() == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString() -> { - "it" + val lambda = + PsiTreeUtil.getParentOfType(reference, KtFunctionLiteral::class.java, true) ?: continue + if (inRegion(lambda, span)) continue + lambda } else -> { @@ -264,35 +297,58 @@ private fun KaSession.enclosingReturnType(enclosing: KtDeclaration): String? = ?.takeUnless(::isUnrenderable) /** - * Locals declared inside the region and read after it (R7). Exactly one is supported. + * What the region declares that the code after it still uses (R7). * - * "Read after it" 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. + * 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, -): List { - val declared = descendantsOf(elements, KtProperty::class.java) - if (declared.isEmpty()) return emptyList() +): RegionOutputs { + val declared = descendantsOf(elements, KtNamedDeclaration::class.java) + if (declared.isEmpty()) return RegionOutputs.NONE - val laterReads = + val laterReferences = PsiTreeUtil .collectElementsOfType(enclosing, KtSimpleNameExpression::class.java) .filter { it.textRange.startOffset >= span.end } - .mapNotNull { - runCatching { - it.mainReference - ?.resolveToSymbols() - ?.firstOrNull() - ?.psi - }.getOrNull() - }.toSet() - - return declared.filter { it in laterReads } + 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). @@ -305,9 +361,9 @@ private fun KaSession.reassignedOuterVar( for (reference in simpleNamesIn(elements)) { if (!reference.isWriteTarget()) continue val symbol = - runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() as? KaVariableSymbol - ?: continue - if (symbol.isVal) continue + 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 @@ -337,14 +393,48 @@ private fun hasExit( ): 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 is fine only when its lambda is inside the region. - if (returnExpression.getLabelName() == null) return true - val lambda = PsiTreeUtil.getParentOfType(returnExpression, KtFunctionLiteral::class.java, true) - if (lambda == null || !inRegion(lambda, span)) return true + // 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, @@ -358,16 +448,21 @@ private fun hasLoopExit( } } +private fun typeParameterNamesOf(enclosing: KtDeclaration): List = + (enclosing as? KtNamedFunction)?.typeParameters?.mapNotNull { it.name }.orEmpty() + /** - * The name of the enclosing function's type parameter the region uses, 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). + * 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( - enclosing: KtDeclaration, + names: List, elements: List, ): String? { - val names = (enclosing as? KtNamedFunction)?.typeParameters?.mapNotNull { it.name }.orEmpty() if (names.isEmpty()) return null val typeTexts = @@ -376,62 +471,101 @@ private fun typeParameterIn( 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? { - val construct = enclosingScopingCall(elements.first(), enclosing) ?: return null - val enclosingClass = PsiTreeUtil.getParentOfType(enclosing, KtClassOrObject::class.java, true) - for (reference in simpleNamesIn(elements)) { + // A qualified selector already has its receiver written out next to it. val parent = reference.parent if (parent is KtQualifiedExpression && parent.selectorExpression === reference) continue - if (parent is KtCallExpression && parent.calleeExpression !== reference) continue - val symbol = - runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() as? KaCallableSymbol - ?: continue - val declarationPsi = runCatching { symbol.psi }.getOrNull() ?: continue - - // A local or a member of the class the new function joins needs nothing. - if (PsiTreeUtil.isAncestor(enclosing, declarationPsi, true)) continue - if (enclosingClass != null && PsiTreeUtil.isAncestor(enclosingClass, declarationPsi, true)) continue - // A top-level declaration resolves unchanged from anywhere in the file. - if (declarationPsi.parent is KtFile) continue - // Anything else reached without a qualifier came in through the scoping receiver. - if (inRegion(declarationPsi, span)) continue - return construct + val lambda = implicitReceiverLambdaFor(reference) ?: continue + if (inRegion(lambda, span)) continue + if (!PsiTreeUtil.isAncestor(enclosing, lambda, true)) continue + return (lambda.parent as? KtLambdaExpression)?.let { callOwning(it)?.calleeName() } ?: UNNAMED_SCOPING_CONSTRUCT } return null } -/** The callee name of the nearest receiver-binding scoping call between [element] and [enclosing]. */ -private fun enclosingScopingCall( - element: PsiElement, - enclosing: KtDeclaration, -): String? { - var current: PsiElement? = element - while (current != null && current !== enclosing) { - if (current is KtFunctionLiteral) { - val call = PsiTreeUtil.getParentOfType(current, KtCallExpression::class.java, true) - val callee = (call?.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() - if (callee != null && callee in RECEIVER_SCOPING_FUNCTIONS) return callee - } - current = current.parent - } - return null +/** + * 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 applied = + callSource + .resolveToCall() + ?.successfulCallOrNull>() + ?.partiallyAppliedSymbol + receiverLambda(applied?.dispatchReceiver) ?: receiverLambda(applied?.extensionReceiver) + }.getOrNull() + +private fun receiverLambda(receiver: KaReceiverValue?): KtFunctionLiteral? { + val owner = (receiver as? KaImplicitReceiverValue)?.symbol ?: 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 { owner.psi }.getOrNull() + ?: runCatching { (owner as? KaReceiverParameterSymbol)?.owningCallableSymbol?.psi }.getOrNull() + ?: return null + return psi as? KtFunctionLiteral ?: (psi as? KtLambdaExpression)?.functionLiteral } /** `suspend` is added when the region calls one, or touches `coroutineContext` (R10). */ diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index 6e31cd89f6..47e4b95769 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -365,4 +365,218 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { 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()") + + val refusal = plan(content, start, end).refusal + + // A `val` at the call site cannot carry an output the following code assigns to. + assertTrue(refusal is ExtractionRefusal.MultipleOutputs) + assertEquals(listOf("result"), (refusal as ExtractionRefusal.MultipleOutputs).names) + } + + @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("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(): 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 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, + ) + } } From ceb11bd45a534f669f81d1080af936810adaf67a Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 17:32:56 +0000 Subject: [PATCH 09/17] ADFA-5080: Refuse the receiver, label and smart-cast cases that emit broken Kotlin --- .../utils/refactor/ExtractMethodPlan.kt | 9 + .../kotlin/utils/refactor/MethodSignature.kt | 153 ++++++++++--- .../lsp/kotlin/utils/refactor/Occurrences.kt | 13 +- .../refactor/ExtractMethodPlanEndToEndTest.kt | 202 ++++++++++++++++++ 4 files changed, 347 insertions(+), 30 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index ec8dc179a9..17b39e3da3 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -110,6 +110,15 @@ sealed interface ExtractionRefusal { * 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 } /** diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 2685c0db16..5c95af11f6 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -4,6 +4,8 @@ import com.itsaky.androidide.lsp.kotlin.utils.renderName import org.jetbrains.kotlin.analysis.api.KaExperimentalApi 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 @@ -14,6 +16,7 @@ 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 @@ -29,6 +32,7 @@ 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 @@ -37,12 +41,14 @@ 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 @@ -107,7 +113,11 @@ internal fun KaSession.buildCandidate( // The tail-return exception holds only when nothing else flows out (R8). if (tailReturn && output != null) return refuse(ExtractionRefusal.ExitsRegion) - val parameters = capturedParameters(enclosing, elements, span) ?: return refuse(ExtractionRefusal.UnrenderableType) + val parameters = + when (val captured = capturedParameters(enclosing, elements, span)) { + is CaptureResult.Captured -> captured.parameters + is CaptureResult.Refused -> return refuse(captured.refusal) + } val returnTypeText = when { @@ -128,10 +138,15 @@ internal fun KaSession.buildCandidate( } }.takeUnless { it == "Unit" } + 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). - renderedTypeParameterIn(typeParameterNames, parameters.map { it.typeText } + listOfNotNull(returnTypeText)) - ?.let { return refuse(ExtractionRefusal.UsesTypeParameter(it)) } + // 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 { @@ -152,6 +167,12 @@ internal fun KaSession.buildCandidate( // 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 modifiers = + buildList { + // A local function joins a block, and a visibility modifier on one does not compile. + if (anchor.parent !is KtBlockExpression) add("private") + if (usesSuspend(elements)) add("suspend") + } return SignatureResult.Success( ExtractMethodCandidate( @@ -165,8 +186,8 @@ internal fun KaSession.buildCandidate( }, takenNames = takenNames, annotations = if (usesComposable(elements)) listOf("@Composable") else emptyList(), - modifiers = if (usesSuspend(elements)) listOf("private", "suspend") else listOf("private"), - receiverTypeText = (enclosing as? KtNamedFunction)?.receiverTypeReference?.text, + modifiers = modifiers, + receiverTypeText = receiverTypeText, parameters = parameters, returnTypeText = returnTypeText, body = body, @@ -220,14 +241,14 @@ private fun descendantsOf( * 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). * - * Returns null when a type cannot be rendered as source, which declines the extraction rather than - * emitting text that will not compile. + * 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, -): List? { +): CaptureResult { val parameters = mutableListOf() val seen = mutableSetOf() @@ -263,10 +284,31 @@ private fun KaSession.capturedParameters( } if (!seen.add(key)) continue - val typeText = renderedSymbolType(symbol) ?: return null + 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. Asked only of values, since a smart cast is the only thing that can make the two differ. + if (symbol is KaVariableSymbol) { + val usedTypeText = renderedTypeOrNull(reference) + if (usedTypeText != null && usedTypeText != typeText) { + return CaptureResult.Refused(ExtractionRefusal.SmartCastParameter(reference.getReferencedName())) + } + } parameters += MethodParameter(name = reference.getReferencedName(), typeText = typeText) } - return parameters + return CaptureResult.Captured(parameters) +} + +/** 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 } /** A type that cannot be written out as source -- anonymous, intersection, or a resolution error. */ @@ -325,7 +367,12 @@ private fun KaSession.outputsOf( elements: List, span: TextSpan, ): RegionOutputs { - val declared = descendantsOf(elements, KtNamedDeclaration::class.java) + // 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 = @@ -439,15 +486,31 @@ private fun hasLoopExit( elements: List, span: TextSpan, ): Boolean { - val jumps = + val jumps: List = descendantsOf(elements, KtBreakExpression::class.java) + descendantsOf(elements, KtContinueExpression::class.java) return jumps.any { jump -> - val loop = PsiTreeUtil.getParentOfType(jump, KtLoopExpression::class.java, true) + 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 +} + private fun typeParameterNamesOf(enclosing: KtDeclaration): List = (enclosing as? KtNamedFunction)?.typeParameters?.mapNotNull { it.name }.orEmpty() @@ -533,13 +596,35 @@ private fun KaSession.innerImplicitReceiver( if (parent is KtQualifiedExpression && parent.selectorExpression === reference) continue val lambda = implicitReceiverLambdaFor(reference) ?: continue - if (inRegion(lambda, span)) continue - if (!PsiTreeUtil.isAncestor(enclosing, lambda, true)) continue - return (lambda.parent as? KtLambdaExpression)?.let { callOwning(it)?.calleeName() } ?: UNNAMED_SCOPING_CONSTRUCT + 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). @@ -549,25 +634,43 @@ private fun KaSession.implicitReceiverLambdaFor(reference: KtSimpleNameExpressio // 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() + // An assignment target resolves to the whole compound access, which is not a member call and + // would otherwise slip through carrying its receiver with it: `n += 1` inside `apply { }`. val applied = - callSource - .resolveToCall() - ?.successfulCallOrNull>() - ?.partiallyAppliedSymbol + call?.successfulCallOrNull>()?.partiallyAppliedSymbol + ?: call?.successfulCallOrNull()?.variableCall?.partiallyAppliedSymbol + ?: call?.successfulCallOrNull()?.getterCall?.partiallyAppliedSymbol receiverLambda(applied?.dispatchReceiver) ?: receiverLambda(applied?.extensionReceiver) }.getOrNull() -private fun receiverLambda(receiver: KaReceiverValue?): KtFunctionLiteral? { - val owner = (receiver as? KaImplicitReceiverValue)?.symbol ?: return null +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 { owner.psi }.getOrNull() - ?: runCatching { (owner as? KaReceiverParameterSymbol)?.owningCallableSymbol?.psi }.getOrNull() + 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). */ private fun KaSession.usesSuspend(elements: List): Boolean { if (simpleNamesIn(elements).any { it.getReferencedName() == "coroutineContext" }) return true 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 1c365f0482..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, diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index 47e4b95769..603067343d 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -561,6 +561,208 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { ) } + @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. + assertEquals(emptyList(), candidate.modifiers) + assertEquals( + """ + package p + fun demo(a: Int): Int { + fun inner(b: Int): Int { + return doubled(b) + } + + fun doubled(b: Int): Int { + return b * 2 + } + return inner(a) + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "doubled")!!), + ) + } + + @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 parameter whose type cannot be written out is declined`() { val content = From 290e3ad6a9e3b3cf3bee2ad80f6018ed73eaa2de Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 17:57:13 +0000 Subject: [PATCH 10/17] ADFA-5080: Declare a local extracted function before its call site --- .../utils/refactor/ExtractMethodEdit.kt | 32 ++++-- .../utils/refactor/ExtractMethodPlan.kt | 8 ++ .../kotlin/utils/refactor/MethodSignature.kt | 74 +++++++++++-- .../utils/refactor/ExtractMethodEditTest.kt | 77 ++++++++++++++ .../refactor/ExtractMethodPlanEndToEndTest.kt | 100 +++++++++++++++++- 5 files changed, 266 insertions(+), 25 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt index 11a441c5f2..4d8f9c4f1e 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt @@ -1,13 +1,15 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor /** - * The two replacements an extraction performs: the new function, then the call that replaces the + * The two replacements an extraction performs: the new function, and the call that replaces the * region. * - * **The 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. - * The insertion point sits after the region, so emitting the call first would shift it and corrupt - * the file. Descending document order is the only safe order. + * **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 @@ -25,7 +27,9 @@ fun buildExtractMethodRewrites( ): List? { val span = candidate.span if (span.end > fileText.length) return null - if (candidate.insertOffset > fileText.length || candidate.insertOffset < span.end) 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 @@ -47,13 +51,21 @@ fun buildExtractMethodRewrites( val declaration = buildString { - // A blank line separates the new function from the declaration it follows. - append(newline).append(newline) 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) { @@ -63,9 +75,9 @@ fun buildExtractMethodRewrites( } return listOf( - RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), declaration), + RewriteSpan(TextSpan(candidate.insertOffset, candidate.insertOffset), insertionText), RewriteSpan(span, callText), - ) + ).sortedByDescending { it.span.start } } /** diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 17b39e3da3..920f0b1ed0 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -119,6 +119,14 @@ sealed interface ExtractionRefusal { 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 } /** diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 5c95af11f6..2b2c64d540 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -167,10 +167,11 @@ internal fun KaSession.buildCandidate( // 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 modifiers = buildList { // A local function joins a block, and a visibility modifier on one does not compile. - if (anchor.parent !is KtBlockExpression) add("private") + if (!isLocalTarget) add("private") if (usesSuspend(elements)) add("suspend") } @@ -192,7 +193,10 @@ internal fun KaSession.buildCandidate( returnTypeText = returnTypeText, body = body, callSite = callSite, - insertOffset = anchor.textRange.endOffset, + // 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), ), ) @@ -284,22 +288,64 @@ private fun KaSession.capturedParameters( } if (!seen.add(key)) continue + val name = reference.getReferencedName() + // 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)) + } + 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. Asked only of values, since a smart cast is the only thing that can make the two differ. - if (symbol is KaVariableSymbol) { - val usedTypeText = renderedTypeOrNull(reference) - if (usedTypeText != null && usedTypeText != typeText) { - return CaptureResult.Refused(ExtractionRefusal.SmartCastParameter(reference.getReferencedName())) + // 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 = reference.getReferencedName(), typeText = typeText) + 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 +} + +@OptIn(KaExperimentalApi::class) +private fun KaSession.usedTypeOf(expression: KtExpression): UsedType { + val rendered = + runCatching { expression.expressionType?.let { renderName(it) } }.getOrNull() ?: return UsedType.Absent + return if (isUnrenderable(rendered)) UsedType.Unrenderable else UsedType.Rendered(rendered) +} + /** Either the derived parameter list or the reason there cannot be one. */ private sealed interface CaptureResult { data class Captured( @@ -511,8 +557,13 @@ private fun targetLoopFor(jump: KtExpressionWithLabel): KtLoopExpression? { return null } +/** An accessor's type parameters live on its property, the same place its receiver does. */ private fun typeParameterNamesOf(enclosing: KtDeclaration): List = - (enclosing as? KtNamedFunction)?.typeParameters?.mapNotNull { it.name }.orEmpty() + 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 @@ -635,8 +686,9 @@ private fun KaSession.implicitReceiverLambdaFor(reference: KtSimpleNameExpressio val callSource = (reference.parent as? KtCallExpression)?.takeIf { it.calleeExpression === reference } ?: reference val call = callSource.resolveToCall() - // An assignment target resolves to the whole compound access, which is not a member call and - // would otherwise slip through carrying its receiver with it: `n += 1` inside `apply { }`. + // 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 diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt index 32b4baa504..d722edabcd 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEditTest.kt @@ -81,6 +81,83 @@ class ExtractMethodEditTest { ) } + @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) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index 603067343d..cf39509536 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -704,23 +704,115 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { 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. + // `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: Int): Int { + return b * 2 + } + fun inner(b: Int): Int { return doubled(b) } + return inner(a) + } + """.trimIndent(), + apply(content, buildExtractMethodRewrites(result.fileText, candidate, "doubled")!!), + ) + } - fun doubled(b: Int): Int { + @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(), - apply(content, buildExtractMethodRewrites(result.fileText, candidate, "doubled")!!), + """.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, ) } From 0db25d27f1dae25896337153310e85988ccd3df6 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 18:17:11 +0000 Subject: [PATCH 11/17] ADFA-5080: Add the extract-method sheet state and strings --- .../refactor/ui/ExtractMethodUiState.kt | 50 ++++++ .../refactor/ui/ExtractMethodViewModel.kt | 80 ++++++++++ .../ui/ExtractVariableSheetContent.kt | 69 -------- .../lsp/kotlin/refactor/ui/SheetComponents.kt | 85 ++++++++++ .../refactor/ui/ExtractMethodViewModelTest.kt | 147 ++++++++++++++++++ resources/src/main/res/values/strings.xml | 16 ++ 6 files changed, 378 insertions(+), 69 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodUiState.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModel.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/SheetComponents.kt create mode 100644 lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodViewModelTest.kt 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/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/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 3ebed7e55b..94823eed10 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -539,6 +539,22 @@ 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 + The selection produces more than one value: %1$s + 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 field, which only exists inside this accessor + The selection uses %1$s under a smart cast that cannot be written outside it + The selection uses %1$s, which goes out of scope once the selection moves Select fields No fields selected No fields found From cf52d4560f01e5f44cfb5660f2e1b5c1eb6e735d Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 18:24:36 +0000 Subject: [PATCH 12/17] ADFA-5080: Reword two extract-method refusal messages --- resources/src/main/res/values/strings.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 94823eed10..71e000dc4f 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -552,8 +552,8 @@ 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 field, which only exists inside this accessor - The selection uses %1$s under a smart cast that cannot be written outside it + 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 From 626d97553615ec29e8ffe0b555926bed76770b47 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 18:30:08 +0000 Subject: [PATCH 13/17] ADFA-5080: Add the extract-method Compose sheet --- .../kotlin/refactor/ui/ExtractMethodSheet.kt | 96 +++++++++++++++++++ .../refactor/ui/ExtractMethodSheetContent.kt | 94 ++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheet.kt create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt 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)) + } + } + } +} From 13be859e92669eba99ddb45885cf40b6124181db Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Mon, 10 Aug 2026 18:44:05 +0000 Subject: [PATCH 14/17] ADFA-5080: Wire up the extract-method code action --- docs/features/kotlin-extract-method.md | 7 +- .../androidide/idetooltips/TooltipTag.kt | 1 + .../lsp/kotlin/KotlinCodeActionsMenu.kt | 2 + .../lsp/kotlin/actions/ExtractMethodAction.kt | 213 ++++++++++++++++++ .../kotlin/KotlinCodeActionTooltipTagTest.kt | 2 + 5 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt diff --git a/docs/features/kotlin-extract-method.md b/docs/features/kotlin-extract-method.md index 1b8c065c5c..45ddc42da0 100644 --- a/docs/features/kotlin-extract-method.md +++ b/docs/features/kotlin-extract-method.md @@ -1,7 +1,7 @@ # Kotlin extract method (K2 LSP) - **Ticket:** ADFA-5080 (subtask of ADFA-3317; split out of ADFA-4826, which now covers extract variable only) -- **Status:** Requirements only - not implemented +- **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`. @@ -149,8 +149,11 @@ Including inherited names is a correctness requirement, not a nicety: a private | `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 | -Five of the seven are actionable - they tell the user what to change - and two of them (`ReassignsOuterVar`, `InnerImplicitReceiver`) are common enough that a generic message would read as the feature being broken. 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. +Eight of the ten 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. 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. The refusal lives on `ExtractMethodPlan` only; extract variable keeps its single "nothing to extract" behaviour unchanged. 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..f1b7027b5b --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -0,0 +1,213 @@ +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.NotASingleRegion) + val nioPath = data.requireFile().toPath() + val env = + server.compilationEnvironmentFor(nioPath) + ?: return ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + + 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.NotASingleRegion)) + 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. + */ + private fun applyChoice( + 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) + } + + is ExtractionRefusal.MultipleOutputs -> { + context.getString(R.string.msg_extract_method_multiple_outputs, refusal.names.joinToString(", ")) + } + + is ExtractionRefusal.ReassignsOuterVar -> { + context.getString(R.string.msg_extract_method_reassigns_outer_var, refusal.name) + } + + ExtractionRefusal.ExitsRegion -> { + context.getString(R.string.msg_extract_method_exits_region) + } + + is ExtractionRefusal.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/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) } From 6bc76f7de241e430e9c06a0d83da5ba3d7984f13 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 11 Aug 2026 13:26:19 +0000 Subject: [PATCH 15/17] ADFA-5080: Stop the analysis emitting Kotlin that does not compile Five ordinary shapes produced a broken file rather than a refusal, which ADR 0012 rules out: the refactoring moves code and declines where it cannot. - Signature types render fully qualified. A short name resolves only when the file already imports it, and a local's type usually comes from inference, so `val d = java.util.Date()` emitted an unresolved `Date`. `usedTypeOf` moves to the same renderer, or every capture would read as a smart cast. - A platform type is emitted as its flexible type's lower bound instead of `String!`, which does not parse. `!` anywhere in a rendered type now counts as unrenderable, catching the nested `List` the lower bound leaves. - `suspend` is no longer added for a call the region only makes inside a nested suspend-typed lambda. `launchIt { work() }` in a non-suspend function emitted a `suspend fun` its own call site could not call. Inline lambdas still propagate, and extracting from inside such a lambda still adds `suspend`. - The capture loop skips the selector of a qualified expression, refuses a value whose type is a class local to the enclosing declaration, and refuses rather than drops a local class or object used as a qualifier. `h.n` used to emit both a parameter named `n` that no call site had and a `Holder` type the new function could not see. - A tail return from a secondary constructor takes `Unit`, not the constructed class. `return extracted(...)` on a Unit call is legal in a constructor. Refusal quality and failure isolation, in the same pass: - `MultipleOutputs` split. It fired for three situations, two of which are one value, and rendered "produces more than one value: result". A single output the call site cannot receive back is now `OutputNotReturnable`. - `CouldNotAnalyse` added. A missing environment, an unreachable KtFile and a thrown error all reported "Select an expression, or whole statements inside one block" - the most confident message in the set, aimed at a selection nothing had looked at. Cancellation is re-thrown rather than swallowed. - `takenNamesFor` tests the local-`fun` target before the containing class, so a new local validates against its siblings instead of the class's members. - `applyChoice` runs from a Compose click handler outside every framework guard; its body is now wrapped. The feature doc's R4, R5, R7, R10, R14 and R15 are corrected to match, and its claim that the version guard lives on `RefactoringPlan` is dropped - the two actions each do their own comparison. --- docs/features/kotlin-extract-method.md | 22 +- .../lsp/kotlin/actions/ExtractMethodAction.kt | 28 +- .../utils/refactor/ExtractMethodPlan.kt | 21 +- .../utils/refactor/ExtractMethodPlanner.kt | 16 +- .../kotlin/utils/refactor/MethodSignature.kt | 231 ++++++++++--- .../refactor/ExtractMethodPlanEndToEndTest.kt | 315 +++++++++++++++++- resources/src/main/res/values/strings.xml | 2 + 7 files changed, 558 insertions(+), 77 deletions(-) diff --git a/docs/features/kotlin-extract-method.md b/docs/features/kotlin-extract-method.md index 45ddc42da0..f9965545a2 100644 --- a/docs/features/kotlin-extract-method.md +++ b/docs/features/kotlin-extract-method.md @@ -63,9 +63,9 @@ As with extract variable: **no `prepare()` visibility gate** (deciding extractab 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, and the version is re-read on confirm with a mismatch refusing the edit. Shared via the `RefactoringPlan` supertype. +**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. That one rule produces the conventional answer in every context: +**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 | |---|---| @@ -81,7 +81,7 @@ Unlike extract variable there is no scope chain and no ceiling, because anything - **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 with the existing `renderName(KaType)`. A type that cannot be rendered - an anonymous or intersection type, or a resolution failure - **declines the extraction** (`UnrenderableType`) rather than emitting uncompilable text. +- **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: @@ -95,7 +95,7 @@ Unlike extract variable there is no scope chain and no ceiling, because anything 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 is supported; **two or more declines** (`MultipleOutputs`, naming them). +**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. @@ -116,7 +116,7 @@ Declined: a `return` anywhere but the tail position, a `break`/`continue` whose **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. +- **`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. @@ -143,7 +143,9 @@ Including inherited names is a correctness requirement, not a nicety: a private | 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 | @@ -153,13 +155,15 @@ Including inherited names is a correctness requirement, not a nicety: a private | `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 | -Eight of the ten 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. 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. +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 after the enclosing declaration - emitted as **two `TextEdit`s in one `DocumentChange`, ordered new-function-first (descending document order)**. +**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). Emitting the call site first would shift the insertion point and corrupt the file. +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. @@ -249,7 +253,7 @@ New files, all in `lsp/kotlin`: 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`, `documentVersion`, the version guard), introduced in the extract-variable PR so this one is purely additive. +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. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt index f1b7027b5b..5dd2d95f65 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -59,11 +59,11 @@ class ExtractMethodAction : BaseKotlinCodeAction() { override suspend fun execAction(data: ActionData): ExtractMethodPlan { val server = data.get() - ?: return ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) val nioPath = data.requireFile().toPath() val env = server.compilationEnvironmentFor(nioPath) - ?: return ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) val cursor = data.requireEditor().cursor return buildExtractMethodPlan( @@ -86,7 +86,7 @@ class ExtractMethodAction : BaseKotlinCodeAction() { val context = data.requireContext() if (result.isEmpty) { - flashInfo(refusalMessage(context, result.refusal ?: ExtractionRefusal.NotASingleRegion)) + flashInfo(refusalMessage(context, result.refusal ?: ExtractionRefusal.CouldNotAnalyse)) return } @@ -111,11 +111,25 @@ class ExtractMethodAction : BaseKotlinCodeAction() { * 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() @@ -171,10 +185,18 @@ class ExtractMethodAction : BaseKotlinCodeAction() { 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) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 920f0b1ed0..afe51cfab3 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -76,14 +76,29 @@ sealed interface ExtractionRefusal { data object NotASingleRegion : ExtractionRefusal /** - * The region declares something the code after it still needs, and a single returned value cannot - * carry it (R7): two or more locals, a destructuring declaration, a local `fun` or class, or a - * local the following code reassigns. [names] is what is in the way, so the message can name it. + * 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, 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 index 55143ab0c1..ebc20d4eab 100644 --- 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 @@ -7,6 +7,7 @@ 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") @@ -18,7 +19,11 @@ private val logger = LoggerFactory.getLogger("ExtractMethodPlanner") * * 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). + * 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, @@ -31,7 +36,7 @@ internal fun buildExtractMethodPlan( runCatching { val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() - ?: return ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + ?: return ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) env.project.read { val fileText = ktFile.text @@ -54,9 +59,11 @@ internal fun buildExtractMethodPlan( 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.NotASingleRegion + ?: ExtractionRefusal.CouldNotAnalyse return@analyzeMaybeDangling ExtractMethodPlan.refused(refusal, fileText, documentVersion) } @@ -75,6 +82,7 @@ internal fun buildExtractMethodPlan( } } }.getOrElse { error -> + if (error is CancellationException) throw error logger.warn("Failed to build extract-method plan for {}", nioPath, error) - ExtractMethodPlan.refused(ExtractionRefusal.NotASingleRegion) + ExtractMethodPlan.refused(ExtractionRefusal.CouldNotAnalyse) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 2b2c64d540..03bfb1d7b5 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor import com.itsaky.androidide.lsp.kotlin.utils.renderName import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundArrayAccessCall import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundVariableAccessCall @@ -20,6 +21,10 @@ 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 @@ -63,6 +68,11 @@ private const val UNNAMED_SCOPING_CONSTRUCT = "lambda" private const val BACKING_FIELD_NAME = "field" +private const val COROUTINE_CONTEXT_NAME = "coroutineContext" + +/** As [SIGNATURE_TYPE_RENDERER] 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( @@ -101,15 +111,17 @@ internal fun KaSession.buildCandidate( 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 -- a second local, a destructuring entry, a local - // `fun`, or a local reassigned afterwards -- is refused rather than silently dropped (R7). - if (outputs.declarations.size > 1 || - outputs.declarations.any { it !is KtProperty } || - outputs.writtenAfter.isNotEmpty() - ) { + // 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 output = outputs.declarations.singleOrNull() as? KtProperty + 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) @@ -126,7 +138,14 @@ internal fun KaSession.buildCandidate( } tailReturn -> { - enclosingReturnType(enclosing) ?: return refuse(ExtractionRefusal.UnrenderableType) + // 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 -> { @@ -136,7 +155,7 @@ internal fun KaSession.buildCandidate( else -> { null } - }.takeUnless { it == "Unit" } + }.takeUnless { it == UNIT_TYPE_TEXT } val receiverTypeText = receiverTypeTextOf(enclosing) @@ -162,12 +181,12 @@ internal fun KaSession.buildCandidate( else -> CallSiteForm.Call } - val takenNames = takenNamesFor(enclosing) // 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. @@ -240,6 +259,35 @@ private fun descendantsOf( type: Class, ): List = elements.flatMap { PsiTreeUtil.collectElementsOfType(it, type) } +/** + * Whether [reference] is the selector of a qualified expression -- the `n` in `h.n`, the `f` in + * `h.f()`. A call wraps its callee, so the call expression is what the qualified expression holds. + */ +private fun isQualifiedSelector(reference: KtSimpleNameExpression): Boolean { + val selector = (reference.parent as? KtCallExpression)?.takeIf { it.calleeExpression === reference } ?: reference + val parent = selector.parent + return parent is KtQualifiedExpression && parent.selectorExpression === selector +} + +/** + * 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 @@ -257,9 +305,27 @@ private fun KaSession.capturedParameters( val seen = mutableSetOf() for (reference in simpleNamesIn(elements).sortedBy { it.textRange.startOffset }) { - val symbol = - runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() as? KaCallableSymbol - ?: continue + // The selector of a qualified expression already has its receiver written out next to it and + // resolves through that receiver wherever the code lives, so it is never a capture. Without this + // the `n` in `h.n` became a parameter and the call site passed a name that does not exist. + // `innerImplicitReceiver` has the same guard for the same reason. + if (isQualifiedSelector(reference)) continue + + 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 = @@ -275,7 +341,7 @@ private fun KaSession.capturedParameters( // 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 && - reference.getReferencedName() == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString() -> { + name == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString() -> { val lambda = PsiTreeUtil.getParentOfType(reference, KtFunctionLiteral::class.java, true) ?: continue if (inRegion(lambda, span)) continue @@ -288,7 +354,6 @@ private fun KaSession.capturedParameters( } if (!seen.add(key)) continue - val name = reference.getReferencedName() // 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). @@ -296,6 +361,12 @@ private fun KaSession.capturedParameters( 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 @@ -339,10 +410,9 @@ private sealed interface UsedType { ) : UsedType } -@OptIn(KaExperimentalApi::class) private fun KaSession.usedTypeOf(expression: KtExpression): UsedType { val rendered = - runCatching { expression.expressionType?.let { renderName(it) } }.getOrNull() ?: return UsedType.Absent + runCatching { expression.expressionType?.let { renderTypeText(it) } }.getOrNull() ?: return UsedType.Absent return if (isUnrenderable(rendered)) UsedType.Unrenderable else UsedType.Rendered(rendered) } @@ -357,30 +427,54 @@ private sealed interface CaptureResult { ) : CaptureResult } -/** A type that cannot be written out as source -- anonymous, intersection, or a resolution error. */ +/** + * A type that cannot be written out as source -- anonymous, intersection, a resolution error, or a + * platform type [renderTypeText] could not reduce (`List`, where the `!` is on a type + * argument). `!` is not Kotlin syntax anywhere, so its presence alone settles it. + */ private fun isUnrenderable(text: String): Boolean = text.isBlank() || text.contains("anonymous") || text.contains("ERROR") || - text.contains(" & ") + text.contains(" & ") || + text.contains('!') +/** + * Types in the emitted signature are rendered **fully qualified**. + * + * 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 -- `val d = java.util.Date()` names `Date` + * nowhere -- so a short name is unresolved as often as not, and this refactoring adds no imports. + * Verbose, but it always resolves. [receiverTypeTextOf] deliberately stays on source text instead: + * that text is already in the file. + */ @OptIn(KaExperimentalApi::class) -private fun KaSession.renderedSymbolType(symbol: KaCallableSymbol): String? = - runCatching { renderName(symbol.returnType) }.getOrNull()?.takeUnless(::isUnrenderable) +private val SIGNATURE_TYPE_RENDERER = KaTypeRendererForSource.WITH_QUALIFIED_NAMES +/** + * One type as the signature would print it, before the [isUnrenderable] check. + * + * A platform type is unwrapped to 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. + * Only the outermost bound is unwrapped, so a `!` on a type argument survives to [isUnrenderable]. + */ @OptIn(KaExperimentalApi::class) +private fun KaSession.renderTypeText(type: KaType): String = + renderName((type as? KaFlexibleType)?.lowerBound ?: type, SIGNATURE_TYPE_RENDERER) + +private fun KaSession.renderedSymbolType(symbol: KaCallableSymbol): String? = + runCatching { renderTypeText(symbol.returnType) }.getOrNull()?.takeUnless(::isUnrenderable) + private fun KaSession.renderedTypeOrNull(expression: KtExpression): String? = - runCatching { expression.expressionType?.let { renderName(it) } }.getOrNull()?.takeUnless(::isUnrenderable) + runCatching { expression.expressionType?.let { renderTypeText(it) } }.getOrNull()?.takeUnless(::isUnrenderable) -@OptIn(KaExperimentalApi::class) private fun KaSession.renderedDeclarationType(property: KtProperty): String? = - runCatching { (property.symbol as? KaCallableSymbol)?.returnType?.let { renderName(it) } } + runCatching { (property.symbol as? KaCallableSymbol)?.returnType?.let { renderTypeText(it) } } .getOrNull() ?.takeUnless(::isUnrenderable) -@OptIn(KaExperimentalApi::class) private fun KaSession.enclosingReturnType(enclosing: KtDeclaration): String? = - runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType?.let { renderName(it) } } + runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType?.let { renderTypeText(it) } } .getOrNull() ?.takeUnless(::isUnrenderable) @@ -643,8 +737,7 @@ private fun KaSession.innerImplicitReceiver( ): String? { for (reference in simpleNamesIn(elements)) { // A qualified selector already has its receiver written out next to it. - val parent = reference.parent - if (parent is KtQualifiedExpression && parent.selectorExpression === reference) continue + if (isQualifiedSelector(reference)) continue val lambda = implicitReceiverLambdaFor(reference) ?: continue if (isBoundOutsideRegion(enclosing, lambda, span)) return constructNameFor(lambda) @@ -723,16 +816,59 @@ private fun receiverTypeTextOf(enclosing: KtDeclaration): String? = else -> null } -/** `suspend` is added when the region calls one, or touches `coroutineContext` (R10). */ -private fun KaSession.usesSuspend(elements: List): Boolean { - if (simpleNamesIn(elements).any { it.getReferencedName() == "coroutineContext" }) return true - return descendantsOf(elements, KtCallExpression::class.java).any { call -> - runCatching { - (call.resolveToCall()?.successfulFunctionCallOrNull()?.symbol as? KaNamedFunctionSymbol)?.isSuspend - }.getOrNull() == true +/** + * `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). @@ -752,12 +888,27 @@ private fun KaSession.usesComposable(elements: List): Boolean = /** * 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): Set { +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 = @@ -772,14 +923,6 @@ private fun KaSession.takenNamesFor(enclosing: KtDeclaration): Set { return fromScope + declared } - // A local `fun` target: the enclosing block's own declarations. Otherwise the file's top level. - val block = enclosing.parent - if (block is KtBlockExpression) { - return PsiTreeUtil - .collectElementsOfType(block, KtDeclaration::class.java) - .mapNotNull { it.name } - .toSet() - } return enclosing.containingKtFile.declarations .mapNotNull { it.name } .toSet() diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index cf39509536..8ececb967a 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -1,10 +1,14 @@ 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 @@ -51,8 +55,9 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { val result = plan(content, content.indexOf("b * a") + 1) val candidate = result.candidates.first { it.label == "b * a" } - assertEquals(listOf("b" to "Int", "a" to "Int"), candidate.parameters.map { it.name to it.typeText }) - assertEquals("Int", candidate.returnTypeText) + // 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) } @@ -94,7 +99,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { val candidate = result.candidates.single() assertEquals(CallSiteForm.AssignOutput("doubled"), candidate.callSite) - assertEquals("Int", candidate.returnTypeText) + assertEquals("kotlin.Int", candidate.returnTypeText) } @Test @@ -152,7 +157,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { val candidate = result.candidates.single() assertEquals(CallSiteForm.Return, candidate.callSite) - assertEquals("Int", candidate.returnTypeText) + assertEquals("kotlin.Int", candidate.returnTypeText) assertEquals( """ package p @@ -161,7 +166,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { return finish(doubled) } - private fun finish(doubled: Int): Int { + private fun finish(doubled: kotlin.Int): kotlin.Int { return doubled + 1 } """.trimIndent(), @@ -357,7 +362,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { return total(a, b) } - private fun total(a: Int, b: Int): Int { + private fun total(a: kotlin.Int, b: kotlin.Int): kotlin.Int { return a + b } } @@ -417,11 +422,9 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { """.trimIndent() val (start, end) = selection(content, "var result", "var result = compute()") - val refusal = plan(content, start, end).refusal - - // A `val` at the call site cannot carry an output the following code assigns to. - assertTrue(refusal is ExtractionRefusal.MultipleOutputs) - assertEquals(listOf("result"), (refusal as ExtractionRefusal.MultipleOutputs).names) + // 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 @@ -479,7 +482,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { // `helper()` comes from the supertype, not from `with`'s receiver. assertNull(result.refusal) - assertEquals("Int", result.candidates.first { it.label == "helper() + 1" }.returnTypeText) + assertEquals("kotlin.Int", result.candidates.first { it.label == "helper() + 1" }.returnTypeText) } @Test @@ -533,7 +536,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { backing = value } - private fun next(): Int { + private fun next(): kotlin.Int { return backing + 1 } } @@ -711,7 +714,7 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { """ package p fun demo(a: Int): Int { - fun doubled(b: Int): Int { + fun doubled(b: kotlin.Int): kotlin.Int { return b * 2 } @@ -855,6 +858,290 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { ) } + @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 qualified selector is not turned into a parameter`() { + val content = + """ + package p + class Holder(val n: Int) + fun demo(h: Holder): Int { + return h.n + 1 + } + """.trimIndent() + val (start, end) = selection(content, "return h.n + 1", "return h.n + 1") + + val candidate = plan(content, start, end).candidates.single() + + // `n` resolves through `h` wherever the code lives; passing it would name a nonexistent local. + assertEquals(listOf("h"), candidate.parameters.map { it.name }) + } + + @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 = diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 71e000dc4f..22fa37ae8c 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -546,7 +546,9 @@ 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 From b95542afaa50263a81acda28a282a4d0182bdfd3 Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Tue, 11 Aug 2026 13:57:46 +0000 Subject: [PATCH 16/17] ADFA-5080: Revert the qualified-selector capture guard The guard added in 0a20c0d76 dropped a selector that still needed capturing, and emitted a broken file in two shapes the previous behaviour refused: - a local extension `fun` called as `h.twice()` was skipped, so nothing refused and the moved body called a function out of scope there. - pointing `innerImplicitReceiver` at the same helper skipped a *call* selector, losing the `with`-receiver refusal for a member extension -- the pervasive Compose shape, `with(density) { size.toPx() }`. Both were reproduced before the revert and re-checked after it. The shape the guard was meant to fix, a member of a local class reached as `h.f()`, refuses identically without it: the capture loop is offset-ordered, so the receiver is refused for its local type before the selector is ever reached. `innerImplicitReceiver` gets its original guard back, with a comment on why it must stay shallow. The capture loop gets a comment on why it has none, so the guard does not come back. The refusals for a local class type and for a local class or object used as a qualifier are untouched. The test that defended the guard used a top-level class, whose members the pre-existing ancestor test already skips, so it passed either way. It is replaced by one test per broken shape, both of which fail against the guard. Three doc statements the previous commit should have moved and did not: - R8 said the extracted function takes the enclosing function's return type; the secondary-constructor exception lived only in a code comment. - R16 promised a refusal for anything thrown; cancellation is now re-thrown deliberately, and the reason it is safe belongs next to the promise. - R11's preview example predated fully-qualified rendering. --- docs/features/kotlin-extract-method.md | 8 +++-- .../kotlin/utils/refactor/MethodSignature.kt | 29 +++++++---------- .../refactor/ExtractMethodPlanEndToEndTest.kt | 31 +++++++++++++++---- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/docs/features/kotlin-extract-method.md b/docs/features/kotlin-extract-method.md index f9965545a2..041c51c66c 100644 --- a/docs/features/kotlin-extract-method.md +++ b/docs/features/kotlin-extract-method.md @@ -105,6 +105,8 @@ The refused case is the accumulator loop, which is a genuinely common extraction **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.** @@ -126,7 +128,7 @@ Declined: a `return` anywhere but the tail position, a `break`/`continue` whose 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, e.g. `private suspend fun loadUser(id: String): 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. +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. @@ -169,7 +171,9 @@ The ordering is mandatory, not stylistic. `IDELanguageClientImpl.applyActionEdit 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 plus a log line, never an uncaught throw - the action framework catches only `IllegalArgumentException` and this runs on a scope with no exception handler. +**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 diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 03bfb1d7b5..dfc9da3b8d 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -259,16 +259,6 @@ private fun descendantsOf( type: Class, ): List = elements.flatMap { PsiTreeUtil.collectElementsOfType(it, type) } -/** - * Whether [reference] is the selector of a qualified expression -- the `n` in `h.n`, the `f` in - * `h.f()`. A call wraps its callee, so the call expression is what the qualified expression holds. - */ -private fun isQualifiedSelector(reference: KtSimpleNameExpression): Boolean { - val selector = (reference.parent as? KtCallExpression)?.takeIf { it.calleeExpression === reference } ?: reference - val parent = selector.parent - return parent is KtQualifiedExpression && parent.selectorExpression === selector -} - /** * The name of a class declared inside [enclosing] that [type] is written in terms of, or null. * @@ -305,12 +295,11 @@ private fun KaSession.capturedParameters( val seen = mutableSetOf() for (reference in simpleNamesIn(elements).sortedBy { it.textRange.startOffset }) { - // The selector of a qualified expression already has its receiver written out next to it and - // resolves through that receiver wherever the code lives, so it is never a capture. Without this - // the `n` in `h.n` became a parameter and the call site passed a name that does not exist. - // `innerImplicitReceiver` has the same guard for the same reason. - if (isQualifiedSelector(reference)) continue - + // 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() @@ -736,8 +725,12 @@ private fun KaSession.innerImplicitReceiver( span: TextSpan, ): String? { for (reference in simpleNamesIn(elements)) { - // A qualified selector already has its receiver written out next to it. - if (isQualifiedSelector(reference)) continue + // 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) diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt index 8ececb967a..1209829719 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlanEndToEndTest.kt @@ -1010,21 +1010,40 @@ class ExtractMethodPlanEndToEndTest : KtLspTest() { } @Test - fun `a qualified selector is not turned into a parameter`() { + 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 { - return h.n + 1 + fun Holder.twice(): Int = n * 2 + return h.twice() + 1 } """.trimIndent() - val (start, end) = selection(content, "return h.n + 1", "return h.n + 1") + val (start, end) = selection(content, "return h.twice() + 1", "return h.twice() + 1") - val candidate = plan(content, start, end).candidates.single() + // 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") - // `n` resolves through `h` wherever the code lives; passing it would name a nonexistent local. - assertEquals(listOf("h"), candidate.parameters.map { it.name }) + // `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 From 13df52dc831846558b715d404c3c7f8f5b26cd4d Mon Sep 17 00:00:00 2001 From: Akash Yadav Date: Wed, 12 Aug 2026 17:18:47 +0000 Subject: [PATCH 17/17] ADFA-5080: Use the shared type-text helpers --- .../kotlin/utils/refactor/MethodSignature.kt | 60 ++++--------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index dfc9da3b8d..987f98bbde 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -1,9 +1,6 @@ package com.itsaky.androidide.lsp.kotlin.utils.refactor -import com.itsaky.androidide.lsp.kotlin.utils.renderName -import org.jetbrains.kotlin.analysis.api.KaExperimentalApi import org.jetbrains.kotlin.analysis.api.KaSession -import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundArrayAccessCall import org.jetbrains.kotlin.analysis.api.resolution.KaCompoundVariableAccessCall @@ -70,7 +67,7 @@ private const val BACKING_FIELD_NAME = "field" private const val COROUTINE_CONTEXT_NAME = "coroutineContext" -/** As [SIGNATURE_TYPE_RENDERER] prints it. A `Unit` return type is left off the signature entirely. */ +/** 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. */ @@ -400,9 +397,11 @@ private sealed interface UsedType { } private fun KaSession.usedTypeOf(expression: KtExpression): UsedType { - val rendered = - runCatching { expression.expressionType?.let { renderTypeText(it) } }.getOrNull() ?: return UsedType.Absent - return if (isUnrenderable(rendered)) UsedType.Unrenderable else UsedType.Rendered(rendered) + 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. */ @@ -416,56 +415,17 @@ private sealed interface CaptureResult { ) : CaptureResult } -/** - * A type that cannot be written out as source -- anonymous, intersection, a resolution error, or a - * platform type [renderTypeText] could not reduce (`List`, where the `!` is on a type - * argument). `!` is not Kotlin syntax anywhere, so its presence alone settles it. - */ -private fun isUnrenderable(text: String): Boolean = - text.isBlank() || - text.contains("anonymous") || - text.contains("ERROR") || - text.contains(" & ") || - text.contains('!') - -/** - * Types in the emitted signature are rendered **fully qualified**. - * - * 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 -- `val d = java.util.Date()` names `Date` - * nowhere -- so a short name is unresolved as often as not, and this refactoring adds no imports. - * Verbose, but it always resolves. [receiverTypeTextOf] deliberately stays on source text instead: - * that text is already in the file. - */ -@OptIn(KaExperimentalApi::class) -private val SIGNATURE_TYPE_RENDERER = KaTypeRendererForSource.WITH_QUALIFIED_NAMES - -/** - * One type as the signature would print it, before the [isUnrenderable] check. - * - * A platform type is unwrapped to 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. - * Only the outermost bound is unwrapped, so a `!` on a type argument survives to [isUnrenderable]. - */ -@OptIn(KaExperimentalApi::class) -private fun KaSession.renderTypeText(type: KaType): String = - renderName((type as? KaFlexibleType)?.lowerBound ?: type, SIGNATURE_TYPE_RENDERER) - private fun KaSession.renderedSymbolType(symbol: KaCallableSymbol): String? = - runCatching { renderTypeText(symbol.returnType) }.getOrNull()?.takeUnless(::isUnrenderable) + runCatching { symbol.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } private fun KaSession.renderedTypeOrNull(expression: KtExpression): String? = - runCatching { expression.expressionType?.let { renderTypeText(it) } }.getOrNull()?.takeUnless(::isUnrenderable) + runCatching { expression.expressionType }.getOrNull()?.let { renderedTypeTextOrNull(it) } private fun KaSession.renderedDeclarationType(property: KtProperty): String? = - runCatching { (property.symbol as? KaCallableSymbol)?.returnType?.let { renderTypeText(it) } } - .getOrNull() - ?.takeUnless(::isUnrenderable) + runCatching { (property.symbol as? KaCallableSymbol)?.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } private fun KaSession.enclosingReturnType(enclosing: KtDeclaration): String? = - runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType?.let { renderTypeText(it) } } - .getOrNull() - ?.takeUnless(::isUnrenderable) + runCatching { (enclosing.symbol as? KaCallableSymbol)?.returnType }.getOrNull()?.let { renderedTypeTextOrNull(it) } /** * What the region declares that the code after it still uses (R7).