diff --git a/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md b/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md new file mode 100644 index 0000000000..92a2a0b16b --- /dev/null +++ b/docs/adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md @@ -0,0 +1,52 @@ +# 0012. Refactoring UI lives in the owning LSP module + +- **Status:** Proposed +- **Date:** 2026-08-03 +- **Deciders:** Code On The Go team + +## Context + +The K2 Kotlin LSP is gaining interactive refactorings: extract variable and extract method (ADFA-4826), inline variable (ADFA-4827), semantic rename (ADFA-4825). Unlike every existing Kotlin code action, these cannot be a single fire-and-forget edit — the user has to choose an expression, a name, a target scope, and whether to replace other occurrences. That is a real UI surface, not a `DialogUtils` one-liner. + +[ADR 0009](0009-jetpack-compose-for-new-ui.md) settles *what* that UI is built with (Compose, UDF, `ViewModel` + `StateFlow`). It says nothing about *where* language-specific UI lives, and the module graph makes that a genuine question: + +- `editor` depends on `lsp/kotlin` (`editor/build.gradle.kts`), so the dependency flows **LSP -> editor**. An LSP module cannot reach the editor or `app`. +- `ActionData` carries only a `Context` and the editor; there is no service-lookup mechanism for an LSP module to call *up* into a UI layer. +- `lsp/java` already owns UI code today — `AutoFixImportsAction` builds and shows a `DialogUtils` chooser directly. + +So a refactoring in `lsp/kotlin` either renders its own UI, or a new inversion mechanism has to be invented for it. + +## Decision + +**A language server module owns the UI for its own refactorings.** `lsp/kotlin` enables Compose and hosts the refactoring bottom sheets; the same applies to any future `lsp/*` module that grows an interactive refactoring. + +- Compose is enabled per-module exactly as `flamegraph`, `floating-window` and `profiler` do it: the `kotlin-compose` plugin, `compose = true`, and the Compose BOM with `ui`/`foundation`/`material3`. +- The UI is a `BottomSheetDialogFragment` hosting a `ComposeView`. The hosting `FragmentActivity` is found by walking `ContextWrapper.baseContext` up from `ActionData`'s `Context` — no new `ActionData` key, no change to the `editor` module. +- **The analysis/UI split is enforced by data, not by module boundaries.** The action's background pass produces a plain-data plan (candidate expressions, scope chains, occurrence ranges, suggested name, document version); the sheet performs no analysis and holds no PSI. All refactoring logic lives in pure functions, unit-testable without an editor, an activity, or Compose. +- ADR 0009 otherwise applies unchanged: `ViewModel` + `StateFlow`, sealed `UiEvent`, `collectAsStateWithLifecycle()`. + +## Consequences + +**Positive** +- No new indirection: one module, one PR per refactoring, no interface to register or resolve. +- Consistent with `lsp/java` already owning its dialogs, so there is one rule for LSP-owned UI rather than two. +- The plain-data plan boundary keeps the valuable logic testable regardless of where the UI sits, so the placement decision does not compromise test coverage. + +**Negative / costs** +- A language server module gains a UI surface, which is a layering smell: `lsp/kotlin` is no longer purely a language service. +- Compose and `lifecycle-viewmodel` are added to a module that previously had neither, growing its build surface and bringing ktlint's compose-rules ruleset to bear on it. +- Walking the `ContextWrapper` chain for a `FragmentActivity` is an implicit dependency on how the editor is hosted; a future change to that hosting breaks it at runtime rather than at compile time. +- If three or more `lsp/*` modules end up with Compose UI, extracting a shared UI module becomes worthwhile and this decision will need revisiting. + +## Alternatives considered + +- **Render in `editor`, invert via an interface.** Declare a refactoring-UI interface in `editorApi` or `lsp/models`, implement it in `editor`, have `lsp/kotlin` call up through it. Cleanest layering. Rejected: nothing registers such an implementation today, so it means inventing a service-lookup mechanism for one sheet, and the interface would be guessed from a single client. +- **Render in `app`.** `app` is the integration point and already hosts `BottomSheetDialogFragment`s and `ILanguageClient`. Rejected: same inversion problem, and it puts Kotlin-specific refactoring UI in the module where nothing else language-specific lives. +- **A new `lsp/kotlin-ui` module.** Keeps Compose out of `lsp/kotlin` without inverting. Rejected for now: a new Gradle module in a ~80-module build is disproportionate for one sheet. Reconsider once extract-method and inline-variable have landed and the UI surface is known. + +## Related + +- [ADR 0009](0009-jetpack-compose-for-new-ui.md) — Compose for new UI; this ADR answers *where*, not *what*. +- [ADR 0006](0006-koin-dependency-injection.md) — Koin DI, unchanged. +- [ADR 0010](0010-navigation-resolves-via-analysis-api.md) — the K2 Analysis API as the Kotlin semantic source of truth. +- [ARCHITECTURE.md](../../ARCHITECTURE.md) — module map, layering, UDF. diff --git a/docs/adr/README.md b/docs/adr/README.md index 7139d240d5..554f429bee 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -25,3 +25,4 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0009](0009-jetpack-compose-for-new-ui.md) | Build new UI in Jetpack Compose, not XML Views | Proposed | | [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 | diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md new file mode 100644 index 0000000000..44e27107ef --- /dev/null +++ b/docs/features/kotlin-extract-variable.md @@ -0,0 +1,271 @@ +# Kotlin extract variable (K2 LSP) + +- **Ticket:** ADFA-4826 (subtask of ADFA-3317; split out of the closed ADFA-3324 "Refactoring"). Extract method was originally part of this subtask and is now ADFA-5080. +- **Status:** Implemented in `lsp/kotlin/utils/refactor/` and `lsp/kotlin/refactor/ui/`, pending on-device QA. Still to land in this PR: the `ExtractionPlan` -> `ExtractVariablePlan` rename under a sealed `RefactoringPlan` supertype, which arrives with extract method (ADFA-5080). +- **Module:** `lsp/kotlin` + +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). + +## Language + +This section is the glossary for the whole refactoring family - extract variable, extract method (ADFA-5080), inline variable (ADFA-4827), rename (ADFA-4825). Prefer these terms over ad-hoc synonyms in code, tests, docs and review comments. + +**Selection**: +The user's raw offsets from the editor caret, before any processing. A cursor is the degenerate selection where start equals end. Trimmed and snapped before it becomes an extraction region, so it is *not* interchangeable with one. +_Avoid_: range (that's `Range`, the LSP line/column type), region. + +**Extraction region**: +The contiguous text an extraction reads its body from. For extract variable it is always an expression candidate; extract method adds statement ranges. +_Avoid_: target (overloaded with go-to-definition's target and with the insertion site), extent, fragment. + +**Expression candidate**: +A `KtExpression` at the selection that is a legal extraction target. Ordered innermost-first, at most `MAX_CANDIDATES` (3) of them, so the chooser stays scannable on a phone. +_Avoid_: candidate expression when naming code (the type is `CandidateExpression`, but the term is "expression candidate"), match, option. + +**Text span**: +A half-open offset range `[start, end)` into the analysed file's text - the type `TextSpan`. Purely positional; it carries no meaning about what it covers. +_Avoid_: range, offset pair. + +**Legal scope chain**: +The ordered anchors available for the new declaration, innermost first: outward from the candidate's own statement through enclosing blocks, crossing a lambda boundary only when nothing lambda-scoped is referenced, and stopping at the enclosing named function, accessor or `init` body. +_Avoid_: scope list, parent chain. + +**Anchor scope**: +The chain member the user picked. The `val` is declared inside it. + +**Anchor form**: +How the declaration is woven into an anchor scope, since not all Kotlin scopes are blocks: `ExistingBlock`, `WrapInBraces`, or `ConvertExpressionBody`. + +**Anchor point**: +The exact insertion offset - the start of the line holding the first statement *within the anchor +scope* that contains a replaced occurrence. Recorded per rung in the plan (`ExistingBlock`'s +`statementSpans`), because it is the only thing that makes an outer rung differ from an inner one. + +**Occurrence**: +A site inside the anchor scope that is structurally equal to the candidate *and* whose every name reference resolves to the same declaration. Sites made unsound by an intervening write are excluded, so an occurrence set is always safe to replace wholesale. +_Avoid_: duplicate, match, usage. + +**Refactoring plan**: +The complete result of the background analysis pass, carrying the analysed `fileText` and its `documentVersion`. Plain data: no PSI, no symbols, no session. Currently `ExtractionPlan`; extract method (ADFA-5080) adds a sealed `RefactoringPlan` supertype and renames this to `ExtractVariablePlan`, its subtype. +_Avoid_: model, result, context. + +**Rewrite span**: +The single text replacement an extraction performs - a `TextSpan` plus its replacement text (`RewriteSpan`), converted to one `TextEdit` at the boundary. + +## Scope + +### In scope + +An expression inside any executable body: a function body, a property accessor, an `init` block, a constructor, or a lambda. Both a bare cursor and a selection, since a cursor is just the selection where start equals end. + +### Out of scope + +Positions where no `val` can precede the expression, all rejected up front by `isExtractionPosition`: + +- **Annotation arguments** - must be compile-time constants. +- **Default parameter values** - evaluated per call, and a hoisted local would not be in scope. +- **Super-constructor delegation arguments** - nothing can precede them. +- **Anything outside an executable body**, notably a class-body property initializer. Converting one to a getter would turn compute-once into compute-per-access, so it is declined rather than silently changing evaluation semantics. + +## Requirements + +**R1 - Trigger.** An "Extract variable" item (`action_extract_variable`) appears in the editor code-actions menu for Kotlin files, id `ide.editor.lsp.kt.extractVariable`, tooltip tag `EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE = "editor.codeactions.kotlin.extractvariable"`. Tooltip *content* is keyed by tag in the out-of-repo tooltips database, so the tag shows no text until a row exists for it - a hand-off item, not code. + +There is deliberately **no `prepare()` visibility gate**. Deciding whether anything is extractable needs a K2 analysis session, which is far too costly for `prepare()` (UI thread, per menu item). The action stays visible on any Kotlin file and reports "nothing to extract" instead, matching `OrganizeImportsAction` and `ImplementMembersAction`. `requiresUIThread = false`, so the selection is read on a background thread; a torn read while the user is mid-edit can only produce a plan the version guard (R3) then refuses. + +**R2 - Region.** The selection is whitespace-trimmed first, because a touch-screen selection routinely carries a leading or trailing space; a whitespace-only selection yields nothing. For a cursor, the element is looked up at the offset and then at `offset - 1`, so a caret resting just past a token still resolves. + +From the innermost element the parent chain is walked outwards, collecting legal targets and stopping at the enclosing declaration. Illegal nodes along the way are **skipped rather than terminating the walk**, so `if (c) a else b` is still offered from inside one of its branches. At most 3 candidates, innermost first, deduplicated by range. + +An expression is not a legal target when it is: a block, a loop, `return`/`throw`/`break`/`continue`, an operation reference, `super`, a lambda (the `{ ... }` expression and the literal inside it -- outside its call site the parameter types are gone, so `val v = { it.length + 1 }` does not compile), the selector of a qualified expression (`b` in `a.b`), a call's callee (`foo` in `foo(x)`), the left side of an assignment, or a **bare literal**. Excluding bare literals removes the only case where omitting the type annotation could change meaning - an `Int` literal where a `Long` is expected, or a bare `null` inferring `Nothing?`. + +When the trimmed selection exactly equals the innermost candidate's range, the user has already said which expression they mean and the chooser is not shown (`selectionMatchedCandidate`). + +**R3 - Live offsets and the version guard.** Analysis runs against `ktSymbolIndex.getCurrentKtFile(path)`, PSI refreshed to the open document's current version - an offset resolved against stale text points at the wrong element. The `KtFile` is fetched *before* entering `project.read`: the refresh needs `project.write`, and awaiting it under the read lock deadlocks. + +The plan records the document version it was computed against. On confirm, the version is re-read and the edit is **refused** if it has moved on (`msg_extract_variable_file_changed`) - 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. + +**R4 - Value filter.** A candidate whose type is `Unit` or `Nothing` is dropped: `val u = println(x)` compiles but is pointless. A candidate whose legal scope chain is empty is dropped too - a candidate with no legal anchor is not a candidate. + +**R5 - Scope chain.** Anchors are enumerated outward from the candidate's own statement, each one of three anchor forms: + +| Anchor form | When | Emitted as | +|---|---|---| +| `ExistingBlock` | the scope already has a `{ ... }` body | a new statement line | +| `WrapInBraces` | a braceless statement position: `if (c) foo()`, a `when` entry, a braceless loop body | the statement is replaced by a braced block holding the declaration and the original statement | +| `ConvertExpressionBody` | an expression-bodied function or accessor, `fun area(r: Int) = r * r` | `=` and the body become a block body; `return` is added unless the declaration returns `Unit`; the return type is written into the signature when the declaration does not spell one out, because a block body with no declared type returns `Unit` | + +A written-out return type is rendered fully qualified and then shortened to its simple name only where +that name already resolves in the file -- an exact import, a star import of its package, or a +default-imported package such as `kotlin.collections`. Everything else stays qualified: verbose, but it +compiles, and this refactoring adds no imports. When the type cannot be written as source at all +(anonymous, intersection, an unresolved type, or a platform type the renderer cannot reduce) the rung +is declined rather than emitting a block body that does not compile. + +Each rung is labelled with the construct that owns it -- `fun name`, `getter`, `setter`, `init block`, +`lambda`, `if block`, `else block`, `for loop`, `while loop`, `do-while loop`, `when branch` -- so the +`Declare in` list reads as a place rather than as a nesting level. A braced control-structure body is +wrapped in a container node, so the owner is the block's grandparent, not its parent. + +The walk stops after the enclosing named function, accessor or `init` body. A class body or file is never an anchor. Lambda boundaries are crossed during the syntactic walk, then **truncated afterwards** by the innermost scope holding a declaration the candidate references - so a candidate using `it` or a lambda parameter can never be hoisted out of that lambda. `it` needs its own case: it has no source PSI, so a value-parameter symbol with no PSI referenced by the name `it` is taken to be the innermost enclosing lambda's implicit parameter. That is a property of the language, not a guess about the text. + +Braceless control-structure bodies are wrapped in a container node, so the `if`/loop is the grandparent; without unwrapping, no braceless body is ever detected and the declaration silently hoists to the enclosing block instead of braces being added. + +**R6 - Occurrences.** Two sites are the same expression when they are structurally identical (whitespace and comments ignored) *and* every name reference in them resolves to the same declaration. The symbol check is the point: text or structure alone would match `config.timeout` inside a nested lambda where `config` is a different `config`. ADFA-3324 states the standard outright - text-based matching breaks things. + +Source declarations are compared by PSI identity, which is exactly the question being asked ("the same `val`?"); symbols without source PSI fall back to symbol equality. A resolution failure reads as "not the same" rather than propagating. + +Matches must themselves be legal targets - in `a.a`, a candidate of `a` matches the selector too, and rewriting it would produce `v.v`. Overlapping matches are dropped so no site is rewritten twice. + +An occurrence set is then restricted to a contiguous run around the candidate that **no write to a referenced mutable interrupts**: + +```kotlin +var limit = 1 +foo(limit + 1) // occurrence +limit = 5 +foo(limit + 1) // same expression, different value +``` + +Unsound sites are excluded rather than warned about, so "Replace all N occurrences" can never produce wrong code and N is always achievable. The walk grows outward from the candidate - never dropping the site the user selected - and stops in each direction at the first write it would cross. Writes counted: plain assignment, the augmented forms, and `++`/`--`, against any `var` the candidate reads. + +Occurrence sets are ascending by offset and always contain the candidate's own span, so `occurrences.size` is the count shown in "Replace all N occurrences". Narrowing to an inner scope can only shrink the set, never grow it. + +**R7 - Name.** The suggestion is derived from the expression's shape first (`items.size` -> `size`, `getFoo()` -> `foo`, an interpolated string -> `text`), then its rendered type (`List` -> `list`), then `"value"`; shape beats type because `size`, `count` and `name` are far better names than `int` and `string`. It is then uniquified with a numeric suffix. + +Validation returns a `NameProblem` - `Blank`, `NotAnIdentifier`, `Keyword`, `AlreadyTaken` - rather than throwing, since the input is a text field. Only Kotlin's **hard** keywords are rejected; soft and modifier keywords (`by`, `data`, `it`) are legal names. Backtick-quoted names are rejected: legal Kotlin, but a poor generated local, and accepting them would mean validating the quoted form too. + +Taken names are every declaration name in the file - deliberately conservative rather than scope-exact. Being over-broad costs a `size1` where `size` would have done; being under-broad generates code that shadows something. It is also purely syntactic, so it needs no analysis and is unit-testable. + +**R8 - Sheet.** One surface holding every choice, with no navigation between steps: expression chooser, name field, scope chooser, replace-all checkbox, Cancel/Extract. The four are interdependent - a different expression changes the scope list and the occurrence count - so they are shown together where that relationship is visible, rather than across sequential dialogs the user would have to back out of to explore. + +Each chooser is hidden when it has nothing to ask: the expression chooser when there is one candidate or the selection already matched one, the scope chooser when the chain has one rung, the replace-all checkbox at an occurrence count of one. Changing the expression re-suggests the name, because the old one described the old expression. + +**R9 - Edit.** Exactly **one** `TextEdit`, built as a `RewriteSpan` covering one contiguous span. `IDELanguageClientImpl.applyActionEdits` applies each edit in its own `runOnUiThread` with no `beginBatchEdit`, and every range is interpreted against the *current* text - so a list of N edits would be applied against positions already shifted by its predecessors and would cost N undo steps with a typing window between each. Occurrences are substituted right-to-left within the span so an earlier substitution cannot shift a later offset. + +The span is anchored on the chosen rung's statement, not on the occurrence: for an outer rung the +declaration goes above the whole enclosing statement, at that statement's indentation. + +A block written on one line -- `items.map { it.length + 1 }`, `fun f(n: Int): Int { return n * 2 }`, +a one-line `if` body -- is expanded instead: the content between the braces moves onto its own line +with the declaration above it and the closing brace below. Anchoring on the statement's line start +there would place the declaration *before* the `{`, outside the scope the value belongs to, which +leaves a lambda's `it` unresolved. The braces themselves and a lambda's `param ->` header are left +where they are. + +Whether a block counts as "one line" takes two conditions, not one. A single check against where the +block's content starts is not enough: a lambda body's block does not own its braces, so its content +span sits at the body's first token even when that token starts its own line, and comparing that alone +against the line start would wrongly expand an ordinary multi-line lambda. Both must hold: something +other than indentation already precedes the statement on its line (the brace, a header, or a prior +semicolon-separated statement), *and* the block's own content contains no newline (so re-emitting it +as a single line loses nothing). A multi-line lambda fails the first and keeps its shape; a multi-line +block with two semicolon-separated statements on one line satisfies the first but fails the second, so +it also keeps its shape, with the declaration hoisted above the whole line instead. + +A block that fails *both* conditions -- something besides indentation precedes the statement on its +line, but the block's own content spans more than one line, as in `items.forEach { log(x)\n\tlog(y) }` +-- is **declined** rather than hoisted. Hoisting would anchor before the block's own opening delimiter, +outside the scope the user picked, which is unsound whenever anything inside that scope (a lambda's +`it`, say) is not visible there. + +The emitted text is **fully indented**: code-action edits bypass the editor's auto-indent (raw `Content.replace`), and `CMD_FORMAT_CODE` is a no-op for Kotlin. The indent unit is inferred from the file's own lines (a tab if any line is tab-indented, else the smallest positive run of leading spaces, defaulting to a tab), mirroring `ImplementMembersAction`; CRLF is used only when the file already contains it, so the edit never mixes line endings. + +**R10 - Responsiveness.** One background analysis pass produces the plan for *all* candidates at once; the sheet then performs pure string and offset arithmetic on it. Nothing re-enters analysis on confirm, which keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation unit-testable without an editor, an activity or Compose. Analysis runs at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine, so cancelling the action aborts the analysis. + +**R11 - Failure isolation.** Anything thrown in the analysis pipeline degrades to an empty plan and 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; reporting "nothing to extract" is always safe. A missing `FragmentActivity` or fragment manager logs and flashes `msg_cannot_perform_fix` rather than failing silently. + +## Non-goals + +- **Extract to a `val` outside an executable body** - a class property or a top-level `val`. That is a different refactoring with different scope rules. +- **Extract `var`, `lateinit`, or a property with accessors.** Always a `val`. +- **An explicit type annotation** on the generated declaration. Bare literals are excluded (R2) precisely so inference cannot change meaning. +- **Occurrences outside the anchor scope**, or across files. +- **Renaming the declaration in place after the edit** - ADFA-4825. +- **Formatting the result.** `CMD_FORMAT_CODE` is a no-op for Kotlin; R9 emits indented text instead. +- **Extract method** - ADFA-5080, which shares this vocabulary and these primitives. + +## Acceptance criteria + +1. "Extract variable" appears in the code-actions menu of a Kotlin file and is absent in a non-Kotlin file. +2. A cursor inside `a + b * c` offers the innermost-first candidates and extracting the selected one produces `val = ...` on its own line above, correctly indented. +3. A selection that exactly matches an expression skips the expression chooser. +4. A caret immediately after an identifier resolves the same as one inside it. +5. A cursor on a bare literal, on whitespace, in a comment, or in an annotation argument reports "No expression to extract here". +6. An expression appearing three times in the same block reports "Replace all 3 occurrences" and rewrites all three. +7. The same expression with an intervening reassignment of a `var` it reads offers only the contiguous sound run. +8. An expression using `it` inside a lambda offers no anchor outside that lambda. +9. Extracting from `if (c) foo(x + 1)` wraps the branch in braces with the declaration inside. +10. With a candidate inside a braced `if` inside a function, picking `fun name` in `Declare in` puts the declaration above the `if`, and picking `if block` puts it inside the branch. +11. Extracting from `return items.map { it.length + 1 }` puts the declaration inside the lambda and expands the block over three lines; the same holds for a one-line function body. +12. Extracting from `fun area(r: Int): Int = r * r` converts it to a block body with `return`, leaving the declared type alone; extracting from `fun area(r: Int) = r * r` converts it *and* writes `: Int` into the signature. +13. Extracting from a `Unit`-returning expression-bodied function converts it without adding `return` and without writing a type. +14. A name that is blank, not an identifier, a hard keyword, or already used disables Extract and shows the matching message. +15. Editing the file while the sheet is open, then confirming, reports "The file changed. Try extracting again." and leaves the file untouched. +16. One undo restores the file exactly. +17. A file indented with spaces receives space-indented output; a CRLF file keeps CRLF. + +## Design + +Per [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md), `lsp/kotlin` owns its refactoring UI, and the analysis/UI split is enforced **by data rather than by module boundaries**: the background pass produces a plain-data plan, and the sheet holds no PSI and performs no analysis. + +``` +ExtractVariableAction.execAction (background) lsp/kotlin/actions + server.compilationEnvironmentFor(path) ?: empty plan + cursor -> [selectionStart, selectionEnd) + -> buildExtractionPlan(...) utils/refactor/ExtractVariablePlanner.kt + ktFile = env.ktSymbolIndex.getCurrentKtFile(path).get() [R3: before project.read] + env.project.read { + candidateExpressionsAt(ktFile, start, end) utils/refactor/CandidateExpressions.kt [R2] + analyzeMaybeDangling(INTERACTIVE, cancelChecker) { [R10] + per candidate: type filter [R4] + enclosingScopeFrames + truncateAtCeiling ScopeChain.kt / Occurrences.kt [R5] + findOccurrences + excludeUnsoundOccurrences Occurrences.kt [R6] + suggestVariableName + visibleNamesAt NameSuggestion.kt / Occurrences.kt [R7] + } + } + <- ExtractVariablePlan (plain data, no PSI) + +ExtractVariableAction.postExec (UI thread) + empty -> flashInfo("No expression to extract here") [R11] + findFragmentActivity() -> ExtractVariableSheet.show refactor/ui [R8] + ExtractVariableViewModel: StateFlow, sealed UiEvent + on confirm -> ExtractionChoice + version re-read; mismatch -> refuse [R3] + buildExtractVariableRewrite -> RewriteSpan -> toTextEdit utils/refactor/ExtractVariableEdit.kt [R9] + client.performCodeAction(one DocumentChange, one TextEdit) +``` + +Components: + +- **`utils/refactor/ExtractionPlan.kt`** - `TextSpan`, `AnchorForm`, `ScopeOption`, `CandidateExpression`, the plan, `collapseForLabel`. To be renamed to `ExtractVariablePlan` under a sealed `RefactoringPlan` carrying `fileText`, `documentVersion` and the shared version guard, so ADFA-5080 adds a subtype rather than renaming this one. Both refactorings share these *primitives*, not the aggregate: extract method has no scope chain, so `ScopeOption`/`AnchorForm`/`CandidateExpression` are not shared. +- **`CandidateExpressions.kt`** - purely syntactic, no analysis session, hence unit-testable on its own (R2). +- **`ScopeChain.kt`** - the syntactic chain and the three anchor forms (R5); indentation and newline detection shared with the edit builder. +- **`Occurrences.kt`** - symbol-aware structural equality, the occurrence search, the unsoundness filter, the referenced-declaration ceiling, and `visibleNamesAt` (R5, R6, R7). +- **`NameSuggestion.kt`** - suggestion and validation, no analysis session (R7). +- **`ExtractVariableEdit.kt`** - `RewriteSpan`, the three anchor-form rewrites, `toTextEdit` (R9). Pure text and offsets. +- **`refactor/ui/`** - `ExtractVariableSheet` (a `BottomSheetDialogFragment` hosting a `ComposeView`), stateless `ExtractVariableSheetContent`, `ExtractVariableViewModel` + `ExtractVariableUiState` + sealed `ExtractVariableUiEvent`. `LabelledSection` and `OptionList` become shared with ADFA-5080. The ViewModel uses a plain `ViewModelProvider.Factory` rather than a Koin definition: it is sheet-scoped, injects nothing, and takes the plan as a runtime argument. +- **`ExtractVariableAction`** extending `BaseKotlinCodeAction`, registered in `KotlinCodeActionsMenu`; the only class that touches the editor, the document version or the language client. +- **`common-compose`** - `IdeTheme`/`IdeColorScheme`, shared with `profiler` and `floating-window` so the sheet matches the IDE's theme. + +## Verification + +Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), split so a failure localises to one layer: + +- **`RefactorPrimitivesTest`** - no analysis session: selection trimming, candidate collection and the legal-target rules (R2), indent/newline detection, name suggestion and validation (R7), the unsoundness filter as a pure function (R6). +- **`ExtractVariablePlanEndToEndTest`** - analysis-backed: the value filter (R4), scope chains and the lambda ceiling (R5), occurrence sets including the `it` and same-name-different-symbol cases (R6). +- **`ExtractVariableEditTest`** - pure text: the three anchor forms, right-to-left substitution, indentation and CRLF (R9). +- **`ExtractVariableViewModelTest`** - state derivation: chooser visibility, candidate switching re-suggesting the name, replace-all clamping, `choice()` refusing an invalid name (R8). +- **`KotlinCodeActionTooltipTagTest`** - every action carries a tooltip tag (R1). + +`prepare()`/`ActionData` and the sheet itself are not unit-testable, consistent with the other Kotlin code actions. They are covered by on-device QA from the acceptance criteria, recorded in ADFA-4826's "Steps to QA" field. + +## 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 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 +- [ARCHITECTURE.md](../../ARCHITECTURE.md) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 9c4e15649b..af4cc0b7f0 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -96,6 +96,8 @@ androidx-fragment = { module = "androidx.fragment:fragment", version.ref = "frag androidx-lifecycle-viewmodel-ktx = { module = "androidx.lifecycle:lifecycle-viewmodel-ktx", version.ref = "lifecycleViewmodelKtx" } androidx-lifecycle-process = { module = "androidx.lifecycle:lifecycle-process", version.ref = "lifecycleViewmodelKtx" } androidx-lifecycle-runtime-ktx = { module = "androidx.lifecycle:lifecycle-runtime-ktx", version.ref = "lifecycleViewmodelKtx" } +# Provides collectAsStateWithLifecycle(), the state-collection API mandated by ADR 0009. +androidx-lifecycle-runtime-compose = { module = "androidx.lifecycle:lifecycle-runtime-compose", version.ref = "lifecycleViewmodelKtx" } androidx-palette-ktx = { module = "androidx.palette:palette-ktx", version.ref = "paletteKtx" } androidx-preference-ktx = { module = "androidx.preference:preference-ktx", version.ref = "preferenceKtxVersion" } androidx-recyclerview-v132 = { module = "androidx.recyclerview:recyclerview", version.ref = "recyclerview" } 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 02a0571d1f..ac8fd24d98 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -93,6 +93,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH = "editor.codeactions.kotlin.trycatch" 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 EXIT_TO_MAIN = "exit.to.main" diff --git a/lsp/kotlin/build.gradle.kts b/lsp/kotlin/build.gradle.kts index 9b16f87796..d25dd4a40a 100644 --- a/lsp/kotlin/build.gradle.kts +++ b/lsp/kotlin/build.gradle.kts @@ -21,11 +21,18 @@ plugins { id("com.android.library") id("kotlin-android") id("kotlin-kapt") + alias(libs.plugins.kotlin.compose) } android { namespace = "${BuildConfig.PACKAGE_NAME}.lsp.kotlin" + // The refactoring bottom sheets are Compose (ADR 0009); they live here rather than in a UI + // module because `editor` depends on this module, not the reverse (ADR 0012). + buildFeatures { + compose = true + } + kotlin.compilerOptions { freeCompilerArgs.addAll("-Xcontext-parameters") } @@ -51,6 +58,22 @@ dependencies { implementation(projects.subprojects.projects) implementation(projects.subprojects.projectModels) + implementation(projects.commonCompose) + + implementation(platform(libs.compose.bom)) + implementation(libs.compose.runtime) + implementation(libs.compose.ui) + implementation(libs.compose.foundation) + implementation(libs.compose.material3) + implementation(libs.compose.ui.tooling.preview) + debugImplementation(libs.compose.ui.tooling) + + implementation(libs.androidx.fragment.ktx) + implementation(libs.androidx.lifecycle.runtime.ktx) + implementation(libs.androidx.lifecycle.viewmodel.ktx) + implementation(libs.androidx.lifecycle.runtime.compose) + implementation(libs.google.material) + implementation(libs.common.jsonrpc) implementation(libs.common.kotlin) implementation(libs.common.kotlin.coroutines.core) 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 1188a15022..311d0dadd6 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.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction @@ -39,5 +40,6 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { SurroundWithTryCatchAction(), NullSafetyAction(), ImplementMembersAction(), + ExtractVariableAction(), ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt new file mode 100644 index 0000000000..086c8060f7 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractVariableAction.kt @@ -0,0 +1,155 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +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.ExtractVariableSheet +import com.itsaky.androidide.lsp.kotlin.refactor.ui.ExtractionChoice +import com.itsaky.androidide.lsp.kotlin.refactor.ui.findFragmentActivity +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractVariableRewrite +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildExtractionPlan +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 + +/** + * Extracts the expression at the cursor, or the selected one, into a local `val`. + * + * The work is split so nothing heavy touches the UI thread: [execAction] runs one background analysis + * pass and returns a plain-data [ExtractionPlan] covering every candidate, then [postExec] shows the + * sheet and turns the user's choice into a single text edit with pure offset arithmetic. + */ +class ExtractVariableAction : BaseKotlinCodeAction() { + companion object { + const val ID = "ide.editor.lsp.kt.extractVariable" + } + + override var titleTextRes: Int = R.string.action_extract_variable + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE + + override val id: String = ID + override var label: String = "" + + // Analysis must not run on the UI thread. The selection is therefore read at the top of + // execAction on a background thread, as ImplementMembersAction does; 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() (UI thread). The action stays visible on any + // Kotlin file and reports "nothing to extract" instead. Matches OrganizeImportsAction and + // ImplementMembersAction. + + override suspend fun execAction(data: ActionData): ExtractionPlan { + val server = data.get() ?: return ExtractionPlan.empty() + val nioPath = data.requireFile().toPath() + val env = server.compilationEnvironmentFor(nioPath) ?: return ExtractionPlan.empty() + + val cursor = data.requireEditor().cursor + val selectionStart = minOf(cursor.left, cursor.right) + val selectionEnd = maxOf(cursor.left, cursor.right) + + return buildExtractionPlan( + env = env, + nioPath = nioPath, + selectionStart = selectionStart, + selectionEnd = selectionEnd, + 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 ExtractionPlan) return + + if (result.isEmpty) { + flashInfo(R.string.msg_extract_variable_nothing_to_extract) + return + } + + val activity = + data.requireContext().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 = ExtractVariableSheet.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 one edit and hands it 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: ExtractionPlan, + choice: ExtractionChoice, + ) { + val file = data.requireFile() + val nioPath = file.toPath() + if (documentVersionOf(nioPath) != plan.documentVersion) { + flashInfo(R.string.msg_extract_variable_file_changed) + return + } + + val rewrite = + buildExtractVariableRewrite( + fileText = plan.fileText, + candidateSpan = choice.candidate.span, + scope = choice.scope, + name = choice.name, + replaceAll = choice.replaceAll, + ) ?: run { + logger.warn("Could not build an extract-variable 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 variable.") + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = listOf(DocumentChange(file = nioPath, edits = listOf(rewrite.toTextEdit(plan.fileText)))), + kind = CodeActionKind.QuickFix, + // The rewrite is emitted fully indented; CMD_FORMAT_CODE is a no-op for Kotlin anyway. + command = Command("", ""), + ), + ) + } + + /** -1 when the document is not open, which never matches a real version and so fails the guard. */ + private fun documentVersionOf(path: Path): Int = FileManager.getActiveDocument(path)?.version ?: -1 +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt new file mode 100644 index 0000000000..17ffdf7dba --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheet.kt @@ -0,0 +1,117 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import android.content.Context +import android.content.ContextWrapper +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.ExtractionPlan + +/** + * Hosts [ExtractVariableSheetContent]. + * + * 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, which is the same outcome the action's document-version guard would reach anyway. + */ +class ExtractVariableSheet : BottomSheetDialogFragment() { + private var plan: ExtractionPlan? = null + private var onChoice: ((ExtractionChoice) -> Unit)? = null + + private val viewModel: ExtractVariableViewModel by viewModels { + ExtractVariableViewModel.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 { + // The sheet's window is torn down with the fragment's view, so dispose with it. + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { + IdeTheme { + val state by viewModel.uiState.collectAsStateWithLifecycle() + ExtractVariableSheetContent( + state = state, + onEvent = ::handleEvent, + ) + } + } + } + } + + private fun handleEvent(event: ExtractVariableUiEvent) { + when (event) { + ExtractVariableUiEvent.Confirmed -> { + viewModel.choice()?.let { choice -> onChoice?.invoke(choice) } + dismiss() + } + + ExtractVariableUiEvent.Dismissed -> { + dismiss() + } + + else -> { + viewModel.onEvent(event) + } + } + } + + companion object { + private const val TAG = "extract_variable_sheet" + + /** + * Shows the sheet on [activity], calling [onChoice] once if the user confirms. + * + * Returns false when the sheet could not be shown, so the caller can report a failure rather + * than silently doing nothing. + */ + fun show( + activity: FragmentActivity, + plan: ExtractionPlan, + onChoice: (ExtractionChoice) -> Unit, + ): Boolean { + val manager = activity.supportFragmentManager + if (manager.isStateSaved || manager.isDestroyed) return false + ExtractVariableSheet() + .apply { + this.plan = plan + this.onChoice = onChoice + }.show(manager, TAG) + return true + } + } +} + +/** + * Finds the [FragmentActivity] hosting this context by unwrapping the [ContextWrapper] chain. + * + * A view inflated into an activity reports that activity as its context, but a theme overlay wraps it, + * so a direct cast is not reliable. `ActionData` carries only the editor's `Context`, and adding a + * `FragmentActivity` key would only move the same unwrapping one module upstream, into `editor`. + */ +fun Context.findFragmentActivity(): FragmentActivity? { + var context: Context? = this + while (context != null) { + if (context is FragmentActivity) return context + context = (context as? ContextWrapper)?.baseContext + } + return null +} 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 new file mode 100644 index 0000000000..25409974ee --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableSheetContent.kt @@ -0,0 +1,200 @@ +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.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 +import androidx.compose.ui.Alignment +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 + +/** + * The extract-variable sheet: one surface holding every choice, with no navigation between steps. + * + * Expression, name, scope and replace-all are interdependent -- picking a different expression changes + * the scope list and the occurrence count -- so they are shown together, where that relationship is + * visible, rather than across sequential dialogs the user would have to back out of to explore. + * + * Stateless: all state arrives in [state] and every interaction leaves as an [ExtractVariableUiEvent]. + */ +@Composable +fun ExtractVariableSheetContent( + state: ExtractVariableUiState, + onEvent: (ExtractVariableUiEvent) -> 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_variable), + 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(ExtractVariableUiEvent.CandidateSelected(it)) }, + ) + } + } + + OutlinedTextField( + value = state.name, + onValueChange = { onEvent(ExtractVariableUiEvent.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(), + ) + + if (state.showScopePicker) { + LabelledSection(stringResource(R.string.label_extract_variable_scope)) { + OptionList( + options = state.scopeLabels, + selected = state.selectedScope, + monospace = false, + onSelect = { onEvent(ExtractVariableUiEvent.ScopeSelected(it)) }, + ) + } + } + + if (state.showReplaceAll) { + val replaceAllLabel = + pluralStringResource( + R.plurals.label_extract_variable_replace_all, + state.occurrenceCount, + state.occurrenceCount, + ) + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = + Modifier + .fillMaxWidth() + .toggleable( + value = state.replaceAll, + role = Role.Checkbox, + onValueChange = { onEvent(ExtractVariableUiEvent.ReplaceAllChanged(it)) }, + ), + ) { + Checkbox( + checked = state.replaceAll, + // Null so the row, not the box, is the single accessibility target. + onCheckedChange = null, + ) + Text( + text = replaceAllLabel, + modifier = Modifier.padding(start = 8.dp), + ) + } + } + + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.End, + ) { + TextButton(onClick = { onEvent(ExtractVariableUiEvent.Dismissed) }) { + Text(stringResource(android.R.string.cancel)) + } + Button( + onClick = { onEvent(ExtractVariableUiEvent.Confirmed) }, + enabled = state.canConfirm, + modifier = Modifier.padding(start = 8.dp), + ) { + Text(stringResource(R.string.action_extract)) + } + } + } +} + +@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/ExtractVariableUiState.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt new file mode 100644 index 0000000000..c5937f79dd --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableUiState.kt @@ -0,0 +1,71 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.CandidateExpression +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ScopeOption + +/** + * Everything the extract-variable sheet renders, derived entirely from the + * [com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan]. + * + * [showCandidatePicker] is false when the plan holds a single candidate, or when the user's selection + * already matched an expression exactly -- in both cases asking which expression they meant would be + * asking a question they have already answered. + * + * [occurrenceCount] counts every site the selected scope would rewrite, **including** the one the user + * selected, so "Replace all 3 occurrences" means three sites in total. [showReplaceAll] is false at a + * count of one, where the toggle would have nothing to do. + */ +data class ExtractVariableUiState( + val candidateLabels: List, + val selectedCandidate: Int, + val showCandidatePicker: Boolean, + val name: String, + val nameProblem: NameProblem?, + val scopeLabels: List, + val selectedScope: Int, + val occurrenceCount: Int, + val replaceAll: Boolean, +) { + val showReplaceAll: Boolean get() = occurrenceCount > 1 + + val showScopePicker: Boolean get() = scopeLabels.size > 1 + + val canConfirm: Boolean get() = nameProblem == null +} + +/** What the sheet reports back up; the ViewModel never touches the document itself. */ +sealed interface ExtractVariableUiEvent { + data class CandidateSelected( + val index: Int, + ) : ExtractVariableUiEvent + + data class NameChanged( + val name: String, + ) : ExtractVariableUiEvent + + data class ScopeSelected( + val index: Int, + ) : ExtractVariableUiEvent + + data class ReplaceAllChanged( + val replaceAll: Boolean, + ) : ExtractVariableUiEvent + + data object Confirmed : ExtractVariableUiEvent + + data object Dismissed : ExtractVariableUiEvent +} + +/** + * The user's finished decision, handed to the action to turn into an edit. + * + * Kept free of offsets and text so the sheet stays a pure chooser: resolving this into a rewrite, and + * checking the document has not moved on, both belong to the action. + */ +data class ExtractionChoice( + val candidate: CandidateExpression, + val scope: ScopeOption, + val name: String, + val replaceAll: Boolean, +) diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt new file mode 100644 index 0000000000..6d9494593e --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModel.kt @@ -0,0 +1,118 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import androidx.lifecycle.ViewModel +import androidx.lifecycle.ViewModelProvider +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +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 [ExtractionPlan] and nothing else. + * + * The plan already contains every candidate's scope chain and occurrence set, so switching expression + * or scope is pure recomputation -- no analysis, no PSI, no I/O. That is what lets this class hold all + * the sheet's logic while remaining a plain unit test. + * + * A plain [ViewModelProvider.Factory] rather than a Koin definition (ADR 0006/0009 resolve ViewModels + * through Koin): this one is sheet-scoped, injects nothing, and takes the plan as a runtime argument, + * so a Koin definition would add indirection without providing anything. + */ +class ExtractVariableViewModel( + private val plan: ExtractionPlan, +) : ViewModel() { + private val _uiState = MutableStateFlow(initialState()) + val uiState: StateFlow = _uiState.asStateFlow() + + private fun initialState(): ExtractVariableUiState = stateFor(candidateIndex = 0, scopeIndex = 0, replaceAll = false, name = null) + + fun onEvent(event: ExtractVariableUiEvent) { + val current = _uiState.value + when (event) { + is ExtractVariableUiEvent.CandidateSelected -> { + if (event.index == current.selectedCandidate) return + // A different expression means a different suggested name, scope chain and count, so the + // name is re-suggested rather than carried over -- the old one described the old expression. + _uiState.value = stateFor(event.index, scopeIndex = 0, replaceAll = false, name = null) + } + + is ExtractVariableUiEvent.ScopeSelected -> { + if (event.index == current.selectedScope) return + _uiState.value = + stateFor(current.selectedCandidate, event.index, current.replaceAll, current.name) + } + + is ExtractVariableUiEvent.NameChanged -> { + _uiState.value = + current.copy( + name = event.name, + nameProblem = validateVariableName(event.name, candidate(current.selectedCandidate).takenNames), + ) + } + + is ExtractVariableUiEvent.ReplaceAllChanged -> { + _uiState.value = current.copy(replaceAll = event.replaceAll) + } + + ExtractVariableUiEvent.Confirmed, ExtractVariableUiEvent.Dismissed -> { + Unit + } + } + } + + /** The user's decision, or null when the name is unusable. */ + fun choice(): ExtractionChoice? { + val state = _uiState.value + if (!state.canConfirm) return null + val candidate = candidate(state.selectedCandidate) + val scope = candidate.scopes.getOrNull(state.selectedScope) ?: return null + return ExtractionChoice( + candidate = candidate, + scope = scope, + name = state.name, + // A single occurrence makes the toggle meaningless, and the sheet hides it; make sure a + // stale `true` from a previous candidate cannot leak into the choice. + replaceAll = state.replaceAll && state.occurrenceCount > 1, + ) + } + + private fun candidate(index: Int) = plan.candidates[index.coerceIn(plan.candidates.indices)] + + /** + * Recomputes the whole state for a (candidate, scope) pair. [name] carries the user's typed name + * across a scope change; pass null to take the candidate's suggestion. + */ + private fun stateFor( + candidateIndex: Int, + scopeIndex: Int, + replaceAll: Boolean, + name: String?, + ): ExtractVariableUiState { + val candidate = candidate(candidateIndex) + val boundedScope = scopeIndex.coerceIn(candidate.scopes.indices) + val scope = candidate.scopes[boundedScope] + val resolvedName = name ?: candidate.suggestedName + val occurrenceCount = scope.occurrences.size + + return ExtractVariableUiState( + candidateLabels = plan.candidates.map { it.label }, + selectedCandidate = candidateIndex.coerceIn(plan.candidates.indices), + showCandidatePicker = plan.candidates.size > 1 && !plan.selectionMatchedCandidate, + name = resolvedName, + nameProblem = validateVariableName(resolvedName, candidate.takenNames), + scopeLabels = candidate.scopes.map { it.label }, + selectedScope = boundedScope, + occurrenceCount = occurrenceCount, + replaceAll = replaceAll && occurrenceCount > 1, + ) + } + + companion object { + fun factory(plan: ExtractionPlan): ViewModelProvider.Factory = + object : ViewModelProvider.Factory { + @Suppress("UNCHECKED_CAST") + override fun create(modelClass: Class): T = ExtractVariableViewModel(plan) as T + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt new file mode 100644 index 0000000000..2f6b6fd3ef --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/CandidateExpressions.kt @@ -0,0 +1,225 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtAnnotationEntry +import org.jetbrains.kotlin.psi.KtAnonymousInitializer +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtBreakExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtConstantExpression +import org.jetbrains.kotlin.psi.KtContinueExpression +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtLambdaExpression +import org.jetbrains.kotlin.psi.KtLiteralStringTemplateEntry +import org.jetbrains.kotlin.psi.KtLoopExpression +import org.jetbrains.kotlin.psi.KtOperationReferenceExpression +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtReturnExpression +import org.jetbrains.kotlin.psi.KtStringTemplateEntry +import org.jetbrains.kotlin.psi.KtStringTemplateExpression +import org.jetbrains.kotlin.psi.KtSuperExpression +import org.jetbrains.kotlin.psi.KtSuperTypeListEntry +import org.jetbrains.kotlin.psi.KtThrowExpression + +/** How many candidate expressions are ever offered. Keeps the chooser scannable on a phone. */ +const val MAX_CANDIDATES = 3 + +/** + * The purely syntactic result of resolving a cursor or selection to extraction targets. + * + * [expressions] is innermost-first and at most [MAX_CANDIDATES] long. [selectionMatchedInnermost] is + * true when the caller passed a non-empty selection whose trimmed range is exactly the innermost + * candidate's range -- the user has already said which expression they mean, so the UI can skip + * asking. + */ +data class CandidateSyntax( + val expressions: List, + val selectionMatchedInnermost: Boolean, +) { + companion object { + val NONE = CandidateSyntax(emptyList(), selectionMatchedInnermost = false) + } +} + +/** + * Resolves `[selectionStart, selectionEnd)` in [file] to candidate expressions. A cursor is the + * degenerate case where the two offsets are equal, so callers need only one code path. + * + * The selection is whitespace-trimmed first, because a touch-screen selection routinely carries a + * leading or trailing space. From the resulting innermost element the parent chain is walked + * outwards, keeping legal targets ([isLegalExtractionTarget]) and stopping at the enclosing + * declaration. Blocks and other illegal nodes along the way are skipped rather than terminating the + * walk, so `if (c) a else b` is still offered from inside one of its branches. + * + * Returns [CandidateSyntax.NONE] when the position cannot host an extraction at all -- see + * [isExtractionPosition]. + */ +fun candidateExpressionsAt( + file: KtFile, + selectionStart: Int, + selectionEnd: Int, +): CandidateSyntax { + val text = file.text + val (start, end) = trimToCode(text, selectionStart, selectionEnd) ?: return CandidateSyntax.NONE + + val anchor = innermostElementFor(file, start, end) ?: return CandidateSyntax.NONE + if (!isExtractionPosition(anchor)) return CandidateSyntax.NONE + + val collected = mutableListOf() + val seen = mutableSetOf>() + var element: PsiElement? = anchor + while (element != null && element !is KtFile) { + if (element is KtDeclaration && element !is KtFunctionLiteral) break + if (element is KtExpression && element.isLegalExtractionTarget()) { + val range = element.textRange.startOffset to element.textRange.endOffset + if (seen.add(range)) { + collected += element + if (collected.size == MAX_CANDIDATES) break + } + } + element = element.parent + } + + if (collected.isEmpty()) return CandidateSyntax.NONE + + val innermost = collected.first().textRange + val matched = + selectionStart != selectionEnd && + innermost.startOffset == start && + innermost.endOffset == end + return CandidateSyntax(collected, matched) +} + +/** + * Trims whitespace off both ends of `[start, end)`. Returns null when nothing but whitespace was + * selected. A cursor (start == end) is returned unchanged. + */ +internal fun trimToCode( + text: String, + start: Int, + end: Int, +): Pair? { + if (start < 0 || end > text.length || start > end) return null + if (start == end) return start to end + var s = start + var e = end + while (s < e && text[s].isWhitespace()) s++ + while (e > s && text[e - 1].isWhitespace()) e-- + return if (s == e) null else s to e +} + +/** + * The innermost element covering `[start, end)`. For a cursor, [KtFile.findElementAt] is tried at + * the offset and then just before it, so a caret sitting immediately after a token still resolves. + */ +private fun innermostElementFor( + file: KtFile, + start: Int, + end: Int, +): PsiElement? { + if (start == end) { + val at = file.findElementAt(start)?.takeUnless { it is PsiWhiteSpace } + val before = file.findElementAt((start - 1).coerceAtLeast(0))?.takeUnless { it is PsiWhiteSpace } + return at ?: before + } + val first = file.findElementAt(start) ?: return null + val last = file.findElementAt(end - 1) ?: return null + return PsiTreeUtil.findCommonParent(first, last) +} + +/** + * Whether [element] sits somewhere an extraction can legally be anchored. + * + * Rejects the positions where no `val` can precede the expression: + * - **annotation arguments** -- must be compile-time constants; + * - **default parameter values** -- evaluated per call, and a hoisted local would not be in scope; + * - **super-constructor delegation arguments** -- nothing can precede them; + * - **anything outside an executable body** -- notably a class-body property initializer, which has + * no block to insert into. Converting one to a getter would change compute-once into + * compute-per-access, so it is declined instead. + */ +internal fun isExtractionPosition(element: PsiElement): Boolean { + if (PsiTreeUtil.getParentOfType(element, KtAnnotationEntry::class.java, false) != null) return false + if (PsiTreeUtil.getParentOfType(element, KtSuperTypeListEntry::class.java, false) != null) return false + + val parameter = PsiTreeUtil.getParentOfType(element, KtParameter::class.java, false) + if (parameter != null && parameter.defaultValue?.isAncestorOf(element) == true) return false + + return enclosingExecutableBody(element) != null +} + +/** + * The nearest enclosing thing with a body that can hold statements: a lambda, a named or anonymous + * function, a property accessor, an `init` block, or a constructor. Null when [element] is not + * inside any of them. + */ +internal fun enclosingExecutableBody(element: PsiElement): PsiElement? { + var current: PsiElement? = element + while (current != null && current !is KtFile) { + if (current is KtFunctionLiteral) return current + if (current is KtDeclarationWithBody && current.bodyExpression?.isAncestorOf(element) == true) return current + if (current is KtAnonymousInitializer && current.body?.isAncestorOf(element) == true) return current + current = current.parent + } + return null +} + +private fun PsiElement.isAncestorOf(other: PsiElement): Boolean = PsiTreeUtil.isAncestor(this, other, false) + +/** + * Whether this expression is a thing whose value can be bound to a `val`. + * + * Excluded, and why: + * - blocks, loops, `return`/`throw`/`break`/`continue` -- no useful value to bind; + * - lambdas, literal and wrapper alike -- outside their call site the parameter types are gone; + * - operator tokens and call callees (`foo` in `foo(x)`) -- fragments, not expressions; + * - the selector of a qualified expression (`b` in `a.b`) -- only meaningful with its receiver; + * - the left side of an assignment -- a write target, not a value; + * - `super` -- not a value; + * - **bare literals** (`1`, `"text"`) -- extracting them is pointless, and excluding them removes + * the only case where omitting a type annotation could change meaning (an `Int` literal where a + * `Long` is expected, or a bare `null` inferring `Nothing?`). + */ +internal fun KtExpression.isLegalExtractionTarget(): Boolean { + if (this is KtBlockExpression) return false + if (this is KtLoopExpression) return false + if (this is KtReturnExpression || this is KtThrowExpression) return false + if (this is KtBreakExpression || this is KtContinueExpression) return false + if (this is KtOperationReferenceExpression) return false + if (this is KtSuperExpression) return false + if (this is KtFunctionLiteral) return false + // The wrapper around the literal. A hoisted lambda loses the parameter types its call site was + // supplying, so `{ it.length + 1 }` becomes uncompilable the moment it leaves the call. + if (this is KtLambdaExpression) return false + if (isBareLiteral()) return false + + val parent = parent + if (parent is KtQualifiedExpression && parent.selectorExpression === this) return false + if (parent is KtCallExpression && parent.calleeExpression === this) return false + if (parent is KtBinaryExpression && + parent.operationToken == KtTokens.EQ && + parent.left === this + ) { + return false + } + return true +} + +/** A numeric/boolean/char/null literal, or a string with no interpolation. */ +private fun KtExpression.isBareLiteral(): Boolean = + when (this) { + is KtConstantExpression -> true + is KtStringTemplateExpression -> entries.all { it.isLiteralEntry() } + else -> false + } + +private fun KtStringTemplateEntry.isLiteralEntry(): Boolean = this is KtLiteralStringTemplateEntry diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt new file mode 100644 index 0000000000..2eed7a334d --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEdit.kt @@ -0,0 +1,274 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.models.TextEdit +import com.itsaky.androidide.models.Position +import com.itsaky.androidide.models.Range + +/** + * The one text replacement an extraction performs: replace `[span]` with [newText]. + * + * **Deliberately a single replacement, not a list of edits.** `IDELanguageClientImpl.applyActionEdits` + * applies each `TextEdit` in its own `runOnUiThread` with no `beginBatchEdit`, and every range is + * computed against the *original* text -- so a list of N edits would be applied against positions + * already shifted by its predecessors, and would cost the user N undo steps with a typing window + * between each. Rewriting one contiguous span sidesteps all of it. + */ +data class RewriteSpan( + val span: TextSpan, + val newText: String, +) + +/** + * Builds the extraction rewrite, or null when the inputs cannot produce one. + * + * [name] is the final variable name -- the caller has already validated it. [replaceAll] selects + * between every occurrence in [scope] and only [candidateSpan]. + * + * Occurrences are substituted right-to-left within the rewritten span so earlier substitutions + * cannot shift later offsets, and the whole span is emitted as one replacement. + */ +fun buildExtractVariableRewrite( + fileText: String, + candidateSpan: TextSpan, + scope: ScopeOption, + name: String, + replaceAll: Boolean, +): RewriteSpan? { + val targets = + (if (replaceAll) scope.occurrences else listOf(candidateSpan)) + .sortedBy { it.start } + .takeIf { it.isNotEmpty() } ?: return null + // Only targets are bounds-checked against fileText; contentSpan/statementSpans are trusted + // unchecked. That is safe only because fileText is the plan's own text, not the live document -- + // if a caller ever passed live text here instead, those spans would need the same check. + if (targets.any { it.end > fileText.length }) return null + + val expression = fileText.substring(candidateSpan.start, candidateSpan.end) + val declaration = "val $name = $expression" + + return when (val form = scope.anchorForm) { + is AnchorForm.ExistingBlock -> existingBlockRewrite(fileText, form, targets, declaration, name) + is AnchorForm.WrapInBraces -> wrapInBracesRewrite(fileText, form, targets, declaration, name) + is AnchorForm.ConvertExpressionBody -> convertExpressionBodyRewrite(fileText, form, targets, declaration, name) + } +} + +/** + * Inserts the declaration as its own line before the anchor statement, and rewrites everything from + * there through the last occurrence. + * + * The anchor is the statement *of this scope* that holds the first served occurrence, so picking an + * outer rung hoists the declaration above the enclosing statement rather than leaving it where the + * inner rung would have put it. The rewritten span starts at that statement's line start so the + * declaration lands on a line of its own at the right indentation, and ends at the last occurrence so + * untouched trailing code is left alone. + * + * Null when no statement of the scope contains the occurrence, which would mean the plan and the text + * disagree; the caller reports that rather than guessing. + */ +private fun existingBlockRewrite( + fileText: String, + form: AnchorForm.ExistingBlock, + targets: List, + declaration: String, + name: String, +): RewriteSpan? { + val first = targets.first() + val last = targets.last() + val anchor = form.statementSpans.firstOrNull { it.start <= first.start && first.end <= it.end } ?: return null + val lineStart = lineStartOffset(fileText, anchor.start) + + // A block written on one line needs the declaration expanded inside the braces instead of hoisted + // above the line. `contentSpan.start` is not a reliable signal by itself: a lambda body's block + // does not own its braces, so `contentSpan.start` sits at the body's first token even when that + // token starts its own line -- comparing it to `lineStart` alone would misfire on an ordinary + // multi-line lambda. Two conditions together are what actually mean "one line": something other + // than indentation already precedes the statement on its line (the brace, a header, or a prior + // semicolon-separated statement), *and* the block's content itself contains no newline (so + // re-emitting it as a single line loses nothing). + val linePrefix = fileText.substring(lineStart, anchor.start) + val contentIsOneLine = !fileText.substring(form.contentSpan.start, form.contentSpan.end).contains('\n') + if (linePrefix.isNotBlank() && contentIsOneLine) { + return oneLineBlockRewrite(fileText, form, targets, declaration, name) + } + + // A lambda body's content starts right at its first token with no owned whitespace, so `lineStart` + // sits before `contentSpan.start` on plain indentation alone -- that gap must not trigger a + // decline. What does mean "outside the block" is *real code* in that gap: the block's own opening + // delimiter (a call and its brace, a header) sharing the anchor's line, which only happens for the + // multi-line case the one-line check above did not catch. Anchoring there would put the + // declaration before that delimiter, outside the scope the user picked. Declining is safe; hoisting + // is not. + if (form.contentSpan.start > lineStart && fileText.substring(lineStart, form.contentSpan.start).isNotBlank()) { + return null + } + + val indent = leadingIndentAt(fileText, anchor.start) + val newline = detectNewline(fileText) + + val span = TextSpan(lineStart, last.end) + val body = replaceOccurrences(fileText, span, targets, name) + return RewriteSpan(span = span, newText = indent + declaration + newline + body) +} + +/** + * Puts the declaration inside a block written on one line, moving the block's content and its closing + * brace onto their own lines. + * + * Only the content between the braces is rewritten: the braces, and a lambda's `param ->` header, + * stay exactly where they are, so the expansion cannot disturb the call around it. + */ +private fun oneLineBlockRewrite( + fileText: String, + form: AnchorForm.ExistingBlock, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val content = form.contentSpan + val newline = detectNewline(fileText) + val indent = leadingIndentAt(fileText, content.start) + val innerIndent = indent + detectIndentUnit(fileText) + + // A block that does not own its braces (a lambda body) stops short of them, leaving a single + // space between the content span and the brace on each side. Widen the replaced span over that + // gap so it does not survive the rewrite as a stray "{ " or " }". + val span = TextSpan(startOfWhitespaceBefore(fileText, content.start), endOfWhitespaceAfter(fileText, content.end)) + val body = replaceOccurrences(fileText, content, targets, name).trim() + + val newText = + buildString { + append(newline) + append(innerIndent).append(declaration).append(newline) + append(innerIndent).append(body).append(newline) + append(indent) + } + return RewriteSpan(span = span, newText = newText) +} + +/** Wraps a braceless statement in a block containing the declaration and the original statement. */ +private fun wrapInBracesRewrite( + fileText: String, + form: AnchorForm.WrapInBraces, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + // Occurrences in a braceless scope are confined to the statement itself (the frame's search + // range *is* this span), so no cross-span targets are possible; replaceOccurrences filters anyway. + val span = TextSpan(form.bodyStart, form.bodyEnd) + val newline = detectNewline(fileText) + val body = replaceOccurrences(fileText, span, targets, name) + + val newText = + buildString { + append('{').append(newline) + append(form.innerIndent).append(declaration).append(newline) + append(form.innerIndent).append(body).append(newline) + append(form.indent).append('}') + } + return RewriteSpan(span, newText) +} + +/** Converts `= expr` into a block body holding the declaration and a `return` of the rewritten body. */ +private fun convertExpressionBodyRewrite( + fileText: String, + form: AnchorForm.ConvertExpressionBody, + targets: List, + declaration: String, + name: String, +): RewriteSpan { + val bodySpan = TextSpan(form.bodyStart, form.bodyEnd) + val newline = detectNewline(fileText) + val body = replaceOccurrences(fileText, bodySpan, targets, name) + val returned = if (form.needsReturn) "return $body" else body + + // Writing a type means rewriting from the end of the signature, not from the `=`: starting at the + // `=` would leave the space in front of it and emit `fun area(r: Int) : Int {`. + val spanStart = + if (form.returnTypeText == null) form.assignStart else startOfWhitespaceBefore(fileText, form.assignStart) + val header = form.returnTypeText?.let { ": $it " } ?: "" + + val newText = + buildString { + append(header).append('{').append(newline) + append(form.innerIndent).append(declaration).append(newline) + append(form.innerIndent).append(returned).append(newline) + append(form.indent).append('}') + } + return RewriteSpan(TextSpan(spanStart, form.bodyEnd), newText) +} + +/** The offset where the run of whitespace ending at [offset] begins. */ +private fun startOfWhitespaceBefore( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index > 0 && text[index - 1].isWhitespace()) index-- + return index +} + +/** The offset where the run of whitespace starting at [offset] ends. */ +private fun endOfWhitespaceAfter( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index < text.length && text[index].isWhitespace()) index++ + return index +} + +/** + * Returns `[span]`'s text with every occurrence inside it replaced by [name]. Substitutes + * right-to-left so an earlier replacement cannot invalidate a later offset. + */ +private fun replaceOccurrences( + fileText: String, + span: TextSpan, + targets: List, + name: String, +): String { + val builder = StringBuilder(fileText.substring(span.start, span.end)) + targets + .filter { it.start >= span.start && it.end <= span.end } + .sortedByDescending { it.start } + .forEach { builder.replace(it.start - span.start, it.end - span.start, name) } + return builder.toString() +} + +/** CRLF only when the file already uses it, so the edit does not mix line endings. */ +internal fun detectNewline(text: String): String = if (text.contains("\r\n")) "\r\n" else "\n" + +/** + * Converts a [RewriteSpan] into the `TextEdit` the language client consumes. [Position] carries + * line, column *and* index; all three are filled so neither the client's line/column path nor any + * index-based consumer sees a stale value. + */ +fun RewriteSpan.toTextEdit(fileText: String): TextEdit = + TextEdit( + range = + Range( + positionAt(fileText, span.start), + positionAt(fileText, span.end), + ), + newText = newText, + ) + +internal fun positionAt( + text: String, + offset: Int, +): Position { + val clamped = offset.coerceIn(0, text.length) + var line = 0 + var lineStart = 0 + var i = 0 + while (i < clamped) { + if (text[i] == '\n') { + line++ + lineStart = i + 1 + } + i++ + } + return Position(line, clamped - lineStart, clamped) +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt new file mode 100644 index 0000000000..5f25af3e5c --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanner.kt @@ -0,0 +1,182 @@ +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 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.symbols.KaCallableSymbol +import org.jetbrains.kotlin.analysis.api.types.KaType +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.psi.KtCallableDeclaration +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtPropertyAccessor +import org.slf4j.LoggerFactory +import java.nio.file.Path + +private val logger = LoggerFactory.getLogger("ExtractVariablePlanner") + +/** + * Computes the whole [ExtractionPlan] in one background analysis pass. + * + * The current [KtFile] is fetched *before* entering [read] -- blocking on + * `getCurrentKtFile(...).get()` inside `project.read` deadlocks. + * + * Returns an empty plan both when there is genuinely nothing to extract and whenever anything in + * this pipeline throws: the action framework only catches [IllegalArgumentException] and this runs on + * a scope with no exception handler, so an uncaught throw would crash the app. Degrading to an empty + * plan is always safe -- the action reports "nothing to extract" instead of rewriting anything. + */ +internal fun buildExtractionPlan( + env: AbstractCompilationEnvironment, + nioPath: Path, + selectionStart: Int, + selectionEnd: Int, + documentVersion: Int, + cancelChecker: ScheduledCancelChecker, +): ExtractionPlan = + runCatching { + val ktFile = env.ktSymbolIndex.getCurrentKtFile(nioPath).get() ?: return ExtractionPlan.empty() + env.project.read { + val syntax = candidateExpressionsAt(ktFile, selectionStart, selectionEnd) + if (syntax.expressions.isEmpty()) return@read ExtractionPlan.empty(ktFile.text, documentVersion) + + analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { + val candidates = syntax.expressions.mapNotNull { candidateFor(it) } + ExtractionPlan( + fileText = ktFile.text, + documentVersion = documentVersion, + candidates = candidates, + // Only meaningful while the innermost candidate survived filtering; otherwise the + // user's selection no longer corresponds to the first option shown. + selectionMatchedCandidate = + syntax.selectionMatchedInnermost && + candidates.firstOrNull()?.span?.start == + syntax.expressions + .first() + .textRange.startOffset, + ) + } + } + }.getOrElse { error -> + logger.warn("Failed to build extract-variable plan for {}", nioPath, error) + ExtractionPlan.empty() + } + +/** + * Turns one syntactic candidate into a [CandidateExpression], or null when it should not be offered. + * + * Dropped when the expression produces no useful value (`Unit`, `Nothing` -- `val u = println(x)` + * compiles but is pointless) or when nothing remains of its legal scope chain. + */ +@OptIn(KaExperimentalApi::class) +private fun KaSession.candidateFor(expression: KtExpression): CandidateExpression? { + val type = runCatching { expression.expressionType }.getOrNull() + if (type == null || isValuelessType(type)) return null + + val frames = truncateAtCeiling(enclosingScopeFrames(expression), referencedDeclarationCeiling(expression)) + if (frames.isEmpty()) return null + + val span = TextSpan(expression.textRange.startOffset, expression.textRange.endOffset) + val file = expression.containingKtFile + val scopes = frames.mapNotNull { scopeOptionFor(expression, span, it, file) } + if (scopes.isEmpty()) return null + val takenNames = visibleNamesAt(expression) + + return CandidateExpression( + label = collapseForLabel(expression.text), + span = span, + suggestedName = suggestVariableName(expression, runCatching { renderName(type) }.getOrNull(), takenNames), + takenNames = takenNames, + scopes = scopes, + ) +} + +/** + * Builds one scope option, resolving its occurrence set and fixing up expression-body details. + * + * Returns null when the rung cannot be honoured: converting an expression body whose return type is + * neither declared nor renderable would emit a block body that does not compile, and declining is + * always safe -- the decline-rather-than-rewrite principle that ADR 0013 records, landing alongside + * extract method (ADFA-5080). + */ +private fun KaSession.scopeOptionFor( + expression: KtExpression, + span: TextSpan, + frame: ScopeFrame, + file: KtFile, +): ScopeOption? { + val matches = findOccurrences(expression, frame.scopeElement, frame.searchRange) + val writes = writeOffsetsFor(expression, frame.scopeElement) + val occurrences = excludeUnsoundOccurrences(matches, span, writes) + + val anchorForm = + when (val form = frame.anchorForm) { + is AnchorForm.ConvertExpressionBody -> { + val declaration = frame.scopeElement.parent as? KtDeclarationWithBody + val needsReturn = expressionBodyNeedsReturn(frame.scopeElement) + val returnTypeText = + if (needsReturn && declaration != null && !declaration.declaresReturnType()) { + returnTypeTextOf(declaration, file) ?: return null + } else { + null + } + form.copy(needsReturn = needsReturn, returnTypeText = returnTypeText) + } + + else -> { + form + } + } + + return ScopeOption(label = frame.label, anchorForm = anchorForm, occurrences = occurrences) +} + +/** Whether the declaration spells its return type out, in which case nothing needs writing. */ +private fun KtDeclarationWithBody.declaresReturnType(): Boolean = + when (this) { + // KtPropertyAccessor.returnTypeReference is deprecated in favour of the identical typeReference. + is KtPropertyAccessor -> typeReference != null + + is KtCallableDeclaration -> typeReference != null + + else -> false + } + +/** The declaration's resolved return type, or null when it cannot be resolved. */ +private fun KaSession.returnTypeOf(declaration: KtDeclarationWithBody): KaType? = + runCatching { (declaration.symbol as? KaCallableSymbol)?.returnType }.getOrNull() + +/** The declaration's return type as source text, shortened where the file can resolve it. */ +private fun KaSession.returnTypeTextOf( + declaration: KtDeclarationWithBody, + file: KtFile, +): String? { + val type = returnTypeOf(declaration) ?: return null + val rendered = renderedTypeTextOrNull(type) ?: return null + return shortenTypeText(rendered, importedNamesOf(file), starImportedPackagesOf(file)) +} + +/** + * Whether converting an expression body to a block body needs a `return`. + * + * False only for a `Unit`-returning function, where `return expr` on a non-`Unit` expression would + * not compile and is unnecessary anyway. `Nothing` is deliberately not folded in here even though + * [isValuelessType] treats it like `Unit` for the R4 candidate filter -- a `Nothing`-returning + * function needs its `return` and its written-out type kept, or a caller using it in a `Nothing` + * position (`x ?: boom()`) stops compiling. Defaults to true, which is right for everything else + * including property accessors. + */ +private fun KaSession.expressionBodyNeedsReturn(bodyExpression: PsiElement): Boolean { + val declaration = bodyExpression.parent as? KtDeclarationWithBody ?: return true + val returnType = returnTypeOf(declaration) ?: return true + return !runCatching { returnType.isUnitType }.getOrDefault(false) +} + +/** `Unit` and `Nothing` carry no value worth binding to a `val`. */ +private fun KaSession.isValuelessType(type: KaType): Boolean = runCatching { type.isUnitType || type.isNothingType }.getOrDefault(false) 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 new file mode 100644 index 0000000000..be948e179b --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractionPlan.kt @@ -0,0 +1,177 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** A half-open offset range `[start, end)` into the analysed file's text. */ +data class TextSpan( + val start: Int, + val end: Int, +) { + init { + require(start <= end) { "start=$start > end=$end" } + } + + val length: Int get() = end - start + + fun overlaps(other: TextSpan): Boolean = start < other.end && other.start < end +} + +/** + * How the new declaration is woven into an anchor scope. Kotlin scopes are not all blocks, so + * three shapes are needed; [ExistingBlock] is by far the common one. + */ +sealed interface AnchorForm { + /** + * The scope already has a `{ ... }` body (function body, `if` block, lambda body, ...), so the + * declaration is a new statement line inside it. + * + * [statementSpans] are the block's direct child statements, ascending. The anchor point is the + * first of them containing the first served occurrence -- which is what makes an outer rung differ + * from an inner one. Anchoring on the occurrence's own line instead would make every rung of a + * chain produce the same edit. + * + * [contentSpan] is the region *inside* the braces. It tells a block written on one line + * (`items.map { it.length + 1 }`) from a multi-line one, where inserting at the statement's line + * start would put the declaration outside the braces. + */ + data class ExistingBlock( + val contentSpan: TextSpan, + val statementSpans: List, + ) : AnchorForm + + /** + * A braceless statement position -- `if (c) foo()`, a `when` entry, a braceless loop body. + * `[bodyStart, bodyEnd)` (the statement) is replaced by a braced block holding the declaration + * and the original statement. No `return` is involved. + */ + data class WrapInBraces( + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, + ) : AnchorForm + + /** + * An expression-bodied function or property accessor -- `fun area(r: Int) = r * r`. The `=` and + * the body are replaced by a block body. [needsReturn] is false only when the declaration + * returns `Unit`, where `return` is both unnecessary and wrong for a non-`Unit` expression. + * + * [returnTypeText] is the type to write into the signature, or null when there is nothing to write + * -- the declaration already spells its type out, or the block body infers `Unit` anyway. A block + * body with no declared type returns `Unit`, so `return ` without this would not compile. + */ + data class ConvertExpressionBody( + val assignStart: Int, + val bodyStart: Int, + val bodyEnd: Int, + val indent: String, + val innerIndent: String, + val needsReturn: Boolean, + val returnTypeText: String? = null, + ) : AnchorForm +} + +/** + * One member of a candidate's legal scope chain: a place the declaration may go, together with the + * occurrences that are sound to replace there. + * + * [occurrences] is ascending by offset and always contains the candidate's own span, so + * `occurrences.size` is the count shown as "Replace all N occurrences". Narrowing to an inner scope + * can only shrink this set, never grow it. + */ +data class ScopeOption( + val label: String, + val anchorForm: AnchorForm, + val occurrences: List, +) + +/** + * A legal extraction target and everything the UI needs to act on it. + * + * [label] is the expression's source text with runs of whitespace collapsed, so a multi-line + * expression stays readable in a one-line list item. + * + * [scopes] is the legal scope chain, innermost first, and is never empty -- a candidate with no + * legal anchor is not a candidate. + */ +data class CandidateExpression( + val label: String, + val span: TextSpan, + val suggestedName: String, + val takenNames: Set, + val scopes: List, +) + +/** + * The complete result of the background analysis pass, and the central type of the extract/inline + * refactorings. + * + * ## Vocabulary + * + * Used verbatim throughout this package, its tests and its review comments -- prefer these over + * ad-hoc synonyms. + * + * - **Candidate expression** -- a [org.jetbrains.kotlin.psi.KtExpression] at the cursor or selection + * that is a legal extraction target. At most [MAX_CANDIDATES], ordered innermost-first. + * - **Legal scope chain** -- the ordered anchors available for the new declaration: outward from the + * candidate's own statement through enclosing blocks, crossing a lambda boundary only when nothing + * lambda-scoped is referenced, and stopping at the enclosing method body. + * - **Anchor scope** -- the chain member the user picked. The `val` is declared inside it. + * - **Anchor point** -- the exact insertion offset: the start of the line holding the first statement + * *within the anchor scope* that contains a replaced occurrence, or inside the braces when that + * statement shares its line with a block written on one line. + * - **Occurrence** -- a site inside the anchor scope that is structurally equal to the candidate *and* + * whose every name reference resolves to the same symbol. Sites made unsound by an intervening + * reassignment are excluded, so an occurrence set is always safe to replace wholesale. + * - **Extraction plan** -- this type. + * + * ## Why plain data + * + * The user's choices (which expression, what name, which scope, replace-all or not) arrive *after* + * analysis, from a sheet. Rather than re-entering analysis on confirm, one background pass produces + * this plan for *all* candidates at once and the UI does pure string/offset arithmetic on it. That + * keeps PSI off the UI thread, removes the stale-PSI window, and makes the whole derivation + * unit-testable without an editor, an activity or Compose. + * + * [fileText] is the text the offsets here refer to, carried so the UI can build the replacement text + * without PSI; [documentVersion] is what makes that safe -- if the live document has moved on by the + * time the user confirms, the plan is discarded rather than applied against shifted offsets. + * + * [selectionMatchedCandidate] is true when the user's selection exactly matched the innermost + * candidate, meaning they already expressed which expression they want and the UI should not ask. + */ +data class ExtractionPlan( + val fileText: String, + val documentVersion: Int, + val candidates: List, + val selectionMatchedCandidate: Boolean, +) { + val isEmpty: Boolean get() = candidates.isEmpty() + + companion object { + fun empty( + fileText: String = "", + documentVersion: Int = -1, + ) = ExtractionPlan(fileText, documentVersion, emptyList(), selectionMatchedCandidate = false) + } +} + +/** + * Collapses whitespace runs so a multi-line expression reads as one line in a list item. + * + * The space before a `.` or `?.` is then removed: a wrapped call chain is the most common multi-line + * expression in Kotlin, and a plain collapse turns `items\n\t.filter { ... }` into + * `items .filter { ... }`, which reads as a typo in a list the user is choosing from. + */ +internal fun collapseForLabel( + text: String, + maxLength: Int = 80, +): String { + val collapsed = + text + .replace(WHITESPACE_RUN, " ") + .replace(SPACE_BEFORE_DOT, "$1") + .trim() + return if (collapsed.length <= maxLength) collapsed else collapsed.take(maxLength - 3) + "..." +} + +private val WHITESPACE_RUN = Regex("\\s+") +private val SPACE_BEFORE_DOT = Regex(" (\\??\\.)") 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 new file mode 100644 index 0000000000..3427571a18 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/NameSuggestion.kt @@ -0,0 +1,155 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.psi.KtArrayAccessExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.KtParenthesizedExpression +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtStringTemplateExpression + +/** Used when neither the expression's shape nor its type suggests anything better. */ +const val FALLBACK_NAME = "value" + +/** + * Kotlin's hard keywords -- the ones that are never valid identifiers. Soft and modifier keywords + * (`by`, `data`, `it`, ...) are legal names and are deliberately absent. + */ +private val HARD_KEYWORDS = + setOf( + "as", + "break", + "class", + "continue", + "do", + "else", + "false", + "for", + "fun", + "if", + "in", + "interface", + "is", + "null", + "object", + "package", + "return", + "super", + "this", + "throw", + "true", + "try", + "typealias", + "typeof", + "val", + "var", + "when", + "while", + ) + +/** Why a proposed name cannot be used. Null-free alternative to throwing for user input. */ +enum class NameProblem { + Blank, + NotAnIdentifier, + Keyword, + AlreadyTaken, +} + +/** + * Validates a user-supplied name against Kotlin's identifier rules and the names already visible at + * the anchor point. Returns null when the name is usable. + * + * Backtick-quoted names are rejected rather than supported: they are legal Kotlin but a poor + * suggestion for a generated local, and accepting them would mean validating the quoted form too. + */ +fun validateVariableName( + name: String, + takenNames: Set, +): NameProblem? { + if (name.isBlank()) return NameProblem.Blank + if (!isIdentifier(name)) return NameProblem.NotAnIdentifier + if (name in HARD_KEYWORDS) return NameProblem.Keyword + if (name in takenNames) return NameProblem.AlreadyTaken + return null +} + +private fun isIdentifier(name: String): Boolean { + if (name.isEmpty()) return false + if (!(name[0].isLetter() || name[0] == '_')) return false + return name.all { it.isLetterOrDigit() || it == '_' } +} + +/** + * Suggests a name for the value [expression] produces. + * + * Tried in order: + * 1. **The expression's shape** -- `items.size` -> `size`, `a.b.c()` -> `c`, `getFoo()` -> `foo`, + * `foo(x)` -> `foo`, an interpolated string -> `text`, `xs[i]` -> `xs` element naming. + * 2. **The resolved type**, lowercased -- `List` -> `list`, `Duration` -> `duration`. Pass null + * when the type is unavailable. + * 3. [FALLBACK_NAME]. + * + * The result is then made unique against [takenNames] by appending `1`, `2`, ... Shape beats type + * because `size`, `count` and `name` are far better names than `int` and `string`, and type-derived + * names collide constantly. + */ +fun suggestVariableName( + expression: KtExpression, + typeName: String?, + takenNames: Set, +): String { + val base = + nameFromShape(expression) + ?: typeName?.let(::nameFromType) + ?: FALLBACK_NAME + val sanitised = base.takeIf { isIdentifier(it) && it !in HARD_KEYWORDS } ?: FALLBACK_NAME + return makeUnique(sanitised, takenNames) +} + +private fun nameFromShape(expression: KtExpression): String? = + when (expression) { + is KtParenthesizedExpression -> expression.expression?.let(::nameFromShape) + is KtQualifiedExpression -> expression.selectorExpression?.let(::nameFromShape) + is KtCallExpression -> (expression.calleeExpression as? KtNameReferenceExpression)?.getReferencedName()?.let(::stripAccessorPrefix) + is KtNameReferenceExpression -> expression.getReferencedName().let(::stripAccessorPrefix) + is KtStringTemplateExpression -> "text" + is KtArrayAccessExpression -> expression.arrayExpression?.let(::nameFromShape) + else -> null + }?.takeIf { it.isNotBlank() } + +/** `getFoo` -> `foo`, `isReady` -> `ready`. Leaves anything else alone. */ +private fun stripAccessorPrefix(name: String): String { + for (prefix in ACCESSOR_PREFIXES) { + if (name.length > prefix.length && + name.startsWith(prefix) && + name[prefix.length].isUpperCase() + ) { + return name.substring(prefix.length).decapitaliseFirst() + } + } + return name +} + +private val ACCESSOR_PREFIXES = listOf("get", "is", "has") + +/** `List` -> `list`, `kotlin.time.Duration` -> `duration`, `Array` -> `array`. */ +private fun nameFromType(typeName: String): String? = + typeName + .substringBefore('<') + .substringAfterLast('.') + .trimEnd('?', '!') + .takeIf { it.isNotBlank() } + ?.decapitaliseFirst() + +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( + base: String, + takenNames: Set, +): String { + if (base !in takenNames) return base + var suffix = 1 + while ("$base$suffix" in takenNames) suffix++ + return "$base$suffix" +} 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 new file mode 100644 index 0000000000..61eea683ae --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/Occurrences.kt @@ -0,0 +1,273 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.analysis.api.KaSession +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.builtins.StandardNames +import org.jetbrains.kotlin.com.intellij.psi.PsiComment +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.idea.references.mainReference +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.KtUnaryExpression +import org.jetbrains.kotlin.psi.psiUtil.parents + +/** + * Whether [a] and [b] are the same expression for extraction purposes: structurally identical *and* + * every name reference in them resolving to the same declaration. + * + * The symbol check is the whole point. Text or structure alone would happily match `config.timeout` + * inside a nested lambda where `config` is a different `config`, or an `it` that means something + * else -- replacing those would silently change behaviour. The parent ticket (ADFA-3324) states the + * standard outright: text-based matching breaks things. + */ +internal fun KaSession.isSameExpression( + a: PsiElement, + b: PsiElement, +): Boolean { + if (a === b) return true + if (a.node?.elementType != b.node?.elementType) return false + + if (a is KtSimpleNameExpression && b is KtSimpleNameExpression) { + if (a.getReferencedName() != b.getReferencedName()) return false + if (!resolvesToSameDeclaration(a, b)) return false + } + + val childrenA = meaningfulChildren(a) + val childrenB = meaningfulChildren(b) + if (childrenA.size != childrenB.size) return false + if (childrenA.isEmpty()) return a.text == b.text + return childrenA.indices.all { isSameExpression(childrenA[it], childrenB[it]) } +} + +/** Whitespace and comments are formatting, not structure, so they never affect equality. */ +private fun meaningfulChildren(element: PsiElement): List = + element.children.filter { it !is PsiWhiteSpace && it !is PsiComment } + +/** + * Whether two same-named references point at the same declaration. + * + * Source declarations are compared by PSI identity, which is exactly the question being asked ("the + * same `val`?"). Symbols without source PSI -- library members, compiler-generated declarations -- + * fall back to symbol equality. Resolution over broken code throws, and a throw here must read as + * "not the same" rather than crash the action. + */ +private fun KaSession.resolvesToSameDeclaration( + a: KtSimpleNameExpression, + b: KtSimpleNameExpression, +): Boolean = + runCatching { + val symbolA = a.mainReference?.resolveToSymbols()?.firstOrNull() ?: return false + val symbolB = b.mainReference?.resolveToSymbols()?.firstOrNull() ?: return false + val psiA = symbolA.declarationPsi() + val psiB = symbolB.declarationPsi() + if (psiA != null || psiB != null) psiA === psiB else symbolA == symbolB + }.getOrDefault(false) + +private fun KaSymbol.declarationPsi(): PsiElement? = runCatching { psi }.getOrNull() + +/** + * Every site in [searchRoot] within [searchRange] that is the same expression as [candidate] and is + * itself a legal place to put the variable reference. + * + * The legality filter matters: in `a.a`, a candidate of `a` matches the selector too, but rewriting + * a selector would produce `v.v`. Overlapping matches are dropped so no site is rewritten twice. + * Ascending by offset, and always contains [candidate] itself. + */ +internal fun KaSession.findOccurrences( + candidate: KtExpression, + searchRoot: PsiElement, + searchRange: TextSpan, +): List { + val elementType = candidate.node?.elementType + val matches = + PsiTreeUtil + .collectElements(searchRoot) { element -> + element.node?.elementType == elementType && + element is KtExpression && + element.textRange.startOffset >= searchRange.start && + element.textRange.endOffset <= searchRange.end + }.filterIsInstance() + .filter { it === candidate || (it.isLegalExtractionTarget() && isSameExpression(candidate, it)) } + .map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) } + .sortedBy { it.start } + + val accepted = mutableListOf() + for (match in matches) { + if (accepted.none { it.overlaps(match) }) accepted += match + } + return accepted +} + +/** + * The innermost scope that must contain the declaration, or null when the candidate references + * nothing declared inside the enclosing scopes. + * + * This is what stops a hoist from escaping a lambda it depends on: if the candidate uses `it` or a + * lambda parameter, that lambda's body comes back as the ceiling and every outer rung of the scope + * chain is dropped by [truncateAtCeiling]. + */ +internal fun KaSession.referencedDeclarationCeiling(candidate: KtExpression): PsiElement? { + var deepest: PsiElement? = null + var deepestDepth = -1 + for (reference in candidate.collectDescendantsOfType()) { + val symbol = runCatching { reference.mainReference?.resolveToSymbols()?.firstOrNull() }.getOrNull() ?: continue + val body = constrainingBodyFor(reference, symbol) ?: continue + val depth = depthOf(body) + if (depth > deepestDepth) { + deepest = body + deepestDepth = depth + } + } + return deepest +} + +/** + * The scope [reference] pins the declaration inside, or null when it constrains nothing. + * + * 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. + */ +private fun constrainingBodyFor( + reference: KtSimpleNameExpression, + symbol: KaSymbol, +): PsiElement? { + val declaration = runCatching { symbol.psi }.getOrNull() + if (declaration == null) { + if (symbol is KaValueParameterSymbol && reference.getReferencedName() == StandardNames.IMPLICIT_LAMBDA_PARAMETER_NAME.asString()) { + return PsiTreeUtil.getParentOfType(reference, KtFunctionLiteral::class.java, true)?.bodyExpression + } + return null + } + if (!PsiTreeUtil.isAncestor(reference.containingFile, declaration, false)) return null + return enclosingExecutableBody(declaration) +} + +private fun depthOf(element: PsiElement): Int = element.parents.count() + +private inline fun PsiElement.collectDescendantsOfType(): List = + PsiTreeUtil.collectElementsOfType(this, T::class.java).toList() + +/** + * Restricts [occurrences] to a contiguous run around [candidateSpan] that no write to a referenced + * mutable interrupts. + * + * A `var` the candidate reads can be reassigned between two occurrences, and then the two sites do + * not hold the same value even though they are the same expression: + * + * ``` + * var limit = 1 + * foo(limit + 1) // occurrence + * limit = 5 + * foo(limit + 1) // same expression, different value + * ``` + * + * Rather than warn, unsound sites are simply excluded, so "Replace all N occurrences" can never + * produce wrong code and N is always achievable. The walk grows outwards from the candidate -- never + * dropping the site the user actually selected -- and stops in each direction at the first write it + * would have to cross. + */ +internal fun excludeUnsoundOccurrences( + occurrences: List, + candidateSpan: TextSpan, + writeOffsets: List, +): List { + if (occurrences.isEmpty()) return occurrences + val ordered = occurrences.sortedBy { it.start } + val candidateIndex = ordered.indexOfFirst { it.start == candidateSpan.start && it.end == candidateSpan.end } + if (candidateIndex < 0) return listOf(candidateSpan) + + val writes = writeOffsets.sorted() + + fun writeBetween( + from: Int, + to: Int, + ): Boolean = writes.any { it in from until to } + + val accepted = mutableListOf(ordered[candidateIndex]) + for (i in candidateIndex - 1 downTo 0) { + if (writeBetween(ordered[i].end, ordered[candidateIndex].start)) break + accepted.add(0, ordered[i]) + } + for (i in candidateIndex + 1 until ordered.size) { + if (writeBetween(ordered[candidateIndex].end, ordered[i].start)) break + accepted += ordered[i] + } + return accepted +} + +/** + * Offsets of writes, within [searchRoot], to any mutable the candidate reads. Feeds + * [excludeUnsoundOccurrences]. + * + * Counts plain assignment, the augmented forms (`+=` and friends) and `++`/`--`. A `val` cannot be + * written, so only [KaVariableSymbol]s that report themselves mutable are tracked. + */ +internal fun KaSession.writeOffsetsFor( + candidate: KtExpression, + searchRoot: PsiElement, +): List { + val mutableDeclarations = + candidate + .collectDescendantsOfType() + .mapNotNull { reference -> + runCatching { + (reference.mainReference?.resolveToSymbols()?.firstOrNull() as? KaVariableSymbol) + ?.takeIf { !it.isVal } + ?.psi + }.getOrNull() + }.toSet() + if (mutableDeclarations.isEmpty()) return emptyList() + + return searchRoot + .collectDescendantsOfType() + .filter { it.isWriteTarget() } + .filter { reference -> + runCatching { + reference.mainReference + ?.resolveToSymbols() + ?.firstOrNull() + ?.psi + }.getOrNull() in mutableDeclarations + }.map { it.textRange.startOffset } +} + +/** Whether this reference is being written to rather than read. */ +private 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 + return false +} + +private val ASSIGNMENT_TOKENS = + setOf(KtTokens.EQ, KtTokens.PLUSEQ, KtTokens.MINUSEQ, KtTokens.MULTEQ, KtTokens.DIVEQ, KtTokens.PERCEQ) + +private val INCREMENT_TOKENS = setOf(KtTokens.PLUSPLUS, KtTokens.MINUSMINUS) + +/** + * Names a suggestion must avoid: every declaration name in the file. + * + * Deliberately conservative rather than scope-exact. A real scope query would need resolution and + * would let `size` be suggested in one function because the collision is in another -- correct, but + * the cost of being over-broad is only a `size1` where `size` would have done, while the cost of + * being under-broad is generated code that shadows something. Cheap, needs no analysis, and being + * purely syntactic it is unit-testable. + */ +internal fun visibleNamesAt(candidate: KtExpression): Set = + PsiTreeUtil + .collectElementsOfType(candidate.containingFile, KtDeclaration::class.java) + .mapNotNullTo(mutableSetOf()) { it.name } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt new file mode 100644 index 0000000000..d89c570e5a --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ScopeChain.kt @@ -0,0 +1,293 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtAnonymousInitializer +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtContainerNodeForControlStructureBody +import org.jetbrains.kotlin.psi.KtDeclarationWithBody +import org.jetbrains.kotlin.psi.KtDoWhileExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtForExpression +import org.jetbrains.kotlin.psi.KtFunctionLiteral +import org.jetbrains.kotlin.psi.KtIfExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtPropertyAccessor +import org.jetbrains.kotlin.psi.KtWhenEntry +import org.jetbrains.kotlin.psi.KtWhileExpression + +/** + * One rung of the legal scope chain, before occurrences are known. + * + * [scopeElement] is the PSI node that *is* the scope, used to decide whether a referenced + * declaration lives inside it (see [truncateAtCeiling]). [searchRange] bounds the occurrence search + * for this rung. + */ +data class ScopeFrame( + val label: String, + val scopeElement: PsiElement, + val searchRange: TextSpan, + val anchorForm: AnchorForm, +) + +/** + * Enumerates the scopes [candidate] could be hoisted into, innermost first. + * + * Walks outward from the candidate's own statement. Each rung is one of the three [AnchorForm] + * shapes: a real block, a braceless statement position that needs braces, or an expression body that + * needs converting. The walk stops after the enclosing **named function, accessor or `init` block** + * body -- the ceiling agreed for this refactoring. A class body or file is never an anchor, so a + * property initializer outside any executable body yields nothing (already rejected earlier by + * [isExtractionPosition]). + * + * Lambda boundaries are *crossed* here: whether crossing is actually legal depends on what the + * candidate references, which needs resolution, so it is applied afterwards by [truncateAtCeiling]. + */ +fun enclosingScopeFrames(candidate: KtExpression): List { + val text = candidate.containingFile.text + val frames = mutableListOf() + var inner: PsiElement = candidate + + while (true) { + val parent = inner.parent ?: break + if (parent is KtFile) break + + val frame = frameFor(inner, text) + if (frame == null) { + // Most nodes are not themselves anchorable -- a value argument, an argument list, a lambda + // literal. Keep climbing rather than stopping, otherwise the chain would end at the first + // such node and, in particular, a candidate inside a lambda could never be hoisted out of + // it even when that is legal. + inner = parent + continue + } + + frames += frame + // A named function / accessor / init body is the ceiling: record it, then stop. + if (isCeilingBody(frame.scopeElement)) break + inner = frame.scopeElement.parent ?: break + } + return frames +} + +/** + * Drops the rungs that lie outside [ceiling] -- the innermost scope holding a declaration the + * candidate references. Passing null keeps the whole chain (nothing scoped inside was referenced). + * + * This is what enforces "crossing a lambda boundary is allowed only when nothing lambda-scoped is + * referenced": if the candidate uses `it` or a lambda parameter, the lambda body *is* the ceiling + * and every outer rung disappears. + */ +fun truncateAtCeiling( + frames: List, + ceiling: PsiElement?, +): List { + if (ceiling == null) return frames + val kept = frames.takeWhile { PsiTreeUtil.isAncestor(ceiling, it.scopeElement, false) || it.scopeElement === ceiling } + return kept.ifEmpty { frames.take(1) } +} + +/** + * Builds the rung whose scope directly contains [inner], or null when [inner] is not in a position + * this refactoring anchors in. + */ +private fun frameFor( + inner: PsiElement, + text: String, +): ScopeFrame? { + val parent = inner.parent ?: return null + + // A braceless control-structure body is wrapped in a container node, so the `if`/loop is the + // grandparent, not the parent. Without unwrapping, no braceless body is ever detected and the + // declaration silently hoists to the enclosing block instead of braces being added. + val controlOwner = (parent as? KtContainerNodeForControlStructureBody)?.parent + + if (parent is KtBlockExpression) { + return ScopeFrame( + label = blockLabel(parent), + scopeElement = parent, + searchRange = parent.textRange.let { TextSpan(it.startOffset, it.endOffset) }, + anchorForm = + AnchorForm.ExistingBlock( + contentSpan = contentSpanOf(parent), + statementSpans = + parent.statements.map { TextSpan(it.textRange.startOffset, it.textRange.endOffset) }, + ), + ) + } + + val bracelessOwner = controlOwner ?: parent + val bracelessLabel = bracelessOwnerLabel(inner, bracelessOwner) + if (bracelessLabel != null) { + val indent = leadingIndentAt(text, bracelessOwner.textRange.startOffset) + val span = TextSpan(inner.textRange.startOffset, inner.textRange.endOffset) + return ScopeFrame( + label = bracelessLabel, + scopeElement = inner, + searchRange = span, + anchorForm = + AnchorForm.WrapInBraces( + bodyStart = span.start, + bodyEnd = span.end, + indent = indent, + innerIndent = indent + detectIndentUnit(text), + ), + ) + } + + if (parent is KtDeclarationWithBody && parent.bodyExpression === inner && !parent.hasBlockBody()) { + val assign = parent.equalsToken ?: return null + val indent = leadingIndentAt(text, parent.textRange.startOffset) + val span = TextSpan(inner.textRange.startOffset, inner.textRange.endOffset) + return ScopeFrame( + label = declarationLabel(parent), + scopeElement = inner, + searchRange = span, + anchorForm = + AnchorForm.ConvertExpressionBody( + assignStart = assign.textRange.startOffset, + bodyStart = span.start, + bodyEnd = span.end, + indent = indent, + innerIndent = indent + detectIndentUnit(text), + // Filled in by the caller, which has the resolved return type. + needsReturn = true, + ), + ) + } + + return null +} + +/** True for the body of a named function, accessor or `init` block -- where the chain stops. */ +private fun isCeilingBody(scopeElement: PsiElement): Boolean { + val owner = scopeElement.parent ?: return false + return when (owner) { + is KtNamedFunction, is KtPropertyAccessor, is KtAnonymousInitializer -> true + else -> false + } +} + +/** + * The name shown for a block rung. + * + * A braceless *or* braced control-structure body is wrapped in a container node, so the `if`/loop is + * the block's grandparent; without unwrapping, every braced branch reads as a generic "block". + * `getThen()`/`getElse()` return the unwrapped body expression, never the container, so branch + * identity is decided by checking if the container's parent matches what `then`/`else` point at + * (by comparing `owner.then?.parent === container`). + */ +private fun blockLabel(block: KtBlockExpression): String { + val parent = block.parent + val container = parent as? KtContainerNodeForControlStructureBody + return when (val owner = container?.parent ?: parent) { + is KtNamedFunction -> "fun ${owner.name ?: ""}" + is KtPropertyAccessor -> if (owner.isGetter) "getter" else "setter" + is KtAnonymousInitializer -> "init block" + is KtFunctionLiteral -> "lambda" + is KtIfExpression -> if (owner.then?.parent === container) "if block" else "else block" + is KtForExpression -> "for loop" + is KtWhileExpression -> "while loop" + is KtDoWhileExpression -> "do-while loop" + is KtWhenEntry -> "when branch" + else -> "block" + } +} + +private fun declarationLabel(declaration: KtDeclarationWithBody): String = + when (declaration) { + is KtNamedFunction -> "fun ${declaration.name ?: ""}" + is KtPropertyAccessor -> if (declaration.isGetter) "getter" else "setter" + else -> "body" + } + +/** A label when [inner] is a braceless body, else null. */ +private fun bracelessOwnerLabel( + inner: PsiElement, + parent: PsiElement, +): String? = + when (parent) { + is KtIfExpression -> { + if (parent.then === inner) { + "if branch" + } else if (parent.`else` === inner) { + "else branch" + } else { + null + } + } + + is KtForExpression -> { + if (parent.body === inner) "for body" else null + } + + is KtWhileExpression -> { + if (parent.body === inner) "while body" else null + } + + is KtDoWhileExpression -> { + if (parent.body === inner) "do-while body" else null + } + + is KtWhenEntry -> { + if (parent.expression === inner) "when branch" else null + } + + else -> { + null + } + } + +/** + * The region inside a block's braces. + * + * A function, `if` or loop body owns its braces, so they are trimmed off. A lambda body block does not + * -- the braces and any `param ->` header belong to the enclosing function literal -- so its own range + * already *is* the content, which is what keeps the header on the brace line when the block is + * expanded. Ownership is decided structurally, by the block's parent, rather than by sniffing the + * block's own text for a leading `{` and trailing `}`: a lambda body whose sole statement is itself a + * lambda literal (`{ x -> { x + 1 } }`) has text that looks brace-owned, and sniffing it would trim off + * that inner lambda's own braces and return its interior instead of the outer body's full content. + */ +internal fun contentSpanOf(block: KtBlockExpression): TextSpan { + val range = block.textRange + return if (block.parent is KtFunctionLiteral) { + TextSpan(range.startOffset, range.endOffset) + } else { + TextSpan(range.startOffset + 1, range.endOffset - 1) + } +} + +/** Offset of the start of the line containing [offset]. */ +internal fun lineStartOffset( + text: String, + offset: Int, +): Int = text.lastIndexOf('\n', (offset - 1).coerceAtLeast(0)).let { if (it < 0) 0 else it + 1 } + +/** The run of spaces/tabs at the start of [offset]'s line. */ +internal fun leadingIndentAt( + text: String, + offset: Int, +): String { + val lineStart = lineStartOffset(text, offset) + return text.substring(lineStart, offset.coerceAtLeast(lineStart)).takeWhile { it == ' ' || it == '\t' } +} + +/** + * One indentation level for [text], inferred from its own lines: a tab if any line is tab-indented, + * otherwise the smallest positive run of leading spaces, defaulting to a tab (the project + * convention). Code-action edits bypass the editor's auto-indent, so emitted text must already match + * the file's style. Mirrors the detection in `ImplementMembersAction`. + */ +internal fun detectIndentUnit(text: String): String { + var minSpaces = Int.MAX_VALUE + for (line in text.splitToSequence('\n')) { + if (line.isEmpty()) continue + if (line[0] == '\t') return "\t" + if (line[0] != ' ') continue + val spaces = line.takeWhile { it == ' ' }.length + if (spaces in 1 until minSpaces) minSpaces = spaces + } + return if (minSpaces == Int.MAX_VALUE) "\t" else " ".repeat(minSpaces) +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt new file mode 100644 index 0000000000..b2b3efbae1 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/TypeText.kt @@ -0,0 +1,112 @@ +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.types.KaFlexibleType +import org.jetbrains.kotlin.analysis.api.types.KaType +import org.jetbrains.kotlin.psi.KtFile + +/** + * Types are rendered **fully qualified** and only then shortened against what the file can resolve. + * + * A short name resolves only when the file imports it or it comes from a default-imported package, and + * a refactoring that adds imports would be a much larger change -- so qualified is the safe starting + * point and [shortenTypeText] gives back readability where it provably costs nothing. + */ +@OptIn(KaExperimentalApi::class) +private val QUALIFIED_TYPE_RENDERER = KaTypeRendererForSource.WITH_QUALIFIED_NAMES + +/** Packages whose simple names resolve with no import at all on the JVM/Android target. */ +private val DEFAULT_IMPORTED_PACKAGES = + setOf( + "kotlin", + "kotlin.annotation", + "kotlin.collections", + "kotlin.comparisons", + "kotlin.io", + "kotlin.jvm", + "kotlin.ranges", + "kotlin.sequences", + "kotlin.text", + "java.lang", + ) + +/** A dotted run of identifiers -- one qualified name inside rendered type text. */ +private val QUALIFIED_NAME = Regex("""[\p{L}_][\p{L}\p{Nd}_]*(?:\.[\p{L}_][\p{L}\p{Nd}_]*)+""") + +/** + * A type that cannot be written out as source -- anonymous, intersection, a resolution error, or a + * platform type the renderer could not reduce (`List`, where the `!` is on a type argument). + * `!` is not Kotlin syntax anywhere, so its presence alone settles it. + * + * The `"anonymous"` and `"ERROR"` substring checks are not unambiguous -- a real type named + * `com.example.AnonymousUser` or `p.ERRORS` would also match. Both fail safe: a false positive only + * declines the rung instead of emitting a block body that does not compile, so the heuristic is left + * as-is rather than made precise. + */ +internal fun isUnrenderableTypeText(text: String): Boolean = + text.isBlank() || + text.contains("anonymous") || + text.contains("ERROR") || + text.contains(" & ") || + text.contains('!') + +/** + * One type as source text, fully qualified, or null when it cannot be written out. + * + * A platform type is unwrapped to its lower bound first: the renderer prints `String!`, which does not + * parse. Only the outermost bound is unwrapped, so a `!` on a type argument still reaches + * [isUnrenderableTypeText]. + * + * Lets a failure from the renderer itself propagate, so a caller that must tell "the renderer threw" + * from "the type is unrenderable" can. [renderedTypeTextOrNull] is the catching form most callers want. + */ +@OptIn(KaExperimentalApi::class) +internal fun KaSession.typeTextOrNull(type: KaType): String? = + renderName((type as? KaFlexibleType)?.lowerBound ?: type, QUALIFIED_TYPE_RENDERER) + .takeUnless(::isUnrenderableTypeText) + +internal fun KaSession.renderedTypeTextOrNull(type: KaType): String? = runCatching { typeTextOrNull(type) }.getOrNull() + +/** + * Replaces each qualified name in [rendered] with its simple name when that name already resolves in + * the file -- because the file imports it exactly, star-imports its package, or it comes from a + * default-imported package. Everything else stays qualified: verbose, but it always compiles. + * + * Purely textual, so it needs no analysis session and is unit-testable on its own. A nested class + * (`com.example.Outer.Inner`) is only shortened by an import of the nested name itself; an import of + * the outer class leaves it alone rather than emitting an unresolvable `Inner`. + * + * A star import is trusted only when nothing else in the file imports the same simple name from a + * different package -- that explicit import would resolve first, so writing the short name here would + * silently name the wrong type. + */ +internal fun shortenTypeText( + rendered: String, + importedNames: Set, + starImportedPackages: Set, +): String = + QUALIFIED_NAME.replace(rendered) { match -> + val qualified = match.value + val container = qualified.substringBeforeLast('.') + val simpleName = qualified.substringAfterLast('.') + val resolvable = + qualified in importedNames || + container in DEFAULT_IMPORTED_PACKAGES || + (container in starImportedPackages && importedNames.none { it.endsWith(".$simpleName") }) + if (resolvable) simpleName else qualified + } + +/** The fully qualified names [file] imports by name. Syntactic: no analysis session needed. */ +internal fun importedNamesOf(file: KtFile): Set = + file.importDirectives + .filterNot { it.isAllUnder } + .mapNotNullTo(mutableSetOf()) { it.importedFqName?.asString() } + +/** The packages [file] star-imports (`import com.example.*`). */ +internal fun starImportedPackagesOf(file: KtFile): Set = + file.importDirectives + .filter { it.isAllUnder } + .mapNotNullTo(mutableSetOf()) { it.importedFqName?.asString() } 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 352e81eaec..504b4e50de 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.ExtractVariableAction import com.itsaky.androidide.lsp.kotlin.actions.FindReferencesAction import com.itsaky.androidide.lsp.kotlin.actions.GoToDefinitionAction import com.itsaky.androidide.lsp.kotlin.actions.ImplementMembersAction @@ -41,6 +42,7 @@ class KotlinCodeActionTooltipTagTest { NullSafetyAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_NULL_SAFETY_FIX, 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, ) assertEquals(expected, actualTags) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt new file mode 100644 index 0000000000..1b008a941b --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractVariableViewModelTest.kt @@ -0,0 +1,195 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import com.itsaky.androidide.lsp.kotlin.utils.refactor.AnchorForm +import com.itsaky.androidide.lsp.kotlin.utils.refactor.CandidateExpression +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ExtractionPlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.NameProblem +import com.itsaky.androidide.lsp.kotlin.utils.refactor.ScopeOption +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. + * + * Every choice the sheet offers is recomputed from the plan, so all of this is exercisable as plain + * state transitions -- which is the point of keeping the plan plain data. + */ +class ExtractVariableViewModelTest { + private fun scope( + label: String, + occurrences: Int, + ) = ScopeOption( + label = label, + anchorForm = AnchorForm.ExistingBlock(contentSpan = TextSpan(0, 100), statementSpans = emptyList()), + occurrences = (0 until occurrences).map { TextSpan(it * 10, it * 10 + 5) }, + ) + + private fun candidate( + label: String, + suggestedName: String, + scopes: List, + takenNames: Set = emptySet(), + ) = CandidateExpression( + label = label, + span = TextSpan(0, 5), + suggestedName = suggestedName, + takenNames = takenNames, + scopes = scopes, + ) + + private fun plan( + candidates: List, + selectionMatched: Boolean = false, + ) = ExtractionPlan( + fileText = "unused", + documentVersion = 1, + candidates = candidates, + selectionMatchedCandidate = selectionMatched, + ) + + private val threeCandidatePlan = + plan( + listOf( + candidate("items.size", "size", listOf(scope("lambda", 1), scope("fun demo", 3))), + candidate("items.size * 2", "size1", listOf(scope("fun demo", 2))), + candidate("wrap(items.size * 2)", "wrap", listOf(scope("fun demo", 1))), + ), + ) + + @Test + fun `starts on the innermost candidate, innermost scope, replace-all off`() { + val state = ExtractVariableViewModel(threeCandidatePlan).uiState.value + + assertEquals(0, state.selectedCandidate) + assertEquals(0, state.selectedScope) + assertEquals("size", state.name) + assertFalse(state.replaceAll) + assertTrue(state.canConfirm) + } + + @Test + fun `shows the candidate picker only when there is a real choice`() { + assertTrue(ExtractVariableViewModel(threeCandidatePlan).uiState.value.showCandidatePicker) + + val single = plan(listOf(candidate("items.size", "size", listOf(scope("fun demo", 1))))) + assertFalse(ExtractVariableViewModel(single).uiState.value.showCandidatePicker) + } + + @Test + fun `an exact selection suppresses the candidate picker`() { + // The user already said which expression they meant by selecting it. + val matched = plan(threeCandidatePlan.candidates, selectionMatched = true) + assertFalse(ExtractVariableViewModel(matched).uiState.value.showCandidatePicker) + } + + @Test + fun `changing the expression re-derives name, scopes and count`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + + viewModel.onEvent(ExtractVariableUiEvent.CandidateSelected(1)) + val state = viewModel.uiState.value + + assertEquals("size1", state.name) + assertEquals(listOf("fun demo"), state.scopeLabels) + assertEquals(2, state.occurrenceCount) + assertEquals(0, state.selectedScope) + } + + @Test + fun `changing the scope changes the occurrence count`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + assertEquals(1, viewModel.uiState.value.occurrenceCount) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertEquals(1, viewModel.uiState.value.selectedScope) + assertEquals(3, viewModel.uiState.value.occurrenceCount) + } + + @Test + fun `a scope change keeps the name the user typed`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("mySize")) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertEquals("mySize", viewModel.uiState.value.name) + } + + @Test + fun `the replace-all toggle is hidden at a single occurrence`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + assertFalse(viewModel.uiState.value.showReplaceAll) + + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + + assertTrue(viewModel.uiState.value.showReplaceAll) + } + + @Test + fun `an invalid name blocks confirming`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("val")) + + assertEquals(NameProblem.Keyword, viewModel.uiState.value.nameProblem) + assertFalse(viewModel.uiState.value.canConfirm) + assertNull(viewModel.choice()) + } + + @Test + fun `a name colliding with a visible declaration is rejected`() { + val colliding = + plan(listOf(candidate("items.size", "size1", listOf(scope("fun demo", 1)), takenNames = setOf("size")))) + val viewModel = ExtractVariableViewModel(colliding) + + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("size")) + + assertEquals(NameProblem.AlreadyTaken, viewModel.uiState.value.nameProblem) + } + + @Test + fun `the choice carries the selected expression, scope, name and toggle`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + viewModel.onEvent(ExtractVariableUiEvent.NameChanged("total")) + + val choice = viewModel.choice() + assertNotNull(choice) + assertEquals("items.size", choice!!.candidate.label) + assertEquals("fun demo", choice.scope.label) + assertEquals("total", choice.name) + assertTrue(choice.replaceAll) + } + + @Test + fun `replace-all cannot leak from a wider scope into a single-occurrence one`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + assertTrue(viewModel.uiState.value.replaceAll) + + // Back to the lambda scope, which has one occurrence and no visible toggle. + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(0)) + + assertFalse(viewModel.uiState.value.replaceAll) + assertFalse(viewModel.choice()!!.replaceAll) + } + + @Test + fun `switching expression resets replace-all`() { + val viewModel = ExtractVariableViewModel(threeCandidatePlan) + viewModel.onEvent(ExtractVariableUiEvent.ScopeSelected(1)) + viewModel.onEvent(ExtractVariableUiEvent.ReplaceAllChanged(true)) + + viewModel.onEvent(ExtractVariableUiEvent.CandidateSelected(1)) + + assertFalse(viewModel.uiState.value.replaceAll) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt new file mode 100644 index 0000000000..a670badf17 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariableEditTest.kt @@ -0,0 +1,562 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Rewrite construction, with no PSI and no analysis session involved. + * + * Every assertion is on the **resulting file text** rather than on offsets. Indentation is the thing + * most likely to be wrong here -- code-action edits bypass the editor's auto-indent, so the emitted + * text has to be final -- and a range assertion cannot see an indentation bug at all. + */ +class ExtractVariableEditTest { + private fun apply( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + + private fun spanOf( + text: String, + snippet: String, + fromIndex: Int = 0, + ): TextSpan { + val start = text.indexOf(snippet, fromIndex) + require(start >= 0) { "'$snippet' not found" } + return TextSpan(start, start + snippet.length) + } + + private fun allSpansOf( + text: String, + snippet: String, + ): List { + val spans = mutableListOf() + var from = 0 + while (true) { + val start = text.indexOf(snippet, from) + if (start < 0) break + spans += TextSpan(start, start + snippet.length) + from = start + snippet.length + } + return spans + } + + /** + * The block rung of a single-block fixture: content is everything between the first `{` and the + * last `}`, and [statements] are the block's direct child statements in source order. + * + * Only correct for a fixture with exactly one brace pair -- a nested one (e.g. a class wrapping a + * function) needs its `AnchorForm.ExistingBlock` built by hand instead. + */ + private fun existingBlock( + text: String, + vararg statements: String, + ) = AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = statements.map { spanOf(text, it) }, + ) + + private fun rewrite( + text: String, + candidate: TextSpan, + anchorForm: AnchorForm, + occurrences: List, + name: String, + replaceAll: Boolean, + ) = buildExtractVariableRewrite( + fileText = text, + candidateSpan = candidate, + scope = ScopeOption("scope", anchorForm, occurrences), + name = name, + replaceAll = replaceAll, + ) + + @Test + fun `inserts the declaration above the statement and replaces the selected occurrence`() { + val text = "fun f(items: List) {\n\tprintln(items.size * 2)\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `replace-all rewrites every occurrence and anchors above the first`() { + val text = + "fun f(items: List) {\n" + + "\tprintln(items.size * 2)\n" + + "\tlog(items.size * 2)\n" + + "\tuse(items.size * 2)\n" + + "}" + val occurrences = allSpansOf(text, "items.size * 2") + // The user selected the middle one; the declaration must still hoist above the first. + val candidate = occurrences[1] + + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)", "log(items.size * 2)", "use(items.size * 2)"), + occurrences, + "size", + replaceAll = true, + )!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "\tlog(size)\n" + + "\tuse(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `replace-all off leaves the other occurrences alone`() { + val text = + "fun f(items: List) {\n" + + "\tprintln(items.size * 2)\n" + + "\tlog(items.size * 2)\n" + + "}" + val occurrences = allSpansOf(text, "items.size * 2") + + val result = + rewrite( + text, + occurrences[0], + existingBlock(text, "println(items.size * 2)", "log(items.size * 2)"), + occurrences, + "size", + replaceAll = false, + )!! + + assertEquals( + "fun f(items: List) {\n" + + "\tval size = items.size * 2\n" + + "\tprintln(size)\n" + + "\tlog(items.size * 2)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `matches the file's space indentation rather than assuming tabs`() { + val text = "fun f(items: List) {\n println(items.size * 2)\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! + + assertEquals( + "fun f(items: List) {\n" + + " val size = items.size * 2\n" + + " println(size)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `keeps CRLF line endings when the file uses them`() { + val text = "fun f(items: List) {\r\n\tprintln(items.size * 2)\r\n}" + val candidate = spanOf(text, "items.size * 2") + + val result = + rewrite( + text, + candidate, + existingBlock(text, "println(items.size * 2)"), + listOf(candidate), + "size", + replaceAll = false, + )!! + + assertEquals( + "fun f(items: List) {\r\n" + + "\tval size = items.size * 2\r\n" + + "\tprintln(size)\r\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `deeper indentation is preserved`() { + val text = "class C {\n\tfun f(items: List) {\n\t\tprintln(items.size * 2)\n\t}\n}" + val candidate = spanOf(text, "items.size * 2") + // Two brace pairs are nested here, so `existingBlock`'s "first { .. last }" heuristic would + // grab the class's braces instead of `fun f`'s -- built by hand for the inner pair instead. + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, "\n\t\tprintln(items.size * 2)\n\t"), + statementSpans = listOf(spanOf(text, "println(items.size * 2)")), + ) + + val result = + rewrite( + text, + candidate, + form, + listOf(candidate), + "size", + replaceAll = false, + )!! + + assertEquals( + "class C {\n" + + "\tfun f(items: List) {\n" + + "\t\tval size = items.size * 2\n" + + "\t\tprintln(size)\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `wraps a braceless if branch in braces`() { + val text = "fun f(c: Boolean, a: A) {\n\tif (c) log(a.b)\n}" + val candidate = spanOf(text, "a.b") + val body = spanOf(text, "log(a.b)") + val form = + AnchorForm.WrapInBraces( + bodyStart = body.start, + bodyEnd = body.end, + indent = "\t", + innerIndent = "\t\t", + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "b", replaceAll = false)!! + + assertEquals( + "fun f(c: Boolean, a: A) {\n" + + "\tif (c) {\n" + + "\t\tval b = a.b\n" + + "\t\tlog(b)\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `converts an expression body to a block body with return`() { + val text = "fun area(r: Int) = r * r + r * r" + val occurrences = allSpansOf(text, "r * r") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = occurrences.first().start, + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = true, + ) + + val result = rewrite(text, occurrences.first(), form, occurrences, "square", replaceAll = true)!! + + assertEquals( + "fun area(r: Int) {\n" + + "\tval square = r * r\n" + + "\treturn square + square\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `omits return when the expression body function returns Unit`() { + val text = "fun show(a: A) = log(a.b)" + val candidate = spanOf(text, "a.b") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = text.indexOf("log(a.b)"), + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = false, + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "b", replaceAll = false)!! + + assertEquals( + "fun show(a: A) {\n" + + "\tval b = a.b\n" + + "\tlog(b)\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `writes the return type into the signature when the declaration has none`() { + val text = "fun area(r: Int) = r * r" + val candidate = spanOf(text, "r * r") + val form = + AnchorForm.ConvertExpressionBody( + assignStart = text.indexOf('='), + bodyStart = candidate.start, + bodyEnd = text.length, + indent = "", + innerIndent = "\t", + needsReturn = true, + returnTypeText = "Int", + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "squared", replaceAll = false)!! + + assertEquals( + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `null when there is nothing to replace`() { + val text = "fun f() {}" + assertNull( + buildExtractVariableRewrite( + fileText = text, + candidateSpan = TextSpan(0, 3), + scope = ScopeOption("scope", AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), emptyList()), + name = "value", + replaceAll = true, + ), + ) + } + + @Test + fun `null when an occurrence lies outside the file`() { + val text = "fun f() {}" + assertNull( + buildExtractVariableRewrite( + fileText = text, + candidateSpan = TextSpan(0, 3), + scope = + ScopeOption( + "scope", + AnchorForm.ExistingBlock(TextSpan(9, 9), emptyList()), + listOf(TextSpan(0, text.length + 5)), + ), + name = "value", + replaceAll = true, + ), + ) + } + + @Test + fun `the inner rung declares inside the if block`() { + val text = + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\treturn a + b * 2\n" + + "\t}\n" + + "\treturn 0\n" + + "}" + val candidate = spanOf(text, "a + b * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, "\n\t\treturn a + b * 2\n\t"), + statementSpans = listOf(spanOf(text, "return a + b * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "total", replaceAll = false)!! + + assertEquals( + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\tval total = a + b * 2\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `the outer rung declares above the enclosing statement`() { + val text = + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tif (flag) {\n" + + "\t\treturn a + b * 2\n" + + "\t}\n" + + "\treturn 0\n" + + "}" + val candidate = spanOf(text, "a + b * 2") + // The function block's rung: its statements are the whole `if` and the trailing `return 0`. + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = + listOf( + spanOf(text, "if (flag) {\n\t\treturn a + b * 2\n\t}"), + spanOf(text, "return 0"), + ), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "total", replaceAll = false)!! + + assertEquals( + "fun f(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tval total = a + b * 2\n" + + "\tif (flag) {\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `position index line and column all agree`() { + val text = "aa\nbbb\nc" + val position = positionAt(text, text.indexOf('c')) + assertEquals(2, position.line) + assertEquals(0, position.column) + assertEquals(7, position.index) + } + + @Test + fun `expands a one-line lambda so the declaration lands inside the braces`() { + val text = "fun f(items: List): List {\n\treturn items.map { it.length + 1 }\n}" + val candidate = spanOf(text, "it.length + 1") + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, " it.length + 1 "), + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval length = it.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `expanding a one-line lambda keeps its parameter header on the brace line`() { + val text = "fun f(items: List): List {\n\treturn items.map { item -> item.length + 1 }\n}" + val candidate = spanOf(text, "item.length + 1") + // A lambda body block excludes the `item ->` header, so the header is outside the content span. + val form = + AnchorForm.ExistingBlock( + contentSpan = spanOf(text, " item.length + 1 "), + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "length", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map { item ->\n" + + "\t\tval length = item.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `expands a one-line function body`() { + val text = "fun f(n: Int): Int { return n * 2 }" + val candidate = spanOf(text, "n * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = listOf(spanOf(text, "return n * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "doubled", replaceAll = false)!! + + assertEquals( + "fun f(n: Int): Int {\n" + + "\tval doubled = n * 2\n" + + "\treturn doubled\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `widening is a no-op when a one-line lambda has no interior spaces`() { + val text = "fun f(items: List): List {\n\treturn items.map {it + 1}\n}" + val candidate = spanOf(text, "it + 1") + val form = + AnchorForm.ExistingBlock( + contentSpan = candidate, + statementSpans = listOf(candidate), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "value", replaceAll = false)!! + + assertEquals( + "fun f(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval value = it + 1\n" + + "\t\tvalue\n" + + "\t}\n" + + "}", + apply(text, result), + ) + } + + @Test + fun `keeps CRLF line endings when expanding a one-line block`() { + val text = "fun f(n: Int): Int { return n * 2 }\r\nval x = 1" + val candidate = spanOf(text, "n * 2") + val form = + AnchorForm.ExistingBlock( + contentSpan = TextSpan(text.indexOf('{') + 1, text.lastIndexOf('}')), + statementSpans = listOf(spanOf(text, "return n * 2")), + ) + + val result = rewrite(text, candidate, form, listOf(candidate), "doubled", replaceAll = false)!! + + assertEquals( + "fun f(n: Int): Int {\r\n" + + "\tval doubled = n * 2\r\n" + + "\treturn doubled\r\n" + + "}\r\nval x = 1", + apply(text, result), + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt new file mode 100644 index 0000000000..08430fc2a7 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractVariablePlanEndToEndTest.kt @@ -0,0 +1,901 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import org.jetbrains.kotlin.com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtIfExpression +import org.jetbrains.kotlin.psi.KtLambdaExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +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 parts of the plan that need real symbol resolution: candidate filtering, the legal scope chain + * across lambda boundaries, occurrence matching by symbol identity, and reassignment soundness. + * + * Where a rewrite is produced, the assertion is on the **resulting file text** -- the only assertion + * that can catch an indentation or off-by-one error. + */ +class ExtractVariablePlanEndToEndTest : KtLspTest() { + private fun plan( + content: String, + start: Int, + end: Int = start, + ): ExtractionPlan { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + return buildExtractionPlan(env, path, start, end, documentVersion = 1, cancelChecker = noopCancelChecker()) + } + + private fun apply( + text: String, + rewrite: RewriteSpan, + ): String = text.substring(0, rewrite.span.start) + rewrite.newText + text.substring(rewrite.span.end) + + @Test + fun `offers the innermost three candidates, innermost first`() { + val content = + """ + package p + class B { fun c(): Int = 1 } + class A { val b: B = B() } + fun wrap(n: Int): Int = n + fun demo(a: A) { + wrap(a.b.c() * 2) + } + """.trimIndent() + + // Anchor on the call site, not the `fun c()` declaration that appears earlier in the file. + val result = plan(content, content.indexOf("a.b.c()") + "a.b.c".length) + + assertEquals( + listOf("a.b.c()", "a.b.c() * 2", "wrap(a.b.c() * 2)"), + result.candidates.map { it.label }, + ) + } + + @Test + fun `does not offer bare literals`() { + val content = + """ + package p + fun demo(n: Int): Int { + return n * 2 + } + """.trimIndent() + + val result = plan(content, content.indexOf("2", content.indexOf("n * 2"))) + + assertFalse(result.candidates.any { it.label == "2" }) + assertTrue(result.candidates.any { it.label == "n * 2" }) + } + + @Test + fun `offers nothing for a class-body property initializer`() { + val content = + """ + package p + fun compute(): Int = 1 + class C { + val x = compute() + compute() + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("compute() + compute()") + 1).isEmpty) + } + + @Test + fun `offers nothing for a default parameter value`() { + val content = + """ + package p + fun base(): Int = 1 + fun demo(n: Int = base() * 2) { + println(n) + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("base() * 2") + 1).isEmpty) + } + + @Test + fun `offers nothing when the cursor is in a comment`() { + val content = + """ + package p + fun demo() { + // nothing here + } + """.trimIndent() + + assertTrue(plan(content, content.indexOf("nothing")).isEmpty) + } + + @Test + fun `a selection matching an expression exactly short-circuits the chooser`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(n: Int) { + wrap(n * 2) + } + """.trimIndent() + val start = content.indexOf("n * 2") + + val result = plan(content, start, start + "n * 2".length) + + assertTrue(result.selectionMatchedCandidate) + assertEquals("n * 2", result.candidates.first().label) + } + + @Test + fun `an off-boundary selection still resolves, without short-circuiting`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(n: Int) { + wrap(n * 2) + } + """.trimIndent() + val start = content.indexOf("n * 2") + + // Selection stops mid-expression, as a touch-screen drag routinely does. + val result = plan(content, start, start + 3) + + assertFalse(result.selectionMatchedCandidate) + assertEquals("n * 2", result.candidates.first().label) + } + + @Test + fun `a shadowed name in a nested lambda is not the same expression`() { + val content = + """ + package p + class Config(val timeout: Int) + fun log(n: Int) {} + fun demo(config: Config, list: List) { + log(config.timeout) + list.forEach { config -> log(config.timeout) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("config.timeout") + 1) + val functionScope = + result.candidates + .first() + .scopes + .first() + + // `config` inside the lambda is a different declaration, so only one occurrence exists. + assertEquals(1, functionScope.occurrences.size) + } + + @Test + fun `the same expression in both branches of an if is one occurrence set`() { + val content = + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun warn(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) { + log(a.b) + } else { + warn(a.b) + } + } + """.trimIndent() + + val result = plan(content, content.indexOf("a.b") + 1) + val candidate = result.candidates.first { it.label == "a.b" } + // The outermost rung is the function body, which contains both branches. + val functionScope = candidate.scopes.last() + + assertEquals(2, functionScope.occurrences.size) + } + + @Test + fun `a reassignment between occurrences drops the unsound one`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(): Int { + var limit = 1 + wrap(limit + 1) + limit = 5 + wrap(limit + 1) + return limit + } + """.trimIndent() + + val result = plan(content, content.indexOf("limit + 1") + 1) + val candidate = result.candidates.first { it.label == "limit + 1" } + val functionScope = candidate.scopes.last() + + // Both sites are the same expression, but `limit = 5` makes the second a different value. + assertEquals(1, functionScope.occurrences.size) + } + + @Test + fun `a candidate using the implicit lambda parameter cannot be hoisted out of the lambda`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(items: List) { + items.forEach { log(it.length + 1) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("it.length + 1") + 1) + val candidate = result.candidates.first { it.label == "it.length + 1" } + + // `it` belongs to the lambda, so the lambda body is the only legal anchor. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + } + + @Test + fun `a lambda-invariant candidate can be hoisted to the enclosing function`() { + val content = + """ + package p + class Config(val timeout: Int) + fun log(n: Int) {} + fun demo(config: Config, items: List) { + items.forEach { log(config.timeout * 2) } + } + """.trimIndent() + + val result = plan(content, content.indexOf("config.timeout * 2") + 1) + val candidate = result.candidates.first { it.label == "config.timeout * 2" } + + // Nothing lambda-scoped is referenced, so hoisting out to the function body is offered. + assertEquals(listOf("lambda", "fun demo"), candidate.scopes.map { it.label }) + } + + @Test + fun `suggests a name from the expression shape`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List) { + wrap(items.size * 2) + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size") + 1) + + assertEquals("size", result.candidates.first { it.label == "items.size" }.suggestedName) + } + + @Test + fun `does not suggest a name that is already taken`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List) { + val size = 0 + wrap(items.size * 2) + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size") + 1) + + assertEquals("size1", result.candidates.first { it.label == "items.size" }.suggestedName) + } + + @Test + fun `end to end rewrite replaces all occurrences in the function body`() { + val content = + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List): Int { + wrap(items.size * 2) + return items.size * 2 + } + """.trimIndent() + + val result = plan(content, content.indexOf("items.size * 2") + 1) + val candidate = result.candidates.first { it.label == "items.size * 2" } + val scope = candidate.scopes.last() + assertEquals(2, scope.occurrences.size) + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "size", replaceAll = true) + assertNotNull(rewrite) + + assertEquals( + """ + package p + fun wrap(n: Int): Int = n + fun demo(items: List): Int { + val size = items.size * 2 + wrap(size) + return size + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } + + @Test + fun `end to end rewrite converts an expression-bodied function to a block body`() { + val content = + """ + package p + fun area(r: Int) = r * r + r * r + """.trimIndent() + + val result = plan(content, content.indexOf("r * r") + 1) + val candidate = result.candidates.first { it.label == "r * r" } + val scope = candidate.scopes.first() + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "square", replaceAll = true) + assertNotNull(rewrite) + + assertEquals( + """ + package p + fun area(r: Int): Int { + val square = r * r + return square + square + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } + + @Test + fun `end to end rewrite wraps a braceless if branch`() { + val content = + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) log(a.b + 1) + } + """.trimIndent() + + val result = plan(content, content.indexOf("a.b + 1") + 1) + val candidate = result.candidates.first { it.label == "a.b + 1" } + val scope = candidate.scopes.first() + + val rewrite = + buildExtractVariableRewrite(result.fileText, candidate.span, scope, "offset", replaceAll = false) + assertNotNull(rewrite) + + assertEquals( + """ + package p + class A(val b: Int) + fun log(n: Int) {} + fun demo(c: Boolean, a: A) { + if (c) { + val offset = a.b + 1 + log(offset) + } + } + """.trimIndent(), + apply(content, rewrite!!), + ) + } + + @Test + fun `does not offer the lambda that wraps the expression`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { + it.length + 1 + } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + // `{ it.length + 1 }` must not appear between the two: a hoisted lambda loses the `it` the call + // site was supplying. + assertEquals( + listOf("it.length + 1", "items.map { it.length + 1 }"), + result.candidates.map { it.label }, + ) + } + + @Test + fun `labels a braced if branch by its owner`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b * 2 + } + return 0 + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + assertEquals( + listOf("if block", "fun demo"), + result.candidates + .first() + .scopes + .map { it.label }, + ) + } + + @Test + fun `labels a braced else branch by its owner`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return 0 + } else { + return a + b * 2 + } + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + + assertEquals( + listOf("else block", "fun demo"), + result.candidates + .first() + .scopes + .map { it.label }, + ) + } + + @Test + fun `converting an inferred-type expression body writes the type out`() { + val content = + """ + package p + fun area(r: Int) = r * r + """.trimIndent() + + val target = "r * r" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "squared", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `a declared return type is not written twice`() { + val content = + """ + package p + fun area(r: Int): Int = r * r + """.trimIndent() + + val target = "r * r" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "squared", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun area(r: Int): Int {\n" + + "\tval squared = r * r\n" + + "\treturn squared\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `picking the outer rung hoists the declaration above the enclosing statement`() { + val content = + """ + package p + fun demo(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b * 2 + } + return 0 + } + """.trimIndent() + + val target = "a + b * 2" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + assertEquals(listOf("if block", "fun demo"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes[1], + name = "total", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(flag: Boolean, a: Int, b: Int): Int {\n" + + "\tval total = a + b * 2\n" + + "\tif (flag) {\n" + + "\t\treturn total\n" + + "\t}\n" + + "\treturn 0\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `contentSpanOf finds the region inside a block's braces`() { + val content = + """ + package p + fun functionBody(a: Int, b: Int): Int { + return a + b + } + fun ifBody(flag: Boolean, a: Int, b: Int): Int { + if (flag) { + return a + b + } + return 0 + } + fun lambdaWithHeader(items: List): List { + return items.map { x -> x + 1 } + } + fun lambdaWithoutHeader(items: List): List { + return items.map { it + 1 } + } + fun emptyBody() {} + fun nestedLambda(items: List): List<() -> Int> { + return items.map { x -> { x + 1 } } + } + """.trimIndent() + val ktFile = createSourceFile("Main.kt", content) + val functions = ktFile.declarations.filterIsInstance().associateBy { it.name } + + fun contentOf(block: KtBlockExpression): String { + val span = contentSpanOf(block) + return content.substring(span.start, span.end) + } + + assertEquals("\n\treturn a + b\n", contentOf(functions.getValue("functionBody").bodyBlockExpression!!)) + + val ifBody = functions.getValue("ifBody").bodyBlockExpression!! + val ifThen = PsiTreeUtil.findChildOfType(ifBody, KtIfExpression::class.java)!!.then as KtBlockExpression + assertEquals("\n\tif (flag) {\n\t\treturn a + b\n\t}\n\treturn 0\n", contentOf(ifBody)) + assertEquals("\n\t\treturn a + b\n\t", contentOf(ifThen)) + + val lambdaWithHeaderBody = + PsiTreeUtil + .findChildOfType( + functions.getValue("lambdaWithHeader").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + .bodyExpression!! + val lambdaWithHeaderContent = contentOf(lambdaWithHeaderBody) + // The `x ->` header belongs to the enclosing function literal, not to this block. + assertFalse(lambdaWithHeaderContent.contains("->")) + assertEquals("x + 1", lambdaWithHeaderContent.trim()) + + val lambdaWithoutHeaderBody = + PsiTreeUtil + .findChildOfType( + functions.getValue("lambdaWithoutHeader").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + .bodyExpression!! + assertEquals("it + 1", contentOf(lambdaWithoutHeaderBody).trim()) + + assertEquals("", contentOf(functions.getValue("emptyBody").bodyBlockExpression!!)) + + // The outer lambda's sole statement is itself a lambda literal, so its text alone (`{ x + 1 }`) + // looks brace-owned; the content must still be that whole statement, not the inner lambda's + // interior. + val nestedOuterLambda = + PsiTreeUtil.findChildOfType( + functions.getValue("nestedLambda").bodyBlockExpression, + KtLambdaExpression::class.java, + )!! + val nestedOuterBody = nestedOuterLambda.bodyExpression!! + assertEquals("{ x + 1 }", contentOf(nestedOuterBody).trim()) + + val nestedInnerLambda = PsiTreeUtil.findChildOfType(nestedOuterBody, KtLambdaExpression::class.java)!! + assertEquals("x + 1", contentOf(nestedInnerLambda.bodyExpression!!).trim()) + } + + @Test + fun `a Unit-returning expression body gets neither a type nor a return`() { + val content = + """ + package p + fun report(value: Int) { + println(value) + } + fun show(text: String) = report(text.length + 1) + """.trimIndent() + + val target = "text.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun report(value: Int) {\n" + + "\tprintln(value)\n" + + "}\n" + + "fun show(text: String) {\n" + + "\tval length = text.length + 1\n" + + "\treport(length)\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `converting a Nothing-returning expression body preserves the signature`() { + val content = + """ + package p + fun boom(name: String) = error("bad " + name) + fun demo(x: Int?): Int = x ?: boom("missing") + """.trimIndent() + + val target = "\"bad \" + name" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "message", + replaceAll = false, + )!! + + // `boom`'s inferred return type is `Nothing`; folding it into the `Unit` case would drop both + // the `return` and the written-out `: Nothing`, and `x ?: boom(...)` would stop compiling. + assertEquals( + "package p\n" + + "fun boom(name: String): Nothing {\n" + + "\tval message = \"bad \" + name\n" + + "\treturn error(message)\n" + + "}\n" + + "fun demo(x: Int?): Int = x ?: boom(\"missing\")", + apply(content, rewrite), + ) + } + + @Test + fun `extracting from a one-line lambda stays inside the lambda`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { it.length + 1 } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `it` is lambda-scoped, so the lambda is the ceiling: there is no outer rung to choose. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval length = it.length + 1\n" + + "\t\tlength\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `extracting from a multi-line lambda with a header on its own line is not collapsed`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { x -> + x + 1 + } + } + """.trimIndent() + + val target = "x + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `x` is the lambda's own parameter, so the lambda is still the ceiling. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "next", + replaceAll = false, + )!! + + // The body already starts its own line, so this is the normal path, not the one-line + // expansion: the header and the closing brace are left exactly where they were. + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map { x ->\n" + + "\t\tval next = x + 1\n" + + "\t\tnext\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `extracting from a multi-line lambda without a header is not collapsed`() { + val content = + """ + package p + fun demo(items: List): List { + return items.map { + it + 1 + } + } + """.trimIndent() + + val target = "it + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "next", + replaceAll = false, + )!! + + assertEquals( + "package p\n" + + "fun demo(items: List): List {\n" + + "\treturn items.map {\n" + + "\t\tval next = it + 1\n" + + "\t\tnext\n" + + "\t}\n" + + "}", + apply(content, rewrite), + ) + } + + @Test + fun `declines a lambda whose first statement shares the brace line but the block spans several lines`() { + val content = + """ + package p + fun log(n: Int) {} + fun demo(items: List) { + items.forEach { log(it.length + 1) + log(it) } + } + """.trimIndent() + + val target = "it.length + 1" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + // `it` is lambda-scoped, so the lambda body is the only legal anchor. + assertEquals(listOf("lambda"), candidate.scopes.map { it.label }) + + // The statement shares the opening-brace line, but the block itself spans two lines, so this is + // not the one-line expansion case. Anchoring at the line start would put the declaration before + // the lambda's `{`, where `it` is out of scope -- declining is the only safe outcome here. + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "length", + replaceAll = false, + ) + assertNull(rewrite) + } + + @Test + fun `extracting from a semicolon-joined statement leaves the block multi-line`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + val x = a + 1; return x + b + } + """.trimIndent() + + val target = "x + b" + val result = plan(content, content.indexOf(target), content.indexOf(target) + target.length) + val candidate = result.candidates.first() + + val rewrite = + buildExtractVariableRewrite( + fileText = result.fileText, + candidateSpan = candidate.span, + scope = candidate.scopes.first(), + name = "sum", + replaceAll = false, + )!! + + // A statement already precedes the candidate on this line, but the block itself spans several + // lines, so this is not a one-line block: the declaration hoists above the whole line instead + // of expanding it, and the two semicolon-joined statements stay together. + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\tval sum = x + b\n" + + "\tval x = a + 1; return sum\n" + + "}", + apply(content, rewrite), + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt new file mode 100644 index 0000000000..6303290b14 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/RefactorPrimitivesTest.kt @@ -0,0 +1,202 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** The analysis-free primitives the refactoring is built from: name rules, indentation, soundness. */ +class RefactorPrimitivesTest { + @Test + fun `rejects blank names`() { + assertEquals(NameProblem.Blank, validateVariableName("", emptySet())) + assertEquals(NameProblem.Blank, validateVariableName(" ", emptySet())) + } + + @Test + fun `rejects non-identifiers`() { + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("1size", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("my size", emptySet())) + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("size!", emptySet())) + // Backticked names are legal Kotlin but deliberately unsupported for a generated local. + assertEquals(NameProblem.NotAnIdentifier, validateVariableName("`size`", emptySet())) + } + + @Test + fun `rejects hard keywords but allows soft ones`() { + assertEquals(NameProblem.Keyword, validateVariableName("val", emptySet())) + assertEquals(NameProblem.Keyword, validateVariableName("when", emptySet())) + assertEquals(NameProblem.Keyword, validateVariableName("this", emptySet())) + // `it`, `data` and `by` are soft keywords -- perfectly legal identifiers. + assertNull(validateVariableName("it", emptySet())) + assertNull(validateVariableName("data", emptySet())) + assertNull(validateVariableName("by", emptySet())) + } + + @Test + fun `rejects names already in use`() { + assertEquals(NameProblem.AlreadyTaken, validateVariableName("size", setOf("size"))) + assertNull(validateVariableName("size", setOf("count"))) + } + + @Test + fun `accepts underscores and digits`() { + assertNull(validateVariableName("_size", emptySet())) + assertNull(validateVariableName("size2", emptySet())) + } + + @Test + fun `detects a tab indent unit`() { + assertEquals("\t", detectIndentUnit("fun f() {\n\tval x = 1\n}")) + } + + @Test + fun `detects the smallest space indent unit`() { + assertEquals(" ", detectIndentUnit("fun f() {\n val x = 1\n val y = 2\n}")) + assertEquals(" ", detectIndentUnit("fun f() {\n val x = 1\n}")) + } + + @Test + fun `falls back to a tab when nothing is indented`() { + assertEquals("\t", detectIndentUnit("fun f() {}")) + } + + @Test + fun `leading indent is read from the offset's own line`() { + val text = "class C {\n\t\tval x = 1\n}" + assertEquals("\t\t", leadingIndentAt(text, text.indexOf("val x"))) + assertEquals("", leadingIndentAt(text, text.indexOf("class"))) + } + + @Test + fun `line start is found for the first and later lines`() { + val text = "aa\nbbb\nc" + assertEquals(0, lineStartOffset(text, 1)) + assertEquals(3, lineStartOffset(text, 4)) + assertEquals(7, lineStartOffset(text, 7)) + } + + @Test + fun `label collapses whitespace and truncates`() { + assertEquals("items.filter { it > 0 }", collapseForLabel("items\n\t.filter { it > 0 }")) + assertEquals("a?.b", collapseForLabel("a\n\t?.b")) + assertEquals("aaaaaaa...", collapseForLabel("aaaaaaaaaaaa", maxLength = 10)) + } + + @Test + fun `trim drops surrounding whitespace from a selection`() { + val text = " items.size " + assertEquals(2 to 12, trimToCode(text, 0, text.length)) + } + + @Test + fun `trim leaves a cursor untouched and rejects a whitespace-only selection`() { + assertEquals(3 to 3, trimToCode("a b", 3, 3)) + assertNull(trimToCode("a b", 1, 5)) + } + + @Test + fun `soundness keeps every occurrence when nothing is written`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + assertEquals( + occurrences, + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = emptyList()), + ) + } + + @Test + fun `soundness drops occurrences separated from the candidate by a write`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + // A reassignment between the second and third sites: the third no longer holds the same value. + assertEquals( + listOf(TextSpan(10, 20), TextSpan(30, 40)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(45)), + ) + } + + @Test + fun `soundness drops earlier occurrences when the write precedes the candidate`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + assertEquals( + listOf(TextSpan(30, 40), TextSpan(50, 60)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(25)), + ) + } + + @Test + fun `soundness always keeps the occurrence the user selected`() { + val occurrences = listOf(TextSpan(10, 20), TextSpan(30, 40), TextSpan(50, 60)) + // Writes on both sides isolate the candidate, but it must never be dropped. + assertEquals( + listOf(TextSpan(30, 40)), + excludeUnsoundOccurrences(occurrences, TextSpan(30, 40), writeOffsets = listOf(25, 45)), + ) + } + + @Test + fun `soundness falls back to the candidate alone when it is not among the occurrences`() { + assertEquals( + listOf(TextSpan(70, 80)), + excludeUnsoundOccurrences(listOf(TextSpan(10, 20)), TextSpan(70, 80), writeOffsets = emptyList()), + ) + } + + @Test + fun `shortens types from Kotlin's default-imported packages`() { + assertEquals("Int", shortenTypeText("kotlin.Int", emptySet(), emptySet())) + assertEquals( + "List", + shortenTypeText("kotlin.collections.List", emptySet(), emptySet()), + ) + } + + @Test + fun `keeps a type qualified when its short name would not resolve`() { + assertEquals("java.util.Date", shortenTypeText("java.util.Date", emptySet(), emptySet())) + // An import of the enclosing class is not an import of the nested one. + assertEquals( + "com.example.Outer.Inner", + shortenTypeText("com.example.Outer.Inner", setOf("com.example.Outer"), emptySet()), + ) + } + + @Test + fun `shortens a type the file already imports, by name or by star`() { + assertEquals("Date", shortenTypeText("java.util.Date", setOf("java.util.Date"), emptySet())) + assertEquals("Date", shortenTypeText("java.util.Date", emptySet(), setOf("java.util"))) + assertEquals( + "Flow", + shortenTypeText( + "kotlinx.coroutines.flow.Flow", + setOf("kotlinx.coroutines.flow.Flow", "com.example.Widget"), + emptySet(), + ), + ) + } + + @Test + fun `a star import is skipped when a colliding name is imported from elsewhere`() { + // An explicit import of a different `Date` shadows the star import, so shortening would + // resolve to the wrong type. + assertEquals( + "java.util.Date", + shortenTypeText("java.util.Date", setOf("com.example.Date"), setOf("java.util")), + ) + // With nothing colliding, the star import still shortens as before. + assertEquals( + "Date", + shortenTypeText("java.util.Date", emptySet(), setOf("java.util")), + ) + } + + @Test + fun `unrenderable type text is recognised`() { + assertTrue(isUnrenderableTypeText("")) + assertTrue(isUnrenderableTypeText("kotlin.collections.List")) + assertTrue(isUnrenderableTypeText("")) + assertTrue(isUnrenderableTypeText("ERROR CLASS: unresolved")) + assertTrue(isUnrenderableTypeText("kotlin.Any & kotlin.Comparable<*>")) + assertFalse(isUnrenderableTypeText("kotlin.Int")) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index b8e7660fef..3ebed7e55b 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -521,6 +521,24 @@ Suppress \'unchecked\' warning Uncomment line Convert to statement + + + Extract variable + Extract variable + Expression + Name + Declare in + + Replace %1$d occurrence + Replace all %1$d occurrences + + Extract + Enter a name + Not a valid Kotlin name + That is a Kotlin keyword + That name is already used + No expression to extract here + The file changed. Try extracting again. Select fields No fields selected No fields found