Skip to content

Introduce new refactoring code actions based on the Swift syntax tree. #1179

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Sources/SourceKitLSP/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ target_sources(SourceKitLSP PRIVATE
Clang/ClangLanguageService.swift)
target_sources(SourceKitLSP PRIVATE
Swift/AdjustPositionToStartOfIdentifier.swift
Swift/CodeActions/ConvertIntegerLiteral.swift
Swift/CodeActions/SyntaxCodeActionProvider.swift
Swift/CodeActions/SyntaxCodeActions.swift
Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift
Swift/CodeCompletion.swift
Swift/CodeCompletionSession.swift
Swift/CommentXML.swift
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 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 SwiftRefactor
import SwiftSyntax

// TODO: Make the type IntegerLiteralExprSyntax.Radix conform to CaseEnumerable
// in swift-syntax.

extension IntegerLiteralExprSyntax.Radix {
static var allCases: [Self] = [.binary, .octal, .decimal, .hex]
}

/// Syntactic code action provider to convert integer literals between
/// different bases.
struct ConvertIntegerLiteral: SyntaxCodeActionProvider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case for this, checking that the refactoring do actually show up when retrieving code actions? Ie. a test in https://github.com/apple/sourcekit-lsp/blob/main/Tests/SourceKitLSPTests/CodeActionTests.swift

static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
guard
let token = scope.firstToken,
let integerExpr = token.parent?.as(IntegerLiteralExprSyntax.self),
let integerValue = Int(
integerExpr.split().value.filter { $0 != "_" },
radix: integerExpr.radix.size
)
else {
return []
}

var actions = [CodeAction]()
let currentRadix = integerExpr.radix
for radix in IntegerLiteralExprSyntax.Radix.allCases {
guard radix != currentRadix else {
continue
}

//TODO: Add this to swift-syntax?
let prefix: String
switch radix {
case .binary:
prefix = "0b"
case .octal:
prefix = "0o"
case .hex:
prefix = "0x"
case .decimal:
prefix = ""
}

let convertedValue: ExprSyntax =
"\(raw: prefix)\(raw: String(integerValue, radix: radix.size))"
let edit = TextEdit(
range: scope.snapshot.range(of: integerExpr),
newText: convertedValue.description
)
actions.append(
CodeAction(
title: "Convert \(integerExpr) to \(convertedValue)",
kind: .refactorInline,
edit: WorkspaceEdit(changes: [scope.snapshot.uri: [edit]])
)
)
}

return actions
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 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 LSPLogging
import LanguageServerProtocol
import SwiftRefactor
import SwiftSyntax

/// Describes types that provide one or more code actions based on purely
/// syntactic information.
protocol SyntaxCodeActionProvider {
/// Produce code actions within the given scope. Each code action
/// corresponds to one syntactic transformation that can be performed, such
/// as adding or removing separators from an integer literal.
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction]
}

/// Defines the scope in which a syntactic code action occurs.
struct SyntaxCodeActionScope {
/// The snapshot of the document on which the code actions will be evaluated.
var snapshot: DocumentSnapshot

/// The actual code action request, which can specify additional parameters
/// to guide the code actions.
var request: CodeActionRequest

/// The source file in which the syntactic code action will operate.
var file: SourceFileSyntax

/// The UTF-8 byte range in the source file in which code actions should be
/// considered, i.e., where the cursor or selection is.
var range: Range<AbsolutePosition>

init(
snapshot: DocumentSnapshot,
syntaxTree tree: SourceFileSyntax,
request: CodeActionRequest
) throws {
self.snapshot = snapshot
self.request = request
self.file = tree

let start = snapshot.absolutePosition(of: request.range.lowerBound)
let end = snapshot.absolutePosition(of: request.range.upperBound)
let left = file.token(at: start)
let right = file.token(at: end)
let leftOff = left?.position ?? AbsolutePosition(utf8Offset: 0)
let rightOff = right?.endPosition ?? leftOff
Comment on lines +54 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d prefer to fail the creation of SyntaxCodeActionScope and emit a logger.fault if we can’t find a token at a specific position. It’s the kind of thing where things start failing and then it’s hard to track down what’s going wrong if there’s no logging.

self.range = leftOff..<rightOff
}

/// The first token in the
var firstToken: TokenSyntax? {
file.token(at: range.lowerBound)
}
}
24 changes: 24 additions & 0 deletions Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 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 SwiftRefactor

/// List of all of the syntactic code action providers, which can be used
/// to produce code actions using only the swift-syntax tree of a file.
let allSyntaxCodeActions: [SyntaxCodeActionProvider.Type] = [
AddSeparatorsToIntegerLiteral.self,
ConvertIntegerLiteral.self,
FormatRawStringLiteral.self,
MigrateToNewIfLetSyntax.self,
OpaqueParameterToGeneric.self,
RemoveSeparatorsFromIntegerLiteral.self,
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 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 SwiftRefactor
import SwiftSyntax

/// Protocol that adapts a SyntaxRefactoringProvider (that comes from
/// swift-syntax) into a SyntaxCodeActionProvider.
protocol SyntaxRefactoringCodeActionProvider: SyntaxCodeActionProvider, SyntaxRefactoringProvider {
static var title: String { get }
}

/// SyntaxCodeActionProviders with a \c Void context can automatically be
/// adapted provide a code action based on their refactoring operation.
extension SyntaxRefactoringCodeActionProvider where Self.Context == Void {
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
guard
let token = scope.firstToken,
let node = token.parent?.as(Input.self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we traverse the parents until we find a node of type Input? That way you don’t need to start eg. a MigrateToNewIfLetSyntax refactoring on the if token but you could also start it from within the condition or the body (not sure if we want the latter though, maybe we should stop the traversal at some pre-defined nodes like CodeBlockItemListSyntax and MemberBlockItemListSyntax to prevent that).

Similarly, the OpaqueParameterToGeneric refactoring action can’t currently be invoked from one of the some types, which might be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this is something that should be handled by the MigrateToNewIfLetSyntax, OpaqueParameterToGeneric, etc., refactorings themselves, because we don't necessarily know how far out in the tree we should go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable.

else {
return []
}

guard let refactored = Self.refactor(syntax: node) else {
return []
}

let edit = TextEdit(
range: scope.snapshot.range(of: node),
newText: refactored.description
)

return [
CodeAction(
title: Self.title,
kind: .refactorInline,
edit: WorkspaceEdit(changes: [scope.snapshot.uri: [edit]])
)
]
}
}

// Adapters for specific refactoring provides in swift-syntax.

extension AddSeparatorsToIntegerLiteral: SyntaxRefactoringCodeActionProvider {
public static var title: String { "Add digit separators" }
}

extension FormatRawStringLiteral: SyntaxRefactoringCodeActionProvider {
public static var title: String {
"Convert string literal to minimal number of '#'s"
}
}

extension MigrateToNewIfLetSyntax: SyntaxRefactoringCodeActionProvider {
public static var title: String { "Migrate to shorthand 'if let' syntax" }
}

extension OpaqueParameterToGeneric: SyntaxRefactoringCodeActionProvider {
public static var title: String { "Expand 'some' parameters to generic parameters" }
}

extension RemoveSeparatorsFromIntegerLiteral: SyntaxRefactoringCodeActionProvider {
public static var title: String { "Remove digit separators" }
}
40 changes: 36 additions & 4 deletions Sources/SourceKitLSP/Swift/SwiftLanguageService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -699,22 +699,31 @@ extension SwiftLanguageService {
}

public func codeAction(_ req: CodeActionRequest) async throws -> CodeActionRequestResponse? {
let providersAndKinds: [(provider: CodeActionProvider, kind: CodeActionKind)] = [
let providersAndKinds: [(provider: CodeActionProvider, kind: CodeActionKind?)] = [
(retrieveSyntaxCodeActions, nil),
(retrieveRefactorCodeActions, .refactor),
(retrieveQuickFixCodeActions, .quickFix),
]
let wantedActionKinds = req.context.only
let providers = providersAndKinds.filter { wantedActionKinds?.contains($0.1) != false }
let providers: [CodeActionProvider] = providersAndKinds.compactMap {
if let wantedActionKinds, let kind = $0.1, !wantedActionKinds.contains(kind) {
return nil
}

return $0.provider
}
let codeActionCapabilities = capabilityRegistry.clientCapabilities.textDocument?.codeAction
let codeActions = try await retrieveCodeActions(req, providers: providers.map { $0.provider })
let codeActions = try await retrieveCodeActions(req, providers: providers)
let response = CodeActionRequestResponse(
codeActions: codeActions,
clientCapabilities: codeActionCapabilities
)
return response
}

func retrieveCodeActions(_ req: CodeActionRequest, providers: [CodeActionProvider]) async throws -> [CodeAction] {
func retrieveCodeActions(_ req: CodeActionRequest, providers: [CodeActionProvider]) async throws
-> [CodeAction]
{
Comment on lines +724 to +726
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d prefer to format this as

  func retrieveCodeActions(
    _ req: CodeActionRequest, 
    providers: [CodeActionProvider]
  ) async throws -> [CodeAction]
  {

guard providers.isEmpty == false else {
return []
}
Expand All @@ -725,6 +734,17 @@ extension SwiftLanguageService {
// Ignore any providers that failed to provide refactoring actions.
return []
}
}.flatMap { $0 }.sorted { $0.title < $1.title }
}

func retrieveSyntaxCodeActions(_ request: CodeActionRequest) async throws -> [CodeAction] {
let uri = request.textDocument.uri
let snapshot = try documentManager.latestSnapshot(uri)

let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot)
let scope = try SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request)
return await allSyntaxCodeActions.concurrentMap { provider in
return provider.codeActions(in: scope)
}.flatMap { $0 }
}

Expand Down Expand Up @@ -1091,6 +1111,18 @@ extension DocumentSnapshot {
return lowerBound..<upperBound
}

/// Extracts the range of the given syntax node in terms of positions within
/// this source file.
func range(
of node: some SyntaxProtocol,
callerFile: StaticString = #fileID,
callerLine: UInt = #line
) -> Range<Position> {
let lowerBound = self.position(of: node.position)
let upperBound = self.position(of: node.endPosition)
Comment on lines +1121 to +1122
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should pass callerFile and callerLine into position(of:) so that the logger fault message contains the file and line number that is the real originator if position conversion fails.

return lowerBound..<upperBound
}

/// Converts the given UTF-16-based line:column range to a UTF-8-offset-based `ByteSourceRange`.
///
/// If the bounds of the range do not refer to a valid positions with in the snapshot, this function adjusts them to
Expand Down
7 changes: 6 additions & 1 deletion Tests/SourceKitLSPTests/CodeActionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,12 @@ final class CodeActionTests: XCTestCase {
command: expectedCommand
)

XCTAssertEqual(result, .codeActions([expectedCodeAction]))
guard case .codeActions(let codeActions) = result else {
XCTFail("Expected code actions")
return
}

XCTAssertTrue(codeActions.contains(expectedCodeAction))
}

func testSemanticRefactorRangeCodeActionResult() async throws {
Expand Down