-
Notifications
You must be signed in to change notification settings - Fork 376
Add Inline temp variable code action #2529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
amanthatdoescares
wants to merge
3
commits into
swiftlang:main
Choose a base branch
from
amanthatdoescares:feature/inline-temp-variable
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
232 changes: 232 additions & 0 deletions
232
Sources/SwiftLanguageService/CodeActions/InlineTempVariable.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors | ||
| // Licensed under Apache License v2.0 with Runtime Library Exception | ||
| // | ||
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| @_spi(SourceKitLSP) import LanguageServerProtocol | ||
| import SourceKitLSP | ||
| import SwiftExtensions | ||
| import SwiftSyntax | ||
|
|
||
| /// Inline temp variable: replace a temporary `let` binding with its value at all usage sites, | ||
| /// then remove the declaration. | ||
| /// | ||
| /// ## Before | ||
| /// ```swift | ||
| /// func example() { | ||
| /// let basePrice = item.price | ||
| /// let total = basePrice * quantity | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// ## After | ||
| /// ```swift | ||
| /// func example() { | ||
| /// let total = item.price * quantity | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// When the inlined value is an expression that may need parentheses for precedence (e.g. `1 + 2` | ||
| /// inlined into `basePrice * 3`), parentheses are added: `(1 + 2) * 3`. | ||
| @_spi(Testing) public struct InlineTempVariable: SyntaxCodeActionProvider { | ||
| static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] { | ||
| guard let (variableDecl, name, initializer, codeBlock, declItem) = findInlineableBinding(in: scope) else { | ||
| return [] | ||
| } | ||
|
|
||
| let references = collectReferences(to: name, after: variableDecl.endPosition, in: codeBlock) | ||
| guard !references.isEmpty else { | ||
| return [] | ||
| } | ||
|
|
||
| let snapshot = scope.snapshot | ||
| var textEdits: [TextEdit] = [] | ||
|
|
||
| // Replace each reference with the initializer value, adding parentheses when needed for precedence. | ||
| // Preserve the original trailing trivia (eg. spaces before the following operator) so surrounding | ||
| // spacing stays unchanged. | ||
| for ref in references { | ||
| let token = ref.baseName | ||
| let replacementCore = replacementTextForInlining( | ||
| initializer: initializer, | ||
| at: ref, | ||
| in: codeBlock | ||
| ) | ||
| let replacementText = replacementCore + token.trailingTrivia.description | ||
| textEdits.append( | ||
| TextEdit( | ||
| range: snapshot.range(of: token), | ||
| newText: replacementText | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| // Remove the declaration (the entire code block item so we remove the newline too). | ||
| textEdits.append(TextEdit( | ||
| range: snapshot.range(of: declItem), | ||
| newText: "" | ||
| )) | ||
|
|
||
| // Apply edits from end to start so earlier edits don't invalidate positions. | ||
| textEdits.sort { $0.range.lowerBound > $1.range.lowerBound } | ||
|
|
||
| return [ | ||
| CodeAction( | ||
| title: "Inline variable", | ||
| kind: .refactorInline, | ||
| edit: WorkspaceEdit(changes: [snapshot.uri: textEdits]) | ||
| ) | ||
| ] | ||
| } | ||
|
|
||
| /// Finds a `let name = expr` binding that can be inlined, and its enclosing code block and item. | ||
| private static func findInlineableBinding(in scope: SyntaxCodeActionScope) | ||
| -> (VariableDeclSyntax, name: String, initializer: ExprSyntax, CodeBlockSyntax, CodeBlockItemSyntax)? { | ||
| guard let node = scope.innermostNodeContainingRange else { | ||
| return nil | ||
| } | ||
|
|
||
| let variableDecl = node.findParentOfSelf( | ||
| ofType: VariableDeclSyntax.self, | ||
| stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) } | ||
| ) | ||
| guard let variableDecl, variableDecl.bindingSpecifier.tokenKind == .keyword(.let) else { | ||
| return nil | ||
| } | ||
|
|
||
| guard let binding = variableDecl.bindings.only, | ||
| let pattern = binding.pattern.as(IdentifierPatternSyntax.self), | ||
| let initializer = binding.initializer?.value | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| let name = pattern.identifier.text | ||
| guard !name.isEmpty else { | ||
| return nil | ||
| } | ||
|
|
||
| guard let codeBlockItem = variableDecl.parent?.as(CodeBlockItemSyntax.self), | ||
| let codeBlockItemList = codeBlockItem.parent?.as(CodeBlockItemListSyntax.self), | ||
| let codeBlock = codeBlockItemList.parent?.as(CodeBlockSyntax.self) | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| return (variableDecl, name, initializer, codeBlock, codeBlockItem) | ||
| } | ||
|
|
||
| /// Collects all `DeclReferenceExprSyntax` in `block` that reference `name` and occur after `afterPosition`. | ||
| private static func collectReferences( | ||
| to name: String, | ||
| after afterPosition: AbsolutePosition, | ||
| in block: CodeBlockSyntax | ||
| ) -> [DeclReferenceExprSyntax] { | ||
| let collector = DeclReferenceCollector(name: name, afterPosition: afterPosition) | ||
| collector.walk(block) | ||
| return collector.references | ||
| } | ||
|
|
||
| /// Returns the text to use when inlining `initializer` at the given reference site. | ||
| /// Adds parentheses when needed to preserve precedence (e.g. `1 + 2` inlined into `x * 3` → `(1 + 2) * 3`). | ||
| private static func replacementTextForInlining( | ||
| initializer: ExprSyntax, | ||
| at reference: DeclReferenceExprSyntax, | ||
| in codeBlock: CodeBlockSyntax | ||
| ) -> String { | ||
| let needsParens = initializerNeedsParenthesesAtUseSite(initializer, reference: reference) | ||
| if needsParens { | ||
| return "(\(initializer.trimmed))" | ||
| } | ||
| return initializer.trimmed.description | ||
| } | ||
|
|
||
| /// Returns true if the initializer expression should be wrapped in parentheses when inlined at the reference. | ||
| /// This preserves correctness when the initializer contains operators with lower precedence than the context | ||
| /// (e.g. inlining `1 + 2` into `basePrice * 3` must yield `(1 + 2) * 3`). | ||
| private static func initializerNeedsParenthesesAtUseSite( | ||
| _ initializer: ExprSyntax, | ||
| reference: DeclReferenceExprSyntax | ||
| ) -> Bool { | ||
| // Simple expressions (literals, single identifiers, member access) don't need parens. | ||
| if !initializer.isCompositeForInlining { | ||
| return false | ||
| } | ||
|
|
||
| // Walk up the ancestor chain: the reference may be nested (e.g. inside LabeledExprSyntax in a tuple), | ||
| // so we need to find if we're used as an operand in a binary/sequence expression. | ||
| var node: Syntax? = Syntax(reference) | ||
| while let n = node { | ||
| if n.is(CodeBlockItemSyntax.self) || n.is(CodeBlockSyntax.self) || n.is(MemberBlockItemSyntax.self) { | ||
| break | ||
| } | ||
| if n.is(InfixOperatorExprSyntax.self) || n.is(SequenceExprSyntax.self) { | ||
| return true | ||
| } | ||
| if n.is(TernaryExprSyntax.self) || n.is(FunctionCallExprSyntax.self) || n.is(SubscriptCallExprSyntax.self) { | ||
| return true | ||
| } | ||
| if n.is(AwaitExprSyntax.self) || n.is(TryExprSyntax.self) { | ||
| return true | ||
| } | ||
| node = n.parent | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| } | ||
|
|
||
| // MARK: - DeclReferenceCollector | ||
|
|
||
| private final class DeclReferenceCollector: SyntaxVisitor { | ||
| private let name: String | ||
| private let afterPosition: AbsolutePosition | ||
| private(set) var references: [DeclReferenceExprSyntax] = [] | ||
|
|
||
| init(name: String, afterPosition: AbsolutePosition) { | ||
| self.name = name | ||
| self.afterPosition = afterPosition | ||
| super.init(viewMode: .sourceAccurate) | ||
| } | ||
|
|
||
| override func visit(_ node: DeclReferenceExprSyntax) -> SyntaxVisitorContinueKind { | ||
| if node.baseName.text == name, node.position >= afterPosition { | ||
| references.append(node) | ||
| } | ||
| return .visitChildren | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Helpers | ||
|
|
||
| private extension ExprSyntax { | ||
| /// Whether this expression is "composite" for the purpose of inlining: if inlined into another | ||
| /// expression, it may need parentheses to preserve meaning (e.g. `1 + 2` in `x * 3`). | ||
| var isCompositeForInlining: Bool { | ||
| switch self.kind { | ||
| case .arrayExpr, .booleanLiteralExpr, .closureExpr, .declReferenceExpr, .dictionaryExpr, | ||
| .floatLiteralExpr, .forceUnwrapExpr, .functionCallExpr, .integerLiteralExpr, .memberAccessExpr, | ||
| .nilLiteralExpr, .optionalChainingExpr, .postfixOperatorExpr, .stringLiteralExpr, .superExpr, | ||
| .subscriptCallExpr: | ||
| return false | ||
| case .tupleExpr: | ||
| if let single = self.as(TupleExprSyntax.self)?.elements.only, single.label == nil { | ||
| return single.expression.isCompositeForInlining | ||
| } | ||
| return true | ||
| default: | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| var trimmed: ExprSyntax { | ||
| self.with(\.leadingTrivia, []).with(\.trailingTrivia, []) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors | ||
| // Licensed under Apache License v2.0 with Runtime Library Exception | ||
| // | ||
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import LanguageServerProtocol | ||
| import SKTestSupport | ||
| import SKUtilities | ||
| import SourceKitLSP | ||
| import XCTest | ||
|
|
||
| private typealias CodeActionCapabilities = TextDocumentClientCapabilities.CodeAction | ||
| private typealias CodeActionLiteralSupport = CodeActionCapabilities.CodeActionLiteralSupport | ||
| private typealias CodeActionKindCapabilities = CodeActionLiteralSupport.CodeActionKindValueSet | ||
|
|
||
| private let clientCapabilitiesWithCodeActionSupport: ClientCapabilities = { | ||
| var documentCapabilities = TextDocumentClientCapabilities() | ||
| var codeActionCapabilities = CodeActionCapabilities() | ||
| codeActionCapabilities.codeActionLiteralSupport = .init( | ||
| codeActionKind: .init(valueSet: [.refactorInline]) | ||
| ) | ||
| documentCapabilities.codeAction = codeActionCapabilities | ||
| documentCapabilities.completion = .init(completionItem: .init(snippetSupport: true)) | ||
| return ClientCapabilities(workspace: nil, textDocument: documentCapabilities) | ||
| }() | ||
|
|
||
| final class InlineTempVariableTests: SourceKitLSPTestCase { | ||
| private func validateCodeAction( | ||
| input: String, | ||
| expectedOutput: String?, | ||
| title: String = "Inline variable", | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) async throws { | ||
| let testClient = try await TestSourceKitLSPClient(capabilities: clientCapabilitiesWithCodeActionSupport) | ||
| let uri = DocumentURI(for: .swift) | ||
| let positions = testClient.openDocument(input, uri: uri) | ||
|
|
||
| let range: Range<Position> | ||
| if input.contains("1️⃣") && input.contains("2️⃣") { | ||
| range = positions["1️⃣"]..<positions["2️⃣"] | ||
| } else if input.contains("1️⃣") { | ||
| let pos = positions["1️⃣"] | ||
| range = pos..<pos | ||
| } else { | ||
| XCTFail("Missing marker 1️⃣ in input", file: file, line: line) | ||
| return | ||
| } | ||
|
|
||
| let request = CodeActionRequest( | ||
| range: range, | ||
| context: .init(), | ||
| textDocument: TextDocumentIdentifier(uri) | ||
| ) | ||
| let result = try await testClient.send(request) | ||
|
|
||
| guard case .codeActions(let codeActions) = result else { | ||
| XCTFail("Expected code actions response", file: file, line: line) | ||
| return | ||
| } | ||
|
|
||
| let action = codeActions.first(where: { $0.title == title }) | ||
| if let expectedOutput { | ||
| guard let action else { | ||
| let available = codeActions.map(\.title) | ||
| XCTFail("Action '\(title)' not found. Available: \(available)", file: file, line: line) | ||
| return | ||
| } | ||
|
|
||
| guard let edit = action.edit else { | ||
| XCTFail("Action '\(title)' has no edit", file: file, line: line) | ||
| return | ||
| } | ||
|
|
||
| let changes = edit.changes?[uri] ?? [] | ||
| let cleanInput = extractMarkers(input).textWithoutMarkers | ||
| let resultingText = apply(edits: changes, to: cleanInput) | ||
| XCTAssertEqual(resultingText, expectedOutput, file: file, line: line) | ||
| } else { | ||
| XCTAssertNil(action, "Expected action '\(title)' to be not offered", file: file, line: line) | ||
| } | ||
| } | ||
|
|
||
| func testInlineTempVariableSimple() async throws { | ||
| try await validateCodeAction( | ||
| input: """ | ||
| func example() { | ||
| let 1️⃣basePrice = item.price | ||
| let total = basePrice * quantity | ||
| } | ||
| """, | ||
| expectedOutput: """ | ||
| func example() { | ||
| let total = item.price * quantity | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testInlineTempVariableWithParenthesesForPrecedence() async throws { | ||
| try await validateCodeAction( | ||
| input: """ | ||
| func example() { | ||
| let 1️⃣basePrice = 1 + 2 | ||
| let total = basePrice * 3 | ||
| } | ||
| """, | ||
| expectedOutput: """ | ||
| func example() { | ||
| let total = (1 + 2) * 3 | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testInlineTempVariableMultipleUsages() async throws { | ||
| try await validateCodeAction( | ||
| input: """ | ||
| func example() { | ||
| let 1️⃣x = foo() | ||
| let a = x + 1 | ||
| let b = x * 2 | ||
| } | ||
| """, | ||
| expectedOutput: """ | ||
| func example() { | ||
| let a = foo() + 1 | ||
| let b = foo() * 2 | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testInlineTempVariableNotShownWhenNoUsages() async throws { | ||
| try await validateCodeAction( | ||
| input: """ | ||
| func example() { | ||
| let 1️⃣basePrice = item.price | ||
| } | ||
| """, | ||
| expectedOutput: nil | ||
| ) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is trying to re-implement lexical lookup and won’t take shadowing and other name lookup rules into account. Luckily all of this is implemented in
SwiftLexicalLookup. Could you use that library instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference collection now uses SwiftLexicalLookup, i used the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, you are still only comparing there base name here, not actually calling int
SwiftLexicalLookup.