ADFA-5080: Kotlin extract method code action (K2 LSP) - #1655
ADFA-5080: Kotlin extract method code action (K2 LSP)#1655itsaky-adfa wants to merge 17 commits into
Conversation
itsaky-adfa
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
14d7ade to
a8111a6
Compare
a8111a6 to
9bb5100
Compare
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).
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<String!>` 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.
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.
9bb5100 to
34305df
Compare
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Correctness review of the extract-method action. I read every non-test source file in the PR in full, plus the base-PR primitives it builds on (CandidateExpressions.kt, TypeText.kt, Occurrences.kt, ExtractVariableAction.kt) for context. Four findings, inline.
Summary
| Severity | Where | What |
|---|---|---|
| High | MethodSignature.kt enclosingDeclaration |
An anonymous fun(...) { } expression is used as the insertion anchor, so the new function lands inside an argument list or property initializer and the file no longer parses. |
| Medium | MethodSignature.kt usesComposable |
Only KtCallExpressions are inspected, so @Composable property getters (MaterialTheme.colorScheme, LocalDensity.current) produce a function without @Composable. |
| Low | MethodSignature.kt hasExit / isTailReturn |
returns inside a nested function declared within the region count as region exits, falsely refusing with ExitsRegion. |
| Low | ExtractMethodEdit.kt reindent |
Re-indentation rewrites the interior of multi-line raw strings, changing their value. |
Checked and clean
Edit ordering (sortedByDescending { it.span.start }), the local-function before/after anchor split, the document-version guard, the insertOffset-inside-region guard, the Unit / tail-return / secondary-constructor return-type cases, usesSuspend's nested-suspend-lambda exclusion, and the receiver / backing-field / type-parameter / smart-cast refusals all hold up. The localTypeNameIn gap for anonymous-object types is covered downstream - isUnrenderableTypeText rejects anonymous, so those decline as UnrenderableType rather than being emitted. The Confirmed-with-null-choice path in ExtractMethodSheet.handleEvent is unreachable (the button is gated on canConfirm), and the silent !shown path matches existing ExtractVariableAction behaviour.
One non-bug worth a follow-up
Fully-qualified type text in signatures (kotlin.Int) is deliberate, documented under R5 and asserted by the end-to-end tests - but it is inconsistent with ExtractVariablePlanner, which runs the same rendering through shortenTypeText. Readability follow-up, not a correctness issue.
| var current: PsiElement? = element.parent | ||
| while (current != null) { | ||
| when (current) { | ||
| is KtNamedFunction, is KtPropertyAccessor, is KtAnonymousInitializer, is KtSecondaryConstructor -> { |
There was a problem hiding this comment.
HIGH - an anonymous function expression is treated as an insertion anchor, emitting code that does not parse.
enclosingDeclaration matches is KtNamedFunction, and Kotlin's PSI represents an anonymous function expression (fun(v: View) { ... } used as a value) as a KtNamedFunction with a null name. enclosingExecutableBody already accepts it, so isExtractionPosition passes.
For view.setOnClickListener(fun(v: View) { doWork() }), extracting doWork() gives enclosing = anchor = the anonymous function, whose parent is a KtValueArgument. So isLocalTarget (line 185) is false and insertOffset = anchor.textRange.endOffset (line 215) - immediately before the closing ) of the call. The emission is:
view.setOnClickListener(fun(v: View) {
extracted()
}
private fun extracted() { doWork() })The same shape with val f = fun(): Int { ... } inserts a private fun into the middle of a property initializer inside a block - an illegal private local function, declared after its call site.
The insertOffset guard in ExtractMethodEdit.kt only rejects offsets inside the region, so it does not catch this. R4's target table in docs/features/kotlin-extract-method.md enumerates member / top-level / lambda / local-fun / companion and has no row for anonymous functions; no test covers them.
Suggested fix: treat a nameless KtNamedFunction like a KtFunctionLiteral here - skip it and keep walking up.
| * `@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<KtExpression>): Boolean = |
There was a problem hiding this comment.
MEDIUM - usesComposable misses @Composable property getters, so common Compose regions extract into a function that does not compile.
It only walks KtCallExpression descendants and resolves them as function calls. MaterialTheme.colorScheme, MaterialTheme.typography and LocalDensity.current are @Composable/@ReadOnlyComposable property getters, not calls.
Extracting MaterialTheme.colorScheme.primary or LocalDensity.current.density therefore yields:
private fun extracted(): Color {
return MaterialTheme.colorScheme.primary
}with no @Composable - exactly the compile failure R10 and the feature doc say the annotation exists to prevent. Given CoGo's Compose focus these are everyday shapes.
Fix: also resolve KtSimpleNameExpressions to KaPropertySymbol and check getter?.annotations, mirroring how innerImplicitReceiver resolves references rather than only calls.
| } | ||
|
|
||
| /** Any `return`, `break` or `continue` whose target lies outside the region (R8). */ | ||
| private fun hasExit( |
There was a problem hiding this comment.
LOW - hasExit (and isTailReturn just above) count returns belonging to nested functions declared inside the region, producing a false refusal with a misleading message.
descendantsOf(elements, KtReturnExpression::class.java) collects every return in the subtree, including ones inside a local fun or anonymous fun declared within the region. A statement range containing
fun helper(): Int { return 1 }has that return with getLabelName() == null, so the loop below returns true and the whole region is refused as ExitsRegion - "The selection jumps out of itself with return, break or continue" - even though the jump never leaves the region. isTailReturn is skewed the same way via returns.size != 1.
Fix: skip returns whose nearest enclosing KtDeclarationWithBody is itself inside the region.
| baseIndent: String, | ||
| newline: String, | ||
| ): List<String> = | ||
| text.split(newline).mapIndexed { index, line -> |
There was a problem hiding this comment.
LOW - reindent rewrites the interior of multi-line raw string literals, silently changing their value.
It strips baseIndent from and re-prefixes bodyIndent to every line of the region text, with no awareness of string literals. Whenever bodyIndent != baseIndent - the normal case for a region nested inside a lambda or an if (e.g. baseIndent = "\t\t", indent = "", bodyIndent = "\t") - a """...""" literal in the region loses one indent level from each of its continuation lines.
The result compiles but the runtime string differs from before the refactoring, which contradicts ADR 0013's "the refactoring moves code, it never edits the interior of what it moved". (A literal followed by .trimIndent() is unaffected; a bare one is not.)
Detecting a KtStringTemplateExpression overlapping a line and leaving those lines untouched - or declining - would close it.
2 participants
Jira: ADFA-5080
Adds the "Extract method" code action to the Kotlin K2 LSP: lift a selected statement range or expression into a new function and replace it with a call. Top of a 3-PR stack; requires #1654, whose refactoring primitives, glossary and sheet patterns it builds on.
What's here
RefactoringPlanhoisted into a shared sealed supertype (ExtractVariablePlanbecomes a sibling ofExtractMethodPlan).MethodSignature: derives the extracted function's parameters, return type and modifiers from the region's data flow, or a typed refusal.ExtractMethodActionwiring.docs/features/kotlin-extract-method.md- R1-R16, non-goals, 21 acceptance criteria.16 commits, 28 files, +4359/-83.
The governing principle (ADR 0013)
The 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 R7-R10 are that principle applied to one case each - refused receivers, labelled jumps, and smart-cast narrowing that would not survive the move. A refusal is a designed outcome surfaced as a specific message, not an error.
Two limitations this creates are tracked separately:
varas the single outputNotes for reviewers
with(density) { size.toPx() }. Replaced by one test per broken shape.stageclaimed 0011 forcommand-analysis-priority(feat(ADFA-4824): Find usages in the Kotlin K2 LSP #1624) while this branch was in flight, shifting this stack's ADRs down to 0012/0013.Verification
:lsp:kotlin:compileV8DebugKotlinpasses.:lsp:kotlin:testV7DebugUnitTest- 401 tests, 0 failures.spotlessCheckpasses.prepare()/ActionDataare not unit-testable; covered by ADFA-5080's "Steps to QA".editor.codeactions.kotlin.extractmethodneeds a row in the out-of-repo tooltips database. Hand-off item, not code.Stack
Review and merge bottom-up.
Stack created with GitHub Stacks CLI • Give Feedback 💬