Add shared cursorInfo infrastructure and forEach-to-forIn code action#2588
Add shared cursorInfo infrastructure and forEach-to-forIn code action#2588MoonMao42 wants to merge 14 commits into
Conversation
| /// Lazy, shared cache for a single `cursorInfo` request across concurrent code action providers. | ||
| /// | ||
| /// The first call to `get()` triggers the sourcekitd request; subsequent calls await the same `Task`. | ||
| actor SharedCursorInfo { |
There was a problem hiding this comment.
I think this whole actor can be generalized as something like:
actor LazyTask<Success: Sendable> {
private let operation: @Sendable () async throws -> Success
init(_ operation: @escaping @Sendable () async throws -> Success) {
self.operation = operation
}
private lazy var task: Task<Success, any Error> = Task {
try await operation()
}
var value: Success {
get async throws {
try await task.value
}
}
}Maybe we already have something like it?
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors |
There was a problem hiding this comment.
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | |
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors |
| return nil | ||
| } | ||
|
|
||
| let visitor = ForEachCallVisitor(rangeToMatch: scope.range) |
There was a problem hiding this comment.
Is SyntaxVisitor really relevant? For example, for:
1️⃣func test() {
2️⃣foo.bar.3️⃣forEach4️⃣ { item in
print(item)
}5️⃣
}6️⃣
I think we should only offer the code action when the curosr/selection is 2️⃣..<4️⃣, 2️⃣..<5️⃣, 3️⃣..<3️⃣, 3️⃣..<4️⃣, and 3️⃣..<5️⃣.
I think using a visitor and returning .visitChildren for every failure case makes it available for 1️⃣..<6️⃣ or even for the entire source file.
Please add test cases as needed.
| return [] | ||
| } | ||
|
|
||
| guard !hasTryOrAwait(match.closure) else { |
There was a problem hiding this comment.
Could you explain what is a case we can't convert it to for-in?
I understand having try? outside the forEach call expression is not easy:
try? foo.forEach { ...; try ...; }to:
do {
for item in foo { ...; try ...; }
} catch { /*ignore*/ }But I'm not sure having try in the closure body prevents anything.
| } | ||
|
|
||
| // Reject if parameter is shorthand $0 syntax | ||
| if param.firstName.text == "$0" { |
There was a problem hiding this comment.
I don't think this handles anonymous closure parameters. $0 does not appear in the closure parameter syntax tree.
It would be great this also rewrites $0. I.e. rewrite
foo.forEach {
print($0)
}with
for element in foo {
print(element)
}It's not super obvious whether we can use element though, we should check if it's not already used in the body.
| } | ||
|
|
||
| override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax { | ||
| DeclSyntax(node) |
There was a problem hiding this comment.
I don't think we should dig into any non-StmtSyntax.
Are there any cases where return in an expression or decl escapes the surrounding function scope?
There was a problem hiding this comment.
I don't see you addressed this? Also applies to other visitors/rewriters. Please avoid dig into unnecessary nodes.
There was a problem hiding this comment.
Addressed — all three visitor/rewriter classes (ClosureEligibilityVisitor, ReturnToContinueRewriter, DollarZeroRewriter) now consistently skip the same five scope-introducing constructs: FunctionDeclSyntax, InitializerDeclSyntax, DeinitializerDeclSyntax, ClosureExprSyntax, AccessorBlockSyntax.
There was a problem hiding this comment.
Please add this file and SharedCursorInfo.swift to https://github.com/swiftlang/sourcekit-lsp/blob/main/Sources/SwiftLanguageService/CMakeLists.txt
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors |
There was a problem hiding this comment.
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | |
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors |
| return nil | ||
| } | ||
|
|
||
| guard let parameters = signature.parameterClause?.as(ClosureParameterClauseSyntax.self) else { |
There was a problem hiding this comment.
What about ClosureShortParameterSyntax? Ideally the conversion should be:
foo.forEach { item in ... } -> for item in foo { ... }
foo.forEach { (item) in ... } -> for item in foo { ... }
foo.forEach { (item: Ty) in ... } -> for item: Ty in foo { ... }
| } | ||
|
|
||
| let codeActionCapabilities = capabilityRegistry.clientCapabilities.textDocument?.codeAction | ||
| let codeActions = try await retrieveCodeActions(req, providers: providers) |
There was a problem hiding this comment.
func retrieveCodeActions(_:providers:) below is now unused.
|
|
||
| if wantedActionKinds == nil { | ||
| allCodeActions += await retrieveSyntaxCodeActions(scope) | ||
| } |
There was a problem hiding this comment.
I think retrieveSyntaxCodeActions was added even if wantedActionKinds exist. Am I missing something?
| /// A lazily-evaluated, shared async value that is computed at most once. | ||
| /// | ||
| /// The first access triggers the operation; subsequent accesses await the same `Task`. | ||
| actor LazyValue<Success: Sendable> { |
There was a problem hiding this comment.
Could you use a different name?
We already have LazyValue with different semantics here https://github.com/swiftlang/sourcekit-lsp/blob/main/Sources/SwiftExtensions/LazyValue.swift
|
Done, thanks for the review. |
| } | ||
|
|
||
| /// Returns cursorInfo at the position of the given syntax node. | ||
| func cursorInfo(at absolutePosition: AbsolutePosition) async throws -> CursorInfo? { |
There was a problem hiding this comment.
Ah sorry I didn't think about what information is in the cursor-info result when I was suggesting the ranges to offer actions.
We'd like to avoid requesting non-shared cursorInfo in textDocument/codeAction request. Could you remove this? And I think it's okay for now to suggest this action only when the cursor is on forEach.
I.e. for
1️⃣func test() {
let array = [1, 2, 3]
2️⃣array.reversed().3️⃣forEach4️⃣ { item in
print(item)
}5️⃣
}6️⃣The action is active only for 3️⃣..<3️⃣ and 3️⃣..<4️⃣.
There was a problem hiding this comment.
Makes sense — the action now activates only when the cursor/selection is exactly on the forEach token, so shared cursorInfo at the request position works.
|
|
||
| // Walk up from the innermost node to find a forEach call expression, | ||
| // stopping at function/closure boundaries to avoid matching distant calls. | ||
| guard let callExpr = node.findParentOfSelf( |
There was a problem hiding this comment.
If it's only active at forEach, the check can be something like:
guard
let token = node.as(TokenSyntax.self),
token.text = "forEach"
let memberName = token.parent?.as(DeclReferenceExprSyntax.self),
let memberAccessExpr = member.parent?.as(MemberAccessExprSyntax.self),
memberAccessExpr.declName.id == memberName.id,
let callExpr = memberAccessExpr.parent?.as(FunctionCallExprSyntax.self),
callExpr.calledExpression.id == memberAccessExpr,
else {
return nil
}
let closure: ClosureExprSyntax?
...There was a problem hiding this comment.
Done — replaced with selectedForEachToken(in:) that validates directly against the request range positions.
| return Match( | ||
| callExpr: callExpr, | ||
| memberAccess: memberAccess, | ||
| collection: collection, |
There was a problem hiding this comment.
forEach(_:) is available on Sequence, so could you name this sequence instead of collection?
| } | ||
|
|
||
| let forEachPosition = match.memberAccess.declName.baseName.positionAfterSkippingLeadingTrivia | ||
| guard let info = try? await scope.cursorInfo(at: forEachPosition), |
There was a problem hiding this comment.
And this can be scope.cursorInfo()
| guard !hasReturnWithValue(match.closure) else { | ||
| return [] | ||
| } | ||
|
|
||
| guard !hasAwait(match.closure) else { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Could you make a single function/visitor checking both? So we can check the eligibility in single pass.
| private func extractParameter(from closure: ClosureExprSyntax) -> ClosureParam? { | ||
| guard let signature = closure.signature else { | ||
| // No signature → anonymous $0 usage. | ||
| let name = generateUniqueName(avoiding: collectIdentifiers(in: closure.statements)) |
There was a problem hiding this comment.
I don't think we need to separate function for this.
Also, could you avoid inserting every identifier in the Set, we're only interested in identifiers starting with element
There was a problem hiding this comment.
Done — containsIdentifier(named:in:) now checks on demand instead of collecting all names.
| } | ||
|
|
||
| override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax { | ||
| DeclSyntax(node) |
There was a problem hiding this comment.
I don't see you addressed this? Also applies to other visitors/rewriters. Please avoid dig into unnecessary nodes.
rintaro
left a comment
There was a problem hiding this comment.
It's getting close!
Just a few nit-picky edge case handling suggestions.
| guard node.expression == nil else { | ||
| return StmtSyntax(node) | ||
| } | ||
| return StmtSyntax(ContinueStmtSyntax()) |
There was a problem hiding this comment.
So I realized this is not enough for cases like
items.forEach { item in
for elem in item.elements {
if condition(elem) {
return
}
}
// ...
}In this case, we need a label on the rewritten for-in:
ITEM: for item in items {
for elem in item.elements {
if condition(elem) {
continue ITEM
}
}
// ...
}That said, I'm fine with ignoring such cases for this PR. But in that case, could you add FIXME and file an issue for that?
| override func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind { .skipChildren } | ||
| override func visit(_ node: DeinitializerDeclSyntax) -> SyntaxVisitorContinueKind { .skipChildren } | ||
| override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { .skipChildren } | ||
| override func visit(_ node: AccessorBlockSyntax) -> SyntaxVisitorContinueKind { .skipChildren } |
There was a problem hiding this comment.
Any type declarations too. i.e. struct, class, enum, etc.
| } | ||
|
|
||
| private func extractParameter(from closure: ClosureExprSyntax) -> ClosureParam? { | ||
| guard let signature = closure.signature else { |
There was a problem hiding this comment.
Rare edge case:
arr.forEach { [] in
print($0)
}In this case signature is non-nil.
Even more edge case:
arr.forEach { [foo = bar] in
print($0, foo)
}I guess we should rewrite this like
do {
let foo = bar
for element in arr {
print(element, foo)
}
}But it's too much effort.
I'm fine with just rejecting (not offering the action) for closures with capture list.
| guard let token = selectedForEachToken(in: scope), | ||
| token.text == "forEach", |
There was a problem hiding this comment.
I'm not sure this is what swift-format suggests. Could you run swift format -i -r . at the package top-level directory? Otherwise the test will fail.
|
|
||
| /// Matches only when the cursor/selection is on the `forEach` token. | ||
| private func findForEachCall(in scope: CodeActionScope) -> Match? { | ||
| guard let token = selectedForEachToken(in: scope), |
There was a problem hiding this comment.
Does scope.innermostNodeContainingRange not work?
There was a problem hiding this comment.
tokenForRefactoring shifts the cursor to the previous token at exact token boundaries, so innermostNodeContainingRange resolves to . instead of forEach when the cursor is at the start of forEach. selectedForEachToken bypasses this by matching against the raw request position.
|
@swift-ci Please test |
|
@MoonMao42 Could you rebase on the current main and fix the build issue? |
| async let refactorActions = wantActionKind(.refactor) ? try await retrieveRefactorCodeActions(scope) : [] | ||
| async let quickFixActions = wantActionKind(.quickFix) ? try await retrieveQuickFixCodeActions(scope) : [] | ||
| async let unusedImportActions = wantActionKind(.sourceOrganizeImports) | ||
| ? try await retrieveRemoveUnusedImportsCodeAction(scope) : [] |
There was a problem hiding this comment.
Ah, I think try await shouldn't be here.
- Generalize SharedCursorInfo into a reusable LazyValue<Success> actor - Extract result tuple into CursorInfoResponse struct - Make sharedCursorInfo non-optional in CodeActionScope - Remove unnecessary `package` access level and error swallowing - Bake additionalParameters into the init closure instead of get() - Remove infrastructure tests that don't verify meaningful behavior
- Replace downward SyntaxVisitor with upward findParentOfSelf walk - Handle all closure parameter forms (simpleInput, parameterClause) - Support $0 shorthand with DollarZeroRewriter (skips nested closures) - Allow try in closure body (forEach is rethrows) - Request cursorInfo at forEach position, not cursor position - Add cursorInfoProvider to CodeActionScope for positional lookups - Narrow return/continue rewriting to skip accessor boundaries - Rename LazyValue to AsyncLazy, delete unused retrieveCodeActions - Add CMakeLists.txt entries, fix copyright years - Rewrite tests using SwiftPMTestProject for proper stdlib resolution
…e and await walk(closure) triggers visit(ClosureExprSyntax) on the root node itself, which returns .skipChildren — making the visitor a no-op. Walk the closure's statements instead so the skip-closure logic only applies to nested closures.
- Use BasicFormat to produce correctly indented for-in output - Trim collection expression trivia for clean output - Tests now verify exact replacement text instead of just action presence - Add selection range boundary tests - Add test for closure body cursor position (should not offer action) - Add test for nested $0 not being rewritten - Add test verifying bare return → continue in output
- Unify scope boundaries across all visitor/rewriter classes to skip FunctionDeclSyntax, InitializerDeclSyntax, DeinitializerDeclSyntax, ClosureExprSyntax, and AccessorBlockSyntax - Merge ReturnWithValueVisitor and AwaitVisitor into single-pass ClosureEligibilityVisitor - Replace findParentOfSelf with token-level matching via selectedForEachToken for precise cursor/selection validation - Rename collection to sequence - Simplify generateUniqueName to avoid collecting all identifiers - Remove cursorInfoProvider from CodeActionScope, use shared cursorInfo - Parallelize code action retrieval with async let
- Handle bare return in nested loops with labeled continue - Skip type declarations (struct, class, enum, etc.) in all visitors - Reject closures with capture lists - Add tests for labeled continue, capture list rejection - Apply swift-format
5a20f1f to
e31adb4
Compare
|
@rintaro It's OK?I format it. |
|
@swift-ci Please test |
I think |
All right. |
|
@swift-ci Please test |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
| self.operation = operation | ||
| } | ||
|
|
||
| private lazy var task: Task<Success, any Error> = Task { |
There was a problem hiding this comment.
This task won’t get cancelled if the codeAction request is cancelled. Could you adopt cancellation here? We could eg. cancel the task if AsyncLazy gets reinitialized.
| } | ||
| return try await self.cursorInfo( | ||
| uri, | ||
| range, |
There was a problem hiding this comment.
Do we really want to execute the cursor info request at range? I just implemented something similar in #2622 and there eg. when you have
var x = 2And you invoke the code action on var, then the code action to convert the stored property to a computed property is applicable but we need to run the cursor info at x to get the type of x to add a type annotation.
There was a problem hiding this comment.
I think this should meet expectations.
There was a problem hiding this comment.
We already request cursorInfo for range in retrieveRefactorCodeActions. The idea was to reuse that if it is sufficient for determining applicability. Otherwise, I don't think we should allow each code action provider to request arbitrary cursorInfo during textDocument/codeAction.
Refactor SharedCursorInfo to cache cursor info responses keyed by Range<Position> instead of computing a single value. This allows code action providers to request semantic information at the position that is most relevant for their specific refactoring. - Convert SharedCursorInfo from AsyncLazy<CursorInfoResponse> to a dictionary-backed actor keyed by Range<Position> - Add cursorInfo(at:) and cursorInfo(for:) helpers on CodeActionScope - Have ForEachToForInCodeAction query cursor info at the forEach token rather than the original request range
# Conflicts: # Sources/SwiftLanguageService/CMakeLists.txt # Sources/SwiftSyntaxCodeActions/ApplyDeMorganLaw.swift # Sources/SwiftSyntaxCodeActions/ConvertIfLetToGuard.swift # Sources/SwiftSyntaxCodeActions/ConvertIntegerLiteral.swift # Sources/SwiftSyntaxCodeActions/ForEachToForInCodeAction.swift # Sources/SwiftSyntaxCodeActions/PackageManifestEdits.swift # Sources/SwiftSyntaxCodeActions/SharedCursorInfo.swift # Sources/SwiftSyntaxCodeActions/SyntaxCodeActionProvider.swift # Sources/SwiftSyntaxCodeActions/SyntaxRefactoringCodeActionProvider.swift
|
@rintaro @ahoppen Maybe we should align on the cursorInfo direction here.I updated the conflict resolution to follow rintaro’s suggestion for this PR. That seems cheaper, and the behavior is easier to reason about. The cases that need cursorInfo at a different position seem like they should go through the structured approach in #2622, rather than having each syntactic code action issue its own cursorInfo request. |
Closes #2584
Closes #2509
Adds
SharedCursorInfoso code action providers can share a single cursorInfo request.SyntaxCodeActionProvider.codeActions(in:)is now async, and the scope object (renamed toCodeActionScope) is created once incodeAction()and passed to all retrieve methods.Also adds a code action to convert
.forEachtofor-inloops, as the first provider that uses this.Before:
After:
Only handles closures with explicit named parameters (
$0shorthand is not supported).