ADFA-4826: Kotlin extract variable code action (K2 LSP) - #1654
Open
itsaky-adfa wants to merge 16 commits into
Open
ADFA-4826: Kotlin extract variable code action (K2 LSP)#1654itsaky-adfa wants to merge 16 commits into
itsaky-adfa wants to merge 16 commits into
Conversation
itsaky-adfa
changed the title
itsaky-adfa
marked this pull request as ready for review
August 11, 2026 14:47
![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.
dara-abijo-adfa
approved these changes
Aug 12, 2026
| b: KtSimpleNameExpression, | ||
| ): Boolean = | ||
| runCatching { | ||
| val symbolA = a.mainReference?.resolveToSymbols()?.firstOrNull() ?: return false |
Contributor
There was a problem hiding this comment.
Unnecessary safe calls here and in a few other lines in this file.
itsaky-adfa
force-pushed
the
feat/ADFA-4826-extract-variable
branch
2 times, most recently
from
August 12, 2026 19:23
5975f3f to
d7d9dbd
Compare
The refactoring bottom sheets are Compose (ADR 0009) and live in this module rather than a UI module because `editor` depends on it, not the reverse (ADR 0011). Adds the lifecycle-runtime-compose catalog entry for collectAsStateWithLifecycle().
One background analysis pass produces a plain-data ExtractionPlan covering every candidate expression - its legal scope chain, occurrence set and suggested name - so the UI does pure offset arithmetic and never touches PSI (ADR 0011). Occurrence matching is symbol-aware, not textual: two sites match only when they are structurally equal and every name reference resolves to the same declaration. Sites made unsound by an intervening write are excluded rather than warned about.
One surface holding every choice - expression, name, scope, replace-all - because they are interdependent: a different expression changes the scope list and the occurrence count, and sequential dialogs would hide that. Each chooser is hidden when it has nothing to ask. State derives entirely from the plan, so the ViewModel is a plain unit test with no editor, activity or Compose. Uses the shared IdeTheme from common-compose.
execAction runs the analysis off the UI thread and returns the plan; postExec shows the sheet and turns the user's choice into one spanning TextEdit. The document version is re-read on confirm - the editor stays reachable while the sheet is open, and applying spans computed against older text would corrupt the file. No prepare() visibility gate: deciding extractability needs an analysis session, far too costly for the UI thread. Records the placement decision as ADR 0011.
Requirements, scope, non-goals, acceptance criteria and the test split, following the kotlin-goto-definition.md template. Also carries the Language section for the whole refactoring family - extract method, inline variable and rename all reuse this vocabulary rather than restating it.
Remove dead code path (owner.then === branch can never be true). Correct the KDoc to accurately describe that getThen()/getElse() return unwrapped body expressions, not containers, so branch identity is checked via owner.then?.parent === container. Add test for braced else branch to prevent regression.
A block whose first served statement shares the opening-brace line but
whose content spans several lines fell through the one-line-expansion
check into the normal hoist path, anchoring above the block's own
opening delimiter -- outside the scope the user picked. For a lambda
this put the declaration where `it` is unresolved, emitting Kotlin that
does not compile.
Also fix contentSpanOf: it decided brace ownership by sniffing the
block's own text for a leading `{` and trailing `}`, which misreads a
lambda whose sole statement is itself a lambda literal
(`{ x -> { x + 1 } }`) as owning its braces, returning the inner
lambda's interior instead of the outer body's content. Ownership is now
decided structurally, from the block's parent.
Nothing was folded into the Unit case when deciding whether an expression-body conversion needs a `return`, so a Nothing-returning function (`fun boom() = error(...)`) lost both its `return` and its inferred return type, silently narrowing it to Unit and breaking a caller that uses it in a Nothing position (`x ?: boom()`). Only Unit is excluded now; Nothing goes through the normal return-type-writing path. Also: - Dedupe the symbol-to-return-type lookup into one KaSession.returnTypeOf, dropping the always-succeeding `as? KtDeclaration` cast. - ScopeChain: drop the unread ScopeFrame.statementSpan field and the dead `branch` local. - TypeText: document that the "anonymous"/"ERROR" substring checks in isUnrenderableTypeText are ambiguous but fail safe, and stop shortening a star-imported type when the file also imports a different type of the same simple name. - docs/features/kotlin-extract-variable.md: reword the Status line, the "Refactoring plan" glossary entry and a code comment that referenced the RefactoringPlan supertype and ADR 0013 as already landed -- both arrive with extract method (ADFA-5080); fix the "Anchor point" glossary entry to match the current anchoring behaviour; renumber the 9a/9b acceptance criteria into real ordered items.
itsaky-adfa
force-pushed
the
feat/ADFA-4826-extract-variable
branch
from
August 13, 2026 13:47
d7d9dbd to
e5d36c1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: ADFA-4826
Adds the "Extract variable" code action to the Kotlin K2 LSP: bind the expression at the cursor (or the selected one) to a new local
valand replace its occurrences with the new name. The first interactive Kotlin code action, so it needs a real UI surface rather than a fire-and-forget edit.Requires #1653 for its theming.
What's here
lsp/kotlin.lsp/kotlin/utils/refactor/: candidate expressions, the legal scope chain, occurrence detection, and the single spanning rewrite.lsp/kotlin/refactor/ui/.ExtractVariableActionwiring: analysis runs off the UI thread, the sheet turns the user's choice into oneTextEdit.docs/features/kotlin-extract-variable.md- requirements R1-R11, glossary for the whole refactoring family, and the test split.5 commits, 25 files, +3386.
Design notes worth a reviewer's attention
prepare()visibility gate. Deciding extractability needs a K2 analysis session, far too costly for the UI thread. The action stays visible and reports "nothing to extract" instead, matchingOrganizeImportsAction/ImplementMembersAction.TextEdit, not several - multi-edit application has stale-position and undo-granularity problems.stageclaimed 0011 forcommand-analysis-priority(feat(ADFA-4824): Find usages in the Kotlin K2 LSP #1624) while this branch was in flight, so it landed as 0012.Verification
:lsp:kotlin:compileV8DebugKotlinpasses.:lsp:kotlin:testV7DebugUnitTest- 318 tests, 0 failures.spotlessCheckpasses.prepare()/ActionDataand the sheet are not unit-testable; covered by the acceptance criteria in ADFA-4826's "Steps to QA".editor.codeactions.kotlin.extractvariableshows no text until a row exists. Hand-off item, not code.Stack
Review and merge bottom-up.
Stack created with GitHub Stacks CLI • Give Feedback 💬