Skip to content

Commit 8c650aa

Browse files
committed
Address my own review comments to swiftlang#1179
Addresses a few minor comments and the following major ones: - Add test cases for the syntax refactorings - Don’t report code actions for refactorings that don’t actually modify the source - Instead of just looking at the parent of the token of the selected range, walk up the syntax tree to find the syntax node to refactor. This makes the refactorings available in a lot more locations.
1 parent 624d4b6 commit 8c650aa

12 files changed

+475
-80
lines changed

Sources/SourceKitLSP/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ add_library(SourceKitLSP STATIC
1414
SourceKitLSPServer+Options.swift
1515
SymbolLocation+DocumentURI.swift
1616
TestDiscovery.swift
17+
TextEdit+IsNoop.swift
1718
WorkDoneProgressManager.swift
1819
Workspace.swift
1920
)

Sources/SourceKitLSP/Rename.swift

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,13 +1490,3 @@ fileprivate extension RelatedIdentifiersResponse {
14901490
}
14911491
}
14921492
}
1493-
1494-
fileprivate extension TextEdit {
1495-
/// Returns `true` the replaced text is the same as the new text
1496-
func isNoOp(in snapshot: DocumentSnapshot) -> Bool {
1497-
if snapshot.text[snapshot.indexRange(of: range)] == newText {
1498-
return true
1499-
}
1500-
return false
1501-
}
1502-
}

Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,13 @@ public struct AddDocumentation: EditRefactoringProvider {
9494
}
9595

9696
extension AddDocumentation: SyntaxRefactoringCodeActionProvider {
97+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
98+
return scope.innermostNodeContainingRange?.findParentOfSelf(
99+
ofType: DeclSyntax.self,
100+
stoppingIf: { $0.is(CodeBlockItemSyntax.self) || $0.is(MemberBlockItemSyntax.self) || $0.is(ExprSyntax.self) }
101+
)
102+
}
103+
97104
static var title: String { "Add documentation" }
98105
}
99106

Sources/SourceKitLSP/Swift/CodeActions/ConvertIntegerLiteral.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@ import LanguageServerProtocol
1414
import SwiftRefactor
1515
import SwiftSyntax
1616

17-
// TODO: Make the type IntegerLiteralExprSyntax.Radix conform to CaseEnumerable
18-
// in swift-syntax.
19-
2017
extension IntegerLiteralExprSyntax.Radix {
2118
static var allCases: [Self] = [.binary, .octal, .decimal, .hex]
2219
}
@@ -26,7 +23,7 @@ extension IntegerLiteralExprSyntax.Radix {
2623
struct ConvertIntegerLiteral: SyntaxCodeActionProvider {
2724
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
2825
guard
29-
let token = scope.firstToken,
26+
let token = scope.innermostNodeContainingRange,
3027
let integerExpr = token.parent?.as(IntegerLiteralExprSyntax.self),
3128
let integerValue = Int(
3229
integerExpr.split().value.filter { $0 != "_" },

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,17 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider {
192192
}
193193

194194
extension ConvertJSONToCodableStruct: SyntaxRefactoringCodeActionProvider {
195+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Syntax? {
196+
var node: Syntax? = scope.innermostNodeContainingRange
197+
while let unwrappedNode = node, ![.codeBlockItem, .memberBlockItem].contains(unwrappedNode.kind) {
198+
if preflightRefactoring(unwrappedNode) != nil {
199+
return unwrappedNode
200+
}
201+
node = unwrappedNode.parent
202+
}
203+
return nil
204+
}
205+
195206
static var title = "Create Codable structs from JSON"
196207
}
197208

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,7 @@ import SwiftSyntax
2020
/// edit a package manifest.
2121
struct PackageManifestEdits: SyntaxCodeActionProvider {
2222
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
23-
guard let token = scope.firstToken,
24-
let call = token.findEnclosingCall()
25-
else {
23+
guard let call = scope.innermostNodeContainingRange?.findEnclosingCall() else {
2624
return []
2725
}
2826

Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActionProvider.swift

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,26 +40,50 @@ struct SyntaxCodeActionScope {
4040
/// considered, i.e., where the cursor or selection is.
4141
var range: Range<AbsolutePosition>
4242

43-
init(
43+
/// The innermost node that contains the entire selected source range
44+
var innermostNodeContainingRange: Syntax?
45+
46+
init?(
4447
snapshot: DocumentSnapshot,
45-
syntaxTree tree: SourceFileSyntax,
48+
syntaxTree file: SourceFileSyntax,
4649
request: CodeActionRequest
47-
) throws {
50+
) {
4851
self.snapshot = snapshot
4952
self.request = request
50-
self.file = tree
53+
self.file = file
5154

52-
let start = snapshot.absolutePosition(of: request.range.lowerBound)
53-
let end = snapshot.absolutePosition(of: request.range.upperBound)
54-
let left = file.token(at: start)
55-
let right = file.token(at: end)
56-
let leftOff = left?.position ?? AbsolutePosition(utf8Offset: 0)
57-
let rightOff = right?.endPosition ?? leftOff
58-
self.range = leftOff..<rightOff
55+
guard let left = tokenForRefactoring(at: request.range.lowerBound, snapshot: snapshot, syntaxTree: file),
56+
let right = tokenForRefactoring(at: request.range.upperBound, snapshot: snapshot, syntaxTree: file)
57+
else {
58+
return nil
59+
}
60+
self.range = left.position..<right.endPosition
61+
self.innermostNodeContainingRange = findCommonAncestorOrSelf(Syntax(left), Syntax(right))
5962
}
63+
}
6064

61-
/// The first token in the
62-
var firstToken: TokenSyntax? {
63-
file.token(at: range.lowerBound)
65+
private func tokenForRefactoring(
66+
at position: Position,
67+
snapshot: DocumentSnapshot,
68+
syntaxTree: SourceFileSyntax
69+
) -> TokenSyntax? {
70+
let absolutePosition = snapshot.absolutePosition(of: position)
71+
if absolutePosition == syntaxTree.endPosition {
72+
// token(at:) will not find the end of file token if the end of file token has length 0. Special case this and
73+
// return the last proper token in this case.
74+
return syntaxTree.endOfFileToken.previousToken(viewMode: .sourceAccurate)
75+
}
76+
guard let token = syntaxTree.token(at: absolutePosition) else {
77+
return nil
78+
}
79+
// See `adjustPositionToStartOfIdentifier`. We need to be a little more aggressive for the refactorings and also
80+
// adjust to the start of punctuation eg. if the end of the selected range is after a `}`, we want the end token for
81+
// the refactoring to be the `}`, not the token after `}`.
82+
if absolutePosition == token.position,
83+
let previousToken = token.previousToken(viewMode: .sourceAccurate),
84+
previousToken.endPositionBeforeTrailingTrivia == absolutePosition
85+
{
86+
return previousToken
6487
}
88+
return token
6589
}

Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,29 +18,35 @@ import SwiftSyntax
1818
/// swift-syntax) into a SyntaxCodeActionProvider.
1919
protocol SyntaxRefactoringCodeActionProvider: SyntaxCodeActionProvider, EditRefactoringProvider {
2020
static var title: String { get }
21+
22+
/// Returns the node that the syntax refactoring should be performed on, if code actions are requested for the given
23+
/// scope.
24+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input?
2125
}
2226

2327
/// SyntaxCodeActionProviders with a \c Void context can automatically be
2428
/// adapted provide a code action based on their refactoring operation.
2529
extension SyntaxRefactoringCodeActionProvider where Self.Context == Void {
2630
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
27-
guard
28-
let token = scope.firstToken,
29-
let node = token.parent?.as(Input.self)
30-
else {
31+
guard let node = nodeToRefactor(in: scope) else {
3132
return []
3233
}
3334

3435
let sourceEdits = Self.textRefactor(syntax: node)
35-
if sourceEdits.isEmpty {
36-
return []
37-
}
3836

39-
let textEdits = sourceEdits.map { edit in
40-
TextEdit(
37+
let textEdits = sourceEdits.compactMap { (edit) -> TextEdit? in
38+
let edit = TextEdit(
4139
range: scope.snapshot.range(of: edit.range),
4240
newText: edit.replacement
4341
)
42+
if edit.isNoOp(in: scope.snapshot) {
43+
return nil
44+
}
45+
return edit
46+
}
47+
48+
if textEdits.isEmpty {
49+
return []
4450
}
4551

4652
return [
@@ -57,22 +63,77 @@ extension SyntaxRefactoringCodeActionProvider where Self.Context == Void {
5763

5864
extension AddSeparatorsToIntegerLiteral: SyntaxRefactoringCodeActionProvider {
5965
public static var title: String { "Add digit separators" }
66+
67+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
68+
return scope.innermostNodeContainingRange?.findParentOfSelf(
69+
ofType: IntegerLiteralExprSyntax.self,
70+
stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
71+
)
72+
}
6073
}
6174

6275
extension FormatRawStringLiteral: SyntaxRefactoringCodeActionProvider {
6376
public static var title: String {
6477
"Convert string literal to minimal number of '#'s"
6578
}
79+
80+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
81+
return scope.innermostNodeContainingRange?.findParentOfSelf(
82+
ofType: StringLiteralExprSyntax.self,
83+
stoppingIf: {
84+
$0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self)
85+
|| $0.keyPathInParent == \ExpressionSegmentSyntax.expressions
86+
}
87+
)
88+
}
6689
}
6790

6891
extension MigrateToNewIfLetSyntax: SyntaxRefactoringCodeActionProvider {
6992
public static var title: String { "Migrate to shorthand 'if let' syntax" }
93+
94+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
95+
return scope.innermostNodeContainingRange?.findParentOfSelf(
96+
ofType: IfExprSyntax.self,
97+
stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
98+
)
99+
}
70100
}
71101

72102
extension OpaqueParameterToGeneric: SyntaxRefactoringCodeActionProvider {
73103
public static var title: String { "Expand 'some' parameters to generic parameters" }
104+
105+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
106+
return scope.innermostNodeContainingRange?.findParentOfSelf(
107+
ofType: DeclSyntax.self,
108+
stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
109+
)
110+
}
74111
}
75112

76113
extension RemoveSeparatorsFromIntegerLiteral: SyntaxRefactoringCodeActionProvider {
77114
public static var title: String { "Remove digit separators" }
115+
116+
static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
117+
return scope.innermostNodeContainingRange?.findParentOfSelf(
118+
ofType: IntegerLiteralExprSyntax.self,
119+
stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
120+
)
121+
}
122+
}
123+
124+
extension Syntax {
125+
/// Finds the innermost parent of the given type while not walking outside of nodes that satisfy `stoppingIf`.
126+
func findParentOfSelf<ParentType: SyntaxProtocol>(
127+
ofType: ParentType.Type,
128+
stoppingIf: (Syntax) -> Bool
129+
) -> ParentType? {
130+
var node: Syntax? = self
131+
while let unwrappedNode = node, !stoppingIf(unwrappedNode) {
132+
if let expectedType = unwrappedNode.as(ParentType.self) {
133+
return expectedType
134+
}
135+
node = unwrappedNode.parent
136+
}
137+
return nil
138+
}
78139
}

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -755,9 +755,10 @@ extension SwiftLanguageService {
755755
return response
756756
}
757757

758-
func retrieveCodeActions(_ req: CodeActionRequest, providers: [CodeActionProvider]) async throws
759-
-> [CodeAction]
760-
{
758+
func retrieveCodeActions(
759+
_ req: CodeActionRequest,
760+
providers: [CodeActionProvider]
761+
) async throws -> [CodeAction] {
761762
guard providers.isEmpty == false else {
762763
return []
763764
}
@@ -776,7 +777,9 @@ extension SwiftLanguageService {
776777
let snapshot = try documentManager.latestSnapshot(uri)
777778

778779
let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot)
779-
let scope = try SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request)
780+
guard let scope = SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request) else {
781+
return []
782+
}
780783
return await allSyntaxCodeActions.concurrentMap { provider in
781784
return provider.codeActions(in: scope)
782785
}.flatMap { $0 }
@@ -1152,8 +1155,8 @@ extension DocumentSnapshot {
11521155
callerFile: StaticString = #fileID,
11531156
callerLine: UInt = #line
11541157
) -> Range<Position> {
1155-
let lowerBound = self.position(of: node.position)
1156-
let upperBound = self.position(of: node.endPosition)
1158+
let lowerBound = self.position(of: node.position, callerFile: callerFile, callerLine: callerLine)
1159+
let upperBound = self.position(of: node.endPosition, callerFile: callerFile, callerLine: callerLine)
11571160
return lowerBound..<upperBound
11581161
}
11591162

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
import LanguageServerProtocol
14+
15+
extension TextEdit {
16+
/// Returns `true` the replaced text is the same as the new text
17+
func isNoOp(in snapshot: DocumentSnapshot) -> Bool {
18+
if snapshot.text[snapshot.indexRange(of: range)] == newText {
19+
return true
20+
}
21+
return false
22+
}
23+
}

0 commit comments

Comments
 (0)