Skip to content

Latest commit

 

History

History
295 lines (208 loc) · 34.5 KB

File metadata and controls

295 lines (208 loc) · 34.5 KB

Kotlin extract method (K2 LSP)

  • Ticket: ADFA-5080 (subtask of ADFA-3317; split out of ADFA-4826, which now covers extract variable only)
  • Status: Implemented
  • Module: lsp/kotlin
  • Vocabulary: the term is method, matching the ticket and the already-fixed tooltip tag editor.codeactions.kotlin.extractmethod, even though the refactoring's output is a Kotlin fun.

Move the expression at the cursor, or a selected range of statements, into a new function, and replace it with a call to that function.

Ships as the top of a three-PR stack: common-compose theming, then extract variable (ADFA-4826), then this. It reuses that PR's primitives - offsets, naming, indentation, edit emission - and adds no new module, no new dependency and no new UI mechanism.

The governing principle is ADR 0014: 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. 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 and the legal-target rules. A bare cursor always takes this path.

Statement range - a non-empty selection that spans statement boundaries snaps outward to whole statements: a touch selection will not land on a boundary. The result must be 1..N statements that are siblings in one KtBlockExpression. A selection spanning two different blocks, or partially covering a statement that cannot be snapped, is declined (NotASingleRegion).

Restricting to siblings in one block excludes every hard case - a selection covering half an if and half its else, a range straddling a lambda boundary - by construction rather than by later filtering, exactly as isLegalExtractionTarget excludes expression fragments today.

R3 - Live offsets and the version guard. Identical to extract variable: analysis runs against getCurrentKtFile(path) fetched before entering project.read, the plan records the document version (on the RefactoringPlan supertype), and each action re-reads the version on confirm with a mismatch refusing the edit.

R4 - Target. One uniform rule, no target picker: the new function is inserted as a sibling of the enclosing declaration - immediately after it, except for a local fun target, where it goes immediately before it. A local function is only visible from its declaration onward, so it has to be declared above the code that calls it; every other target has no such constraint. That one rule produces the conventional answer in every context:

The region sits in The new function becomes
a member function, accessor or init of a class a private fun member of that class
a top-level function or property a private top-level fun
a lambda inside either of the above still a sibling of the enclosing named declaration; the lambda's captures become parameters
an anonymous fun(...) { } used as a value still a sibling of the enclosing named declaration, exactly as for a lambda; PSI gives it the same node type as a named function, but it is a value and nothing can be inserted after it. An anonymous extension function (fun String.() { }) is declined instead: skipping it would drop a receiver the body depends on
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

A region with no enclosing named declaration at all - one inside a lambda or an anonymous fun that is itself in a class-body property initializer - is declined as NotASingleRegion. There is no anchor a sibling could follow, and the message is imprecise about why rather than wrong.

Unlike extract variable there is no scope chain and no ceiling, because anything not visible at the insertion site becomes a parameter instead of constraining the anchor.

R5 - Parameters. A referenced declaration needs a parameter exactly when it is a captured declaration - its PSI lies inside the enclosing declaration. Members of the enclosing class need nothing, because the new function is a member of that same class.

  • Order - first textual appearance in the region, so the signature reads in the order the body uses it.
  • Names - the original identifier, unchanged. it becomes a parameter literally named it, which is legal Kotlin, and the call site passes it.
  • Types - the resolved type rendered fully qualified (KaTypeRendererForSource.WITH_QUALIFIED_NAMES), so java.util.Date rather than Date. Verbose, but a short name resolves only when the file already imports it, and a local's type usually comes from inference rather than a spelled-out type reference - this refactoring adds no imports. A platform type is emitted as its lower bound: the renderer prints String!, which does not parse, and the lower bound is both what IntelliJ writes and what the moved body already assumes. A type that cannot be rendered - anonymous, intersection, a resolution failure, or a ! the lower bound did not remove (a platform type on a type argument) - declines the extraction (UnrenderableType) rather than emitting uncompilable text. A value whose type is a class declared inside the enclosing declaration declines too (CapturedLocalDeclaration): the value survives the move, its type name does not.
  • Not editable. The derived signature is shown read-only (R11). Renaming, reordering or excluding parameters is a desktop-sized dialog; a wrong parameter name is fixable afterwards with rename (ADFA-4825), and a wrong parameter set is not something the user could correct by hand anyway.

R6 - Return type and call-site form. Determined by the region kind and its output:

Case Extracted body Call site
expression candidate return <expr> extracted(args) in the expression's place
statement range, no output the statements; returns Unit extracted(args) as a statement
statement range, one output x the statements, then return x val x = extracted(args)
statement range, tail return (R8) the statements including the return return extracted(args)

A region that always throws still declares Unit; the exception propagates and the call site behaves identically, so throw needs no rule of its own.

R7 - Outputs. An output is a local declared inside the region and read after it. Exactly one plain val/var is supported; two or more declines (MultipleOutputs, naming them), and a single output the call site cannot receive back declines separately (OutputNotReturnable, naming it) - a destructuring entry or a local fun, which a val cannot stand in for, or a local the following code reassigns, which a val cannot be. The two are distinct refusals because "produces more than one value" is simply untrue of the second, and a refusal that misdescribes the situation teaches nothing.

A var declared outside the region and reassigned inside it declines (ReassignsOuterVar, naming the variable), because Kotlin has no out parameters and the faithful emission - a parameter plus var x = x at the top of the body - carries a name-shadowing warning into generated code. This is deliberately stricter than dataflow requires: a reassignment whose result is never read afterwards is still refused, because proving that needs real liveness analysis. ADFA-5082 tracks supporting it.

The refused case is the accumulator loop, which is a genuinely common extraction, so its message must name the variable and read as a limitation rather than a malfunction.

R8 - Exits. Every exit declines (ExitsRegion), with one syntactic exception.

Tail return: when the region's last statement is a return, the region contains no other return, break or continue, and there is no other output, the extracted function takes the enclosing function's return type, keeps the return, and the call site becomes return extracted(args). The exception holds only when that tail return returns from the enclosing declaration itself and carries no label: a return owned by an anonymous fun wrapped around the region would take a return type its own function never returns, and a return@label names something outside the region that the new function does not declare. Both fall through to the ordinary exit check and decline. "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.

Not an exit: a return belonging to a function declared inside the region - a local fun, an anonymous fun, or an anonymous-object override. It moves with its own declaration and its jump never crosses the region boundary, so counting it would refuse a perfectly good extraction with a message describing something the user did not write.

R9 - Receivers.

  • Class dispatch receiver - nothing to do; the new function is a member of the same class.
  • The enclosing declaration's extension receiver - the new function is generated as an extension on the same receiver type, copied syntactically from the enclosing declaration's receiver type reference. The call site needs no change at all: inside fun Foo.original(), this is a Foo, so extracted(args) resolves to private fun Foo.extracted(args).
  • An implicit receiver introduced inside the enclosing declaration - the with(x) { ... } / apply / run / buildString case - declines (InnerImplicitReceiver). Turning that receiver into a parameter would require qualifying every unqualified member access inside the extracted body, which is editing the interior of the moved code. Android code uses these scoping functions heavily, so this refusal will be common and its message must say which construct is in the way.

R10 - Modifiers. Copy nothing from the enclosing declaration; add only what the body needs in order to compile in its new home.

  • Visibility - always private, whether a class member or top-level. Never internal, never open, no annotations copied, no KDoc generated.
  • suspend - added when any call in the region resolves to a suspend function, or the region references coroutineContext. The call site is necessarily already a suspend context. Not added for a suspension the region only performs inside a nested suspend-typed lambda - scope.launch { }, runBlocking { }, any suspend () -> T parameter: the region carries that lambda with it, so the new function needs no suspend, and adding it breaks a call site that is not itself a suspend context. An ordinary inline lambda (forEach, let, run) is not one of these and still propagates suspend outwards. Not detected: invoking a suspend () -> T parameter directly, where the modifier is carried by the functional type and not by the resolved Function0.invoke symbol.
  • @Composable - added when the region uses one: any call resolving to a @Composable-annotated function, or any name reference resolving to a property whose getter is annotated. The second half is not an edge case - MaterialTheme.colorScheme and LocalDensity.current are annotated getters, not calls. This is not polish: CoGo users write Compose apps on the device, and an extracted composable without the annotation does not compile. Not detected: invoking a @Composable-typed lambda parameter, the Compose slot-API shape (fun Card(content: @Composable () -> Unit), extracting content()), because the annotation sits on the functional type and the call resolves to an unannotated Function0.invoke. A region whose only composable use is such an invocation extracts without the modifier.
  • 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) -> name field with its NameProblem message -> signature preview -> Cancel/Extract. There is no scope chooser (R4) and no replace-all checkbox (R13).

The preview is one monospace line: the signature exactly as it will be emitted - modifiers, receiver, parameters, return type. Types render fully qualified (R5), so a real preview reads private suspend fun loadUser(id: kotlin.String): com.example.User. It wraps rather than truncating. No body preview: the body is the code the user selected and can see behind the sheet, so it moves verbatim and previewing it says nothing new, while the signature is the one derived artefact and the one place the derivation can surprise them.

ADR 0013 defers the shared-UI question until the extract-method surface is known; a single generalised sheet would need a state class where half the fields are meaningless to either caller, so that question stays open rather than being settled from one data point.

R12 - Name. Suggestion: for an expression region, the existing shape/type derivation unchanged; for a statement range, the constant extracted, since there is no expression to read a name from and inventing a verb from statement shapes is guesswork. Uniquified as today.

Validation reuses validateVariableName and NameProblem unchanged - so no new error strings - with taken names being every callable name visible in the insertion container, including inherited members (the container's memberScope, not just its declared members) for a class target; every top-level declaration name in the file for a top-level target; enclosing-block declarations for a local target.

Including inherited names is a correctness requirement, not a nicety: a private function accidentally matching a supertype member is an accidental-override compile error. Rejecting any name match rather than only a signature match also means the refactoring never creates an overload the user did not ask for.

R13 - One call site. The region is the only site rewritten. No duplicate detection, no replace-all toggle: exact-duplicate matching would almost never fire, and near-duplicate matching needs anti-unification plus a per-site parameter mapping - a feature in its own right. Occurrences.kt is expression-granular by construction.

R14 - Refusals. The plan carries a typed ExtractionRefusal rather than merely being empty, and postExec maps it to a specific message:

Reason Message intent
NotASingleRegion select an expression, or whole statements inside one block
CouldNotAnalyse the analysis could not run - deliberately neutral, since the selection may have been fine
MultipleOutputs the selection produces more than one value
OutputNotReturnable the selection produces <name>, which cannot be handed back as a return value
ReassignsOuterVar the selection assigns to <name>, declared outside it
ExitsRegion the selection jumps out of itself (return/break/continue)
AnonymousExtensionFunction the selection is inside an anonymous extension function
InnerImplicitReceiver the selection uses members of an enclosing with/apply receiver
UsesTypeParameter the selection uses type parameter <T>
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 <name> under a smart cast that does not hold outside it
CapturedLocalDeclaration the selection uses <name>, which goes out of scope once the selection moves

All but CouldNotAnalyse are actionable - they tell the user what to change - and several (ReassignsOuterVar, InnerImplicitReceiver, UsesBackingField) are common enough that a generic message would read as the feature being broken. CouldNotAnalyse exists precisely so the others stay truthful: a missing compilation environment, an unreachable KtFile or a thrown analysis error must not be reported as NotASingleRegion, which blames a selection nothing ever looked at. Given how much of this design is "decline cleanly", the refusal text is a first-class part of the feature. New entries in resources/.../values/strings.xml, picked up by the next translation batch.

Cancellation is not a refusal at all: buildExtractMethodPlan re-throws CancellationException (which AnalysisPreemptedException is), so a cancelled action ends silently rather than flashing at a user who has moved on.

The refusal lives on ExtractMethodPlan only; extract variable keeps its single "nothing to extract" behaviour unchanged.

R15 - Edit. Two regions change - the region becomes a call, and the new function appears next to the enclosing declaration - emitted as two TextEdits in one DocumentChange, sorted by descending start offset.

The ordering is mandatory, not stylistic. IDELanguageClientImpl.applyActionEdits iterates the edit list in order and editInEditor applies each with line/column ranges against whatever the text is at that moment (the index in Position is ignored), so an earlier edit must never shift a later one. Which edit leads follows from R4 rather than being fixed: a member or top-level target is inserted after its anchor, so the new function leads; a local fun target is inserted before its anchor, so the call site leads.

Known consequence: nothing on that path calls beginBatchEdit, so this is two undo entries, and a single undo leaves a half-refactored, non-compiling file. This knowingly diverges from RewriteSpan's single-replacement rule, which extract variable relies on. ADFA-5081 fixes it properly by batching the edit loop in applyActionEdits, which benefits every multi-edit action; until it lands, the two-step undo is a stated limitation to be covered in QA.

The new function is emitted fully indented at the enclosing declaration's own indentation, separated by one blank line, reusing detectIndentUnit, detectNewline, leadingIndentAt and positionAt. Code-action edits bypass the editor's auto-indent and CMD_FORMAT_CODE is a no-op for Kotlin.

One exception to re-indenting every line: the interior and closing delimiter of a raw (triple-quoted) string literal are emitted byte-for-byte. Their whitespace is part of the literal's value, and the closing delimiter's column sets trimIndent's margin, so shifting either would edit the interior of the moved code (ADR 0014). The candidate carries those literals' spans so the text layer can skip them without needing PSI.

R16 - Responsiveness and failure isolation. As extract variable: one background pass at AnalysisPriority.INTERACTIVE under a cancel checker tied to the action's coroutine produces the whole plan; the sheet does pure string and offset arithmetic and re-enters no analysis on confirm. Anything thrown in the pipeline degrades to a refusal (CouldNotAnalyse) plus a log line, never an uncaught throw - the action framework catches only IllegalArgumentException and this runs on a scope with no exception handler.

CancellationException is the one deliberate exception, and it is re-thrown rather than swallowed - AnalysisPreemptedException is one. A cancelled action has no result worth reporting, and DefaultActionsRegistry.executeAction launches into a scope whose invokeOnCompletion already treats a CancellationException as an ordinary cancel, so re-throwing ends the action quietly instead of flashing a message at a user who has moved on. Swallowing it would also break structured concurrency for whatever cancelled the job. The sheet's confirm path is outside the framework's guards entirely, so ExtractMethodAction.applyChoice wraps its own body.

Non-goals

  • Duplicate or near-duplicate call sites (R13).
  • An editable parameter list - rename, reorder or exclude (R5).
  • Two or more outputs, and a reassigned outer var (R7). The latter is ADFA-5082.
  • Mid-region return/break/continue (R8).
  • Inner with/apply/run receivers (R9).
  • Function-level type parameters (R10).
  • Detecting @Composable or suspend carried by a functional type rather than by the called symbol (R10) - invoking a @Composable () -> Unit or suspend () -> T parameter does not add the modifier.
  • 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 function, or reading a @Composable property such as MaterialTheme.colorScheme, 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 0013: one background pass produces a plain-data plan, the sheet holds no PSI.

ExtractMethodAction.execAction (background)                lsp/kotlin/actions
  server.compilationEnvironmentFor(path) ?: refusal
  -> buildExtractMethodPlan(...)                           utils/refactor/ExtractMethodPlanner.kt
       ktFile = env.ktSymbolIndex.getCurrentKtFile(path).get()          [R3: before project.read]
       env.project.read {
         resolveRegion(ktFile, start, end)                 utils/refactor/ExtractionRegion.kt      [R2]
           expression -> candidateExpressionsAt(...)       (reused unchanged)
           statements -> snap outward, sibling check
         analyzeMaybeDangling(INTERACTIVE, cancelChecker) {                                        [R16]
           captured declarations -> parameters             utils/refactor/MethodSignature.kt       [R5]
           outputs / exits / receivers / modifiers                                          [R6-R10]
           -> ExtractMethodPlan | ExtractionRefusal                                               [R14]
         }
       }
  <- ExtractMethodPlan (plain data, no PSI)

ExtractMethodAction.postExec (UI thread)
  refusal -> flashInfo(message for reason)                 [R14]
  ExtractMethodSheet.show                                  refactor/ui                            [R11]
  on confirm -> version re-read; mismatch -> refuse        [R3]
    buildExtractMethodRewrite -> two RewriteSpans          utils/refactor/ExtractMethodEdit.kt     [R15]
    client.performCodeAction(one DocumentChange, two TextEdits, descending)

New files, all in lsp/kotlin:

  • utils/refactor/ExtractionRegion.kt - the region model and its resolution (R2). Purely syntactic, so unit-testable with no analysis session, exactly as CandidateExpressions.kt is.
  • utils/refactor/MethodSignature.kt - captured declarations to parameters, outputs, exits, receivers, modifiers, and the rendered signature string (R5-R10). The only analysis-dependent part.
  • utils/refactor/ExtractMethodPlan.kt - ExtractMethodPlan (a RefactoringPlan subtype) and ExtractionRefusal.
  • utils/refactor/ExtractMethodPlanner.kt - the single background pass (R3, R16).
  • utils/refactor/ExtractMethodEdit.kt - the two rewrites and their ordering (R15). Pure text and offsets.
  • refactor/ui/ExtractMethod*.kt - sheet, content, ViewModel, state, events (R11).
  • actions/ExtractMethodAction.kt - registered in KotlinCodeActionsMenu; the only class touching the editor, the document version or the language client.
  • TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD - one new constant (R1).

Reused from extract variable unchanged: TextSpan, collapseForLabel, candidateExpressionsAt / CandidateSyntax, isExtractionPosition, enclosingExecutableBody, NameProblem + validateVariableName, suggestVariableName, detectIndentUnit, detectNewline, leadingIndentAt, lineStartOffset, RewriteSpan + toTextEdit, positionAt, renderName.

Deliberately not reused: ScopeOption, AnchorForm and CandidateExpression. Each is shaped by the legal scope chain, which this refactoring does not have (R4) - so the two refactorings share primitives, not the aggregate. What they do share is hoisted into the sealed RefactoringPlan (fileText and documentVersion), introduced in the extract-variable PR so this one is purely additive. The version guard itself - reading the live version and comparing - stays in each action rather than on the supertype, since it needs the ActionData and the action's own "file changed" string; hoisting it is a small cleanup, not a shared primitive today.

Nothing outside lsp/kotlin changes except TooltipTag.kt and values/strings.xml. No new module, no new dependency.

Verification

Unit tests in :lsp:kotlin (flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest), mirroring the extract-variable split so a failure localises to one layer:

  • ExtractMethodRegionTest - no analysis session, PSI only: outward snapping to whole statements, the sibling-in-one-block rule, cross-block rejection, and the expression path (R2).
  • ExtractMethodPlanEndToEndTest - analysis-backed, one case per rule: the parameter set, order and types (R5), the single output and the Unit case (R6, R7), the tail return and the nested-declaration return that is not an exit (R8), the extension receiver (R9), suspend, a @Composable call and a @Composable property getter (R10), the anonymous-function anchor (R4), the recorded multi-line-string spans (R15), and one case per refusal reason (R14).
  • ExtractMethodEditTest - pure text: the two edits and their descending order, the three call-site forms, indentation, raw (triple-quoted) string literals left verbatim, 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 0014 - refactorings decline rather than rewrite unselected code; the principle behind R7-R10
  • ADR 0013 - refactoring UI lives in the owning LSP module
  • 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-5178 - shorten signature type text to match extract variable (revisits R5's fully-qualified rendering)
  • ADFA-5048 - Java extract method, the sibling in lsp/java
  • ARCHITECTURE.md