Skip to content

Add refactoring actions for adding a test targets and products to a package manifest #1193

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 8 commits into from
Apr 27, 2024
1 change: 1 addition & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ let package = Package(
.product(name: "SwiftRefactor", package: "swift-syntax"),
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
.product(name: "SwiftPM-auto", package: "swift-package-manager"),
],
exclude: ["CMakeLists.txt"]
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,18 @@ public struct OptionalVersionedTextDocumentIdentifier: Hashable, Codable, Sendab
self.uri = uri
self.version = version
}

enum CodingKeys: CodingKey {
case uri
case version
}

public func encode(to encoder: any Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)
try container.encode(self.uri, forKey: .uri)

// Note: we use encode(_:forKey:) here instead of encodeIf(_:forKey:)
// because VSCode will drop requests without the explicit 'null'.
try container.encode(self.version, forKey: .version)
}
}
2 changes: 2 additions & 0 deletions Sources/SourceKitLSP/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ target_sources(SourceKitLSP PRIVATE
target_sources(SourceKitLSP PRIVATE
Swift/AdjustPositionToStartOfIdentifier.swift
Swift/CodeActions/ConvertIntegerLiteral.swift
Swift/CodeActions/PackageManifestEdits.swift
Swift/CodeActions/SyntaxCodeActionProvider.swift
Swift/CodeActions/SyntaxCodeActions.swift
Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift
Expand Down Expand Up @@ -74,5 +75,6 @@ target_link_libraries(SourceKitLSP PUBLIC
SwiftSyntax::SwiftRefactor
SwiftSyntax::SwiftSyntax)
target_link_libraries(SourceKitLSP PRIVATE
PackageModelSyntax
$<$<NOT:$<PLATFORM_ID:Darwin>>:FoundationXML>)

243 changes: 243 additions & 0 deletions Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift
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 test cases for these refactorings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 4c1e119

Original file line number Diff line number Diff line change
@@ -0,0 +1,243 @@
//===----------------------------------------------------------------------===//
//
// 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 PackageModel
import PackageModelSyntax
import SwiftRefactor
import SwiftSyntax

/// Syntactic code action provider to provide refactoring actions that
/// edit a package manifest.
struct PackageManifestEdits: SyntaxCodeActionProvider {
static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
guard let token = scope.firstToken,
let call = token.findEnclosingCall()
else {
return []
}

return addTestTargetActions(call: call, in: scope) + addProductActions(call: call, in: scope)
}

/// Produce code actions to add test target(s) if we are currently on
/// a target for which we know how to create a test.
static func addTestTargetActions(
call: FunctionCallExprSyntax,
in scope: SyntaxCodeActionScope
) -> [CodeAction] {
guard let calledMember = call.findMemberAccessCallee(),
targetsThatAllowTests.contains(calledMember),
let targetName = call.findStringArgument(label: "name")
else {
return []
}

do {
// Describe the target we are going to create.
let target = try TargetDescription(
name: "\(targetName)Tests",
dependencies: [.byName(name: targetName, condition: nil)],
type: .test
)

let edits = try AddTarget.addTarget(target, to: scope.file)
return [
CodeAction(
title: "Add test target",
kind: .refactor,
edit: edits.asWorkspaceEdit(snapshot: scope.snapshot)
)
]
Comment on lines +53 to +60
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to do some rough check whether a test target with that name already exists? I think even if we just do a textual search for "\(targetName)Tests" (ie the target name wrapped in quotes), that would cover most cases and would prevent us from showing a code action to add a test target when it obviously already exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're going to do that check, it should happen down in the SwiftPM code. Personally, I'm not sure... someone might want to use the action and then go rename the text. Are we helping them by taking the action away?

Copy link
Member

Choose a reason for hiding this comment

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

That’s a good point. Let’s leave it for now 👍🏽

} catch {
return []
}
}

/// A list of target kinds that allow the creation of tests.
static let targetsThatAllowTests: Set<String> = [
Copy link

Choose a reason for hiding this comment

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

I know this isn't particularly performance-sensitive, but this should probably be an Array. A Set is overkill.

"executableTarget",
"macro",
"target",
]

/// Produce code actions to add a product if we are currently on
/// a target for which we can create a product.
static func addProductActions(
call: FunctionCallExprSyntax,
in scope: SyntaxCodeActionScope
) -> [CodeAction] {
guard let calledMember = call.findMemberAccessCallee(),
targetsThatAllowProducts.contains(calledMember),
let targetName = call.findStringArgument(label: "name")
else {
return []
}

do {
let type: ProductType =
calledMember == "executableTarget"
? .executable
: .library(.automatic)

// Describe the target we are going to create.
let product = try ProductDescription(
name: targetName,
type: type,
targets: [targetName]
)

let edits = try AddProduct.addProduct(product, to: scope.file)
Copy link
Member

Choose a reason for hiding this comment

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

Similar here, do you think there’s a way to check if we already have a product for this target? I think textual search wouldn’t work here.

return [
CodeAction(
title: "Add product to export this target",
kind: .refactor,
edit: edits.asWorkspaceEdit(snapshot: scope.snapshot)
)
]
} catch {
return []
}
}

/// A list of target kinds that allow the creation of tests.
static let targetsThatAllowProducts: Set<String> = [
Copy link

Choose a reason for hiding this comment

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

Same here. An Array would probably be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. I'll clean it up in a subsequent PR, because I'm drowning in unmerged PRs right now

"executableTarget",
"target",
]
}

fileprivate extension PackageEditResult {
/// Translate package manifest edits into a workspace edit. This can
/// involve both modifications to the manifest file as well as the creation
/// of new files.
/// `snapshot` is the latest snapshot of the `Package.swift` file.
func asWorkspaceEdit(snapshot: DocumentSnapshot) -> WorkspaceEdit {
// The edits to perform on the manifest itself.
let manifestTextEdits = manifestEdits.map { edit in
TextEdit(
range: snapshot.range(of: edit.range),
newText: edit.replacement
)
}

// If we couldn't figure out the manifest directory, or there are no
// files to add, the only changes are the manifest edits. We're done
// here.
let manifestDirectoryURL = snapshot.uri.fileURL?
.deletingLastPathComponent()
guard let manifestDirectoryURL, !auxiliaryFiles.isEmpty else {
return WorkspaceEdit(
changes: [snapshot.uri: manifestTextEdits]
)
}

// Use the more full-featured documentChanges, which takes precedence
// over the individual changes to documents.
var documentChanges: [WorkspaceEditDocumentChange] = []

// Put the manifest changes into the array.
documentChanges.append(
.textDocumentEdit(
TextDocumentEdit(
textDocument: .init(snapshot.uri, version: snapshot.version),
edits: manifestTextEdits.map { .textEdit($0) }
)
)
)

// Create an populate all of the auxiliary files.
for (relativePath, contents) in auxiliaryFiles {
guard
let url = URL(
string: relativePath.pathString,
relativeTo: manifestDirectoryURL
)
else {
continue
}

let documentURI = DocumentURI(url)
let createFile = CreateFile(
uri: documentURI
)

let zeroPosition = Position(line: 0, utf16index: 0)
let edit = TextEdit(
range: zeroPosition..<zeroPosition,
newText: contents.description
)

documentChanges.append(.createFile(createFile))
documentChanges.append(
.textDocumentEdit(
TextDocumentEdit(
textDocument: .init(documentURI, version: snapshot.version),
edits: [.textEdit(edit)]
)
)
)
}

return WorkspaceEdit(
changes: [snapshot.uri: manifestTextEdits],
documentChanges: documentChanges
)
}
}

fileprivate extension SyntaxProtocol {
// Find an enclosing call syntax expression.
func findEnclosingCall() -> FunctionCallExprSyntax? {
var current = Syntax(self)
while true {
if let call = current.as(FunctionCallExprSyntax.self) {
return call
}

if let parent = current.parent {
current = parent
continue
}

return nil
}
}
}

fileprivate extension FunctionCallExprSyntax {
/// Find an argument with the given label that has a string literal as
/// its argument.
func findStringArgument(label: String) -> String? {
for arg in arguments {
if arg.label?.text == label {
return arg.expression.as(StringLiteralExprSyntax.self)?
.representedLiteralValue
}
}

return nil
}

/// Find the callee when it is a member access expression referencing
/// a declaration when a specific name.
func findMemberAccessCallee() -> String? {
guard
let memberAccess = self.calledExpression
.as(MemberAccessExprSyntax.self)
else {
return nil
}

return memberAccess.declName.baseName.text
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ let allSyntaxCodeActions: [SyntaxCodeActionProvider.Type] = [
FormatRawStringLiteral.self,
MigrateToNewIfLetSyntax.self,
OpaqueParameterToGeneric.self,
PackageManifestEdits.self,
RemoveSeparatorsFromIntegerLiteral.self,
]
3 changes: 2 additions & 1 deletion Tests/LanguageServerProtocolTests/CodingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ final class CodingTests: XCTestCase {
OptionalVersionedTextDocumentIdentifier(uri, version: nil),
json: """
{
"uri" : "\(urljson)"
"uri" : "\(urljson)",
"version" : null
}
"""
)
Expand Down
Loading