diff --git a/app/src/main/java/com/itsaky/androidide/fragments/sheets/ProgressSheet.java b/app/src/main/java/com/itsaky/androidide/fragments/sheets/ProgressSheet.java index c12831de81..a8cd39da12 100755 --- a/app/src/main/java/com/itsaky/androidide/fragments/sheets/ProgressSheet.java +++ b/app/src/main/java/com/itsaky/androidide/fragments/sheets/ProgressSheet.java @@ -30,68 +30,89 @@ public class ProgressSheet extends BaseBottomSheetFragment { - private LayoutProgressSheetBinding binding; - private String message = ""; - private String subMessage = ""; - private boolean subMessageEnabled = false; - - @Override - public void onViewCreated(@NonNull View view, Bundle savedInstanceState) { - super.onViewCreated(view, savedInstanceState); - - binding.message.setText(message); - - final var params = (ConstraintLayout.LayoutParams) binding.message.getLayoutParams(); - if (subMessageEnabled) { - binding.subMessage.setText(subMessage); - binding.subMessage.setVisibility(View.VISIBLE); - params.bottomToBottom = View.NO_ID; - } else { - binding.subMessage.setVisibility(View.GONE); - params.bottomToBottom = LayoutParams.PARENT_ID; - } - } - - @Nullable - @Override - public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container, - @Nullable Bundle savedInstanceState - ) { - binding = LayoutProgressSheetBinding.inflate(LayoutInflater.from(getContext())); - return binding.getRoot(); - } - - public void setSubMessageEnabled(boolean enabled) { - this.subMessageEnabled = enabled; - } - - public void setSubMessage(String msg) { - this.subMessage = msg; - if (isShowing()) { - binding.subMessage.setText(msg); - } - } - - public ProgressSheet setMessage(String message) { - this.message = message; - if (isShowing()) { - binding.message.setText(message); - } - - return this; - } - - public ProgressSheet setProgressDrawable(Drawable drawable) { - if (isShowing()) { - binding.progress.setIndeterminateDrawable(drawable); - } - return this; - } - - @Override - public void dismiss() { - if (isShowing()) { - super.dismiss(); - } - } + private LayoutProgressSheetBinding binding; + private String message = ""; + private String subMessage = ""; + private boolean subMessageEnabled = false; + + /* A dismiss that arrived before this fragment was attached, replayed in onStart. */ + private boolean dismissPending = false; + + /** + * {@inheritDoc} + * + *

+ * A dismiss that arrives before the enqueued {@code show()} transaction has run is remembered rather than dropped: the fragment is not attached to a fragment manager yet, so dismissing now would throw, but doing nothing would leave the sheet on screen with nothing left to close it. + */ + @Override + public void dismiss() { + if (!isAdded()) { + dismissPending = true; + return; + } + + dismissPending = false; + super.dismiss(); + } + + @Nullable + @Override + public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup container, + @Nullable Bundle savedInstanceState) { + binding = LayoutProgressSheetBinding.inflate(LayoutInflater.from(getContext())); + return binding.getRoot(); + } + + @Override + public void onStart() { + super.onStart(); + if (dismissPending) { + dismissPending = false; + dismissAllowingStateLoss(); + } + } + + @Override + public void onViewCreated(@NonNull View view, Bundle savedInstanceState) { + super.onViewCreated(view, savedInstanceState); + + binding.message.setText(message); + + final var params = (ConstraintLayout.LayoutParams) binding.message.getLayoutParams(); + if (subMessageEnabled) { + binding.subMessage.setText(subMessage); + binding.subMessage.setVisibility(View.VISIBLE); + params.bottomToBottom = View.NO_ID; + } else { + binding.subMessage.setVisibility(View.GONE); + params.bottomToBottom = LayoutParams.PARENT_ID; + } + } + + public ProgressSheet setMessage(String message) { + this.message = message; + if (isShowing()) { + binding.message.setText(message); + } + + return this; + } + + public ProgressSheet setProgressDrawable(Drawable drawable) { + if (isShowing()) { + binding.progress.setIndeterminateDrawable(drawable); + } + return this; + } + + public void setSubMessage(String msg) { + this.subMessage = msg; + if (isShowing()) { + binding.subMessage.setText(msg); + } + } + + public void setSubMessageEnabled(boolean enabled) { + this.subMessageEnabled = enabled; + } } diff --git a/app/src/test/java/com/itsaky/androidide/fragments/sheets/ProgressSheetDismissTest.kt b/app/src/test/java/com/itsaky/androidide/fragments/sheets/ProgressSheetDismissTest.kt new file mode 100644 index 0000000000..b9f319dc56 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/fragments/sheets/ProgressSheetDismissTest.kt @@ -0,0 +1,84 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.fragments.sheets + +import android.os.Looper +import androidx.appcompat.app.AppCompatActivity +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.app.BaseApplication +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config + +/** + * `DialogFragment.show` only enqueues the add transaction, so a dismiss issued in the same + * main-thread pass lands before the sheet exists. Callers that show a progress sheet around work + * that can finish synchronously - `IDELanguageClientImpl.performCodeAction` - rely on that dismiss + * being honoured; dropping it strands the sheet on screen with nothing left to close it. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = ProgressSheetDismissTest.TestApp::class) +class ProgressSheetDismissTest { + open class TestApp : BaseApplication() + + @Test + fun `dismiss issued before the show transaction runs still closes the sheet`() { + val activity = Robolectric.buildActivity(AppCompatActivity::class.java).setup().get() + val manager = activity.supportFragmentManager + + val sheet = ProgressSheet() + sheet.isCancelable = false + sheet.show(manager, TAG) + sheet.dismiss() + + shadowOf(Looper.getMainLooper()).idle() + manager.executePendingTransactions() + + assertThat(manager.findFragmentByTag(TAG)).isNull() + assertThat(sheet.isShowing).isFalse() + } + + @Test + fun `dismiss issued once the sheet is on screen closes it`() { + val activity = Robolectric.buildActivity(AppCompatActivity::class.java).setup().get() + val manager = activity.supportFragmentManager + + val sheet = ProgressSheet() + sheet.isCancelable = false + sheet.show(manager, TAG) + + shadowOf(Looper.getMainLooper()).idle() + manager.executePendingTransactions() + assertThat(sheet.isShowing).isTrue() + + sheet.dismiss() + + shadowOf(Looper.getMainLooper()).idle() + manager.executePendingTransactions() + + assertThat(manager.findFragmentByTag(TAG)).isNull() + assertThat(sheet.isShowing).isFalse() + } + + private companion object { + const val TAG = "progress_sheet_test" + } +} diff --git a/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md b/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md index 92a2a0b16b..ca700de553 100644 --- a/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md +++ b/docs/adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md @@ -1,4 +1,4 @@ -# 0012. Refactoring UI lives in the owning LSP module +# 0013. Refactoring UI lives in the owning LSP module - **Status:** Proposed - **Date:** 2026-08-03 diff --git a/docs/adr/0014-refactorings-decline-rather-than-rewrite.md b/docs/adr/0014-refactorings-decline-rather-than-rewrite.md index d4b8898a04..60801b5ca9 100644 --- a/docs/adr/0014-refactorings-decline-rather-than-rewrite.md +++ b/docs/adr/0014-refactorings-decline-rather-than-rewrite.md @@ -1,4 +1,4 @@ -# 0013. Interactive refactorings decline rather than rewrite unselected code +# 0014. Interactive refactorings decline rather than rewrite unselected code - **Status:** Proposed - **Date:** 2026-08-10 @@ -6,7 +6,7 @@ ## Context -The K2 Kotlin LSP is growing a family of interactive refactorings: extract variable (ADFA-4826), extract method (ADFA-5080), inline variable (ADFA-4827), semantic rename (ADFA-4825). [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) settles where their UI lives and that analysis produces plain data. It says nothing about how capable they should be. +The K2 Kotlin LSP is growing a family of interactive refactorings: extract variable (ADFA-4826), extract method (ADFA-5080), inline variable (ADFA-4827), semantic rename (ADFA-4825). [ADR 0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) settles where their UI lives and that analysis produces plain data. It says nothing about how capable they should be. That question turns out to dominate the requirements. Designing extract method surfaced a run of cases where the transformation the user asked for cannot be performed by *moving* their code - it also needs the moved code's interior edited, or a guess about intent: @@ -29,8 +29,11 @@ Concretely: - **Prefer a stricter rule to a cleverer one** when strictness costs capability and cleverness costs certainty. Extract method refuses a reassigned outer `var` even when the write is provably dead, because proving it needs liveness analysis. - **Never emit code that does not compile, and avoid emitting code that warns.** The two modifiers extract method *does* add - `suspend` and `@Composable` - are required precisely because omitting them breaks compilation. - **A refusal is a backlog item, not a dead end.** Where the refused case is common, file it: ADFA-5082 tracks the reassigned-`var` output. +- **Where part of the request is sound, apply that part and say so.** A refactoring has three outcomes, not two: apply, refuse, or **apply partially**. Inline variable (ADFA-4827) is the case that needs the third - a variable whose value is reassigned partway through can be inlined at the references before the write and nowhere after it, so refusing the whole thing would discard a sound transformation of the earlier half. A partial application must report both counts and what it left behind, and it must leave the file compiling on its own, exactly as a full application does. It is not a licence to apply the doubtful part and hope. -This applies to the whole refactoring family, not just extract method. Inline variable and rename inherit it. +This applies to the whole refactoring family, not just extract method. Inline variable and rename inherit it - inline variable adding the third outcome above, rename presumed to need only the first two until its design says otherwise. + +**Out of scope of this decision.** Whether a refactoring may duplicate an expression that is evaluated more than once. Inline variable does, without checking for side effects (see [kotlin-inline-variable.md](../features/kotlin-inline-variable.md)): the emitted code compiles, carries no warning, and is the user's own expression unedited, so nothing above forbids it. Kotlin offers no way to prove purity, so a check would be a heuristic rather than a stricter rule, and this ADR prefers strictness to cleverness in both directions. ## Consequences @@ -40,12 +43,14 @@ This applies to the whole refactoring family, not just extract method. Inline va - Refusal reasons are cheap to specify, cheap to test (one case each) and cheap to QA, where a clever transformation needs its own test matrix and its own failure modes. - The rules are stateable in a sentence each, which is what makes the feature docs reviewable by someone who has not read the implementation. - Excluding cases by construction keeps the analysis pass small, which matters when it runs on a phone. +- Partial application recovers capability that an all-or-nothing rule would throw away, without weakening the compile-and-do-not-warn guarantee: the sound part is applied and the doubtful part is simply not touched. **Negative / costs** - The refactorings are visibly less capable than a desktop IDE's. Two of extract method's refusals - a reassigned outer `var` (the accumulator loop) and an enclosing `with`/`apply` receiver (pervasive in Android code) - will be hit routinely. - The quality of the *messages* becomes load-bearing. A generic refusal reads as a broken feature, so this decision spends translated strings: roughly seven for extract method alone. - Users arriving from IntelliJ will read some refusals as regressions rather than as design. +- A partial application is harder to *report* than either other outcome, and harder to QA: the message has to convey two counts and a surviving declaration in one flash, and every "how many were left behind" case is its own test. A partial result the user misreads as a complete one is the failure mode to watch. - The line is a judgement, not a formalism. "Editing the interior of the moved code" is clear in the cases above but will need re-application, case by case, in each future refactoring. ## Alternatives considered @@ -57,7 +62,8 @@ This applies to the whole refactoring family, not just extract method. Inline va ## Related -- [ADR 0012](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - where refactoring UI lives; this ADR answers *how capable it is* +- [ADR 0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - where refactoring UI lives; this ADR answers *how capable it is* - [ADR 0010](0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth - [kotlin-extract-method.md](../features/kotlin-extract-method.md) - R7 to R10 and R14 are this decision applied case by case - [kotlin-extract-variable.md](../features/kotlin-extract-variable.md) - the shared vocabulary and primitives +- [kotlin-inline-variable.md](../features/kotlin-inline-variable.md) - R6 to R9 are this decision applied to a subtractive refactoring, and the origin of the third outcome diff --git a/docs/adr/README.md b/docs/adr/README.md index 5b7b7fa226..79ff43650c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -26,5 +26,5 @@ Format is lightweight **MADR / Nygard**: Context → Decision → Consequences | [0010](0010-navigation-resolves-via-analysis-api.md) | Kotlin navigation resolves via the Analysis API, not the symbol index | Proposed | | [0011](0011-command-analysis-priority.md) | User-invoked commands get their own analysis priority | Proposed | | [0012](0012-volatile-build-metadata-out-of-abis.md) | Keep volatile build metadata out of module ABIs | Proposed | -| [0013](0012-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | -| [0014](0013-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | +| [0013](0013-refactoring-ui-lives-in-the-owning-lsp-module.md) | Refactoring UI lives in the owning LSP module | Proposed | +| [0014](0014-refactorings-decline-rather-than-rewrite.md) | Interactive refactorings decline rather than rewrite unselected code | Proposed | diff --git a/docs/features/kotlin-extract-method.md b/docs/features/kotlin-extract-method.md index f6e23ad819..d28183f552 100644 --- a/docs/features/kotlin-extract-method.md +++ b/docs/features/kotlin-extract-method.md @@ -9,7 +9,7 @@ Move the expression at the cursor, or a selected range of statements, into a new Ships as the top of a three-PR stack: `common-compose` theming, then extract variable (ADFA-4826), then this. It reuses that PR's primitives - offsets, naming, indentation, edit emission - and adds no new module, no new dependency and no new UI mechanism. -The governing principle is [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md): this refactoring **moves** code, it never edits the interior of what it moved, and where it cannot do that faithfully it **declines with a specific reason** rather than guessing. Most of the requirements below are that principle applied to one case each. +The governing principle is [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md): this refactoring **moves** code, it never edits the interior of what it moved, and where it cannot do that faithfully it **declines with a specific reason** rather than guessing. Most of the requirements below are that principle applied to one case each. ## Language @@ -135,7 +135,7 @@ Contents, top to bottom: title -> expression chooser (only for an expression reg The preview is **one monospace line: the signature exactly as it will be emitted** - modifiers, receiver, parameters, return type. Types render fully qualified (R5), so a real preview reads `private suspend fun loadUser(id: kotlin.String): com.example.User`. It wraps rather than truncating. No body preview: the body is the code the user selected and can see behind the sheet, so it moves verbatim and previewing it says nothing new, while the signature is the one derived artefact and the one place the derivation can surprise them. -ADR 0012 defers the shared-UI question until the extract-method surface is known; a single generalised sheet would need a state class where half the fields are meaningless to either caller, so that question stays open rather than being settled from one data point. +ADR 0013 defers the shared-UI question until the extract-method surface is known; a single generalised sheet would need a state class where half the fields are meaningless to either caller, so that question stays open rather than being settled from one data point. **R12 - Name.** Suggestion: for an expression region, the existing shape/type derivation unchanged; for a statement range, the constant `extracted`, since there is no expression to read a name from and inventing a verb from statement shapes is guesswork. Uniquified as today. @@ -177,7 +177,7 @@ The ordering is mandatory, not stylistic. `IDELanguageClientImpl.applyActionEdit The new function is emitted **fully indented** at the enclosing declaration's own indentation, separated by one blank line, reusing `detectIndentUnit`, `detectNewline`, `leadingIndentAt` and `positionAt`. Code-action edits bypass the editor's auto-indent and `CMD_FORMAT_CODE` is a no-op for Kotlin. -One exception to re-indenting every line: the interior and closing delimiter of a **raw (triple-quoted) string literal** are emitted byte-for-byte. Their whitespace is part of the literal's value, and the closing delimiter's column sets `trimIndent`'s margin, so shifting either would edit the interior of the moved code (ADR 0013). The candidate carries those literals' spans so the text layer can skip them without needing PSI. +One exception to re-indenting every line: the interior and closing delimiter of a **raw (triple-quoted) string literal** are emitted byte-for-byte. Their whitespace is part of the literal's value, and the closing delimiter's column sets `trimIndent`'s margin, so shifting either would edit the interior of the moved code (ADR 0014). The candidate carries those literals' spans so the text layer can skip them without needing PSI. **R16 - Responsiveness and failure isolation.** As extract variable: one background pass at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine produces the whole plan; the sheet does pure string and offset arithmetic and re-enters no analysis on confirm. Anything thrown in the pipeline degrades to a refusal (`CouldNotAnalyse`) plus a log line, never an uncaught throw - the action framework catches only `IllegalArgumentException` and this runs on a scope with no exception handler. @@ -226,7 +226,7 @@ One exception to re-indenting every line: the interior and closing delimiter of ## Design -Same shape as extract variable, and the same data boundary from [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md): one background pass produces a plain-data plan, the sheet holds no PSI. +Same shape as extract variable, and the same data boundary from [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md): one background pass produces a plain-data plan, the sheet holds no PSI. ``` ExtractMethodAction.execAction (background) lsp/kotlin/actions @@ -285,8 +285,8 @@ The sheet, `prepare()`/`ActionData`, the two-step undo and the new tooltip row a ## Related -- [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code; the principle behind R7-R10 -- [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module +- [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code; the principle behind R7-R10 +- [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module - [kotlin-extract-variable.md](kotlin-extract-variable.md) - ADFA-4826; owns the shared Language section and every primitive reused here - ADFA-5081 - code action edits should be a single undo step (fixes R15's consequence) - ADFA-5082 - support a reassigned outer `var` as the single output (lifts R7's refusal) diff --git a/docs/features/kotlin-extract-variable.md b/docs/features/kotlin-extract-variable.md index c27f5ab1e0..d279416a23 100644 --- a/docs/features/kotlin-extract-variable.md +++ b/docs/features/kotlin-extract-variable.md @@ -6,7 +6,7 @@ Bind the expression at the cursor, or the selected one, to a new local `val`, and replace the occurrences of that expression with the new name. -This is the first *interactive* Kotlin code action: the user chooses an expression, a name, a target scope and whether to replace other occurrences, so it needs a real UI surface rather than a fire-and-forget edit. Where that UI lives is [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do is [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md). +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 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do is [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md). ## Language @@ -222,7 +222,7 @@ The emitted text is **fully indented**: code-action edits bypass the editor's au ## 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. +Per [ADR 0013](../adr/0013-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 @@ -277,8 +277,8 @@ Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotl ## Related -- [ADR 0012](../adr/0012-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module -- [ADR 0013](../adr/0013-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code +- [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module +- [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite unselected code - [ADR 0009](../adr/0009-jetpack-compose-for-new-ui.md) - Compose, UDF, `ViewModel` + `StateFlow` - [ADR 0010](../adr/0010-navigation-resolves-via-analysis-api.md) - the K2 Analysis API as the Kotlin semantic source of truth - [kotlin-extract-method.md](kotlin-extract-method.md) - ADFA-5080, the sibling refactoring diff --git a/docs/features/kotlin-inline-variable.md b/docs/features/kotlin-inline-variable.md new file mode 100644 index 0000000000..fe38480bd8 --- /dev/null +++ b/docs/features/kotlin-inline-variable.md @@ -0,0 +1,291 @@ +# Kotlin inline variable (K2 LSP) + +- **Ticket:** ADFA-4827 (subtask of ADFA-3317; split out of the closed ADFA-3324 "Refactoring") +- **Status:** Implemented. Fourth link in the refactoring stack, based on extract method (ADFA-5080). +- **Module:** `lsp/kotlin` + +Replace the references to a local variable with its initializer, and delete the declaration once nothing needs it. + +The inverse of extract variable (ADFA-4826), and the third interactive Kotlin refactoring. It differs from both extracts in one way that shapes the whole design: it is **subtractive**. Extract adds a declaration next to code the user is looking at; inline rewrites references that are usually *off-screen* and then deletes the line under their finger. Where the UI lives is [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md); what it refuses to do, and what it does only partially, is [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md). + +## Language + +The family glossary lives in [kotlin-extract-variable.md](kotlin-extract-variable.md#language) - *selection*, *text span*, *refactoring plan*, *rewrite span* are defined there and used here unchanged. This feature adds: + +**Target declaration**: +The local variable being inlined - a `KtProperty` with `isLocal` and an initializer. Parameters, loop variables and destructuring entries are not `KtProperty` at all, so they are excluded by construction rather than by a check. +_Avoid_: variable (ambiguous with the references to it), local, symbol. + +**Reference**: +One read of the target declaration inside the enclosing declaration. Deliberately *not* called an occurrence: an **occurrence** in this codebase already means a site structurally equal to an expression (extract variable's `findOccurrences`), which is a different question resolved a different way. Inline matches by *symbol identity*, never by structure. +_Avoid_: occurrence, usage, use site. + +**Cutoff**: +The first offset after the target declaration where the inlined value stops being the value the declaration produced - either a write to the target itself, or a write to a mutable its initializer reads. References before the cutoff are sound; the one exception is a reference inside a body that runs later than its own text position, which is judged separately regardless of where it sits (R6). +_Avoid_: barrier, invalidation point, write boundary. + +**Inlinable reference**: +A reference that may be rewritten: before the cutoff, not deferred, not shadowed, not receiver-shifted, not smart-cast, not unsafe in callee position, and not a write target. Every other reference is left exactly as it is. +_Avoid_: safe reference, valid usage, eligible reference. + +**Partial inline**: +Rewriting the inlinable references while leaving the rest, which necessarily keeps the declaration. The third designed outcome of a refactoring, alongside applying and refusing - see the [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md) amendment. +_Avoid_: partial success, best-effort, incomplete inline. + +**Substitution text**: +The initializer's source text as it lands at one reference - parenthesised per R10, or wrapped as `${...}` when the reference is a string-template entry. One initializer, but not necessarily one substitution text. +_Avoid_: replacement text, inlined value, expansion. + +## Scope + +### In scope + +A local `val` or `var` with an initializer, declared in any executable body - a function body, an accessor, an `init` block, a constructor, or a lambda - in a Kotlin file. Invoked with the cursor on the declaration's name or on any reference to it. + +### Out of scope + +Member properties, top-level properties and anything whose references can leave the file (R2). Inlining those is a multi-file refactoring: the plan model here is one file's text plus one document version, and a `public val`'s callers are not all reachable, let alone editable. + +## Requirements + +**R1 - Trigger.** An "Inline variable" item (`action_inline_variable`) in the editor code-actions menu for Kotlin files, id `ide.editor.lsp.kt.inlineVariable`, tooltip tag `EDITOR_CODE_ACTIONS_KT_INLINE_VARIABLE = "editor.codeactions.kotlin.inlinevariable"` - a new constant in `TooltipTag.kt`, and the tag string is fixed by the out-of-repo tooltips database rather than chosen here. + +As with both extracts: **no `prepare()` visibility gate** - deciding whether the cursor is on an inlinable local needs an analysis session, which `prepare()` runs on the UI thread and must not do - and `requiresUIThread = false` so the cursor is read on a background thread. + +**R2 - Target.** The cursor resolves to exactly one target declaration, from either of two positions: + +- **on the declaration's name** - anywhere in the `total` of `val total = a + b`; +- **on any reference** - the `total` of `println(total)`, resolved to its declaration with `mainReference.resolveToSymbols()`, then required to be a source `KtProperty` in the same file. + +The target must be a `KtProperty` with `isLocal` **and** an initializer. This excludes, without a single dedicated check, function parameters, lambda parameters, `it`, loop variables, `catch` parameters and destructuring entries - none of which is a `KtProperty`. It leaves three refusals to make explicitly, because each is a position a user can reasonably put the cursor in and deserves to be told about: a member or top-level property (`NotALocalVariable`), a local `val` with no initializer (`NoInitializer`, the `val x: Int` then `x = 1` shape, which Kotlin permits), and a destructuring declaration or one of its entries (`DestructuringDeclaration`). + +The destructuring refusal covers the entries and the syntax around them, not the whole node: the initializer is part of it, so `val (p, q) = split(total)` with the caret on `total` resolves `total` normally. + +A caret resting immediately *after* a use - `val y = x| + 1`, `foo(x|)` - is a routine editor position, and there the leaf at the offset is the whitespace or the `)`. Resolution retries one character back, but only when the first attempt refused with `NotAVariable`: every other refusal already names something real at the caret. The declaration position needs no retry, because trailing whitespace is a child of the `KtProperty` and its name-range test still matches. + +Both positions converge on the same target immediately, so there is one analysis pass and one plan shape. The *position* is still recorded on the plan, because R9's mode availability depends on it. + +**R3 - Live offsets and the version guard.** Identical to both extracts: analysis runs against `getCurrentKtFile(path)` fetched *before* entering `project.read`, the plan records the document version on the `RefactoringPlan` supertype, and the action re-reads the live version before emitting edits, refusing on a mismatch. + +**R4 - References.** Every read of the target within the **enclosing declaration** - the named function, accessor, `init` block or constructor whose body contains the declaration, via `enclosingExecutableBody`. A local's scope cannot leave that body, so this search root is complete rather than merely convenient: no index, no other file, no visibility reasoning. + +A reference matches by **symbol identity**, following `Occurrences.kt`'s existing rule - compare the resolved symbol's source PSI to the target `KtProperty` - never by name text, which would match a shadowing declaration's name in a nested scope. + +A reference that is a **write target** (`x = 5`, `x += 1`, `x++`, detected with the existing `isWriteTarget`) is never a reference to inline. It is a *cause* of the cutoff (R5), not a candidate for substitution. + +**Zero references refuses** (`NeverUsed`, naming the variable). With R9's single mode the declaration would otherwise be deleted with nothing inlined, which is a delete-unused-variable action wearing inline's hat, on a line the user may have been about to use. + +**R5 - Cutoff.** The first offset after the declaration where the value changes, from either cause: + +- **a write to the target** - only possible for a `var`; +- **a write to a mutable the initializer reads** - the `val bound = limit + 1` case, where `limit` is reassigned between two references and the two sites no longer hold the same value even though the text is identical. + +Both come from the existing `writeOffsetsFor(candidate, searchRoot)`, called with the initializer as the candidate, plus the target's own write references, filtered to offsets after the declaration's end. + +This matches IntelliJ, whose documented behaviour is that *"the variable must be initialized at declaration; if the initial value is modified somewhere in the code, only the occurrences before modification will be inlined."* The alternative - refusing the whole inline - was rejected: it discards a sound refactoring of the references before the write, and a partial result is what a user coming from a desktop IDE already expects here. + +The cutoff is a purely textual position, and cannot judge a reference whose execution does not follow the text. Two shapes break that correspondence: a body that runs *later* - a lambda, a local function, or a class or object body, the last of these at construction time (`button.setOnClickListener { show(label) }` before a later `index = 1`, where `label` reads `index`) - and a loop body, which runs *again* after everything textually below it, so a reference before the write executes after it on every iteration but the first. Once any write exists at all, such a reference is excluded outright (`DeferredExecution`, R6) rather than judged by where its text falls relative to the cutoff. + +**Known limitation:** the shared `writeOffsetsFor` primitive tests a simple name, so a write through a qualified access - `config.limit = 5` - is not detected as a write at all. A variable whose initializer reads a qualified mutable therefore gets no cutoff. Fixing the shared primitive is out of scope here; extract variable also depends on its current behaviour. + +**R6 - Per-site exclusions.** Five ways a reference is individually unsound. Each **excludes that reference**, leaving it untouched; none refuses the inline, because each is a property of one site rather than of the target. + +- **Out of textual order.** The reference sits inside a body that does not run once, in order, at the offset where its text sits, so it does not read the value the cutoff (R5) would credit it with. Two shapes: a body that runs later - a lambda, a local function, or a class or object body, as in `button.setOnClickListener { show(label) }` followed by a later `index = 1` where `label`'s initializer read `index`, or `var i = 0; val step = i + 1; class L { val y = step }` followed by a later `i = 5`, where `L`'s property initializer runs when `L` is constructed; and a **loop body**, where `val step = i + 1; while (i < 10) { println(step); i += 2 }` puts the reference textually before the write but executes it after the write on every iteration but the first. Once any write exists at all - the cutoff is no longer `Int.MAX_VALUE` - such a reference is excluded outright rather than judged by its textual position relative to the cutoff. A loop that *contains* the declaration is not one of these: the value is recomputed each iteration alongside the reference. Deliberately over-broad: a lambda invoked immediately, `run { show(label) }`, and a reference in a loop whose write sits after the loop are excluded too even though both would have been safe. Over-exclusion is this feature's safe direction. +- **Shadowing.** The initializer reads a name that resolves to something else at the reference. `val a = 1; val x = a + 1; run { val a = 99; f(x) }` would inline to `f(a + 1)` reading the inner `a`. Detected by walking the reference's parents up to the target's own block, checking each intervening scope's declared names - block statements before the site, lambda value parameters and `it`, function parameters, loop and `catch` parameters, destructuring entries, a `when` subject variable, a class or object body - against the set of names the initializer references. The walk also checks the target's own block, restricted to declarations that come after the target's end and before the reference: a declaration before the target is exactly what the initializer legitimately resolves to, which is why the restriction exists. The converse case cannot arise: a local's scope runs to the end of its block, so everything the initializer reads is still in scope at every reference. Only shadowing bites. +- **Receiver shift.** The initializer accesses a member through an implicit receiver, and the reference sits somewhere that introduces a different one. Tested as the conjunction of two cheap questions: does the initializer contain an unqualified reference resolving through an implicit receiver, or a bare `this`; and does anything between the declaration and the reference change the implicit receiver? The bare-`this` half is asked first and separately, because `this` contributes no `KtSimpleNameExpression` - its instance reference is a plain `KtReferenceExpression` - so `val v = this` would leave the simple-name scan with nothing to iterate. The second question has two answers - a receiver-introducing lambda (a `KtFunctionLiteral` whose functional type has a receiver), the `with(other) { ... }` / `apply` / `run` / `buildString` case; and a class or object body, whose own `this` displaces the enclosing one, as in `val label = toString()` referenced inside a later `object : Any() { ... }`. The class-body half is not covered by shadowing: that test compares *declared* names, and an inherited member such as `toString` is declared nowhere. Only both questions together are a problem. This is extract method's `InnerImplicitReceiver` as a per-site exclusion rather than a refusal. +- **Smart cast.** `val b = a.b; if (b != null) b.length` inlines to `a.b.length`, which does not compile - a smart cast needs a stable value, and a property read is not one. Detected with `smartCastInfo` on the reference (`KaDataFlowProvider`); non-null means excluded. +- **Unsafe in callee position.** The reference is the callee of a call and the initializer is anything but a bare name. A lambda or anonymous function - `val f = { n: Int -> n * 2 }` used as `f(3)` - would need `.invoke()` to compile; the rule is not limited to those two shapes, because a callable reference - `val f = ::g` used as `f(3)` - does not parse there at all, and a qualified initializer - `val f = a.b` used as `f(3)` - would silently prefer a member function `b` over `invoke`. Passing the same `f` as an argument (`list.map(f)`) is unaffected and stays inlinable. + +**R7 - An explicit type refuses.** A target declaration carrying an explicit type reference - `val x: Long = 1`, `val s: Any = "text"` - **refuses the whole inline** (`DeclaredTypeIsLoadBearing`, naming the type). + +The declared type participates in the initializer's inference and in overload resolution at every reference: `foo(x)` becomes `foo(1)`, an `Int`, which does not compile, and `val s: Any = "text"` can silently select a different overload. This refuses on the *presence* of the annotation rather than on a comparison against the initializer's type, because the comparison does not work: in `val x: Long = 1` the expected type propagates, so the initializer's `expressionType` **is** `Long` and a naive equality test would call the case safe. Telling the genuinely safe annotations apart needs the initializer's type computed *without* its expected type, which the Analysis API does not offer. + +Deliberately stricter than necessary - `val x: Long = 1L` is refused too - and stated as such per ADR 0014's preference for a stricter rule over a cleverer one. Explicit types on locals are uncommon in idiomatic Kotlin, and the ones that do appear (pinning a supertype, a nullable, a platform type) are usually exactly the load-bearing cases. + +**R8 - Partial inline and the declaration.** The inlinable references (R4-R6) are rewritten; every other reference is left alone. **The declaration is deleted only when nothing is left behind**: every reference was inlined, the target has no writes anywhere, *and* the declaration sits directly in a block. + +The second clause is not redundant. A `var` whose reads were all inlined can still have a later `x = 5` assigning to it, so the declaration is still needed even though no read survives. Removing that write would be dead-store elimination, which is not this refactoring. + +The third clause exists because "every reference was inlined" no longer implies the declaration is safe to remove. A `when` subject variable - `when (val a = compute()) { 1 -> g(a); else -> 0 }` - is a `KtProperty` with `isLocal` true like any other target, so its one reference can be inlined same as any other; but its own text is not a statement, it is the `when`'s subject, so deleting it takes `when (val a = ...)` itself with it and leaves code that does not parse. The fix is a deletion guard, not a refusal: rewriting the reference and keeping the declaration is perfectly sound, so this stays a useful partial-shaped result rather than a decline. + +**Nothing inlinable refuses** (`NothingInlinable`, naming the variable): every reference excluded or past the cutoff means there is no edit to make, and reporting that is better than a no-op. + +**R9 - Modes.** Two, and which are offered depends on the cursor position (R2) and the inlinable count (R8): + +| Cursor on | Offered | +|---|---| +| the declaration's name | all references (no choice - there is no reference at the cursor to single out) | +| a reference, 2+ inlinable | **both** - this reference only, or all references | +| a reference, 1 inlinable | all references (no choice) | +| a reference that is not inlinable | refuses (`ReferenceNotInlinable`) | + +The single-inlinable-reference row collapses deliberately. "This reference only" there would produce the same substitution as "all references" plus a declaration nothing reads - a `variable is never used` warning in generated code, which ADR 0014 forbids emitting. So the case has one honest answer. + +The last row refuses rather than inlining *other* references: rewriting every site except the one under the user's finger reads as the action having done nothing. + +"This reference only" keeps the declaration unconditionally, by definition. + +**R10 - Substitution text.** The initializer's text, wrapped in two situations. + +**Parentheses** are decided by classifying the **initializer alone**, never the reference site: no parentheses when the initializer is a single atomic or postfix expression - a literal, a name, `this`, a qualified chain, a call, an already-parenthesised expression, a lambda - and parentheses for everything else: binary operators, `as`/`is`, `?:`, unary operators, and `if`/`when`/`try` used as an expression. + +Site-sensitive precedence comparison was rejected. It emits marginally cleaner text and has to be right about every parent context - a receiver (`x.length` where `x = a ?: b`), a unary minus over a subtraction, an infix call, a `when` subject - where being wrong emits code that does not compile, and R13 shows no preview on the common path. The cost of the stricter rule is a redundant `return (a + b)`; the cost of the cleverer one is a miscompile the user did not see coming. + +**String templates.** A reference inside a template appears as `$x`, and the short form only accepts a simple name, so `val x = a + b` must become `"total: ${a + b}"` - never `"total: $a + b"`, which silently changes the string. The rule: inside a template entry emit `${...}` unless the substitution text is itself a plain identifier, where `$y` stays short. `true`, `false` and `null` read as identifiers but are keywords, so `"$true"` does not parse - they are rejected explicitly and take the braced form. `this` is the one keyword the short form does accept, and stays short. This makes the template flag a per-reference property of the plan, not a property of the target. + +**R11 - Deleting the declaration.** Three line shapes, all pure span arithmetic: + +- **Alone on its line** - delete from the line start through the line terminator inclusive, leaving no blank line. +- **Sharing its line with real code** - `val x = 1; return g(x)`, or a one-line body `fun f() { val x = 1; g(x) }` - delete the declaration's own span plus a following `;` and one following space if present. Deleting "the line" here would take the `return` or the closing brace with it, which is the defect class extract variable already hit (`ADFA-4826: Decline a block whose statement shares the brace line`). +- **With a trailing comment** - `val total = a + b // running total` - **the comment is preserved** on its own line at the declaration's indentation. + +Preserving the comment is the asymmetry argument: a comment left describing nothing is *visible* and removed with one gesture, while a deleted comment is invisible, and nothing in a diff-less phone UI reports that prose went missing. Comments are the one thing here that cannot be regenerated. A comment on its own line above the declaration is untouched, since only the declaration's own line is ever deleted. + +**R12 - Edit.** One `DocumentChange` carrying **one `TextEdit` per inlined reference plus, when R8 deletes it, one for the declaration, sorted by descending start offset**. + +The ordering is mandatory rather than stylistic, for the reason extract method records: `IDELanguageClientImpl.applyActionEdits` iterates the list in order and applies each edit with **line/column** ranges against the text as it then stands, so an earlier edit must never shift a later one. The declaration precedes every reference, so its deletion always sorts last. Spans never overlap: references are distinct reads, and the declaration's span contains none of them. + +Substitutions are emitted as final text; code-action edits bypass the editor's auto-indent and `CMD_FORMAT_CODE` is a no-op for Kotlin. Nothing re-indents, but nothing needs to - a substitution is intra-line and the deletion removes whole lines. + +**Known consequence:** as with extract method, nothing on this path calls `beginBatchEdit`, so an inline over N references is **N+1 undo entries** and an intermediate undo state does not compile. **ADFA-5081** fixes this properly by batching `applyActionEdits` for every multi-edit action. Working around it here - collapsing everything into one spanning replacement of the whole enclosing declaration - was rejected: it would hide a real bug behind one feature's implementation, rewrite untouched lines, and leave the next multi-edit action to rediscover the problem. + +**R13 - Sheet.** Shown **only when R9 offers a choice**. Every other path applies immediately and reports with `flashInfo` - "Inlined 3 references to `total`", or the partial form naming what was kept. + +`InlineVariableSheet` (a `BottomSheetDialogFragment` hosting a `ComposeView`) and a stateless `InlineVariableSheetContent`, reusing `LabelledSection` / `OptionList` from `refactor/ui/SheetComponents.kt` and `IdeTheme`. **No ViewModel and no UiState**: both extracts need one to own an editable name and a selected scope, and inline has no mutable state at all - an immutable plan, two derived labels, and three events (either mode, or dismissed). A ViewModel holding nothing would be ceremony, and its test would assert that a constant is a constant. + +Contents: title -> the two mode buttons with their derived labels -> the substitution text as one monospace line -> Cancel. + +**The labels are derived by pure functions beside the plan, not composed in the composable**, and unit-tested there - the same discipline as extract method's shared `signatureText`. "Inline all 5 references and delete `total`" versus "Inline 3 of 5 references" is exactly the string that can drift from what the edit does, and R8's deletion rule makes the difference invisible to a reader of the composable. + +**R14 - Refusals and reports.** The plan carries a typed `InlineRefusal` and `postExec` maps it to a specific message. Ten reasons, each naming what is in the way, per ADR 0014: + +| Reason | Message intent | +|---|---| +| `NotAVariable` | place the cursor on a local variable or one of its uses | +| `NotALocalVariable` | only a local variable can be inlined | +| `NoInitializer` | `` has no value at its declaration | +| `DestructuringDeclaration` | a destructuring declaration cannot be inlined | +| `DeclaredTypeIsLoadBearing` | `` is declared ``, and its uses need that type (R7) | +| `NeverUsed` | `` is never used (R4) | +| `NothingInlinable` | no use of `` can be inlined safely (R8) | +| `ReferenceNotInlinable` | this use of `` cannot be inlined safely (R9) | +| `CouldNotAnalyse` | the analysis could not run - deliberately neutral, since the cursor may have been fine | +| `FileChanged` | the file changed while the sheet was open (R3) | + +Plus two success reports: the whole-inline form and the partial form, which must say both counts and that the declaration was kept, because a user who asked to inline everything and got three of five needs to know from the flash rather than by rereading the file. + +`CouldNotAnalyse` exists so the other nine stay truthful - a missing compilation environment or a thrown analysis error must not be reported as `NotAVariable`, which blames a cursor nothing ever looked at. New entries in `resources/.../values/strings.xml`, picked up by the next translation batch. + +Cancellation is not a refusal: the planner re-throws `CancellationException` (`AnalysisPreemptedException` is one), so a cancelled action ends silently. + +**R15 - Responsiveness and failure isolation.** As both extracts: one background pass at `AnalysisPriority.INTERACTIVE` under a cancel checker tied to the action's coroutine builds the whole plan; the sheet does pure string and offset arithmetic and re-enters no analysis on confirm. Anything thrown degrades to `CouldNotAnalyse` plus a log line rather than an uncaught throw, and the sheet's confirm path - outside every guard the action framework provides - wraps its own body in `runCatching`. + +## Non-goals + +- **Member and top-level properties** (R2). A cross-file refactoring with a different plan model. +- **Inline function, inline parameter, inline property accessor.** Separate refactorings; only a local variable is in scope. +- **A purity or side-effect check.** `val n = queue.removeFirst()` with three references inlines into three `removeFirst()` calls. This is deliberate, not an oversight: Kotlin offers nothing to prove purity with, so any check would be a heuristic, and IntelliJ does not check either. The user reads the expression they are inlining and decides. +- **Dead-store elimination** (R8). A `var`'s later write keeps the declaration alive; the write is not removed. +- **Site-sensitive parenthesisation** (R10). +- **Reformatting the result** (R12). Substitution text is emitted final. +- **Atomic undo** of the N+1 edits (R12) - ADFA-5081. +- **A preview on the no-choice paths** (R13). The sheet appears only where there is a decision to make. +- **Java inline variable.** The sibling in `lsp/java`, unticketed. + +## Acceptance criteria + +1. "Inline variable" appears in the code-actions menu of a Kotlin file and is absent in a non-Kotlin file. +2. Cursor on `val total = a + b` with three references inlines all three and removes the declaration line, in one action with no sheet. +3. Cursor on one of those three references offers a choice; picking "this reference only" rewrites exactly that reference and keeps the declaration. +4. Cursor on the single reference of a variable inlines it and deletes the declaration, with no choice offered. +5. `val sum = a + b` referenced in `sum * 2` produces `(a + b) * 2`. +6. `val name = user.name` referenced in `f(name)` produces `f(user.name)` with no parentheses. +7. `val sum = a + b` referenced in `"total: $sum"` produces `"total: ${a + b}"`. +8. `val other = name` referenced in `"hi $other"` produces `"hi $name"`, still in short form. +9. `var count = 1` read twice, then reassigned, then read again inlines the first two reads, keeps the declaration, and reports 2 of 3. +10. `val bound = limit + 1` with `limit` reassigned between two references inlines only the first and keeps the declaration. +11. A `when` subject variable's single reference inlines, but its declaration is never deleted - `when (val a = compute()) { 1 -> g(a); else -> 0 }` keeps `val a = compute()` intact inside the `when`. +12. A reference inside `run { val a = 99; f(x) }`, where the initializer reads an outer `a`, is left untouched and the declaration is kept. +13. A reference inside `with(other) { ... }`, where the initializer uses the enclosing receiver's members, is left untouched. +14. `val b = a.b` used as `if (b != null) b.length` leaves the smart-cast reference untouched. +15. `val x: Long = 1` is refused, and the message names the declared type. +16. A member property is refused with "only a local variable can be inlined". +17. `val x: Int` with a later `x = 1` is refused as having no value at its declaration. +18. A cursor on a destructuring entry is refused specifically, not as "not a variable". +19. An unused local is refused as never used, and the declaration is **not** deleted. +20. A cursor on a reference past the cutoff is refused, and no other reference is rewritten. +21. `val x = 1; return g(x)` on one line inlines to `return g(1)` with the rest of the line intact. +22. `val total = a + b // running total` leaves `// running total` on its own line, correctly indented. +23. Editing the file while the sheet is open, then confirming, reports the file-changed message and leaves the file untouched. +24. Undo restores the file; it currently takes **N+1** undo steps (R12) and intermediate states do not compile. +25. A space-indented file receives space-indented output; a CRLF file keeps CRLF. + +## Design + +Same shape as both extracts, and the same data boundary from [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md): one background pass produces a plain-data plan, the UI holds no PSI. + +```text +InlineVariableAction.execAction (background) lsp/kotlin/actions + server.compilationEnvironmentFor(path) ?: refusal + -> buildInlineVariablePlan(...) utils/refactor/InlineVariablePlanner.kt + ktFile = env.ktSymbolIndex.getCurrentKtFile(path).get() [R3: before project.read] + env.project.read { + analyzeMaybeDangling(INTERACTIVE, cancelChecker) { [R15] + resolveTarget(ktFile, offset) [R2, R7] + references(target, enclosingExecutableBody) [R4] + cutoffAfter(target) [R5] + exclude per site: deferred / shadow / receiver / smartcast / callee [R6] + -> InlineVariablePlan | InlineRefusal [R8, R14] + } + } + <- InlineVariablePlan (plain data, no PSI) + +InlineVariableAction.postExec (UI thread) + refusal -> flashInfo(message for reason) [R14] + no choice -> apply immediately + flashInfo(report) [R9, R13] + choice -> InlineVariableSheet.show refactor/ui [R13] + on apply -> version re-read; mismatch -> refuse [R3] + buildInlineVariableRewrites -> N+1 RewriteSpans utils/refactor/InlineVariableEdit.kt + client.performCodeAction(one DocumentChange, descending) [R12] +``` + +New files, all in `lsp/kotlin`: + +- **`utils/refactor/InlineVariablePlan.kt`** - `InlineVariablePlan` (a `RefactoringPlan` subtype), `InlineReference` (span, template flag, inlinable-or-why), `InlineMode`, `InlineRefusal`, and the derived label/report functions (R13). +- **`utils/refactor/InlineVariablePlanner.kt`** - the single background pass: target resolution, references, cutoff, per-site exclusions (R2-R8, R15). The only analysis-dependent part. +- **`utils/refactor/InlineVariableEdit.kt`** - substitution text, parenthesisation, template wrapping, the three deletion shapes, and the descending edit list (R10-R12). Pure text and offsets. +- **`refactor/ui/InlineVariableSheet.kt`**, **`InlineVariableSheetContent.kt`** - sheet and content only, no ViewModel (R13). +- **`actions/InlineVariableAction.kt`** - registered in `KotlinCodeActionsMenu`; the only class touching the editor, the document version or the language client. +- **`TooltipTag.EDITOR_CODE_ACTIONS_KT_INLINE_VARIABLE`** - one new constant (R1). + +Reused unchanged: `TextSpan`, `RewriteSpan` + `toTextEdit`, `positionAt`, `lineStartOffset`, `leadingIndentAt`, `detectNewline`, `enclosingExecutableBody`, `isWriteTarget`, `writeOffsetsFor`, `collapseForLabel`, `SheetComponents.kt`, `IdeTheme`, and `Occurrences.kt`'s symbol-identity comparison. + +Deliberately **not** reused: `findOccurrences`, `excludeUnsoundOccurrences`, `CandidateExpression`, `ScopeOption` and `AnchorForm`. Inline has no candidate list, no scope chain and no insertion anchor, and its references are found by symbol identity rather than structural equality - the opposite direction from `findOccurrences`. The two families share primitives, not aggregates. `excludeUnsoundOccurrences` is close in spirit to R5 and still not reusable: it grows a contiguous run *outward* from a candidate in both directions, where the cutoff runs *forward* from a fixed declaration. + +Nothing outside `lsp/kotlin` changes except `TooltipTag.kt` and `values/strings.xml`. No new module, no new dependency. + +## Verification + +Unit tests in `:lsp:kotlin` (`flox activate -d flox/local -- ./gradlew :lsp:kotlin:testV7DebugUnitTest`), split so a failure localises to one layer: + +- **`InlineVariablePlanEndToEndTest`** - analysis-backed, one case per rule: both cursor positions (R2), the reference set (R4), the cutoff from each cause (R5), one case per per-site exclusion (R6), the explicit-type refusal (R7), the deletion rule including the `var`-with-a-later-write case and the `when`-subject-variable case (R8), mode availability per row of R9's table, and **one case per refusal reason** (R14). +- **`InlineVariableEditTest`** - pure text: parenthesisation per initializer class, both template forms, the three deletion line shapes, comment preservation on both a tab- and a space-indented fixture, descending edit order, and CRLF preservation (R10-R12). +- **`InlineVariablePlanTest`** - the pure derivations: R9's mode table and R13's labels and reports, against hand-built plans. +- **`RefactorPrimitivesTest`** - extended for whatever R6's scope walk factors out syntactically. +- **`KotlinCodeActionTooltipTagTest`** - one new row (R1). + +Every new scope shape added to R6's shadowing walk needs its own end-to-end case in `InlineVariablePlanEndToEndTest` - that walk is where three separate defects have been found in review (a `when` subject variable, a class or object body, and a redeclaration in the target's own block). + +There is no `ViewModelTest`, because there is no ViewModel (R13); the label and report derivations are tested in `InlineVariablePlanTest` against hand-built plans instead. + +The sheet, `prepare()`/`ActionData`, the N+1 undo and the new tooltip row are not unit-testable; they are covered by on-device QA from the acceptance criteria above, recorded in ADFA-4827's "Steps to QA" field. + +## Related + +- [ADR 0014](../adr/0014-refactorings-decline-rather-than-rewrite.md) - refactorings decline rather than rewrite; amended by this feature to add partial application as a third outcome (R8) +- [ADR 0013](../adr/0013-refactoring-ui-lives-in-the-owning-lsp-module.md) - refactoring UI lives in the owning LSP module +- [kotlin-extract-variable.md](kotlin-extract-variable.md) - ADFA-4826; owns the family Language section and most primitives reused here +- [kotlin-extract-method.md](kotlin-extract-method.md) - ADFA-5080; the branch this one is based on, and the precedent for R12's edit ordering +- ADFA-5081 - code action edits should be a single undo step (fixes R12's consequence) +- ADFA-4825 - semantic rename, the remaining member of the family +- [ARCHITECTURE.md](../../ARCHITECTURE.md) 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 1a46ea1775..5a4eeb4837 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.kt @@ -103,6 +103,7 @@ object TooltipTag { const val EDITOR_CODE_ACTIONS_KT_FIND_REFS = "editor.codeactions.kotlin.findrefs" const val EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE = "editor.codeactions.kotlin.extractvariable" const val EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD = "editor.codeactions.kotlin.extractmethod" + const val EDITOR_CODE_ACTIONS_KT_INLINE_VARIABLE = "editor.codeactions.kotlin.inlinevariable" const val EXIT_TO_MAIN = "exit.to.main" diff --git a/lsp/kotlin/build.gradle.kts b/lsp/kotlin/build.gradle.kts index d25dd4a40a..f5e8bde6b4 100644 --- a/lsp/kotlin/build.gradle.kts +++ b/lsp/kotlin/build.gradle.kts @@ -28,7 +28,7 @@ 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). + // module because `editor` depends on this module, not the reverse (ADR 0013). buildFeatures { compose = true } 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 fbfd5028c3..2e714edcec 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 @@ -12,6 +12,7 @@ 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 +import com.itsaky.androidide.lsp.kotlin.actions.InlineVariableAction import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction @@ -52,5 +53,6 @@ object KotlinCodeActionsMenu : IActionsMenuProvider { ImplementMembersAction(), ExtractVariableAction(), ExtractMethodAction(), + InlineVariableAction(), ) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt index 64e9984774..e14848c71a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ExtractMethodAction.kt @@ -34,7 +34,7 @@ import java.nio.file.Path * [execAction] runs one background analysis pass and returns a plain-data [ExtractMethodPlan]; * [postExec] shows the sheet and turns the user's choice into two text edits with pure offset * arithmetic. Where the region cannot be moved faithfully the plan carries a typed refusal, which - * postExec renders as a specific message rather than a generic failure (ADR 0013). + * postExec renders as a specific message rather than a generic failure (ADR 0014). */ class ExtractMethodAction : BaseKotlinCodeAction() { companion object { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/InlineVariableAction.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/InlineVariableAction.kt new file mode 100644 index 0000000000..d58b66fbe2 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/InlineVariableAction.kt @@ -0,0 +1,278 @@ +package com.itsaky.androidide.lsp.kotlin.actions + +import android.content.Context +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.get +import com.itsaky.androidide.actions.requireContext +import com.itsaky.androidide.actions.requireEditor +import com.itsaky.androidide.actions.requireFile +import com.itsaky.androidide.idetooltips.TooltipTag +import com.itsaky.androidide.lsp.kotlin.KotlinLanguageServer +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.refactor.ui.InlineVariableSheet +import com.itsaky.androidide.lsp.kotlin.refactor.ui.findFragmentActivity +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineMode +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineRefusal +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineReport +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineVariablePlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildInlineVariablePlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.buildInlineVariableRewrites +import com.itsaky.androidide.lsp.kotlin.utils.refactor.reportFor +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 + +/** + * Replaces the references to the local variable at the cursor with its initializer, and removes the + * declaration once nothing needs it. + * + * [execAction] runs one background analysis pass and returns a plain-data [InlineVariablePlan]; + * [postExec] either applies it immediately or, where the mode table leaves a decision, shows the + * sheet. Where a reference or the whole inline cannot be performed faithfully the plan carries a + * typed refusal, which postExec renders as a specific message rather than a generic failure. + */ +class InlineVariableAction : BaseKotlinCodeAction() { + companion object { + /** This action's registration id. */ + const val ID = "ide.editor.lsp.kt.inlineVariable" + } + + override var titleTextRes: Int = R.string.action_inline_variable + override var tooltipTag: String = TooltipTag.EDITOR_CODE_ACTIONS_KT_INLINE_VARIABLE + + override val id: String = ID + override var label: String = "" + + /* Analysis must not run on the UI thread, so the cursor is read at the top of execAction on a + background thread. A torn read while the user is mid-edit can only produce a plan the + document-version guard then refuses to apply. */ + override var requiresUIThread: Boolean = false + + /* Intentionally no prepare() visibility gate: deciding whether the cursor is on an inlinable local + needs a K2 analysis session, far too costly for prepare(). The action stays visible on any Kotlin + file and reports a refusal instead. */ + + override suspend fun execAction(data: ActionData): InlineVariablePlan { + val server = + data.get() + ?: return InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse) + val nioPath = data.requireFile().toPath() + val env = + server.compilationEnvironmentFor(nioPath) + ?: return InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse) + + val cursor = data.requireEditor().cursor + return buildInlineVariablePlan( + env = env, + nioPath = nioPath, + // The selection start: a user who selected the whole name still points at its first character. + offset = minOf(cursor.left, cursor.right), + documentVersion = documentVersionOf(nioPath), + // Ties the analysis to this action's coroutine: cancelling the action aborts the analysis. + cancelChecker = ScheduledCancelChecker(createJobCancelChecker()), + ) + } + + override fun postExec( + data: ActionData, + result: Any, + ) { + super.postExec(data, result) + if (result !is InlineVariablePlan) return + + val context = data.requireContext() + val refusal = result.refusal ?: if (result.modes.isEmpty()) InlineRefusal.CouldNotAnalyse else null + if (refusal != null) { + flashInfo(refusalMessage(context, refusal)) + return + } + + // Only a two-mode choice is a decision; every other path applies immediately. + if (!result.offersChoice) { + applyMode(data, result, result.modes.single()) + return + } + + val activity = + context.findFragmentActivity() + ?: run { + // A wiring problem rather than a user path: the editor is always hosted by one. + logger.warn("No FragmentActivity for the editor context. Cannot show the inline sheet.") + flashError(R.string.msg_cannot_perform_fix) + return + } + + val shown = InlineVariableSheet.show(activity, result) { mode -> applyMode(data, result, mode) } + if (!shown) { + logger.warn("Fragment manager unavailable. Cannot show the inline sheet.") + flashError(R.string.msg_cannot_perform_fix) + } + } + + /** + * Turns the chosen mode into edits and hands them to the language client. + * + * Runs from the sheet's click handler on the choice path, outside `execAction` and so outside every + * guard the action framework provides -- nothing here may throw, hence the [runCatching]. + */ + private fun applyMode( + data: ActionData, + plan: InlineVariablePlan, + mode: InlineMode, + ) { + runCatching { performMode(data, plan, mode) }.onFailure { error -> + logger.error("Failed to apply the inline-variable mode '{}'", mode.name, error) + flashError(R.string.msg_cannot_perform_fix) + } + } + + private fun performMode( + data: ActionData, + plan: InlineVariablePlan, + mode: InlineMode, + ) { + val context = data.requireContext() + val nioPath = data.requireFile().toPath() + // Re-read rather than trust the plan: the editor stays reachable while the sheet is open, and + // applying spans computed against older text would corrupt the file. + if (documentVersionOf(nioPath) != plan.documentVersion) { + flashInfo(refusalMessage(context, InlineRefusal.FileChanged)) + return + } + + val rewrites = + buildInlineVariableRewrites(plan, mode) ?: run { + logger.warn("Could not build an inline-variable rewrite for '{}'", plan.variableName) + flashError(R.string.msg_cannot_perform_fix) + return + } + + val client = + data.languageClient ?: run { + logger.warn("No language client set. Cannot inline variable.") + flashError(R.string.msg_cannot_perform_fix) + return + } + + client.performCodeAction( + CodeActionItem( + title = label, + changes = + listOf( + DocumentChange( + file = nioPath, + // Already in descending document order: applyActionEdits applies these in list order + // with line/column ranges, so an earlier edit must never shift a later one. + edits = rewrites.map { it.toTextEdit(plan.fileText) }, + ), + ), + kind = CodeActionKind.QuickFix, + // The substitutions are emitted final; CMD_FORMAT_CODE is a no-op for Kotlin anyway. + command = Command("", ""), + ), + ) + + flashInfo(reportMessage(context, plan.reportFor(mode))) + } + + /** + * Each refusal names what is in the way; a generic message reads as a broken feature. + * + * Exhaustive with no `else`: a future variant added without a message here is a compile error + * rather than a silent gap. + */ + private fun refusalMessage( + context: Context, + refusal: InlineRefusal, + ): String = + when (refusal) { + InlineRefusal.NotAVariable -> { + context.getString(R.string.msg_inline_variable_not_a_variable) + } + + InlineRefusal.NotALocalVariable -> { + context.getString(R.string.msg_inline_variable_not_local) + } + + is InlineRefusal.NoInitializer -> { + context.getString(R.string.msg_inline_variable_no_initializer, refusal.name) + } + + InlineRefusal.DestructuringDeclaration -> { + context.getString(R.string.msg_inline_variable_destructuring) + } + + is InlineRefusal.DeclaredTypeIsLoadBearing -> { + context.getString(R.string.msg_inline_variable_declared_type, refusal.name, refusal.typeText) + } + + is InlineRefusal.NeverUsed -> { + context.getString(R.string.msg_inline_variable_never_used, refusal.name) + } + + is InlineRefusal.NothingInlinable -> { + context.getString(R.string.msg_inline_variable_nothing_inlinable, refusal.name) + } + + is InlineRefusal.ReferenceNotInlinable -> { + context.getString(R.string.msg_inline_variable_reference_not_inlinable, refusal.name) + } + + InlineRefusal.CouldNotAnalyse -> { + context.getString(R.string.msg_inline_variable_could_not_analyse) + } + + InlineRefusal.FileChanged -> { + context.getString(R.string.msg_inline_variable_file_changed) + } + } + + /** + * The partial form must say both counts and that the declaration was kept: a user who asked to + * inline everything and got three of five needs to know from the flash rather than by rereading the + * file. + */ + private fun reportMessage( + context: Context, + report: InlineReport, + ): String = + when (report) { + is InlineReport.InlinedAndRemoved -> { + context.resources.getQuantityString( + R.plurals.msg_inline_variable_inlined_all, + report.count, + report.count, + report.name, + ) + } + + is InlineReport.InlinedKeepingDeclaration -> { + context.resources.getQuantityString( + R.plurals.msg_inline_variable_inlined_keeping, + report.count, + report.count, + report.name, + ) + } + + is InlineReport.InlinedPartially -> { + context.getString( + R.string.msg_inline_variable_inlined_partially, + report.count, + report.total, + report.name, + ) + } + } + + /** -1 when the document is not open, which never matches a real version and so fails the guard. */ + private fun documentVersionOf(path: Path): Int = FileManager.getActiveDocument(path)?.version ?: -1 +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt index cf0e3cdf92..c77548bda4 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/ExtractMethodSheetContent.kt @@ -23,7 +23,7 @@ import com.itsaky.androidide.resources.R * signature exactly as it will be emitted. * * A sibling of the extract-variable sheet rather than a generalisation of it: a single shared sheet - * would need a state class where half the fields are meaningless to either caller (ADR 0012). + * would need a state class where half the fields are meaningless to either caller (ADR 0013). * * Stateless: all state arrives in [state] and every interaction leaves as an [ExtractMethodUiEvent]. */ diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheet.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheet.kt new file mode 100644 index 0000000000..de6797f797 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheet.kt @@ -0,0 +1,86 @@ +package com.itsaky.androidide.lsp.kotlin.refactor.ui + +import android.os.Bundle +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.ViewCompositionStrategy +import androidx.fragment.app.FragmentActivity +import com.google.android.material.bottomsheet.BottomSheetDialogFragment +import com.itsaky.androidide.common.compose.IdeTheme +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineMode +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineVariablePlan + +/** + * Hosts [InlineVariableSheetContent]. + * + * The plan is handed in directly rather than through fragment arguments: it carries the file's text + * and offset spans, which is neither `Parcelable` nor meaningful to restore -- after process death the + * document may be entirely different. So [plan] is null on a recreated instance and the sheet + * dismisses itself, the same outcome the action's document-version guard would reach anyway. + */ +class InlineVariableSheet : BottomSheetDialogFragment() { + private var plan: InlineVariablePlan? = null + private var onMode: ((InlineMode) -> Unit)? = null + + override fun onCreateView( + inflater: LayoutInflater, + container: ViewGroup?, + savedInstanceState: Bundle?, + ): View? { + val plan = + plan ?: run { + dismissAllowingStateLoss() + return null + } + + return ComposeView(requireContext()).apply { + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { + IdeTheme { + InlineVariableSheetContent( + plan = plan, + onEvent = ::handleEvent, + ) + } + } + } + } + + private fun handleEvent(event: InlineVariableUiEvent) { + when (event) { + is InlineVariableUiEvent.ModeChosen -> { + onMode?.invoke(event.mode) + dismiss() + } + + InlineVariableUiEvent.Dismissed -> { + dismiss() + } + } + } + + companion object { + private const val TAG = "inline_variable_sheet" + + /** + * Shows the sheet on [activity], calling [onMode] once if the user picks a mode. Returns false + * when it could not be shown, so the caller can report a failure rather than doing nothing. + */ + fun show( + activity: FragmentActivity, + plan: InlineVariablePlan, + onMode: (InlineMode) -> Unit, + ): Boolean { + val manager = activity.supportFragmentManager + if (manager.isStateSaved || manager.isDestroyed) return false + InlineVariableSheet() + .apply { + this.plan = plan + this.onMode = onMode + }.show(manager, TAG) + return true + } + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheetContent.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheetContent.kt new file mode 100644 index 0000000000..c3f8d0b041 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/refactor/ui/InlineVariableSheetContent.kt @@ -0,0 +1,121 @@ +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.rememberScrollState +import androidx.compose.foundation.verticalScroll +import androidx.compose.material3.Button +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.font.FontFamily +import androidx.compose.ui.unit.dp +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineLabel +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineMode +import com.itsaky.androidide.lsp.kotlin.utils.refactor.InlineVariablePlan +import com.itsaky.androidide.lsp.kotlin.utils.refactor.labelFor +import com.itsaky.androidide.lsp.kotlin.utils.refactor.substitutionTextFor +import com.itsaky.androidide.resources.R + +/** What the sheet reports back up; it never touches the document itself. */ +sealed interface InlineVariableUiEvent { + /** The user picked [mode]. */ + data class ModeChosen( + val mode: InlineMode, + ) : InlineVariableUiEvent + + /** The user dismissed the sheet without picking a mode. */ + data object Dismissed : InlineVariableUiEvent +} + +/** + * The inline-variable sheet: one button per available mode, and the substitution text. + * + * Stateless and ViewModel-free: there is no mutable state to own -- an immutable plan, two + * derived labels and three events. A ViewModel holding nothing would be ceremony, and its test would + * assert that a constant is a constant. + */ +@Composable +fun InlineVariableSheetContent( + plan: InlineVariablePlan, + onEvent: (InlineVariableUiEvent) -> Unit, + modifier: Modifier = Modifier, +) { + Column( + /* + * Scrollable because everything here grows: at font scale 2.0 both mode labels wrap, and the + * value below renders an arbitrarily long initializer verbatim. Without it the Cancel row is + * pushed off the sheet with no way back to it. + */ + modifier = + modifier + .fillMaxWidth() + .navigationBarsPadding() + .verticalScroll(rememberScrollState()) + .padding(horizontal = 24.dp, vertical = 16.dp), + verticalArrangement = Arrangement.spacedBy(16.dp), + ) { + Text( + text = stringResource(R.string.title_inline_variable), + style = MaterialTheme.typography.titleLarge, + ) + + plan.modes.forEach { mode -> + Button( + onClick = { onEvent(InlineVariableUiEvent.ModeChosen(mode)) }, + modifier = Modifier.fillMaxWidth(), + ) { + Text(plan.labelFor(mode).text()) + } + } + + LabelledSection(stringResource(R.string.label_inline_variable_value)) { + Text( + // The cursor's own reference: the sheet is shown only when the cursor is on an inlinable one. + text = substitutionTextFor(plan, plan.references[plan.cursorReferenceIndex]), + style = MaterialTheme.typography.bodyMedium.copy(fontFamily = FontFamily.Monospace), + modifier = Modifier.fillMaxWidth(), + ) + } + + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.End, + ) { + TextButton(onClick = { onEvent(InlineVariableUiEvent.Dismissed) }) { + Text(stringResource(android.R.string.cancel)) + } + } + } +} + +/** + * The localised form of a label derived beside the plan. The derivation lives with the plan so it + * cannot drift from what the edit does; only the wording lives here. + */ +@Composable +private fun InlineLabel.text(): String = + when (this) { + InlineLabel.ThisReferenceOnly -> { + stringResource(R.string.label_inline_variable_this_reference) + } + + is InlineLabel.AllAndDelete -> { + stringResource(R.string.label_inline_variable_all_and_delete, count, name) + } + + is InlineLabel.AllKeepingDeclaration -> { + stringResource(R.string.label_inline_variable_all_keeping, count, name) + } + + is InlineLabel.PartialKeepingDeclaration -> { + stringResource(R.string.label_inline_variable_partial, count, total, name) + } + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt index 02dc61fe6c..82390257e9 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodEdit.kt @@ -93,7 +93,7 @@ fun buildExtractMethodRewrites( * * A line inside one of [protectedSpans] is emitted byte-for-byte. Those are multi-line string literals, * whose interior whitespace is part of their value, and whose closing delimiter sets `trimIndent`'s - * margin -- moving either edits the interior of the moved code (ADR 0013). + * margin -- moving either edits the interior of the moved code (ADR 0014). */ private fun indentedBodyLines( regionText: String, diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt index 730215ff52..024e4c87cd 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/ExtractMethodPlan.kt @@ -52,7 +52,7 @@ sealed interface CallSiteForm { * * [rawStringSpans] are the raw (triple-quoted) string literals inside the region, in file offsets. * Their interior is whitespace-sensitive, so re-indentation must leave those lines byte-for-byte - * (ADR 0013). + * (ADR 0014). */ data class ExtractMethodCandidate( val label: String, @@ -72,7 +72,7 @@ data class ExtractMethodCandidate( ) /** - * Why a region could not be extracted. A refusal is a designed outcome, not an error (ADR 0013): + * Why a region could not be extracted. A refusal is a designed outcome, not an error (ADR 0014): * each reason gets its own message naming the construct in the way, because a generic one reads as * the feature being broken. */ @@ -141,7 +141,7 @@ sealed interface ExtractionRefusal { /** * A captured value the region uses through a smart cast (R5). Its declared type does not compile * in the new body and its narrowed type does not compile at the call site, so neither emission is - * faithful (ADR 0013). + * faithful (ADR 0014). */ data class SmartCastParameter( val name: String, @@ -160,7 +160,7 @@ sealed interface ExtractionRefusal { * The complete result of the background pass. * * Unlike extract variable's plan this carries a [refusal] rather than merely being empty, because - * "why not" is most of what this refactoring has to say (ADR 0013). [candidates] and [refusal] are + * "why not" is most of what this refactoring has to say (ADR 0014). [candidates] and [refusal] are * mutually exclusive in practice: a non-empty candidate list means at least one region survived. */ data class ExtractMethodPlan( 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 index 1270709429..bc513a8641 100644 --- 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 @@ -146,7 +146,7 @@ private fun KaSession.scopeOptionFor( * * A block body with no declared type returns `Unit`, so a `return` that needs a type neither declared * nor renderable would emit a body that does not compile. Declining is always safe -- the - * decline-rather-than-rewrite principle that ADR 0013 records, landing alongside extract method + * decline-rather-than-rewrite principle that ADR 0014 records, landing alongside extract method * (ADFA-5080). */ private fun KaSession.convertExpressionBodyForm( diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEdit.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEdit.kt new file mode 100644 index 0000000000..d7d53c9ac3 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEdit.kt @@ -0,0 +1,132 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** + * The edits one inline performs: one replacement per inlined reference plus, when the declaration is + * deleted, one for the declaration. + * + * **Descending document order is mandatory, not stylistic.** `IDELanguageClientImpl.applyActionEdits` + * iterates the list and applies each edit with line/column ranges against whatever the text is at + * that moment, so an earlier edit must never shift a later one. The declaration precedes every + * reference, so its deletion always sorts last. Spans never overlap: references are distinct reads and + * the declaration's span contains none of them. + * + * Nothing on that path calls `beginBatchEdit`, so an inline over N references costs the user **N+1** + * undo steps and the intermediate states do not compile. A follow-up change will batch these edits + * into one undo step; collapsing everything into one spanning replacement here was rejected in favor + * of the per-reference edit list described above. + * + * Returns null when there is nothing to rewrite or the offsets cannot be honoured, which the caller + * reports rather than applying. + */ +fun buildInlineVariableRewrites( + plan: InlineVariablePlan, + mode: InlineMode, +): List? { + val text = plan.fileText + val targets = + when (mode) { + InlineMode.ThisReferenceOnly -> { + listOfNotNull(plan.references.getOrNull(plan.cursorReferenceIndex)?.takeIf { it.isInlinable }) + } + + InlineMode.AllReferences -> { + plan.inlinableReferences + } + } + if (targets.isEmpty()) return null + if (targets.any { it.span.end > text.length || it.span.start < 0 }) return null + if (plan.declarationSpan.end > text.length) return null + + val substitutions = targets.map { RewriteSpan(it.span, substitutionTextFor(plan, it)) } + val deletion = + if (mode == InlineMode.AllReferences && plan.canDeleteDeclaration) declarationDeletion(plan) else null + + return (substitutions + listOfNotNull(deletion)).sortedByDescending { it.span.start } +} + +/** + * The initializer's text as it lands at one reference. + * + * Inside a short-form template entry the braces do the delimiting, so the parenthesisation is not + * applied on top: `"total: ${a + b}"`, never `"total: ${(a + b)}"`. The short form survives only for a + * plain identifier, because `$user.name` means `user.toString() + ".name"`. + */ +internal fun substitutionTextFor( + plan: InlineVariablePlan, + reference: InlineReference, +): String { + val value = plan.initializerText + if (reference.isShortTemplateEntry) { + return if (isPlainIdentifier(value)) "\$" + value else "\${" + value + "}" + } + return if (plan.initializerNeedsParentheses) "($value)" else value +} + +/** + * Keywords that read as identifiers but are not: `"$true"` does not parse, so the braced form is the + * only way to substitute one. + * + * `this` is deliberately absent -- `"$this"` is legal Kotlin, the one keyword the short form accepts. + */ +private val KEYWORDS_REJECTED_AFTER_DOLLAR = setOf("true", "false", "null") + +/** Whether [text] is a bare Kotlin identifier, and so legal after a `$` in a template. */ +internal fun isPlainIdentifier(text: String): Boolean { + if (text.isEmpty()) return false + if (text in KEYWORDS_REJECTED_AFTER_DOLLAR) return false + if (!(text[0].isLetter() || text[0] == '_')) return false + return text.all { it.isLetterOrDigit() || it == '_' } +} + +/** + * The deletion of the declaration, in one of three line shapes -- all pure span arithmetic. + * + * Real code on the line means only the declaration's own span goes (plus a following `;` and one + * space): deleting "the line" would take the `return` or the closing brace with it. A trailing comment + * is preserved on its own line at the declaration's indentation, because a comment left describing + * nothing is visible and removed with one gesture, while a deleted comment is invisible. + */ +private fun declarationDeletion(plan: InlineVariablePlan): RewriteSpan { + val text = plan.fileText + val span = plan.declarationSpan + val lineStart = lineStartOffset(text, span.start) + val lineEnd = endOfLineContent(text, span.end) + val prefix = text.substring(lineStart, span.start) + val suffix = text.substring(span.end, lineEnd).trim() + + if (prefix.isNotBlank() || !(suffix.isEmpty() || isWholeLineComment(suffix))) { + var end = span.end + if (text.startsWith(";", end)) end++ + if (text.startsWith(" ", end)) end++ + return RewriteSpan(TextSpan(span.start, end), "") + } + + val newline = detectNewline(text) + val replacement = if (suffix.isEmpty()) "" else leadingIndentAt(text, span.start) + suffix + newline + return RewriteSpan(TextSpan(lineStart, endOfLineWithTerminator(text, lineEnd)), replacement) +} + +/** Whether what follows the declaration on its line is only a comment. */ +private fun isWholeLineComment(suffix: String): Boolean = suffix.startsWith("//") || (suffix.startsWith("/*") && suffix.endsWith("*/")) + +/** The offset of the line terminator at or after [offset], or the end of the text. */ +private fun endOfLineContent( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + while (index < text.length && text[index] != '\n') index++ + // A CRLF file must not leave its lone `\r` behind as line content. + return if (index > offset && text[index - 1] == '\r') index - 1 else index +} + +/** [offset] advanced past the line terminator, so the deletion leaves no blank line. */ +private fun endOfLineWithTerminator( + text: String, + offset: Int, +): Int { + var index = offset.coerceIn(0, text.length) + if (index < text.length && text[index] == '\r') index++ + if (index < text.length && text[index] == '\n') index++ + return index +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlan.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlan.kt new file mode 100644 index 0000000000..70d1e8cdac --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlan.kt @@ -0,0 +1,298 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +/** + * Where the cursor was when the action ran. Recorded because mode availability depends on it: only a + * cursor already sitting on a reference can single that reference out. + */ +enum class InlineCursorPosition { + /** The cursor was on the declaration's own name. */ + Declaration, + + /** The cursor was on one of the target's references. */ + Reference, +} + +/** + * Why one reference cannot be rewritten. Every one of these excludes *that reference* and leaves it + * untouched; none refuses the whole inline, because each is a property of one site. + */ +enum class InlineExclusion { + /** At or after the cutoff: the value the declaration produced no longer holds. */ + PastCutoff, + + /** A name the initializer reads means something else here. */ + Shadowed, + + /** + * The initializer reads through an implicit receiver that something in between replaces: a + * receiver-introducing lambda, or a class or object body whose own `this` displaces it. + */ + ReceiverShift, + + /** The reference is used under a smart cast, which an expression cannot carry. */ + SmartCast, + + /** + * The reference is the callee of a call and the initializer is not a bare name -- a lambda or + * anonymous function would need `.invoke()`, a callable reference does not parse there, and a + * qualified access could silently resolve to a different member than `invoke`. + */ + UnsafeInCalleePosition, + + /** + * The reference is inside a body that does not run once, in order, where its text sits -- a lambda, a + * local function, a class or object body, or a loop body -- so once a write exists the cutoff's + * textual position cannot judge it. + */ + DeferredExecution, +} + +/** + * One read of the target declaration inside the enclosing declaration. + * + * [span] is what the substitution replaces. For a short-form string-template entry (`$x`) it covers + * the whole entry including the `$`, so the substitution can emit either `$name` or `${...}`. + */ +data class InlineReference( + val span: TextSpan, + val isShortTemplateEntry: Boolean, + val exclusion: InlineExclusion?, +) { + /** Whether this reference can be rewritten, meaning nothing excluded it. */ + val isInlinable: Boolean get() = exclusion == null +} + +/** The two things the user can ask for. */ +enum class InlineMode { + /** Rewrites only the reference under the cursor, always keeping the declaration. */ + ThisReferenceOnly, + + /** Rewrites every reference that can be rewritten, removing the declaration when nothing is left behind. */ + AllReferences, +} + +/** + * Why nothing can be inlined. A refusal is a designed outcome, not an error: each reason names what + * is in the way, because a generic message reads as the feature being broken. + */ +sealed interface InlineRefusal { + /** The cursor is not on a variable or one of its uses at all. */ + data object NotAVariable : InlineRefusal + + /** A member or top-level property, whose references can leave the file. */ + data object NotALocalVariable : InlineRefusal + + /** A local declared without a value -- the `val x: Int` then `x = 1` shape, which Kotlin permits. */ + data class NoInitializer( + val name: String, + ) : InlineRefusal + + /** A destructuring declaration or one of its entries. */ + data object DestructuringDeclaration : InlineRefusal + + /** + * An explicit type on the declaration. [typeText] is the annotation as written, so the message + * can name it. + */ + data class DeclaredTypeIsLoadBearing( + val name: String, + val typeText: String, + ) : InlineRefusal + + /** No reference at all: inlining would be a delete-unused-variable action in disguise. */ + data class NeverUsed( + val name: String, + ) : InlineRefusal + + /** Every reference is excluded or past the cutoff, so there is no edit to make. */ + data class NothingInlinable( + val name: String, + ) : InlineRefusal + + /** + * The cursor is on a reference that cannot be rewritten. Rewriting the *other* references + * instead reads as the action having done nothing. + */ + data class ReferenceNotInlinable( + val name: String, + ) : InlineRefusal + + /** + * The analysis could not run -- no compilation environment, no `KtFile`, or something threw. + * Deliberately neutral: the cursor may have been perfectly fine, so it must not be blamed. + */ + data object CouldNotAnalyse : InlineRefusal + + /** The file changed between building the plan and applying it. Raised by the action. */ + data object FileChanged : InlineRefusal +} + +/** + * The complete result of the background pass: plain data, no PSI, so the UI can hold it. + * + * [initializerNeedsParentheses] is decided from the initializer alone during analysis and consumed by + * the edit builder, which stays pure. [cursorReferenceIndex] is -1 when the cursor was on the + * declaration. [canDeleteDeclaration] is the conjunction of three conditions -- every reference + * inlinable, the target never written, *and* the declaration sitting directly in a block -- and is + * honoured only by [InlineMode.AllReferences]. + * + * [declarationSpan] covers the declaration's own text only: the deletion reads what follows it on the + * line to decide what to preserve, so a comment the parser bound to the declaration's tail must be + * outside the span. + */ +data class InlineVariablePlan( + override val fileText: String, + override val documentVersion: Int, + val variableName: String, + val declarationSpan: TextSpan, + val initializerText: String, + val initializerNeedsParentheses: Boolean, + val references: List, + val cursorPosition: InlineCursorPosition, + val cursorReferenceIndex: Int, + val canDeleteDeclaration: Boolean, + val modes: List, + val refusal: InlineRefusal?, +) : RefactoringPlan { + /** The references that can be rewritten. */ + val inlinableReferences: List get() = references.filter { it.isInlinable } + + /** Whether the plan carries a refusal instead of something to apply. */ + val isRefused: Boolean get() = refusal != null + + /** Whether the mode table leaves the user a decision, and so whether the sheet is shown at all. */ + val offersChoice: Boolean get() = modes.size > 1 + + companion object { + /** Builds a plan carrying [refusal] and nothing to apply. */ + fun refused( + refusal: InlineRefusal, + fileText: String = "", + documentVersion: Int = -1, + ) = InlineVariablePlan( + fileText = fileText, + documentVersion = documentVersion, + variableName = "", + declarationSpan = TextSpan(0, 0), + initializerText = "", + initializerNeedsParentheses = false, + references = emptyList(), + cursorPosition = InlineCursorPosition.Declaration, + cursorReferenceIndex = -1, + canDeleteDeclaration = false, + modes = emptyList(), + refusal = refusal, + ) + } +} + +/** + * The mode table. The single-inlinable-reference row collapses deliberately: "this reference only" there + * produces the same substitution as "all references" plus a declaration nothing reads. + * + * A cursor on a reference that is not itself inlinable never reaches here -- the planner refuses with + * [InlineRefusal.ReferenceNotInlinable]. + */ +fun modesFor( + cursorPosition: InlineCursorPosition, + inlinableCount: Int, +): List = + if (cursorPosition == InlineCursorPosition.Reference && inlinableCount >= 2) { + listOf(InlineMode.ThisReferenceOnly, InlineMode.AllReferences) + } else { + listOf(InlineMode.AllReferences) + } + +/** + * What one of the mode buttons says, as data rather than as a string: the plan layer stays free of + * Android resources and the derivation stays unit-testable, while the sheet maps each case to a + * localised string. + */ +sealed interface InlineLabel { + /** Offers rewriting only the reference under the cursor. */ + data object ThisReferenceOnly : InlineLabel + + /** Offers rewriting every reference and removing the declaration. */ + data class AllAndDelete( + val count: Int, + val name: String, + ) : InlineLabel + + /** Offers rewriting every reference, keeping the declaration. */ + data class AllKeepingDeclaration( + val count: Int, + val name: String, + ) : InlineLabel + + /** Offers rewriting [count] of [total] references, keeping the declaration. */ + data class PartialKeepingDeclaration( + val count: Int, + val total: Int, + val name: String, + ) : InlineLabel +} + +/** What the flash says afterwards. Same reasoning as [InlineLabel], in the past tense. */ +sealed interface InlineReport { + /** Every reference was rewritten and the declaration was removed. */ + data class InlinedAndRemoved( + val count: Int, + val name: String, + ) : InlineReport + + /** Every reference was rewritten but the declaration was kept. */ + data class InlinedKeepingDeclaration( + val count: Int, + val name: String, + ) : InlineReport + + /** [count] of [total] references were rewritten and the declaration was kept. */ + data class InlinedPartially( + val count: Int, + val total: Int, + val name: String, + ) : InlineReport +} + +/** + * The label for [mode], derived here rather than composed in the composable. + * + * "Inline all 5 references and remove `total`" versus "Inline 3 of 5 references" is exactly the string + * that can drift from what the edit does, and the deletion rule makes the difference invisible to a + * reader of the composable. + */ +fun InlineVariablePlan.labelFor(mode: InlineMode): InlineLabel = + when (mode) { + InlineMode.ThisReferenceOnly -> { + InlineLabel.ThisReferenceOnly + } + + InlineMode.AllReferences -> { + val count = inlinableReferences.size + when { + count < references.size -> InlineLabel.PartialKeepingDeclaration(count, references.size, variableName) + canDeleteDeclaration -> InlineLabel.AllAndDelete(count, variableName) + else -> InlineLabel.AllKeepingDeclaration(count, variableName) + } + } + } + +/** + * What to report once [mode] has been applied. "This reference only" always keeps the declaration and + * always leaves other references behind, so it is a partial result by definition. + */ +fun InlineVariablePlan.reportFor(mode: InlineMode): InlineReport = + when (mode) { + InlineMode.ThisReferenceOnly -> { + InlineReport.InlinedPartially(1, references.size, variableName) + } + + InlineMode.AllReferences -> { + val count = inlinableReferences.size + when { + count < references.size -> InlineReport.InlinedPartially(count, references.size, variableName) + canDeleteDeclaration -> InlineReport.InlinedAndRemoved(count, variableName) + else -> InlineReport.InlinedKeepingDeclaration(count, variableName) + } + } + } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanner.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanner.kt new file mode 100644 index 0000000000..d4b7a5d6d3 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanner.kt @@ -0,0 +1,604 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.compiler.AbstractCompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling +import com.itsaky.androidide.lsp.kotlin.compiler.read +import org.jetbrains.kotlin.analysis.api.KaSession +import org.jetbrains.kotlin.analysis.api.resolution.KaCallableMemberCall +import org.jetbrains.kotlin.analysis.api.resolution.KaImplicitReceiverValue +import org.jetbrains.kotlin.analysis.api.resolution.successfulCallOrNull +import org.jetbrains.kotlin.analysis.api.symbols.KaAnonymousFunctionSymbol +import org.jetbrains.kotlin.analysis.api.types.KaFunctionType +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.psi.KtArrayAccessExpression +import org.jetbrains.kotlin.psi.KtBlockExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtCallableReferenceExpression +import org.jetbrains.kotlin.psi.KtCatchClause +import org.jetbrains.kotlin.psi.KtClassBody +import org.jetbrains.kotlin.psi.KtClassLiteralExpression +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtCollectionLiteralExpression +import org.jetbrains.kotlin.psi.KtConstantExpression +import org.jetbrains.kotlin.psi.KtDestructuringDeclaration +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.KtLambdaExpression +import org.jetbrains.kotlin.psi.KtLoopExpression +import org.jetbrains.kotlin.psi.KtNamedDeclaration +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtObjectLiteralExpression +import org.jetbrains.kotlin.psi.KtParenthesizedExpression +import org.jetbrains.kotlin.psi.KtPostfixExpression +import org.jetbrains.kotlin.psi.KtProperty +import org.jetbrains.kotlin.psi.KtPropertyAccessor +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.KtSimpleNameStringTemplateEntry +import org.jetbrains.kotlin.psi.KtStringTemplateExpression +import org.jetbrains.kotlin.psi.KtSuperExpression +import org.jetbrains.kotlin.psi.KtThisExpression +import org.jetbrains.kotlin.psi.KtWhenExpression +import org.slf4j.LoggerFactory +import java.nio.file.Path +import kotlin.coroutines.cancellation.CancellationException + +private val logger = LoggerFactory.getLogger("InlineVariablePlanner") + +/** + * Computes the whole [InlineVariablePlan] in one background analysis pass. + * + * The current [KtFile] is fetched *before* entering [read] -- blocking on `getCurrentKtFile(...).get()` + * inside `project.read` deadlocks. + * + * Anything thrown in this pipeline degrades to [InlineRefusal.CouldNotAnalyse] plus a log line: the + * action framework catches only `IllegalArgumentException` and this runs on a scope with no exception + * handler, so an uncaught throw would crash the app. Cancellation is the exception -- it is re-thrown, + * since a cancelled action has no result to report. + */ +internal fun buildInlineVariablePlan( + env: AbstractCompilationEnvironment, + nioPath: Path, + offset: Int, + documentVersion: Int, + cancelChecker: ScheduledCancelChecker, +): InlineVariablePlan = + runCatching { + val ktFile = + env.ktSymbolIndex.getCurrentKtFile(nioPath).get() + ?: return InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse) + + env.project.read { + val fileText = ktFile.text + analyzeMaybeDangling(ktFile, AnalysisPriority.INTERACTIVE, cancelChecker) { + planFor(ktFile, fileText, offset, documentVersion) + } + } + }.getOrElse { error -> + if (error is CancellationException) throw error + logger.warn("Failed to build inline-variable plan for {}", nioPath, error) + InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse) + } + +/** The target declaration and how the cursor found it, or the reason there is none. */ +private sealed interface TargetResolution { + data class Resolved( + val target: KtProperty, + val cursorPosition: InlineCursorPosition, + ) : TargetResolution + + data class Refused( + val refusal: InlineRefusal, + ) : TargetResolution +} + +private fun KaSession.planFor( + ktFile: KtFile, + fileText: String, + offset: Int, + documentVersion: Int, +): InlineVariablePlan { + val resolution = resolveTarget(ktFile, offset) + if (resolution is TargetResolution.Refused) { + return InlineVariablePlan.refused(resolution.refusal, fileText, documentVersion) + } + val resolved = resolution as TargetResolution.Resolved + val target = resolved.target + val name = target.name ?: return InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse, fileText, documentVersion) + + // Order matters: a `val x: Int` with a later `x = 1` carries both an absent initializer and an + // explicit type, and "has no value at its declaration" is the truthful reason. + val initializer = + target.initializer + ?: return InlineVariablePlan.refused(InlineRefusal.NoInitializer(name), fileText, documentVersion) + target.typeReference?.let { typeReference -> + return InlineVariablePlan.refused( + InlineRefusal.DeclaredTypeIsLoadBearing(name, typeReference.text), + fileText, + documentVersion, + ) + } + + val searchRoot = + enclosingExecutableBody(target) + ?: return InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse, fileText, documentVersion) + + val reads = mutableListOf() + val targetWriteOffsets = mutableListOf() + for (candidate in PsiTreeUtil.collectElementsOfType(searchRoot, KtSimpleNameExpression::class.java)) { + if (!resolvesToTarget(candidate, target)) continue + // A write is a *cause* of the cutoff, never a candidate for substitution. + if (candidate.isWriteTarget()) targetWriteOffsets += candidate.textRange.startOffset else reads += candidate + } + + if (reads.isEmpty()) { + return InlineVariablePlan.refused(InlineRefusal.NeverUsed(name), fileText, documentVersion) + } + + val declarationEnd = declarationEndBeforeTrailingComment(target) + val cutoff = cutoffAfter(initializer, searchRoot, targetWriteOffsets, declarationEnd) + + val initializerNames = namesReadBy(initializer) + val initializerUsesImplicitReceiver = readsThroughImplicitReceiver(initializer) + + val references = + reads.map { read -> + val entry = read.parent as? KtSimpleNameStringTemplateEntry + val span = + if (entry != null) { + TextSpan(entry.textRange.startOffset, entry.textRange.endOffset) + } else { + TextSpan(read.textRange.startOffset, read.textRange.endOffset) + } + InlineReference( + span = span, + isShortTemplateEntry = entry != null, + exclusion = + when { + span.start >= cutoff -> InlineExclusion.PastCutoff + + cutoff != Int.MAX_VALUE && runsOutOfTextualOrder(read, target) -> InlineExclusion.DeferredExecution + + isShadowedAt(read, target, initializerNames) -> InlineExclusion.Shadowed + + initializerUsesImplicitReceiver && + changesImplicitReceiverBetween(target, read) -> InlineExclusion.ReceiverShift + + isSmartCast(read) -> InlineExclusion.SmartCast + + initializer !is KtSimpleNameExpression && isCallee(read) -> InlineExclusion.UnsafeInCalleePosition + + else -> null + }, + ) + } + + val cursorReferenceIndex = + if (resolved.cursorPosition == InlineCursorPosition.Reference) { + references.indexOfFirst { offset >= it.span.start && offset <= it.span.end } + } else { + -1 + } + if (resolved.cursorPosition == InlineCursorPosition.Reference) { + val cursorReference = references.getOrNull(cursorReferenceIndex) + // Rewriting every site except the one under the user's finger reads as having done nothing. + if (cursorReference == null || !cursorReference.isInlinable) { + return InlineVariablePlan.refused(InlineRefusal.ReferenceNotInlinable(name), fileText, documentVersion) + } + } + + val inlinable = references.count { it.isInlinable } + if (inlinable == 0) { + /* + * Unlike the other refusals above, the references are already known here, each carrying the + * exclusion that ruled it out -- worth keeping on the plan rather than discarding it the way + * InlineVariablePlan.refused()'s empty-references default would. + */ + return InlineVariablePlan( + fileText = fileText, + documentVersion = documentVersion, + variableName = name, + declarationSpan = TextSpan(target.textRange.startOffset, declarationEnd), + initializerText = initializer.text, + initializerNeedsParentheses = needsParentheses(initializer), + references = references, + cursorPosition = resolved.cursorPosition, + cursorReferenceIndex = cursorReferenceIndex, + canDeleteDeclaration = false, + modes = emptyList(), + refusal = InlineRefusal.NothingInlinable(name), + ) + } + + return InlineVariablePlan( + fileText = fileText, + documentVersion = documentVersion, + variableName = name, + declarationSpan = TextSpan(target.textRange.startOffset, declarationEnd), + initializerText = initializer.text, + initializerNeedsParentheses = needsParentheses(initializer), + references = references, + cursorPosition = resolved.cursorPosition, + cursorReferenceIndex = cursorReferenceIndex, + /* + * The deletion rule's second clause is not redundant: a `var` whose reads were all inlined can + * still have a later `x = 5` assigning to it, so the declaration is still needed. The third + * clause excludes a `when` subject variable and similar shapes: deleting `val a` there takes the + * enclosing `when (val a = ...)` syntax with it, which does not parse. + */ + canDeleteDeclaration = inlinable == references.size && targetWriteOffsets.isEmpty() && target.parent is KtBlockExpression, + modes = modesFor(resolved.cursorPosition, inlinable), + refusal = null, + ) +} + +/** + * The target the cursor points at, from either of the two cursor positions. + * + * A caret resting immediately *after* a use -- `val y = x| + 1`, `foo(x|)` -- is a routine editor + * position, and there the leaf at the offset is the whitespace or the `)`, which resolves to nothing. + * Retrying one character back recovers it. The retry is confined to [InlineRefusal.NotAVariable]: every + * other refusal already names something real at the caret and must not be second-guessed. The + * declaration position needs no retry -- trailing whitespace is a child of the [KtProperty], so its + * name-range test still matches. + */ +private fun KaSession.resolveTarget( + ktFile: KtFile, + offset: Int, +): TargetResolution { + val resolution = resolveTargetAt(ktFile, offset) + if (resolution is TargetResolution.Refused && resolution.refusal == InlineRefusal.NotAVariable && offset > 0) { + return resolveTargetAt(ktFile, offset - 1) + } + return resolution +} + +/** + * One resolution attempt at exactly [offset]. + * + * Function parameters, lambda parameters, `it`, loop variables, `catch` parameters and destructuring + * entries are not [KtProperty] at all, so they are excluded by construction. The three refusals made + * explicitly here are the positions a user can reasonably put the cursor in and deserves to be told + * about. + */ +private fun KaSession.resolveTargetAt( + ktFile: KtFile, + offset: Int, +): TargetResolution { + val leaf = + ktFile.findElementAt(offset) + ?: ktFile.findElementAt(offset - 1) + ?: return TargetResolution.Refused(InlineRefusal.NotAVariable) + + PsiTreeUtil.getParentOfType(leaf, KtDestructuringDeclaration::class.java, false)?.let { destructuring -> + /* + * The initializer is part of the destructuring node, so an unscoped ancestor test refuses a + * perfectly inlinable target: `val (p, q) = split(total)` with the caret on `total`. Only the + * entries and the syntax around them cannot be inlined. + */ + val initializer = destructuring.initializer + if (initializer == null || !PsiTreeUtil.isAncestor(initializer, leaf, false)) { + return TargetResolution.Refused(InlineRefusal.DestructuringDeclaration) + } + } + + val declaration = PsiTreeUtil.getParentOfType(leaf, KtProperty::class.java, false) + val nameRange = declaration?.nameIdentifier?.textRange + if (declaration != null && nameRange != null && offset >= nameRange.startOffset && offset <= nameRange.endOffset) { + if (!declaration.isLocal) return TargetResolution.Refused(InlineRefusal.NotALocalVariable) + return TargetResolution.Resolved(declaration, InlineCursorPosition.Declaration) + } + + val reference = + PsiTreeUtil.getParentOfType(leaf, KtSimpleNameExpression::class.java, false) + ?: return TargetResolution.Refused(InlineRefusal.NotAVariable) + val referenced = + runCatching { + reference.mainReference + ?.resolveToSymbols() + ?.firstNotNullOfOrNull { symbol -> runCatching { symbol.psi }.getOrNull() } + }.getOrNull() ?: return TargetResolution.Refused(InlineRefusal.NotAVariable) + + if (referenced is KtProperty) { + // A reference can resolve into another file (or a library); this plan model is one file's text + // plus one document version, so anything else is out of scope. + if (referenced.containingFile != ktFile) return TargetResolution.Refused(InlineRefusal.NotALocalVariable) + if (!referenced.isLocal) return TargetResolution.Refused(InlineRefusal.NotALocalVariable) + return TargetResolution.Resolved(referenced, InlineCursorPosition.Reference) + } + return TargetResolution.Refused(InlineRefusal.NotAVariable) +} + +/** Whether [reference] resolves to [target], compared by source PSI identity as `Occurrences.kt` does. */ +private fun KaSession.resolvesToTarget( + reference: KtSimpleNameExpression, + target: KtProperty, +): Boolean = + runCatching { + reference.mainReference?.resolveToSymbols()?.any { symbol -> + runCatching { symbol.psi }.getOrNull() === target + } == true + }.getOrDefault(false) + +/** + * The first offset after the declaration where the inlined value stops being the value the + * declaration produced: either a write to the target itself -- only possible for a `var` -- or a + * write to a mutable the initializer reads. + * + * [Int.MAX_VALUE] when nothing writes, so every reference compares as before the cutoff. + */ +private fun KaSession.cutoffAfter( + initializer: KtExpression, + searchRoot: PsiElement, + targetWriteOffsets: List, + declarationEnd: Int, +): Int = + (writeOffsetsFor(initializer, searchRoot) + targetWriteOffsets) + .filter { it >= declarationEnd } + .minOrNull() ?: Int.MAX_VALUE + +/** + * Parenthesisation, decided by classifying the initializer alone: no parentheses for a single + * atomic or postfix expression, parentheses for everything else. + * + * Site-sensitive precedence comparison was rejected: it has to be right about every parent context, + * and no preview is shown on the common path. The cost of the stricter rule is a redundant + * `return (a + b)`; the cost of the cleverer one is a miscompile the user did not see coming. + */ +private fun needsParentheses(initializer: KtExpression): Boolean = + when (initializer) { + is KtConstantExpression, + is KtStringTemplateExpression, + is KtSimpleNameExpression, + is KtThisExpression, + is KtSuperExpression, + is KtCallExpression, + is KtQualifiedExpression, + is KtArrayAccessExpression, + is KtParenthesizedExpression, + is KtLambdaExpression, + is KtNamedFunction, + is KtObjectLiteralExpression, + is KtCollectionLiteralExpression, + is KtCallableReferenceExpression, + is KtClassLiteralExpression, + is KtPostfixExpression, + -> false + + else -> true + } + +/** The names the initializer reads unqualified, which a nested scope could shadow. */ +private fun namesReadBy(initializer: KtExpression): Set = + PsiTreeUtil + .collectElementsOfType(initializer, KtSimpleNameExpression::class.java) + .filterNot { (it.parent as? KtQualifiedExpression)?.selectorExpression === it } + .mapTo(mutableSetOf()) { it.getReferencedName() } + +/** + * Whether a scope between [reference] and the target's own block redeclares a name the initializer + * reads. + * + * The converse case cannot arise: a local's scope runs to the end of its block, so everything the + * initializer reads is still in scope at every reference. Only shadowing bites. + */ +private fun isShadowedAt( + reference: KtSimpleNameExpression, + target: KtProperty, + initializerNames: Set, +): Boolean { + if (initializerNames.isEmpty()) return false + val ceiling = target.parent ?: return false + var child: PsiElement = reference + var scope: PsiElement? = reference.parent + while (scope != null && scope !== ceiling) { + if (declaredNamesIn(scope, child).any { it in initializerNames }) return true + child = scope + scope = scope.parent + } + /* + * The loop above never inspects the ceiling block's own statements. A name the target's own block + * redeclares after the target and before the reference shadows it too -- Kotlin permits this (it + * is a warning, not an error) -- so it needs the same check, bounded to that span only: a + * statement before the target is exactly what the initializer legitimately resolves to. + */ + if (scope === ceiling && ceiling is KtBlockExpression) { + val shadowing = + ceiling.statements + .filter { + it.textRange.startOffset > target.textRange.endOffset && + it.textRange.endOffset <= child.textRange.startOffset + }.flatMapTo(mutableSetOf()) { declaredNamesOf(it) } + if (shadowing.any { it in initializerNames }) return true + } + return false +} + +/** The names [scope] declares that are already in effect at [site]. */ +private fun declaredNamesIn( + scope: PsiElement, + site: PsiElement, +): Set = + when (scope) { + is KtBlockExpression -> { + scope.statements + .filter { it.textRange.endOffset <= site.textRange.startOffset } + .flatMapTo(mutableSetOf()) { declaredNamesOf(it) } + } + + is KtFunctionLiteral -> { + lambdaParameterNames(scope) + } + + is KtNamedFunction -> { + scope.valueParameters.mapNotNullTo(mutableSetOf()) { it.name } + } + + is KtPropertyAccessor -> { + scope.valueParameters.mapNotNullTo(mutableSetOf()) { it.name } + } + + is KtCatchClause -> { + setOfNotNull(scope.catchParameter?.name) + } + + is KtForExpression -> { + val parameter = scope.loopParameter + val entries = parameter?.destructuringDeclaration?.entries?.mapNotNull { it.name } ?: emptyList() + (entries + listOfNotNull(parameter?.name)).toSet() + } + + is KtWhenExpression -> { + setOfNotNull(scope.subjectVariable?.name) + } + + is KtClassBody -> { + scope.declarations.mapNotNullTo(mutableSetOf()) { it.name } + } + + else -> { + emptySet() + } + } + +private fun declaredNamesOf(statement: KtExpression): List = + when (statement) { + is KtDestructuringDeclaration -> statement.entries.mapNotNull { it.name } + is KtNamedDeclaration -> listOfNotNull(statement.name) + else -> emptyList() + } + +/** A lambda with no declared parameters still declares `it`. */ +private fun lambdaParameterNames(lambda: KtFunctionLiteral): Set { + val declared = lambda.valueParameters + /* + * Over-approximates: a zero-argument or receiver lambda (`run`, `with`, `apply`, `buildString`) + * binds no `it`. That only ever leaves a reference alone that could have been rewritten, never + * a wrong rewrite, so this stays syntactic rather than asking the Analysis API for the arity. + */ + if (declared.isEmpty()) return setOf("it") + return declared.flatMapTo(mutableSetOf()) { parameter -> + parameter.destructuringDeclaration?.entries?.mapNotNull { it.name } ?: listOfNotNull(parameter.name) + } +} + +/** + * Whether the initializer reaches a member through an implicit receiver. Half of the receiver-shift + * test; on its own it is perfectly fine. + */ +private fun KaSession.readsThroughImplicitReceiver(initializer: KtExpression): Boolean { + /* + * A bare `this` names the receiver without going through a call, and must be asked *before* the + * simple-name scan rather than inside it: `this` contributes no KtSimpleNameExpression -- its + * instance reference is a plain KtReferenceExpression -- so `val v = this` leaves that scan with an + * empty list, and a check nested in its predicate would never run. + */ + if (PsiTreeUtil.collectElementsOfType(initializer, KtThisExpression::class.java).isNotEmpty()) return true + + return PsiTreeUtil.collectElementsOfType(initializer, KtSimpleNameExpression::class.java).any { reference -> + // A qualified selector already has its receiver written out next to it. + if ((reference.parent as? KtQualifiedExpression)?.selectorExpression === reference) { + return@any false + } + runCatching { + val callSource = + (reference.parent as? KtCallExpression)?.takeIf { it.calleeExpression === reference } ?: reference + val applied = + callSource + .resolveToCall() + ?.successfulCallOrNull>() + ?.partiallyAppliedSymbol + applied?.dispatchReceiver is KaImplicitReceiverValue || + applied?.extensionReceiver is KaImplicitReceiverValue + }.getOrDefault(false) + } +} + +/** + * Whether anything between the declaration and [reference] puts a different implicit receiver in + * scope. The other half of the receiver-shift test. + * + * Two shapes do it: a receiver-introducing lambda -- `with`, `apply`, `run`, `buildString`, a Compose + * scope -- and a class or object body, whose own `this` displaces the enclosing one. The latter is not + * covered by shadowing: `isShadowedAt` compares *declared* names, and an inherited member such as + * `toString` is declared nowhere. + */ +private fun KaSession.changesImplicitReceiverBetween( + target: KtProperty, + reference: KtSimpleNameExpression, +): Boolean { + var current: PsiElement? = reference.parent + while (current != null && !PsiTreeUtil.isAncestor(current, target, false)) { + if (current is KtFunctionLiteral && introducesReceiver(current)) return true + if (current is KtClassOrObject) return true + current = current.parent + } + return false +} + +/** + * Whether [lambda]'s functional type has a receiver. Asked of the anonymous function's symbol first, + * which does not depend on the expected type having propagated to the lambda expression. + */ +private fun KaSession.introducesReceiver(lambda: KtFunctionLiteral): Boolean = + runCatching { + val symbol = lambda.symbol as? KaAnonymousFunctionSymbol + if (symbol?.receiverParameter != null) return true + ((lambda.parent as? KtLambdaExpression)?.expressionType as? KaFunctionType)?.hasReceiver == true + }.getOrDefault(false) + +/** Whether the reference is used under a smart cast, which a property read cannot carry. */ +private fun KaSession.isSmartCast(reference: KtSimpleNameExpression): Boolean = + runCatching { reference.smartCastInfo != null }.getOrDefault(false) + +/** Whether the reference is the callee of a call, rather than a value being passed around. */ +private fun isCallee(reference: KtSimpleNameExpression): Boolean = (reference.parent as? KtCallExpression)?.calleeExpression === reference + +/** + * Whether [reference] sits inside a body that does not run once, in order, at the offset where its + * text sits: a lambda, a local function, a class or object body -- all of which run later, the class + * body at construction time -- or a loop body, which runs again after everything textually below it. + * Once a write exists at all, the cutoff's textual position cannot judge such a reference, so it is + * excluded outright. + * + * A loop that contains the declaration is not one of these: the walk stops at the first ancestor of + * [target], so the value is recomputed on every iteration alongside the reference. + */ +private fun runsOutOfTextualOrder( + reference: KtSimpleNameExpression, + target: KtProperty, +): Boolean { + var current: PsiElement? = reference.parent + while (current != null && !PsiTreeUtil.isAncestor(current, target, false)) { + if (current is KtFunctionLiteral || + current is KtNamedFunction || + current is KtClassOrObject || + current is KtLoopExpression + ) { + return true + } + current = current.parent + } + return false +} + +/** + * Where the declaration's own text ends, with any comment bound to its tail excluded. + * + * The parser binds a comment on the declaration's line into the property, so `textRange.endOffset` + * sits after it. Reporting that as the declaration's end hides the comment from the deletion, which + * then reads the line as having nothing to preserve and takes the comment with it. + */ +private fun declarationEndBeforeTrailingComment(target: KtProperty): Int { + var last: PsiElement? = target.lastChild + while (last is PsiComment || last is PsiWhiteSpace) last = last.prevSibling + return last?.textRange?.endOffset ?: target.textRange.endOffset +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt index 2a05b28bee..9df38e272a 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/MethodSignature.kt @@ -542,7 +542,7 @@ private fun KaSession.resolvedPsi(reference: KtSimpleNameExpression): PsiElement /** * A `var` declared inside the enclosing declaration but outside the region, assigned inside it. - * Kotlin has no `out` parameters, so the faithful emission would shadow a name (R7, ADR 0013). + * Kotlin has no `out` parameters, so the faithful emission would shadow a name (R7, ADR 0014). */ private fun KaSession.reassignedOuterVar( enclosing: KtDeclaration, 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 866ee26d4d..9c3565cb9d 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 @@ -11,6 +11,7 @@ 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 +import com.itsaky.androidide.lsp.kotlin.actions.InlineVariableAction import com.itsaky.androidide.lsp.kotlin.actions.NullSafetyAction import com.itsaky.androidide.lsp.kotlin.actions.OrganizeImportsAction import org.junit.Assert.assertEquals @@ -46,6 +47,7 @@ class KotlinCodeActionTooltipTagTest { TooltipTag.EDITOR_CODE_ACTIONS_KT_SURROUND_TRY_CATCH, ExtractVariableAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_VARIABLE, ExtractMethodAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_EXTRACT_METHOD, + InlineVariableAction.ID to TooltipTag.EDITOR_CODE_ACTIONS_KT_INLINE_VARIABLE, ) assertEquals(expected, actualTags) } diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEditTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEditTest.kt new file mode 100644 index 0000000000..d95f8cf465 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariableEditTest.kt @@ -0,0 +1,508 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The emitted text, with every plan built by hand -- no PSI, no analysis. Assertions are on the + * resulting file text, the only kind that catches an indentation or off-by-one error. + */ +class InlineVariableEditTest { + /** Applies the rewrites in the order they are returned, exactly as the language client does. */ + private fun apply( + text: String, + rewrites: List, + ): String = + rewrites.fold(text) { current, rewrite -> + current.substring(0, rewrite.span.start) + rewrite.newText + current.substring(rewrite.span.end) + } + + /** The span of [fragment], skipping [after] earlier occurrences of it. */ + private fun spanOf( + text: String, + fragment: String, + after: Int = 0, + ): TextSpan { + var start = -1 + repeat(after + 1) { occurrence -> + start = text.indexOf(fragment, start + 1) + require(start >= 0) { "occurrence ${occurrence + 1} of '$fragment' not found" } + } + return TextSpan(start, start + fragment.length) + } + + private fun plan( + fileText: String, + declaration: String, + initializerText: String, + references: List, + initializerNeedsParentheses: Boolean = false, + canDeleteDeclaration: Boolean = true, + cursorReferenceIndex: Int = 0, + name: String = "x", + ) = InlineVariablePlan( + fileText = fileText, + documentVersion = 1, + variableName = name, + declarationSpan = spanOf(fileText, declaration), + initializerText = initializerText, + initializerNeedsParentheses = initializerNeedsParentheses, + references = references, + cursorPosition = InlineCursorPosition.Reference, + cursorReferenceIndex = cursorReferenceIndex, + canDeleteDeclaration = canDeleteDeclaration, + modes = modesFor(InlineCursorPosition.Reference, references.count { it.isInlinable }), + refusal = null, + ) + + private fun reference( + fileText: String, + fragment: String, + after: Int = 0, + isShortTemplateEntry: Boolean = false, + exclusion: InlineExclusion? = null, + ) = InlineReference(spanOf(fileText, fragment, after), isShortTemplateEntry, exclusion) + + @Test + fun `a binary initializer is parenthesised and the declaration line goes`() { + val file = + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\tval sum = a + b\n" + + "\treturn sum * 2\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val sum = a + b", + initializerText = "a + b", + references = listOf(reference(file, "sum", after = 1)), + initializerNeedsParentheses = true, + name = "sum", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + assertNotNull(rewrites) + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\treturn (a + b) * 2\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `an atomic initializer needs no parentheses`() { + val file = + "package p\n" + + "fun demo(user: User): String {\n" + + "\tval name = user.name\n" + + "\treturn f(name)\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val name = user.name", + initializerText = "user.name", + references = listOf(reference(file, "name", after = 2)), + name = "name", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + assertEquals( + "package p\n" + + "fun demo(user: User): String {\n" + + "\treturn f(user.name)\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a template entry is wrapped in braces when the value is not a plain name`() { + val file = + "package p\n" + + "fun demo(a: Int, b: Int): String {\n" + + "\tval sum = a + b\n" + + "\treturn \"total: \$sum\"\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val sum = a + b", + initializerText = "a + b", + references = listOf(reference(file, "\$sum", isShortTemplateEntry = true)), + initializerNeedsParentheses = true, + name = "sum", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + // The braces already delimit the expression, so the parenthesisation is not applied on top. + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): String {\n" + + "\treturn \"total: \${a + b}\"\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a template entry braces a keyword initializer`() { + for (keyword in listOf("true", "false", "null")) { + val file = + "package p\n" + + "fun demo(): String {\n" + + "\tval flag = " + keyword + "\n" + + "\treturn \"flag: \$flag\"\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val flag = " + keyword, + initializerText = keyword, + references = listOf(reference(file, "\$flag", isShortTemplateEntry = true)), + name = "flag", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + // "$true" does not parse: the keyword reads as an identifier but is not one. + assertEquals( + "package p\n" + + "fun demo(): String {\n" + + "\treturn \"flag: \${" + keyword + "}\"\n" + + "}\n", + apply(file, rewrites!!), + ) + } + } + + @Test + fun `a template entry stays in short form for a bare this`() { + val file = + "package p\n" + + "class C {\n" + + "\tfun demo(): String {\n" + + "\t\tval self = this\n" + + "\t\treturn \"me: \$self\"\n" + + "\t}\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val self = this", + initializerText = "this", + references = listOf(reference(file, "\$self", isShortTemplateEntry = true)), + name = "self", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + // "$this" is the one keyword the short form accepts. + assertEquals( + "package p\n" + + "class C {\n" + + "\tfun demo(): String {\n" + + "\t\treturn \"me: \$this\"\n" + + "\t}\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a template entry stays in short form for a plain name`() { + val file = + "package p\n" + + "fun demo(name: String): String {\n" + + "\tval other = name\n" + + "\treturn \"hi \$other\"\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val other = name", + initializerText = "name", + references = listOf(reference(file, "\$other", isShortTemplateEntry = true)), + name = "other", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + assertEquals( + "package p\n" + + "fun demo(name: String): String {\n" + + "\treturn \"hi \$name\"\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a declaration sharing its line keeps the rest of the line`() { + val file = + "package p\n" + + "fun demo(): Int {\n" + + "\tval x = 1; return g(x)\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val x = 1", + initializerText = "1", + references = listOf(reference(file, "x", after = 1)), + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + // Deleting "the line" here would take the `return` with it -- the same defect class the sibling + // extract-variable refactoring already hit. + assertEquals( + "package p\n" + + "fun demo(): Int {\n" + + "\treturn g(1)\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a trailing comment is preserved on its own line at the declaration's indentation`() { + val file = + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\tval total = a + b // running total\n" + + "\treturn total\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val total = a + b", + initializerText = "a + b", + // after = 2: the comment text contains "total" too, so it is the third occurrence. + references = listOf(reference(file, "total", after = 2)), + initializerNeedsParentheses = true, + name = "total", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + "\t// running total\n" + + "\treturn (a + b)\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `a space-indented file keeps its own indentation on the preserved comment`() { + val file = + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + " val total = a + b // running total\n" + + " return total\n" + + "}\n" + val result = + plan( + fileText = file, + declaration = "val total = a + b", + initializerText = "a + b", + // after = 2: the comment text contains "total" too, so it is the third occurrence. + references = listOf(reference(file, "total", after = 2)), + initializerNeedsParentheses = true, + name = "total", + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + + assertEquals( + "package p\n" + + "fun demo(a: Int, b: Int): Int {\n" + + " // running total\n" + + " return (a + b)\n" + + "}\n", + apply(file, rewrites!!), + ) + } + + @Test + fun `edits are sorted descending with the declaration deletion last`() { + val file = + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\tval x = a\n" + + "\treturn x + x\n" + + "}\n" + val first = file.indexOf("x + x") + val references = + listOf( + InlineReference(TextSpan(first, first + 1), false, null), + InlineReference(TextSpan(first + 4, first + 5), false, null), + ) + val result = plan(fileText = file, declaration = "val x = a", initializerText = "a", references = references) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences)!! + + assertEquals(3, rewrites.size) + assertEquals(rewrites.map { it.span.start }.sortedDescending(), rewrites.map { it.span.start }) + assertEquals("", rewrites.last().newText) + assertEquals( + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\treturn a + a\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `this-reference-only rewrites one site and keeps the declaration`() { + val file = + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\tval x = a\n" + + "\treturn x + x\n" + + "}\n" + val first = file.indexOf("x + x") + val references = + listOf( + InlineReference(TextSpan(first, first + 1), false, null), + InlineReference(TextSpan(first + 4, first + 5), false, null), + ) + val result = + plan( + fileText = file, + declaration = "val x = a", + initializerText = "a", + references = references, + cursorReferenceIndex = 1, + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.ThisReferenceOnly)!! + + assertEquals(1, rewrites.size) + assertEquals( + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\tval x = a\n" + + "\treturn x + a\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `an excluded reference is left untouched and the declaration stays`() { + val file = + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\tval x = a\n" + + "\treturn x + x\n" + + "}\n" + val first = file.indexOf("x + x") + val references = + listOf( + InlineReference(TextSpan(first, first + 1), false, null), + InlineReference(TextSpan(first + 4, first + 5), false, InlineExclusion.PastCutoff), + ) + val result = + plan( + fileText = file, + declaration = "val x = a", + initializerText = "a", + references = references, + canDeleteDeclaration = false, + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences)!! + + assertEquals(1, rewrites.size) + assertEquals( + "package p\n" + + "fun demo(a: Int): Int {\n" + + "\tval x = a\n" + + "\treturn a + x\n" + + "}\n", + apply(file, rewrites), + ) + } + + @Test + fun `a CRLF file keeps CRLF when the comment line is re-emitted`() { + val file = + "package p\r\n" + + "fun demo(a: Int): Int {\r\n" + + "\tval x = a // keep me\r\n" + + "\treturn x\r\n" + + "}\r\n" + val at = file.indexOf("return x") + "return ".length + val result = + plan( + fileText = file, + declaration = "val x = a", + initializerText = "a", + references = listOf(InlineReference(TextSpan(at, at + 1), false, null)), + ) + + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences)!! + + assertEquals( + "package p\r\n" + + "fun demo(a: Int): Int {\r\n" + + "\t// keep me\r\n" + + "\treturn a\r\n" + + "}\r\n", + apply(file, rewrites), + ) + } + + @Test + fun `nothing to rewrite returns null rather than an empty edit list`() { + val file = "package p\nfun demo(a: Int) {\n\tval x = a\n}\n" + val result = + plan( + fileText = file, + declaration = "val x = a", + initializerText = "a", + references = emptyList(), + canDeleteDeclaration = false, + ) + + assertNull(buildInlineVariableRewrites(result, InlineMode.AllReferences)) + } + + @Test + fun `a span past the end of the text is refused rather than applied`() { + val file = "package p\nfun demo(a: Int) {\n\tval x = a\n\tg(x)\n}\n" + val result = + plan( + fileText = file, + declaration = "val x = a", + initializerText = "a", + references = listOf(InlineReference(TextSpan(file.length - 1, file.length + 5), false, null)), + ) + + assertNull(buildInlineVariableRewrites(result, InlineMode.AllReferences)) + } + + @Test + fun `a plain identifier is recognised, an expression is not`() { + assertTrue(isPlainIdentifier("name")) + assertTrue(isPlainIdentifier("_x2")) + assertTrue(!isPlainIdentifier("user.name")) + assertTrue(!isPlainIdentifier("a + b")) + assertTrue(!isPlainIdentifier("2fast")) + assertTrue(!isPlainIdentifier("")) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanEndToEndTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanEndToEndTest.kt new file mode 100644 index 0000000000..b1bcf8a3d9 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanEndToEndTest.kt @@ -0,0 +1,1077 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.progress.ICancelChecker +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.CancellationException + +/** + * The parts of the plan that need real resolution: the target from either cursor position, the + * reference set by symbol identity, the cutoff, the deletion rule, the mode table and one case per + * refusal reason. + * + * Where a rewrite is produced the assertion is on the resulting file text, which is the only + * assertion that catches an indentation or off-by-one error. + */ +class InlineVariablePlanEndToEndTest : KtLspTest() { + private fun plan( + content: String, + offset: Int, + ): InlineVariablePlan { + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + return buildInlineVariablePlan(env, path, offset, documentVersion = 1, cancelChecker = noopCancelChecker()) + } + + private fun apply( + text: String, + rewrites: List, + ): String = + rewrites.fold(text) { current, rewrite -> + current.substring(0, rewrite.span.start) + rewrite.newText + current.substring(rewrite.span.end) + } + + /** The offset of [fragment]'s first character, skipping [after] occurrences of it. */ + private fun at( + content: String, + fragment: String, + after: Int = 0, + ): Int { + var index = -1 + repeat(after + 1) { index = content.indexOf(fragment, index + 1) } + return index + } + + @Test + fun `both cursor positions resolve the same target`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + val total = a + b + return total * 2 + } + """.trimIndent() + + val onName = plan(content, at(content, "total")) + val onReference = plan(content, at(content, "total", after = 1)) + + assertEquals("total", onName.variableName) + assertEquals(InlineCursorPosition.Declaration, onName.cursorPosition) + assertEquals(-1, onName.cursorReferenceIndex) + assertEquals("total", onReference.variableName) + assertEquals(InlineCursorPosition.Reference, onReference.cursorPosition) + assertEquals(0, onReference.cursorReferenceIndex) + assertEquals(1, onName.references.size) + assertEquals(listOf(InlineMode.AllReferences), onReference.modes) + } + + @Test + fun `a whole inline rewrites every reference and removes the declaration`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + val total = a + b + return total * 2 + } + """.trimIndent() + + val result = plan(content, at(content, "total")) + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences)!! + + assertTrue(result.canDeleteDeclaration) + assertEquals( + """ + package p + fun demo(a: Int, b: Int): Int { + return (a + b) * 2 + } + """.trimIndent(), + apply(content, rewrites), + ) + } + + @Test + fun `a reference with two or more inlinable offers both modes`() { + val content = + """ + package p + fun demo(a: Int): Int { + val x = a + return x + x + } + """.trimIndent() + + val result = plan(content, at(content, "x", after = 1)) + + assertEquals(2, result.references.size) + assertEquals(listOf(InlineMode.ThisReferenceOnly, InlineMode.AllReferences), result.modes) + assertTrue(result.offersChoice) + } + + @Test + fun `a shadowing declaration's name is not mistaken for a reference`() { + val content = + """ + package p + fun demo(a: Int): Int { + val x = a + val outer = x + return run { + val x = 99 + x + } + } + """.trimIndent() + + val result = plan(content, at(content, "x")) + + // Matching is by symbol identity, never by name text: the inner `val x` and its use belong to a + // different declaration. + assertEquals(1, result.references.size) + assertEquals( + at(content, "x", after = 1), + result.references + .single() + .span.start, + ) + } + + @Test + fun `a var read after its own reassignment is past the cutoff`() { + val content = + """ + package p + fun demo(): Int { + var count = 1 + val a = count + val b = count + count = 2 + return a + b + count + } + """.trimIndent() + + val result = plan(content, at(content, "count")) + + assertEquals(3, result.references.size) + assertEquals(2, result.inlinableReferences.size) + assertEquals(InlineExclusion.PastCutoff, result.references.last().exclusion) + assertEquals(false, result.canDeleteDeclaration) + assertEquals(InlineReport.InlinedPartially(2, 3, "count"), result.reportFor(InlineMode.AllReferences)) + } + + @Test + fun `a write to a mutable the initializer reads sets the cutoff`() { + val content = + """ + package p + fun demo(): Int { + var limit = 1 + val bound = limit + 1 + val first = bound + limit = 5 + val second = bound + return first + second + } + """.trimIndent() + + val result = plan(content, at(content, "bound")) + + assertEquals(2, result.references.size) + assertNull(result.references.first().exclusion) + assertEquals(InlineExclusion.PastCutoff, result.references.last().exclusion) + assertEquals(false, result.canDeleteDeclaration) + } + + @Test + fun `a var with a later write keeps its declaration even when every read is inlined`() { + val content = + """ + package p + fun demo(): Int { + var count = 1 + val a = count + count = 2 + return a + } + """.trimIndent() + + val result = plan(content, at(content, "count")) + + // Removing the write would be dead-store elimination, which is not this refactoring. + assertEquals(1, result.inlinableReferences.size) + assertEquals(false, result.canDeleteDeclaration) + assertEquals( + InlineReport.InlinedKeepingDeclaration(1, "count"), + result.reportFor(InlineMode.AllReferences), + ) + } + + @Test + fun `a reference inside a string template is recorded as a short-form entry`() { + val content = + """ + package p + fun demo(a: Int, b: Int): String { + val sum = a + b + return "total: ${'$'}sum" + } + """.trimIndent() + + val result = plan(content, at(content, "sum")) + val reference = result.references.single() + + assertTrue(reference.isShortTemplateEntry) + // The span covers the whole entry, `$sum`, so the substitution can emit `${a + b}`. + assertEquals(at(content, "${'$'}sum"), reference.span.start) + assertEquals( + """ + package p + fun demo(a: Int, b: Int): String { + return "total: ${'$'}{a + b}" + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } + + @Test + fun `an initializer needing no parentheses is classified as atomic`() { + val content = + """ + package p + class User(val name: String) + fun f(s: String) = s + fun demo(user: User): String { + val name = user.name + return f(name) + } + """.trimIndent() + + // Occurrence count: the class's own `val name` is 0, the local declaration's `name` is 1, its + // `user.name` initializer is 2, so the reference in `f(name)` is 3. + val result = plan(content, at(content, "name", after = 3)) + + assertEquals(false, result.initializerNeedsParentheses) + assertEquals("user.name", result.initializerText) + } + + @Test + fun `an unused local is refused as never used`() { + val content = + """ + package p + fun demo(a: Int) { + val unused = a + } + """.trimIndent() + + assertEquals(InlineRefusal.NeverUsed("unused"), plan(content, at(content, "unused")).refusal) + } + + @Test + fun `a member property is refused as not local`() { + val content = + """ + package p + class C { + val size = 1 + fun demo(): Int = size + } + """.trimIndent() + + assertEquals(InlineRefusal.NotALocalVariable, plan(content, at(content, "size")).refusal) + } + + @Test + fun `a local with no initializer is refused before its declared type is considered`() { + val content = + """ + package p + fun demo(): Int { + val x: Int + x = 1 + return x + } + """.trimIndent() + + // This shape carries an explicit type too, and "has no value at its declaration" + // is the truthful reason. + assertEquals(InlineRefusal.NoInitializer("x"), plan(content, at(content, "x")).refusal) + } + + @Test + fun `an explicit type refuses and names the type`() { + val content = + """ + package p + fun demo(): Long { + val x: Long = 1 + return x + } + """.trimIndent() + + assertEquals( + InlineRefusal.DeclaredTypeIsLoadBearing("x", "Long"), + plan(content, at(content, "x")).refusal, + ) + } + + @Test + fun `a destructuring entry is refused specifically`() { + val content = + """ + package p + fun demo(pair: Pair): Int { + val (first, second) = pair + return first + second + } + """.trimIndent() + + assertEquals( + InlineRefusal.DestructuringDeclaration, + plan(content, at(content, "first")).refusal, + ) + } + + @Test + fun `a cursor on nothing inlinable is refused as not a variable`() { + val content = + """ + package p + fun demo(a: Int): Int { + return a + } + """.trimIndent() + + // A parameter is not a KtProperty, so it is excluded by construction rather than by a check. + assertEquals(InlineRefusal.NotAVariable, plan(content, at(content, "a", after = 1)).refusal) + } + + @Test + fun `a cursor on a reference past the cutoff refuses rather than rewriting the others`() { + val content = + """ + package p + fun demo(): Int { + var count = 1 + val a = count + count = 2 + return count + } + """.trimIndent() + + assertEquals( + InlineRefusal.ReferenceNotInlinable("count"), + plan(content, at(content, "return count") + "return ".length).refusal, + ) + } + + @Test + fun `a file the analysis cannot reach is refused as not analysable`() { + createSourceFile("Main.kt", "package p\n") + val missing = env.sourceRoots.first().resolve("Absent.kt") + + // "Place the cursor on a local variable" would blame a cursor nothing ever looked at. + assertEquals( + InlineRefusal.CouldNotAnalyse, + buildInlineVariablePlan(env, missing, 0, documentVersion = 1, cancelChecker = noopCancelChecker()).refusal, + ) + } + + @Test + fun `cancellation propagates instead of being reported as a refusal`() { + val content = + """ + package p + fun demo(a: Int): Int { + val x = a + return x + } + """.trimIndent() + createSourceFile("Main.kt", content) + val path = env.sourceRoots.first().resolve("Main.kt") + val cancelled = ScheduledCancelChecker(ICancelChecker.CANCELLED) + + assertThrows(CancellationException::class.java) { + buildInlineVariablePlan(env, path, at(content, "x"), documentVersion = 1, cancelChecker = cancelled) + } + } + + @Test + fun `a one-line declaration inlines without taking the rest of the line`() { + val content = + """ + package p + fun g(n: Int) = n + fun demo(): Int { + val x = 1; return g(x) + } + """.trimIndent() + + val result = plan(content, at(content, "x")) + + assertEquals( + """ + package p + fun g(n: Int) = n + fun demo(): Int { + return g(1) + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } + + @Test + fun `a trailing line comment survives the declaration's removal`() { + val content = + """ + package p + fun g(n: Int) = n + fun demo(a: Int, b: Int): Int { + val running = a + b // running total + return g(running) + } + """.trimIndent() + + val result = plan(content, at(content, "running")) + + assertEquals( + """ + package p + fun g(n: Int) = n + fun demo(a: Int, b: Int): Int { + // running total + return g((a + b)) + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } + + @Test + fun `a trailing block comment survives the declaration's removal`() { + val content = + """ + package p + fun g(n: Int) = n + fun demo(a: Int, b: Int): Int { + val running = a + b /* running total */ + return g(running) + } + """.trimIndent() + + val result = plan(content, at(content, "running")) + + assertEquals( + """ + package p + fun g(n: Int) = n + fun demo(a: Int, b: Int): Int { + /* running total */ + return g((a + b)) + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } + + @Test + fun `a when subject variable is inlined but its declaration is never deleted`() { + val content = + """ + package p + fun g(n: Int) = n + fun compute(): Int = 1 + fun demo(): Int { + return when (val a = compute()) { + 1 -> g(a) + else -> 0 + } + } + """.trimIndent() + + val result = plan(content, at(content, "val a") + "val ".length) + + assertEquals(false, result.canDeleteDeclaration) + assertEquals( + """ + package p + fun g(n: Int) = n + fun compute(): Int = 1 + fun demo(): Int { + return when (val a = compute()) { + 1 -> g(compute()) + else -> 0 + } + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } + + @Test + fun `a name redeclared later in the target's own block is not inlined`() { + val content = + """ + package p + fun f(n: Int) = n + fun demo(): Int { + val a = 1 + val x = a + 1 + val a = 99 + return f(x) + } + """.trimIndent() + + val result = plan(content, at(content, "val x") + "val ".length) + + assertEquals(InlineExclusion.Shadowed, result.references.single().exclusion) + } + + @Test + fun `a reference inside a stored lambda is excluded once a write exists`() { + val content = + """ + package p + class Button { + fun setOnClickListener(listener: () -> Unit) {} + } + fun show(s: String) {} + fun demo(button: Button) { + var index = 0 + val label = "item ${'$'}index" + button.setOnClickListener { show(label) } + index = 1 + } + """.trimIndent() + + val result = plan(content, at(content, "val label") + "val ".length) + + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + assertEquals(false, result.canDeleteDeclaration) + } + + @Test + fun `a reference inside a destructuring initializer resolves its own target`() { + val content = + """ + package p + fun split(sep: String): Pair = sep to sep + fun demo(a: String, b: String): String { + val total = a + b + val (p, q) = split(total) + return p + q + } + """.trimIndent() + + val result = plan(content, at(content, "total", after = 1)) + + // The initializer is part of the destructuring node, but only the entries cannot be inlined. + assertNull(result.refusal) + assertEquals("total", result.variableName) + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + assertEquals( + """ + package p + fun split(sep: String): Pair = sep to sep + fun demo(a: String, b: String): String { + val (p, q) = split((a + b)) + return p + q + } + """.trimIndent(), + apply(content, rewrites!!), + ) + } + + @Test + fun `a caret immediately after a reference still resolves it`() { + val content = + """ + package p + fun demo(a: Int, b: Int): Int { + val total = a + b + val y = total + 1 + return y + } + """.trimIndent() + + // The leaf at this offset is the whitespace before the `+`, not the name. + val result = plan(content, at(content, "total", after = 1) + "total".length) + + assertNull(result.refusal) + assertEquals(InlineCursorPosition.Reference, result.cursorPosition) + assertEquals(0, result.cursorReferenceIndex) + } + + @Test + fun `a caret on the closing parenthesis after a reference still resolves it`() { + val content = + """ + package p + fun f(n: Int) = n + fun demo(a: Int, b: Int): Int { + val total = a + b + return f(total) + } + """.trimIndent() + + // The leaf at this offset is the `)`, which has no simple-name ancestor at all. + val result = plan(content, at(content, "total", after = 1) + "total".length) + + assertNull(result.refusal) + assertEquals(InlineCursorPosition.Reference, result.cursorPosition) + assertEquals(0, result.cursorReferenceIndex) + } + + @Test + fun `a reference in a while body is excluded when the loop writes what the initializer reads`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo() { + var i = 0 + val step = i + 1 + while (i < 10) { + println(step) + i += 2 + } + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + /* + * The reference is textually before the write, so a purely textual cutoff would call it + * inlinable and delete the declaration too, turning "1 1 1 1 1" into "1 3 5 7 9". + */ + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + assertEquals(false, result.canDeleteDeclaration) + } + + @Test + fun `a reference in a for body is excluded when the loop writes what the initializer reads`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo(items: List) { + var i = 0 + val step = i + 1 + for (item in items) { + println(step) + i += item + } + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + } + + @Test + fun `a declaration inside the loop body still inlines`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo(items: List) { + var i = 0 + for (item in items) { + val step = i + 1 + println(step) + i += item + } + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + // The value is recomputed on every iteration alongside the reference, so the back edge is moot. + assertNull(result.references.single().exclusion) + val rewrites = buildInlineVariableRewrites(result, InlineMode.AllReferences) + assertEquals( + """ + package p + fun println(n: Int) {} + fun demo(items: List) { + var i = 0 + for (item in items) { + println((i + 1)) + i += item + } + } + """.trimIndent(), + apply(content, rewrites!!), + ) + } + + @Test + fun `a reference in a local class property initializer is excluded once a write exists`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo() { + var i = 0 + val step = i + 1 + class L { + val y = step + } + i = 5 + println(L().y) + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + /* + * The initializer runs when L is constructed, which is after the write, so a purely textual + * cutoff would call the reference inlinable and delete the declaration too, printing 6 instead + * of 1. + */ + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + assertEquals(false, result.canDeleteDeclaration) + } + + @Test + fun `a reference in a local class init block is excluded once a write exists`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo() { + var i = 0 + val step = i + 1 + class L { + init { + println(step) + } + } + i = 5 + L() + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + } + + @Test + fun `a reference in a local class constructor parameter default is excluded once a write exists`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo() { + var i = 0 + val step = i + 1 + class L(val n: Int = step) + i = 5 + println(L().n) + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + } + + @Test + fun `a reference in a local class method body is excluded once a write exists`() { + val content = + """ + package p + fun println(n: Int) {} + fun demo() { + var i = 0 + val step = i + 1 + class L { + fun y() = println(step) + } + i = 5 + L().y() + } + """.trimIndent() + + val result = plan(content, at(content, "val step") + "val ".length) + + assertEquals(InlineExclusion.DeferredExecution, result.references.single().exclusion) + } + + @Test + fun `a reference inside an anonymous object body is left untouched`() { + val content = + """ + package p + fun println(s: String) {} + class A { + fun f() { + val label = toString() + val o = object : Any() { + fun g() = println(label) + } + println(o.toString()) + } + } + """.trimIndent() + + val result = plan(content, at(content, "val label") + "val ".length) + + /* + * The object's own `this` displaces the enclosing one, so `println(toString())` there would + * resolve to the object's inherited toString. Shadowing does not catch it: that test compares + * declared names, and toString is declared nowhere. + */ + assertEquals(InlineExclusion.ReceiverShift, result.references.single().exclusion) + } + + @Test + fun `a bare this initializer is excluded under a receiver lambda`() { + val content = + """ + package p + class Other + fun f(a: Any) {} + class Holder { + fun demo(other: Other) { + val v = this + with(other) { f(v) } + } + } + """.trimIndent() + + val result = plan(content, at(content, "val v") + "val ".length) + + // `f(this)` inside with(other) would resolve to `other`, a silent meaning change. + assertEquals(InlineExclusion.ReceiverShift, result.references.single().exclusion) + } + + @Test + fun `a callable reference initializer in call position is left untouched`() { + val content = + """ + package p + fun g(n: Int) = n + fun demo(): Int { + val f = ::g + return f(3) + } + """.trimIndent() + + val result = plan(content, at(content, "val f") + "val ".length) + + assertEquals(InlineExclusion.UnsafeInCalleePosition, result.references.single().exclusion) + } + + @Test + fun `a shadowed reference is left untouched and the declaration is kept`() { + val content = + """ + package p + fun f(n: Int) = n + fun demo(): Int { + val a = 1 + val x = a + 1 + return run { + val a = 99 + f(x) + } + } + """.trimIndent() + + val result = plan(content, at(content, "val x") + "val ".length) + + // Inlining would produce `f(a + 1)` reading the inner `a`. + assertEquals(1, result.references.size) + assertEquals(InlineExclusion.Shadowed, result.references.single().exclusion) + assertEquals(InlineRefusal.NothingInlinable("x"), result.refusal) + } + + @Test + fun `a when subject variable shadowing the initializer's name is not inlined`() { + val content = + """ + package p + fun f(n: Int) = n + fun demo(): Int { + val a = 1 + val x = a + 1 + return when (val a = 99) { + else -> f(x) + } + } + """.trimIndent() + + val result = plan(content, at(content, "val x") + "val ".length) + + // Inlining would produce `f(a + 1)` reading the subject variable's `a`. + assertEquals(InlineExclusion.Shadowed, result.references.single().exclusion) + } + + @Test + fun `a name shadowed in an anonymous object's body is not inlined`() { + val content = + """ + package p + fun f(n: Int) = n + fun demo(): Int { + val a = 1 + val x = a + 1 + val holder = + object { + val a = 99 + + fun g(): Int = f(x) + } + return holder.g() + } + """.trimIndent() + + val result = plan(content, at(content, "val x") + "val ".length) + + // Inlining would produce `f(a + 1)` reading the object's own `a`. + assertEquals(InlineExclusion.Shadowed, result.references.single().exclusion) + } + + @Test + fun `a reference under a different implicit receiver is left untouched`() { + val content = + """ + package p + class Other { + val label: String = "other" + } + class Holder { + val label: String = "holder" + + fun demo(other: Other): String { + val text = label + "!" + return with(other) { text } + } + } + """.trimIndent() + + val result = plan(content, at(content, "val text") + "val ".length) + + assertEquals(InlineExclusion.ReceiverShift, result.references.single().exclusion) + } + + @Test + fun `an implicit-receiver initializer is fine where no lambda changes the receiver`() { + val content = + """ + package p + class Holder { + val label: String = "holder" + + fun demo(): String { + val text = label + "!" + return text + } + } + """.trimIndent() + + val result = plan(content, at(content, "val text") + "val ".length) + + // Only the conjunction of both questions is a problem; either alone is not. + assertNull(result.references.single().exclusion) + } + + @Test + fun `a receiver lambda is fine where the initializer uses no implicit receiver`() { + val content = + """ + package p + class Other { + val label: String = "other" + } + fun demo(other: Other, prefix: String): String { + val text = prefix + "!" + return with(other) { text } + } + """.trimIndent() + + val result = plan(content, at(content, "val text") + "val ".length) + + // The lambda does introduce a receiver, but the initializer reads only a parameter, so there is + // nothing for the receiver shift to break. + assertNull(result.references.single().exclusion) + } + + @Test + fun `a smart-cast reference is left untouched`() { + val content = + """ + package p + class Box(val value: String?) + fun demo(box: Box): Int { + val b = box.value + return if (b != null) b.length else 0 + } + """.trimIndent() + + val result = plan(content, at(content, "val b") + "val ".length) + + // `box.value.length` does not compile: a smart cast needs a stable value. + assertEquals(2, result.references.size) + assertNull(result.references.first().exclusion) + assertEquals(InlineExclusion.SmartCast, result.references.last().exclusion) + assertEquals(false, result.canDeleteDeclaration) + } + + @Test + fun `a lambda initializer in call position is left untouched`() { + val content = + """ + package p + fun demo(): Int { + val f = { n: Int -> n * 2 } + return f(3) + } + """.trimIndent() + + val result = plan(content, at(content, "val f") + "val ".length) + + // The substitution would be a lambda literal in call position, which needs `.invoke()`. + assertEquals(InlineExclusion.UnsafeInCalleePosition, result.references.single().exclusion) + assertEquals(InlineRefusal.NothingInlinable("f"), result.refusal) + } + + @Test + fun `a lambda initializer passed as an argument stays inlinable`() { + val content = + """ + package p + fun call(f: (Int) -> Int): Int = f(1) + fun demo(): Int { + val f = { n: Int -> n * 2 } + return call(f) + } + """.trimIndent() + + val result = plan(content, at(content, "val f") + "val ".length) + + assertNull(result.references.single().exclusion) + assertEquals( + """ + package p + fun call(f: (Int) -> Int): Int = f(1) + fun demo(): Int { + return call({ n: Int -> n * 2 }) + } + """.trimIndent(), + apply(content, buildInlineVariableRewrites(result, InlineMode.AllReferences)!!), + ) + } +} diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanTest.kt new file mode 100644 index 0000000000..33a69a0b63 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/refactor/InlineVariablePlanTest.kt @@ -0,0 +1,133 @@ +package com.itsaky.androidide.lsp.kotlin.utils.refactor + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The plan's pure derivations: the mode table and the derived labels and reports. Analysis-free, so + * the strings the user reads are pinned without a compilation environment. + */ +class InlineVariablePlanTest { + private fun plan( + references: List, + canDeleteDeclaration: Boolean = true, + cursorPosition: InlineCursorPosition = InlineCursorPosition.Declaration, + cursorReferenceIndex: Int = -1, + ) = InlineVariablePlan( + fileText = "", + documentVersion = 1, + variableName = "total", + declarationSpan = TextSpan(0, 0), + initializerText = "a + b", + initializerNeedsParentheses = true, + references = references, + cursorPosition = cursorPosition, + cursorReferenceIndex = cursorReferenceIndex, + canDeleteDeclaration = canDeleteDeclaration, + modes = modesFor(cursorPosition, references.count { it.isInlinable }), + refusal = null, + ) + + private fun reference(exclusion: InlineExclusion? = null) = + InlineReference(span = TextSpan(0, 0), isShortTemplateEntry = false, exclusion = exclusion) + + @Test + fun `a cursor on the declaration offers only all references`() { + assertEquals(listOf(InlineMode.AllReferences), modesFor(InlineCursorPosition.Declaration, 3)) + } + + @Test + fun `a cursor on a reference with two or more inlinable offers both modes`() { + assertEquals( + listOf(InlineMode.ThisReferenceOnly, InlineMode.AllReferences), + modesFor(InlineCursorPosition.Reference, 2), + ) + } + + @Test + fun `a cursor on a reference with one inlinable collapses to all references`() { + // "This reference only" would leave a declaration nothing reads -- a `never used` warning in + // generated code that this refactoring must decline rather than emit. + assertEquals(listOf(InlineMode.AllReferences), modesFor(InlineCursorPosition.Reference, 1)) + } + + @Test + fun `the all-references label says the declaration goes when nothing is left behind`() { + val result = plan(listOf(reference(), reference(), reference())) + + assertTrue(result.offersChoice.not()) + assertEquals(InlineLabel.AllAndDelete(3, "total"), result.labelFor(InlineMode.AllReferences)) + } + + @Test + fun `the all-references label keeps the declaration when a write survives`() { + val result = plan(listOf(reference(), reference()), canDeleteDeclaration = false) + + assertEquals(InlineLabel.AllKeepingDeclaration(2, "total"), result.labelFor(InlineMode.AllReferences)) + } + + @Test + fun `the all-references label states both counts for a partial inline`() { + val result = plan(listOf(reference(), reference(InlineExclusion.PastCutoff)), canDeleteDeclaration = false) + + assertEquals( + InlineLabel.PartialKeepingDeclaration(1, 2, "total"), + result.labelFor(InlineMode.AllReferences), + ) + } + + @Test + fun `this-reference-only has a fixed label and always keeps the declaration`() { + val result = + plan( + listOf(reference(), reference()), + cursorPosition = InlineCursorPosition.Reference, + cursorReferenceIndex = 0, + ) + + assertEquals(InlineLabel.ThisReferenceOnly, result.labelFor(InlineMode.ThisReferenceOnly)) + assertEquals( + InlineReport.InlinedPartially(1, 2, "total"), + result.reportFor(InlineMode.ThisReferenceOnly), + ) + } + + @Test + fun `the whole-inline report names the count and the removed declaration`() { + val result = plan(listOf(reference(), reference(), reference())) + + assertEquals(InlineReport.InlinedAndRemoved(3, "total"), result.reportFor(InlineMode.AllReferences)) + } + + @Test + fun `the partial report says both counts`() { + val result = + plan( + listOf(reference(), reference(), reference(InlineExclusion.SmartCast)), + canDeleteDeclaration = false, + ) + + assertEquals(InlineReport.InlinedPartially(2, 3, "total"), result.reportFor(InlineMode.AllReferences)) + } + + @Test + fun `a report distinguishes all-inlined-but-kept from partial`() { + val result = plan(listOf(reference(), reference()), canDeleteDeclaration = false) + + assertEquals( + InlineReport.InlinedKeepingDeclaration(2, "total"), + result.reportFor(InlineMode.AllReferences), + ) + } + + @Test + fun `a refused plan carries no references and no modes`() { + val refused = InlineVariablePlan.refused(InlineRefusal.CouldNotAnalyse) + + assertTrue(refused.isRefused) + assertFalse(refused.offersChoice) + assertEquals(emptyList(), refused.references) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 6bdd55490b..6600dc9e9d 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -560,6 +560,34 @@ The selection uses the property\'s backing field, which only exists inside this accessor The selection uses %1$s under a smart cast that does not hold outside the selection The selection uses %1$s, which goes out of scope once the selection moves + + + Inline variable + Inline variable + Value + Inline this reference only + Inline all %1$d references and remove %2$s + Inline all %1$d references, keeping %2$s + Inline %1$d of %2$d references, keeping %3$s + Place the cursor on a local variable or one of its uses + Only a local variable can be inlined + %1$s has no value at its declaration + A destructuring declaration cannot be inlined + %1$s is declared %2$s, and its uses need that type + %1$s is never used + No use of %1$s can be inlined safely + This use of %1$s cannot be inlined safely + Could not analyse the file. Try again. + The file changed. Try inlining again. + + Inlined %1$d reference to %2$s and removed the declaration + Inlined %1$d references to %2$s and removed the declaration + + + Inlined %1$d reference to %2$s, keeping the declaration + Inlined %1$d references to %2$s, keeping the declaration + + Inlined %1$d of %2$d references to %3$s, keeping the declaration Select fields No fields selected No fields found