From 9dc166d8dab4477717150addf604192a5196c37f Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 3 May 2024 13:32:09 -0700 Subject: [PATCH 01/32] Rename diagnose bundle created on disk from `sourcekitd-reproducer` to `sourcekit-lsp-diagnose` The name still dated back to when `sourcekit-lsp diagnose` would only reduce sourcekitd crashes. Now it does quite a bit more. --- Sources/Diagnose/DiagnoseCommand.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Diagnose/DiagnoseCommand.swift b/Sources/Diagnose/DiagnoseCommand.swift index 271f9ebbc..ca01dfdca 100644 --- a/Sources/Diagnose/DiagnoseCommand.swift +++ b/Sources/Diagnose/DiagnoseCommand.swift @@ -309,7 +309,7 @@ public struct DiagnoseCommand: AsyncParsableCommand { dateFormatter.timeZone = NSTimeZone.local let date = dateFormatter.string(from: Date()).replacingOccurrences(of: ":", with: "-") let bundlePath = FileManager.default.temporaryDirectory - .appendingPathComponent("sourcekitd-reproducer-\(date)") + .appendingPathComponent("sourcekit-lsp-diagnose-\(date)") try FileManager.default.createDirectory(at: bundlePath, withIntermediateDirectories: true) if components.isEmpty || components.contains(.crashReports) { From 59130de5e0cd27b71034f753201a6f2400d5881f Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 25 Apr 2024 09:14:34 -0700 Subject: [PATCH 02/32] Delete unnecessary imports of `IndexStoreDB` --- Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift | 1 - Tests/SourceKitLSPTests/MainFilesProviderTests.swift | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift b/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift index 167ea2793..0f78875cc 100644 --- a/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift +++ b/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift @@ -11,7 +11,6 @@ //===----------------------------------------------------------------------===// import Foundation -import IndexStoreDB import LSPLogging import LanguageServerProtocol import SKCore diff --git a/Tests/SourceKitLSPTests/MainFilesProviderTests.swift b/Tests/SourceKitLSPTests/MainFilesProviderTests.swift index 9a3ed121f..492fb9e4d 100644 --- a/Tests/SourceKitLSPTests/MainFilesProviderTests.swift +++ b/Tests/SourceKitLSPTests/MainFilesProviderTests.swift @@ -10,7 +10,6 @@ // //===----------------------------------------------------------------------===// -import IndexStoreDB import LSPTestSupport import LanguageServerProtocol import SKCore @@ -40,7 +39,7 @@ final class MainFilesProviderTests: XCTestCase { name: "MyLibrary", targets: [ .target( - name: "MyLibrary", + name: "MyLibrary", cSettings: [.define("VARIABLE_NAME", to: "fromMyLibrary"), .unsafeFlags(["-Wunused-variable"])] ) ] From 7d49916faac07dcde2b2aa72e40215cbeeee9a10 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 29 Apr 2024 10:51:43 -0700 Subject: [PATCH 03/32] Make `BuildServerBuildSystem.buildSettings(for:language:)` not throwing No particular motivation for this change. --- Sources/SKCore/BuildServerBuildSystem.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SKCore/BuildServerBuildSystem.swift b/Sources/SKCore/BuildServerBuildSystem.swift index c75a7cea0..19d7270fb 100644 --- a/Sources/SKCore/BuildServerBuildSystem.swift +++ b/Sources/SKCore/BuildServerBuildSystem.swift @@ -263,7 +263,7 @@ extension BuildServerBuildSystem: BuildSystem { /// /// Returns `nil` if no build settings have been received from the build /// server yet or if no build settings are available for this file. - public func buildSettings(for document: DocumentURI, language: Language) async throws -> FileBuildSettings? { + public func buildSettings(for document: DocumentURI, language: Language) async -> FileBuildSettings? { return buildSettings[document] } From a95496438b6ebdb184c18dd5afcffa63979324e4 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 29 Apr 2024 10:52:44 -0700 Subject: [PATCH 04/32] Move `Collection.partition(intoNumberOfBatches:)` to `SKSupport` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This way we’ll be able to use it from the semantic indexer. --- Sources/SKSupport/CMakeLists.txt | 1 + .../Collection+PartitionIntoBatches.swift | 35 +++++++++++++++++++ .../Swift/SyntacticTestIndex.swift | 24 ------------- 3 files changed, 36 insertions(+), 24 deletions(-) create mode 100644 Sources/SKSupport/Collection+PartitionIntoBatches.swift diff --git a/Sources/SKSupport/CMakeLists.txt b/Sources/SKSupport/CMakeLists.txt index efc4e010f..657ac0720 100644 --- a/Sources/SKSupport/CMakeLists.txt +++ b/Sources/SKSupport/CMakeLists.txt @@ -5,6 +5,7 @@ add_library(SKSupport STATIC BuildConfiguration.swift ByteString.swift Collection+Only.swift + Collection+PartitionIntoBatches.swift Connection+Send.swift dlopen.swift DocumentURI+CustomLogStringConvertible.swift diff --git a/Sources/SKSupport/Collection+PartitionIntoBatches.swift b/Sources/SKSupport/Collection+PartitionIntoBatches.swift new file mode 100644 index 000000000..ed1effdc9 --- /dev/null +++ b/Sources/SKSupport/Collection+PartitionIntoBatches.swift @@ -0,0 +1,35 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +public extension Collection { + /// Partition the elements of the collection into `numberOfBatches` roughly equally sized batches. + /// + /// Elements are assigned to the batches round-robin. This ensures that elements that are close to each other in the + /// original collection end up in different batches. This is important because eg. test files will live close to each + /// other in the file system and test scanning wants to scan them in different batches so we don't end up with one + /// batch only containing source files and the other only containing test files. + func partition(intoNumberOfBatches numberOfBatches: Int) -> [[Element]] { + var batches: [[Element]] = Array( + repeating: { + var batch: [Element] = [] + batch.reserveCapacity(self.count / numberOfBatches) + return batch + }(), + count: numberOfBatches + ) + + for (index, element) in self.enumerated() { + batches[index % numberOfBatches].append(element) + } + return batches.filter { !$0.isEmpty } + } +} diff --git a/Sources/SourceKitLSP/Swift/SyntacticTestIndex.swift b/Sources/SourceKitLSP/Swift/SyntacticTestIndex.swift index c91e046b5..ec5c33068 100644 --- a/Sources/SourceKitLSP/Swift/SyntacticTestIndex.swift +++ b/Sources/SourceKitLSP/Swift/SyntacticTestIndex.swift @@ -213,27 +213,3 @@ actor SyntacticTestIndex { return await readTask.value } } - -fileprivate extension Collection { - /// Partition the elements of the collection into `numberOfBatches` roughly equally sized batches. - /// - /// Elements are assigned to the batches round-robin. This ensures that elements that are close to each other in the - /// original collection end up in different batches. This is important because eg. test files will live close to each - /// other in the file system and test scanning wants to scan them in different batches so we don't end up with one - /// batch only containing source files and the other only containing test files. - func partition(intoNumberOfBatches numberOfBatches: Int) -> [[Element]] { - var batches: [[Element]] = Array( - repeating: { - var batch: [Element] = [] - batch.reserveCapacity(self.count / numberOfBatches) - return batch - }(), - count: numberOfBatches - ) - - for (index, element) in self.enumerated() { - batches[index % numberOfBatches].append(element) - } - return batches.filter { !$0.isEmpty } - } -} From 46f0d1bac8c7d42727146908e28c80414ba8a5fd Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 08:36:16 -0700 Subject: [PATCH 05/32] =?UTF-8?q?Fix=20issue=20in=20`AsyncQueue`=20that=20?= =?UTF-8?q?didn=E2=80=99t=20respect=20the=20requested=20task=20priority?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/SKSupport/AsyncQueue.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SKSupport/AsyncQueue.swift b/Sources/SKSupport/AsyncQueue.swift index 4829983a7..31d58687d 100644 --- a/Sources/SKSupport/AsyncQueue.swift +++ b/Sources/SKSupport/AsyncQueue.swift @@ -144,7 +144,7 @@ public final class AsyncQueue: Sendable { let dependencies: [PendingTask] = tasks.filter { $0.metadata.isDependency(of: metadata) } // Schedule the task. - let task = Task { [pendingTasks] in + let task = Task(priority: priority) { [pendingTasks] in // IMPORTANT: The only throwing call in here must be the call to // operation. Otherwise the assumption that the task will never throw // if `operation` does not throw, which we are making in `async` does From e5254c0c8ebf5245fa9284bc3222bc00ced5549d Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 29 Apr 2024 10:54:15 -0700 Subject: [PATCH 06/32] Allow `serverOptions` to be specified on `SwiftPMTestProject` This will allow us to conditionally enable background indexing in tests --- Sources/SKTestSupport/MultiFileTestProject.swift | 3 +++ Sources/SKTestSupport/SwiftPMTestProject.swift | 3 +++ 2 files changed, 6 insertions(+) diff --git a/Sources/SKTestSupport/MultiFileTestProject.swift b/Sources/SKTestSupport/MultiFileTestProject.swift index cd9b8f000..68baba274 100644 --- a/Sources/SKTestSupport/MultiFileTestProject.swift +++ b/Sources/SKTestSupport/MultiFileTestProject.swift @@ -13,6 +13,7 @@ import Foundation import LanguageServerProtocol import SKCore +import SourceKitLSP /// The location of a test file within test workspace. public struct RelativeFileLocation: Hashable, ExpressibleByStringLiteral { @@ -79,6 +80,7 @@ public class MultiFileTestProject { public init( files: [RelativeFileLocation: String], workspaces: (URL) async throws -> [WorkspaceFolder] = { [WorkspaceFolder(uri: DocumentURI($0))] }, + serverOptions: SourceKitLSPServer.Options = .testDefault, usePullDiagnostics: Bool = true, testName: String = #function ) async throws { @@ -109,6 +111,7 @@ public class MultiFileTestProject { self.fileData = fileData self.testClient = try await TestSourceKitLSPClient( + serverOptions: serverOptions, usePullDiagnostics: usePullDiagnostics, workspaceFolders: workspaces(scratchDirectory), cleanUp: { [scratchDirectory] in diff --git a/Sources/SKTestSupport/SwiftPMTestProject.swift b/Sources/SKTestSupport/SwiftPMTestProject.swift index fa0a9dea7..aa2737cea 100644 --- a/Sources/SKTestSupport/SwiftPMTestProject.swift +++ b/Sources/SKTestSupport/SwiftPMTestProject.swift @@ -13,6 +13,7 @@ import Foundation import LanguageServerProtocol @_spi(Testing) import SKCore +import SourceKitLSP import TSCBasic public class SwiftPMTestProject: MultiFileTestProject { @@ -41,6 +42,7 @@ public class SwiftPMTestProject: MultiFileTestProject { workspaces: (URL) async throws -> [WorkspaceFolder] = { [WorkspaceFolder(uri: DocumentURI($0))] }, build: Bool = false, allowBuildFailure: Bool = false, + serverOptions: SourceKitLSPServer.Options = .testDefault, usePullDiagnostics: Bool = true, testName: String = #function ) async throws { @@ -63,6 +65,7 @@ public class SwiftPMTestProject: MultiFileTestProject { try await super.init( files: filesByPath, workspaces: workspaces, + serverOptions: serverOptions, usePullDiagnostics: usePullDiagnostics, testName: testName ) From 9c01f876597c57d93be0652c1d328ed0c7e3128e Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 29 Apr 2024 10:57:13 -0700 Subject: [PATCH 07/32] Pass all `SourceKitLSPServer.Options` to `Workspace` This allows us to enable background indexing for a workspace based on a value in `SourceKitLSPServer.Options`. --- Sources/SourceKitLSP/SourceKitLSPServer.swift | 7 +-- Sources/SourceKitLSP/Workspace.swift | 47 ++++++++++--------- .../SourceKitLSPTests/BuildSystemTests.swift | 2 +- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 868057344..55c1080af 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -1177,13 +1177,14 @@ extension SourceKitLSPServer { logger.log("Cannot open workspace before server is initialized") return nil } - let workspaceBuildSetup = self.buildSetup(for: workspaceFolder) + var options = self.options + options.buildSetup = self.options.buildSetup.merging(buildSetup(for: workspaceFolder)) return try? await Workspace( documentManager: self.documentManager, rootUri: workspaceFolder.uri, capabilityRegistry: capabilityRegistry, toolchainRegistry: self.toolchainRegistry, - buildSetup: self.options.buildSetup.merging(workspaceBuildSetup), + options: options, compilationDatabaseSearchPaths: self.options.compilationDatabaseSearchPaths, indexOptions: self.options.indexOptions, reloadPackageStatusCallback: { [weak self] status in @@ -1251,7 +1252,7 @@ extension SourceKitLSPServer { rootUri: req.rootURI, capabilityRegistry: self.capabilityRegistry!, toolchainRegistry: self.toolchainRegistry, - buildSetup: self.options.buildSetup, + options: self.options, underlyingBuildSystem: nil, index: nil, indexDelegate: nil diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index 455da594c..998647eff 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/Sources/SourceKitLSP/Workspace.swift @@ -78,13 +78,13 @@ public final class Workspace { rootUri: DocumentURI?, capabilityRegistry: CapabilityRegistry, toolchainRegistry: ToolchainRegistry, - buildSetup: BuildSetup, + options: SourceKitLSPServer.Options, underlyingBuildSystem: BuildSystem?, index uncheckedIndex: UncheckedIndex?, indexDelegate: SourceKitIndexDelegate? ) async { self.documentManager = documentManager - self.buildSetup = buildSetup + self.buildSetup = options.buildSetup self.rootUri = rootUri self.capabilityRegistry = capabilityRegistry self.uncheckedIndex = uncheckedIndex @@ -117,36 +117,36 @@ public final class Workspace { rootUri: DocumentURI, capabilityRegistry: CapabilityRegistry, toolchainRegistry: ToolchainRegistry, - buildSetup: BuildSetup, + options: SourceKitLSPServer.Options, compilationDatabaseSearchPaths: [RelativePath], indexOptions: IndexOptions = IndexOptions(), reloadPackageStatusCallback: @escaping (ReloadPackageStatus) async -> Void ) async throws { var buildSystem: BuildSystem? = nil - func createSwiftPMBuildSystem(rootUrl: URL) async -> SwiftPMBuildSystem? { - return await SwiftPMBuildSystem( - url: rootUrl, - toolchainRegistry: toolchainRegistry, - buildSetup: buildSetup, - reloadPackageStatusCallback: reloadPackageStatusCallback - ) - } + if let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) { + func createSwiftPMBuildSystem(rootUrl: URL) async -> SwiftPMBuildSystem? { + return await SwiftPMBuildSystem( + url: rootUrl, + toolchainRegistry: toolchainRegistry, + buildSetup: options.buildSetup, + reloadPackageStatusCallback: reloadPackageStatusCallback + ) + } - func createCompilationDatabaseBuildSystem(rootPath: AbsolutePath) -> CompilationDatabaseBuildSystem? { - return CompilationDatabaseBuildSystem( - projectRoot: rootPath, - searchPaths: compilationDatabaseSearchPaths - ) - } + func createCompilationDatabaseBuildSystem(rootPath: AbsolutePath) -> CompilationDatabaseBuildSystem? { + return CompilationDatabaseBuildSystem( + projectRoot: rootPath, + searchPaths: compilationDatabaseSearchPaths + ) + } - func createBuildServerBuildSystem(rootPath: AbsolutePath) async -> BuildServerBuildSystem? { - return await BuildServerBuildSystem(projectRoot: rootPath, buildSetup: buildSetup) - } + func createBuildServerBuildSystem(rootPath: AbsolutePath) async -> BuildServerBuildSystem? { + return await BuildServerBuildSystem(projectRoot: rootPath, buildSetup: options.buildSetup) + } - if let rootUrl = rootUri.fileURL, let rootPath = try? AbsolutePath(validating: rootUrl.path) { let defaultBuildSystem: BuildSystem? = - switch buildSetup.defaultWorkspaceType { + switch options.buildSetup.defaultWorkspaceType { case .buildServer: await createBuildServerBuildSystem(rootPath: rootPath) case .compilationDatabase: createCompilationDatabaseBuildSystem(rootPath: rootPath) case .swiftPM: await createSwiftPMBuildSystem(rootUrl: rootUrl) @@ -184,6 +184,7 @@ public final class Workspace { var index: IndexStoreDB? = nil var indexDelegate: SourceKitIndexDelegate? = nil + let indexOptions = options.indexOptions if let storePath = await firstNonNil(indexOptions.indexStorePath, await buildSystem?.indexStorePath), let dbPath = await firstNonNil(indexOptions.indexDatabasePath, await buildSystem?.indexDatabasePath), let libPath = await toolchainRegistry.default?.libIndexStore @@ -212,7 +213,7 @@ public final class Workspace { rootUri: rootUri, capabilityRegistry: capabilityRegistry, toolchainRegistry: toolchainRegistry, - buildSetup: buildSetup, + options: options, underlyingBuildSystem: buildSystem, index: UncheckedIndex(index), indexDelegate: indexDelegate diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index 4e5cd46f0..c0ac9f0d0 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -101,7 +101,7 @@ final class BuildSystemTests: XCTestCase { rootUri: nil, capabilityRegistry: CapabilityRegistry(clientCapabilities: ClientCapabilities()), toolchainRegistry: ToolchainRegistry.forTesting, - buildSetup: SourceKitLSPServer.Options.testDefault.buildSetup, + options: SourceKitLSPServer.Options.testDefault, underlyingBuildSystem: buildSystem, index: nil, indexDelegate: nil From 221c9b28525c1e332ec11e455cf7c90f145d60b8 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 08:15:56 -0700 Subject: [PATCH 08/32] Move logic to select the toolchain for a document from `SourceKitLSPServer` to `BuildSystemManager` This allows us to determine the toolchain to use during background indexing. It also moves toolchain selection closer to the build system, which is good because when we support multiple toolchains for a single workspace, the build system is what decides which toolchain to use for which document. --- Sources/SKCore/BuildSystemManager.swift | 14 ++++++++ Sources/SKCore/ToolchainRegistry.swift | 28 +++++++++++++++ Sources/SourceKitLSP/SourceKitLSPServer.swift | 28 +-------------- Sources/SourceKitLSP/Workspace.swift | 3 +- .../SKCoreTests/BuildSystemManagerTests.swift | 34 ++++++++++++------- 5 files changed, 67 insertions(+), 40 deletions(-) diff --git a/Sources/SKCore/BuildSystemManager.swift b/Sources/SKCore/BuildSystemManager.swift index a6a44a067..0e03e6b21 100644 --- a/Sources/SKCore/BuildSystemManager.swift +++ b/Sources/SKCore/BuildSystemManager.swift @@ -50,6 +50,11 @@ public actor BuildSystemManager { /// Build system delegate that will receive notifications about setting changes, etc. var delegate: BuildSystemDelegate? + /// The list of toolchains that are available. + /// + /// Used to determine which toolchain to use for a given document. + private let toolchainRegistry: ToolchainRegistry + /// The root of the project that this build system manages. For example, for SwiftPM packages, this is the folder /// containing Package.swift. For compilation databases it is the root folder based on which the compilation database /// was found. @@ -65,6 +70,7 @@ public actor BuildSystemManager { buildSystem: BuildSystem?, fallbackBuildSystem: FallbackBuildSystem?, mainFilesProvider: MainFilesProvider?, + toolchainRegistry: ToolchainRegistry, fallbackSettingsTimeout: DispatchTimeInterval = .seconds(3) ) async { let buildSystemHasDelegate = await buildSystem?.delegate != nil @@ -72,6 +78,7 @@ public actor BuildSystemManager { self.buildSystem = buildSystem self.fallbackBuildSystem = fallbackBuildSystem self.mainFilesProvider = mainFilesProvider + self.toolchainRegistry = toolchainRegistry self.fallbackSettingsTimeout = fallbackSettingsTimeout await self.buildSystem?.setDelegate(self) } @@ -87,6 +94,13 @@ extension BuildSystemManager { self.delegate = delegate } + /// Returns the toolchain that should be used to process the given document. + public func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { + // To support multiple toolchains within a single workspace, we need to ask the build system which toolchain to use + // for this document. + return await toolchainRegistry.defaultToolchain(for: language) + } + /// - Note: Needed so we can set the delegate from a different isolation context. public func setMainFilesProvider(_ mainFilesProvider: MainFilesProvider?) { self.mainFilesProvider = mainFilesProvider diff --git a/Sources/SKCore/ToolchainRegistry.swift b/Sources/SKCore/ToolchainRegistry.swift index 86edbe7df..ee0010015 100644 --- a/Sources/SKCore/ToolchainRegistry.swift +++ b/Sources/SKCore/ToolchainRegistry.swift @@ -12,6 +12,7 @@ import Dispatch import Foundation +import LanguageServerProtocol import SKSupport import struct TSCBasic.AbsolutePath @@ -244,6 +245,33 @@ public final actor ToolchainRegistry { public var darwinToolchainIdentifier: String { return darwinToolchainOverride ?? ToolchainRegistry.darwinDefaultToolchainIdentifier } + + /// The toolchain to use for a document in the given language if the build system doesn't override it. + func defaultToolchain(for language: Language) -> Toolchain? { + let supportsLang = { (toolchain: Toolchain) -> Bool in + // FIXME: the fact that we're looking at clangd/sourcekitd instead of the compiler indicates this method needs a parameter stating what kind of tool we're looking for. + switch language { + case .swift: + return toolchain.sourcekitd != nil + case .c, .cpp, .objective_c, .objective_cpp: + return toolchain.clangd != nil + default: + return false + } + } + + if let toolchain = self.default, supportsLang(toolchain) { + return toolchain + } + + for toolchain in toolchains { + if supportsLang(toolchain) { + return toolchain + } + } + + return nil + } } /// Inspecting internal state for testing purposes. diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 55c1080af..61b973bc6 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -653,32 +653,6 @@ public actor SourceKitLSPServer { return try await client.send(request) } - func toolchain(for uri: DocumentURI, _ language: Language) async -> Toolchain? { - let supportsLang = { (toolchain: Toolchain) -> Bool in - // FIXME: the fact that we're looking at clangd/sourcekitd instead of the compiler indicates this method needs a parameter stating what kind of tool we're looking for. - switch language { - case .swift: - return toolchain.sourcekitd != nil - case .c, .cpp, .objective_c, .objective_cpp: - return toolchain.clangd != nil - default: - return false - } - } - - if let toolchain = await toolchainRegistry.default, supportsLang(toolchain) { - return toolchain - } - - for toolchain in await toolchainRegistry.toolchains { - if supportsLang(toolchain) { - return toolchain - } - } - - return nil - } - /// After the language service has crashed, send `DidOpenTextDocumentNotification`s to a newly instantiated language service for previously open documents. func reopenDocuments(for languageService: LanguageService) async { for documentUri in self.documentManager.openDocuments { @@ -815,7 +789,7 @@ public actor SourceKitLSPServer { return service } - guard let toolchain = await toolchain(for: uri, language), + guard let toolchain = await workspace.buildSystemManager.toolchain(for: uri, language), let service = await languageService(for: toolchain, language, in: workspace) else { return nil diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index 998647eff..bba836a3f 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/Sources/SourceKitLSP/Workspace.swift @@ -91,7 +91,8 @@ public final class Workspace { self.buildSystemManager = await BuildSystemManager( buildSystem: underlyingBuildSystem, fallbackBuildSystem: FallbackBuildSystem(buildSetup: buildSetup), - mainFilesProvider: uncheckedIndex + mainFilesProvider: uncheckedIndex, + toolchainRegistry: toolchainRegistry ) await indexDelegate?.addMainFileChangedCallback { [weak self] in await self?.buildSystemManager.mainFilesChanged() diff --git a/Tests/SKCoreTests/BuildSystemManagerTests.swift b/Tests/SKCoreTests/BuildSystemManagerTests.swift index 54a20dddb..156a2f1c6 100644 --- a/Tests/SKCoreTests/BuildSystemManagerTests.swift +++ b/Tests/SKCoreTests/BuildSystemManagerTests.swift @@ -13,7 +13,7 @@ import BuildServerProtocol import LSPTestSupport import LanguageServerProtocol -import SKCore +@_spi(Testing) import SKCore import TSCBasic import XCTest @@ -37,7 +37,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: nil, fallbackBuildSystem: FallbackBuildSystem(buildSetup: .default), - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. @@ -88,13 +89,14 @@ final class BuildSystemManagerTests: XCTestCase { } func testSettingsMainFile() async throws { - let a = try try DocumentURI(string: "bsm:a.swift") + let a = try DocumentURI(string: "bsm:a.swift") let mainFiles = ManualMainFilesProvider([a: [a]]) let bs = ManualBuildSystem() let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -117,7 +119,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -139,7 +142,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: fallback, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -168,7 +172,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -208,7 +213,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -246,7 +252,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -300,7 +307,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -341,7 +349,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) @@ -383,7 +392,8 @@ final class BuildSystemManagerTests: XCTestCase { let bsm = await BuildSystemManager( buildSystem: bs, fallbackBuildSystem: nil, - mainFilesProvider: mainFiles + mainFilesProvider: mainFiles, + toolchainRegistry: ToolchainRegistry.forTesting ) defer { withExtendedLifetime(bsm) {} } // Keep BSM alive for callbacks. let del = await BSMDelegate(bsm) From 5e4f1b03bfde8d54b9ac1192831eef47d5bfcba4 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 29 Apr 2024 16:48:30 -0700 Subject: [PATCH 09/32] Generalize `BuildSystem.testFiles` to return all source files in a project --- Sources/SKCore/BuildServerBuildSystem.swift | 8 ++--- Sources/SKCore/BuildSystem.swift | 30 +++++++++++++------ Sources/SKCore/BuildSystemManager.swift | 11 ++++++- .../CompilationDatabaseBuildSystem.swift | 11 +++++-- .../SwiftPMBuildSystem.swift | 13 +++++--- Sources/SourceKitLSP/Workspace.swift | 2 +- .../SKCoreTests/BuildSystemManagerTests.swift | 4 +-- .../SourceKitLSPTests/BuildSystemTests.swift | 4 +-- 8 files changed, 57 insertions(+), 26 deletions(-) diff --git a/Sources/SKCore/BuildServerBuildSystem.swift b/Sources/SKCore/BuildServerBuildSystem.swift index 19d7270fb..7bd9a500d 100644 --- a/Sources/SKCore/BuildServerBuildSystem.swift +++ b/Sources/SKCore/BuildServerBuildSystem.swift @@ -317,14 +317,14 @@ extension BuildServerBuildSystem: BuildSystem { return .unhandled } - public func testFiles() async -> [DocumentURI] { - // BuildServerBuildSystem does not support syntactic test discovery + public func sourceFiles() async -> [SourceFileInfo] { + // BuildServerBuildSystem does not support syntactic test discovery or background indexing. // (https://github.com/apple/sourcekit-lsp/issues/1173). return [] } - public func addTestFilesDidChangeCallback(_ callback: @escaping () async -> Void) { - // BuildServerBuildSystem does not support syntactic test discovery + public func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) { + // BuildServerBuildSystem does not support syntactic test discovery or background indexing. // (https://github.com/apple/sourcekit-lsp/issues/1173). } } diff --git a/Sources/SKCore/BuildSystem.swift b/Sources/SKCore/BuildSystem.swift index e9756023d..a0954fd14 100644 --- a/Sources/SKCore/BuildSystem.swift +++ b/Sources/SKCore/BuildSystem.swift @@ -27,6 +27,22 @@ public enum FileHandlingCapability: Comparable, Sendable { case handled } +public struct SourceFileInfo: Sendable { + /// The URI of the source file. + public let uri: DocumentURI + + /// Whether the file might contain test cases. This property is an over-approximation. It might be true for files + /// from non-test targets or files that don't actually contain any tests. Keeping this list of files with + /// `mayContainTets` minimal as possible helps reduce the amount of work that the syntactic test indexer needs to + /// perform. + public let mayContainTests: Bool + + public init(uri: DocumentURI, mayContainTests: Bool) { + self.uri = uri + self.mayContainTests = mayContainTests + } +} + /// Provider of FileBuildSettings and other build-related information. /// /// The primary role of the build system is to answer queries for @@ -88,17 +104,13 @@ public protocol BuildSystem: AnyObject, Sendable { func fileHandlingCapability(for uri: DocumentURI) async -> FileHandlingCapability - /// Returns the list of files that might contain test cases. - /// - /// The returned file list is an over-approximation. It might contain tests from non-test targets or files that don't - /// actually contain any tests. Keeping this list as minimal as possible helps reduce the amount of work that the - /// syntactic test indexer needs to perform. - func testFiles() async -> [DocumentURI] + /// Returns the list of source files in the project. + func sourceFiles() async -> [SourceFileInfo] - /// Adds a callback that should be called when the value returned by `testFiles()` changes. + /// Adds a callback that should be called when the value returned by `sourceFiles()` changes. /// - /// The callback might also be called without an actual change to `testFiles`. - func addTestFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async + /// The callback might also be called without an actual change to `sourceFiles`. + func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async } public let buildTargetsNotSupported = diff --git a/Sources/SKCore/BuildSystemManager.swift b/Sources/SKCore/BuildSystemManager.swift index 0e03e6b21..926037f06 100644 --- a/Sources/SKCore/BuildSystemManager.swift +++ b/Sources/SKCore/BuildSystemManager.swift @@ -191,8 +191,17 @@ extension BuildSystemManager { ) } + public func sourceFiles() async -> [SourceFileInfo] { + return await buildSystem?.sourceFiles() ?? [] + } + public func testFiles() async -> [DocumentURI] { - return await buildSystem?.testFiles() ?? [] + return await sourceFiles().compactMap { (info: SourceFileInfo) -> DocumentURI? in + guard info.mayContainTests else { + return nil + } + return info.uri + } } } diff --git a/Sources/SKCore/CompilationDatabaseBuildSystem.swift b/Sources/SKCore/CompilationDatabaseBuildSystem.swift index 929110f21..5e0e2fe77 100644 --- a/Sources/SKCore/CompilationDatabaseBuildSystem.swift +++ b/Sources/SKCore/CompilationDatabaseBuildSystem.swift @@ -192,11 +192,16 @@ extension CompilationDatabaseBuildSystem: BuildSystem { } } - public func testFiles() async -> [DocumentURI] { - return compdb?.allCommands.map { DocumentURI($0.url) } ?? [] + public func sourceFiles() async -> [SourceFileInfo] { + guard let compdb else { + return [] + } + return compdb.allCommands.map { + SourceFileInfo(uri: DocumentURI($0.url), mayContainTests: true) + } } - public func addTestFilesDidChangeCallback(_ callback: @escaping () async -> Void) async { + public func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) async { testFilesDidChangeCallbacks.append(callback) } } diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift index 4bb6ee64a..c45996073 100644 --- a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift +++ b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift @@ -437,17 +437,22 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem { } } - public func testFiles() -> [DocumentURI] { - return fileToTarget.compactMap { (path, target) -> DocumentURI? in + public func sourceFiles() -> [SourceFileInfo] { + return fileToTarget.compactMap { (path, target) -> SourceFileInfo? in guard target.isPartOfRootPackage else { // Don't consider files from package dependencies as possible test files. return nil } - return DocumentURI(path.asURL) + // We should only set mayContainTests to `true` for files from test targets + // (https://github.com/apple/sourcekit-lsp/issues/1174). + return SourceFileInfo( + uri: DocumentURI(path.asURL), + mayContainTests: true + ) } } - public func addTestFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async { + public func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async { testFilesDidChangeCallbacks.append(callback) } } diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index bba836a3f..50ec6a490 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/Sources/SourceKitLSP/Workspace.swift @@ -97,7 +97,7 @@ public final class Workspace { await indexDelegate?.addMainFileChangedCallback { [weak self] in await self?.buildSystemManager.mainFilesChanged() } - await underlyingBuildSystem?.addTestFilesDidChangeCallback { [weak self] in + await underlyingBuildSystem?.addSourceFilesDidChangeCallback { [weak self] in guard let self else { return } diff --git a/Tests/SKCoreTests/BuildSystemManagerTests.swift b/Tests/SKCoreTests/BuildSystemManagerTests.swift index 156a2f1c6..96ec68901 100644 --- a/Tests/SKCoreTests/BuildSystemManagerTests.swift +++ b/Tests/SKCoreTests/BuildSystemManagerTests.swift @@ -469,11 +469,11 @@ class ManualBuildSystem: BuildSystem { } } - func testFiles() async -> [DocumentURI] { + func sourceFiles() async -> [SourceFileInfo] { return [] } - func addTestFilesDidChangeCallback(_ callback: @escaping () async -> Void) {} + func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) {} } /// A `BuildSystemDelegate` setup for testing. diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index c0ac9f0d0..092520dd8 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -61,11 +61,11 @@ final class TestBuildSystem: BuildSystem { } } - func testFiles() async -> [DocumentURI] { + func sourceFiles() async -> [SourceFileInfo] { return [] } - func addTestFilesDidChangeCallback(_ callback: @escaping () async -> Void) async {} + func addSourceFilesDidChangeCallback(_ callback: @escaping () async -> Void) async {} } final class BuildSystemTests: XCTestCase { From 1f87e7fc4be13bab99ffcaa3addb1a17595a23e6 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 13:47:21 -0700 Subject: [PATCH 10/32] Move `CheckedIndex` to a new `SemanticIndex` module --- Package.swift | 13 +++++++++ Sources/CMakeLists.txt | 1 + Sources/SemanticIndex/CMakeLists.txt | 11 +++++++ .../CheckedIndex.swift | 29 +++++++------------ Sources/SourceKitLSP/CMakeLists.txt | 2 +- Sources/SourceKitLSP/DocumentManager.swift | 15 +++++++++- .../IndexStoreDB+MainFilesProvider.swift | 1 + Sources/SourceKitLSP/Rename.swift | 1 + Sources/SourceKitLSP/SourceKitLSPServer.swift | 1 + Sources/SourceKitLSP/Workspace.swift | 1 + 10 files changed, 55 insertions(+), 20 deletions(-) create mode 100644 Sources/SemanticIndex/CMakeLists.txt rename Sources/{SourceKitLSP => SemanticIndex}/CheckedIndex.swift (95%) diff --git a/Package.swift b/Package.swift index f5c9932d0..9237baae2 100644 --- a/Package.swift +++ b/Package.swift @@ -168,6 +168,18 @@ let package = Package( ] ), + // MARK: SemanticIndex + + .target( + name: "SemanticIndex", + dependencies: [ + "LSPLogging", + "SKCore", + .product(name: "IndexStoreDB", package: "indexstore-db"), + ], + exclude: ["CMakeLists.txt"] + ), + // MARK: SKCore // Data structures and algorithms useful across the project, but not necessarily // suitable for use in other packages. @@ -303,6 +315,7 @@ let package = Package( "LanguageServerProtocol", "LanguageServerProtocolJSONRPC", "LSPLogging", + "SemanticIndex", "SKCore", "SKSupport", "SKSwiftPMWorkspace", diff --git a/Sources/CMakeLists.txt b/Sources/CMakeLists.txt index 368f340eb..4ed6c0350 100644 --- a/Sources/CMakeLists.txt +++ b/Sources/CMakeLists.txt @@ -5,6 +5,7 @@ add_subdirectory(Diagnose) add_subdirectory(LanguageServerProtocol) add_subdirectory(LanguageServerProtocolJSONRPC) add_subdirectory(LSPLogging) +add_subdirectory(SemanticIndex) add_subdirectory(SKCore) add_subdirectory(SKSupport) add_subdirectory(SKSwiftPMWorkspace) diff --git a/Sources/SemanticIndex/CMakeLists.txt b/Sources/SemanticIndex/CMakeLists.txt new file mode 100644 index 000000000..cc49bb903 --- /dev/null +++ b/Sources/SemanticIndex/CMakeLists.txt @@ -0,0 +1,11 @@ + +add_library(SemanticIndex STATIC + CheckedIndex.swift +) +set_target_properties(SemanticIndex PROPERTIES + INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY}) +target_link_libraries(SemanticIndex PRIVATE + LSPLogging + SKCore + IndexStoreDB + $<$>:Foundation>) diff --git a/Sources/SourceKitLSP/CheckedIndex.swift b/Sources/SemanticIndex/CheckedIndex.swift similarity index 95% rename from Sources/SourceKitLSP/CheckedIndex.swift rename to Sources/SemanticIndex/CheckedIndex.swift index 814ed8cc4..49161b4b1 100644 --- a/Sources/SourceKitLSP/CheckedIndex.swift +++ b/Sources/SemanticIndex/CheckedIndex.swift @@ -15,6 +15,15 @@ import IndexStoreDB import LSPLogging import LanguageServerProtocol +/// Essentially a `DocumentManager` from the `SourceKitLSP` module. +/// +/// Protocol is needed because the `SemanticIndex` module is lower-level than the `SourceKitLSP` module. +public protocol InMemoryDocumentManager { + /// Returns true if the file at the given URL has a different content in the document manager than on-disk. This is + /// the case if the user made edits to the file but didn't save them yet. + func fileHasInMemoryModifications(_ url: URL) -> Bool +} + public enum IndexCheckLevel { /// Consider the index out-of-date only if the source file has been deleted on disk. /// @@ -31,7 +40,7 @@ public enum IndexCheckLevel { /// Consider the index out-of-date if the source file has been deleted or modified on disk or if there are /// in-memory modifications in the given `DocumentManager`. - case inMemoryModifiedFiles(DocumentManager) + case inMemoryModifiedFiles(InMemoryDocumentManager) } /// A wrapper around `IndexStoreDB` that checks if returned symbol occurrences are up-to-date with regard to a @@ -257,7 +266,7 @@ private struct IndexOutOfDateChecker { /// `documentManager` must always be the same between calls to `hasFileInMemoryModifications` since it is not part of /// the cache key. This is fine because we always assume the `documentManager` to come from the associated value of /// `CheckLevel.imMemoryModifiedFiles`, which is constant. - private mutating func fileHasInMemoryModifications(_ url: URL, documentManager: DocumentManager) -> Bool { + private mutating func fileHasInMemoryModifications(_ url: URL, documentManager: InMemoryDocumentManager) -> Bool { if let cached = fileHasInMemoryModificationsCache[url] { return cached } @@ -312,19 +321,3 @@ private struct IndexOutOfDateChecker { return fileExists } } - -extension DocumentManager { - /// Returns true if the file at the given URL has a different content in the document manager than on-disk. This is - /// the case if the user made edits to the file but didn't save them yet. - func fileHasInMemoryModifications(_ url: URL) -> Bool { - guard let document = try? latestSnapshot(DocumentURI(url)) else { - return false - } - - guard let onDiskFileContents = try? String(contentsOf: url, encoding: .utf8) else { - // If we can't read the file on disk, it can't match any on-disk state, so it's in-memory state - return true - } - return onDiskFileContents != document.lineTable.content - } -} diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index 12558497a..613b0b328 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/Sources/SourceKitLSP/CMakeLists.txt @@ -1,7 +1,6 @@ add_library(SourceKitLSP STATIC CapabilityRegistry.swift - CheckedIndex.swift DocumentManager.swift DocumentSnapshot+FromFileContents.swift IndexStoreDB+MainFilesProvider.swift @@ -62,6 +61,7 @@ target_link_libraries(SourceKitLSP PUBLIC LanguageServerProtocol LanguageServerProtocolJSONRPC LSPLogging + SemanticIndex SKCore SKSupport SKSwiftPMWorkspace diff --git a/Sources/SourceKitLSP/DocumentManager.swift b/Sources/SourceKitLSP/DocumentManager.swift index 2cf11ef78..2798dd72e 100644 --- a/Sources/SourceKitLSP/DocumentManager.swift +++ b/Sources/SourceKitLSP/DocumentManager.swift @@ -14,6 +14,7 @@ import Dispatch import LSPLogging import LanguageServerProtocol import SKSupport +import SemanticIndex import SwiftSyntax /// An immutable snapshot of a document at a given time. @@ -83,7 +84,7 @@ public final class Document { } } -public final class DocumentManager { +public final class DocumentManager: InMemoryDocumentManager { public enum Error: Swift.Error { case alreadyOpen(DocumentURI) @@ -187,6 +188,18 @@ public final class DocumentManager { return document.latestSnapshot } } + + public func fileHasInMemoryModifications(_ url: URL) -> Bool { + guard let document = try? latestSnapshot(DocumentURI(url)) else { + return false + } + + guard let onDiskFileContents = try? String(contentsOf: url, encoding: .utf8) else { + // If we can't read the file on disk, it can't match any on-disk state, so it's in-memory state + return true + } + return onDiskFileContents != document.lineTable.content + } } extension DocumentManager { diff --git a/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift b/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift index 0f78875cc..1d7c0f11b 100644 --- a/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift +++ b/Sources/SourceKitLSP/IndexStoreDB+MainFilesProvider.swift @@ -14,6 +14,7 @@ import Foundation import LSPLogging import LanguageServerProtocol import SKCore +import SemanticIndex extension UncheckedIndex { public func mainFilesContainingFile(_ uri: DocumentURI) -> Set { diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index bf671f209..81c00fed9 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -14,6 +14,7 @@ import IndexStoreDB import LSPLogging import LanguageServerProtocol import SKSupport +import SemanticIndex import SourceKitD import SwiftSyntax diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 61b973bc6..a43ac5430 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -20,6 +20,7 @@ import PackageLoading import SKCore import SKSupport import SKSwiftPMWorkspace +import SemanticIndex import SourceKitD import struct PackageModel.BuildFlags diff --git a/Sources/SourceKitLSP/Workspace.swift b/Sources/SourceKitLSP/Workspace.swift index 50ec6a490..cb8d8ef52 100644 --- a/Sources/SourceKitLSP/Workspace.swift +++ b/Sources/SourceKitLSP/Workspace.swift @@ -16,6 +16,7 @@ import LanguageServerProtocol import SKCore import SKSupport import SKSwiftPMWorkspace +import SemanticIndex import struct TSCBasic.AbsolutePath import struct TSCBasic.RelativePath From 932f92c63db85b59769faf9cd1874ab2bc51e9df Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 08:44:34 -0700 Subject: [PATCH 11/32] Expose `pollForUnitChangesAndWait` on `CheckedIndex` --- Sources/SemanticIndex/CheckedIndex.swift | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Sources/SemanticIndex/CheckedIndex.swift b/Sources/SemanticIndex/CheckedIndex.swift index 49161b4b1..9df02bea5 100644 --- a/Sources/SemanticIndex/CheckedIndex.swift +++ b/Sources/SemanticIndex/CheckedIndex.swift @@ -47,7 +47,7 @@ public enum IndexCheckLevel { /// `IndexCheckLevel`. /// /// - SeeAlso: Comment on `IndexOutOfDateChecker` -public class CheckedIndex { +public final class CheckedIndex: Sendable { private var checker: IndexOutOfDateChecker private let index: IndexStoreDB @@ -135,6 +135,11 @@ public class CheckedIndex { public func fileHasInMemoryModifications(_ url: URL) -> Bool { return checker.fileHasInMemoryModifications(url) } + + /// Wait for IndexStoreDB to be updated based on new unit files written to disk. + public func pollForUnitChangesAndWait() { + self.index.pollForUnitChangesAndWait() + } } /// A wrapper around `IndexStoreDB` that allows the retrieval of a `CheckedIndex` with a specified check level or the From 9ff1ff1430f70f0e766b5a66f2501ed440d9ede8 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 08:56:31 -0700 Subject: [PATCH 12/32] Allow querying the build system for the language of a document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The build system has potentially more information about a document's language than we do based on the file’s extension. --- Sources/SKCore/BuildServerBuildSystem.swift | 4 ++++ Sources/SKCore/BuildSystem.swift | 10 ++++++++-- Sources/SKCore/BuildSystemManager.swift | 16 ++++++++++++++++ .../SKCore/CompilationDatabaseBuildSystem.swift | 4 ++++ .../SKSwiftPMWorkspace/SwiftPMBuildSystem.swift | 5 +++++ Tests/SKCoreTests/BuildSystemManagerTests.swift | 4 ++++ Tests/SourceKitLSPTests/BuildSystemTests.swift | 4 ++++ 7 files changed, 45 insertions(+), 2 deletions(-) diff --git a/Sources/SKCore/BuildServerBuildSystem.swift b/Sources/SKCore/BuildServerBuildSystem.swift index 7bd9a500d..f45dd93fa 100644 --- a/Sources/SKCore/BuildServerBuildSystem.swift +++ b/Sources/SKCore/BuildServerBuildSystem.swift @@ -267,6 +267,10 @@ extension BuildServerBuildSystem: BuildSystem { return buildSettings[document] } + public func defaultLanguage(for document: DocumentURI) async -> Language? { + return nil + } + public func registerForChangeNotifications(for uri: DocumentURI) { let request = RegisterForChanges(uri: uri, action: .register) _ = self.buildServer?.send(request) { result in diff --git a/Sources/SKCore/BuildSystem.swift b/Sources/SKCore/BuildSystem.swift index a0954fd14..ee48db020 100644 --- a/Sources/SKCore/BuildSystem.swift +++ b/Sources/SKCore/BuildSystem.swift @@ -87,6 +87,13 @@ public protocol BuildSystem: AnyObject, Sendable { /// file or if it hasn't computed build settings for the file yet. func buildSettings(for document: DocumentURI, language: Language) async throws -> FileBuildSettings? + /// If the build system has knowledge about the language that this document should be compiled in, return it. + /// + /// This is used to determine the language in which a source file should be background indexed. + /// + /// If `nil` is returned, the language based on the file's extension. + func defaultLanguage(for document: DocumentURI) async -> Language? + /// Register the given file for build-system level change notifications, such /// as command line flag changes, dependency changes, etc. /// @@ -113,5 +120,4 @@ public protocol BuildSystem: AnyObject, Sendable { func addSourceFilesDidChangeCallback(_ callback: @Sendable @escaping () async -> Void) async } -public let buildTargetsNotSupported = - ResponseError.methodNotFound(BuildTargets.method) +public let buildTargetsNotSupported = ResponseError.methodNotFound(BuildTargets.method) diff --git a/Sources/SKCore/BuildSystemManager.swift b/Sources/SKCore/BuildSystemManager.swift index 926037f06..ea726f942 100644 --- a/Sources/SKCore/BuildSystemManager.swift +++ b/Sources/SKCore/BuildSystemManager.swift @@ -106,6 +106,22 @@ extension BuildSystemManager { self.mainFilesProvider = mainFilesProvider } + /// Returns the language that a document should be interpreted in for background tasks where the editor doesn't + /// specify the document's language. + public func defaultLanguage(for document: DocumentURI) async -> Language? { + if let defaultLanguage = await buildSystem?.defaultLanguage(for: document) { + return defaultLanguage + } + switch document.fileURL?.pathExtension { + case "c": return .c + case "cpp", "cc", "cxx": return .cpp + case "m": return .objective_c + case "mm", "h": return .objective_cpp + case "swift": return .swift + default: return nil + } + } + private func buildSettings( for document: DocumentURI, language: Language diff --git a/Sources/SKCore/CompilationDatabaseBuildSystem.swift b/Sources/SKCore/CompilationDatabaseBuildSystem.swift index 5e0e2fe77..031b1d10c 100644 --- a/Sources/SKCore/CompilationDatabaseBuildSystem.swift +++ b/Sources/SKCore/CompilationDatabaseBuildSystem.swift @@ -114,6 +114,10 @@ extension CompilationDatabaseBuildSystem: BuildSystem { ) } + public func defaultLanguage(for document: DocumentURI) async -> Language? { + return nil + } + public func registerForChangeNotifications(for uri: DocumentURI) async { self.watchedFiles.insert(uri) } diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift index c45996073..881ebc0f0 100644 --- a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift +++ b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift @@ -341,6 +341,11 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem { return nil } + public func defaultLanguage(for document: DocumentURI) async -> Language? { + // TODO (indexing): Query The SwiftPM build system for the document's language + return nil + } + public func registerForChangeNotifications(for uri: DocumentURI) async { self.watchedFiles.insert(uri) } diff --git a/Tests/SKCoreTests/BuildSystemManagerTests.swift b/Tests/SKCoreTests/BuildSystemManagerTests.swift index 96ec68901..8dd89ccf7 100644 --- a/Tests/SKCoreTests/BuildSystemManagerTests.swift +++ b/Tests/SKCoreTests/BuildSystemManagerTests.swift @@ -449,6 +449,10 @@ class ManualBuildSystem: BuildSystem { return map[uri] } + public func defaultLanguage(for document: DocumentURI) async -> Language? { + return nil + } + func registerForChangeNotifications(for uri: DocumentURI) async { } diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index 092520dd8..f31103cea 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -43,6 +43,10 @@ final class TestBuildSystem: BuildSystem { return buildSettingsByFile[document] } + public func defaultLanguage(for document: DocumentURI) async -> Language? { + return nil + } + func registerForChangeNotifications(for uri: DocumentURI) async { watchedFiles.insert(uri) } From d114399270a888c0b0bb53635c6c0475923af50a Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 08:58:31 -0700 Subject: [PATCH 13/32] Add method on `Process` that waits until exit and sends a `SIGINT` to the process if the `Task` is cancelled --- Sources/SKSupport/CMakeLists.txt | 1 + ...rocess+WaitUntilExitWithCancellation.swift | 27 +++++++++++++++++++ .../Swift/DocumentFormatting.swift | 2 +- 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 Sources/SKSupport/Process+WaitUntilExitWithCancellation.swift diff --git a/Sources/SKSupport/CMakeLists.txt b/Sources/SKSupport/CMakeLists.txt index 657ac0720..5453bb59d 100644 --- a/Sources/SKSupport/CMakeLists.txt +++ b/Sources/SKSupport/CMakeLists.txt @@ -11,6 +11,7 @@ add_library(SKSupport STATIC DocumentURI+CustomLogStringConvertible.swift FileSystem.swift LineTable.swift + Process+WaitUntilExitWithCancellation.swift Random.swift Result.swift ThreadSafeBox.swift diff --git a/Sources/SKSupport/Process+WaitUntilExitWithCancellation.swift b/Sources/SKSupport/Process+WaitUntilExitWithCancellation.swift new file mode 100644 index 000000000..aa6f1c13f --- /dev/null +++ b/Sources/SKSupport/Process+WaitUntilExitWithCancellation.swift @@ -0,0 +1,27 @@ +//===----------------------------------------------------------------------===// +// +// 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 Foundation + +import class TSCBasic.Process +import struct TSCBasic.ProcessResult + +public extension Process { + /// Wait for the process to exit. If the task gets cancelled, during this time, send a `SIGINT` to the process. + func waitUntilExitSendingSigIntOnTaskCancellation() async throws -> ProcessResult { + return try await withTaskCancellationHandler { + try await waitUntilExit() + } onCancel: { + signal(SIGINT) + } + } +} diff --git a/Sources/SourceKitLSP/Swift/DocumentFormatting.swift b/Sources/SourceKitLSP/Swift/DocumentFormatting.swift index ed4d59da5..ed58a21a5 100644 --- a/Sources/SourceKitLSP/Swift/DocumentFormatting.swift +++ b/Sources/SourceKitLSP/Swift/DocumentFormatting.swift @@ -154,7 +154,7 @@ extension SwiftLanguageService { writeStream.send(snapshot.text) try writeStream.close() - let result = try await process.waitUntilExit() + let result = try await process.waitUntilExitSendingSigIntOnTaskCancellation() guard result.exitStatus == .terminated(code: 0) else { let swiftFormatErrorMessage: String switch result.stderrOutput { From 8590a4bdc2bb1d8e4deb7219d28a3342d2b6a80a Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 09:24:56 -0700 Subject: [PATCH 14/32] Use an atomic for `notificationIDForLogging` This allows us to get rid of the lock. --- Package.swift | 1 + Sources/CAtomics/include/CAtomics.h | 28 +++++++++++++++++++ Sources/SourceKitLSP/SourceKitLSPServer.swift | 17 ++--------- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/Package.swift b/Package.swift index 9237baae2..b93f1079f 100644 --- a/Package.swift +++ b/Package.swift @@ -312,6 +312,7 @@ let package = Package( name: "SourceKitLSP", dependencies: [ "BuildServerProtocol", + "CAtomics", "LanguageServerProtocol", "LanguageServerProtocolJSONRPC", "LSPLogging", diff --git a/Sources/CAtomics/include/CAtomics.h b/Sources/CAtomics/include/CAtomics.h index effbe62be..a5d273647 100644 --- a/Sources/CAtomics/include/CAtomics.h +++ b/Sources/CAtomics/include/CAtomics.h @@ -63,4 +63,32 @@ static inline void atomic_uint8_set(AtomicUInt8 *atomic, uint8_t newValue) { atomic->value = newValue; } +// MARK: AtomicInt + +typedef struct { + _Atomic(int) value; +} AtomicUInt32; + +__attribute__((swift_name("AtomicUInt32.init(initialValue:)"))) +static inline AtomicUInt32 atomic_int_create(uint8_t initialValue) { + AtomicUInt32 atomic; + atomic.value = initialValue; + return atomic; +} + +__attribute__((swift_name("getter:AtomicUInt32.value(self:)"))) +static inline uint32_t atomic_int_get(AtomicUInt32 *atomic) { + return atomic->value; +} + +__attribute__((swift_name("setter:AtomicUInt32.value(self:_:)"))) +static inline void atomic_uint32_set(AtomicUInt32 *atomic, uint32_t newValue) { + atomic->value = newValue; +} + +__attribute__((swift_name("AtomicUInt32.fetchAndIncrement(self:)"))) +static inline uint32_t atomic_uint32_fetch_and_increment(AtomicUInt32 *atomic) { + return atomic->value++; +} + #endif // SOURCEKITLSP_CATOMICS_H diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index a43ac5430..5c5dc4859 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -11,6 +11,7 @@ //===----------------------------------------------------------------------===// import BuildServerProtocol +import CAtomics import Dispatch import Foundation import IndexStoreDB @@ -812,19 +813,7 @@ public actor SourceKitLSPServer { // MARK: - MessageHandler -private let notificationIDForLoggingLock = NSLock() -private var notificationIDForLogging: Int = 0 - -/// On every call, returns a new unique number that can be used to identify a notification. -/// -/// This is needed so we can consistently refer to a notification using the `category` of the logger. -/// Requests don't need this since they already have a unique ID in the LSP protocol. -private func getNextNotificationIDForLogging() -> Int { - return notificationIDForLoggingLock.withLock { - notificationIDForLogging += 1 - return notificationIDForLogging - } -} +private var notificationIDForLogging = AtomicUInt32(initialValue: 1) extension SourceKitLSPServer: MessageHandler { public nonisolated func handle(_ params: some NotificationType) { @@ -835,7 +824,7 @@ extension SourceKitLSPServer: MessageHandler { self.cancelRequest(params) } - let notificationID = getNextNotificationIDForLogging() + let notificationID = notificationIDForLogging.fetchAndIncrement() let signposter = Logger(subsystem: subsystem, category: "notification-\(notificationID)").makeSignposter() let signpostID = signposter.makeSignpostID() From 47850f47cd925fef8f05b239b85066a334b54de5 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 09:43:52 -0700 Subject: [PATCH 15/32] Allow overriding the current logging subsystem --- Sources/LSPLogging/Logging.swift | 5 +-- Sources/LSPLogging/LoggingScope.swift | 35 +++++++++++++++++++ .../Clang/ClangLanguageService.swift | 2 +- Sources/SourceKitLSP/SourceKitLSPServer.swift | 5 +-- Tests/LSPLoggingTests/LoggingTests.swift | 4 +-- 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/Sources/LSPLogging/Logging.swift b/Sources/LSPLogging/Logging.swift index a67132cca..079924750 100644 --- a/Sources/LSPLogging/Logging.swift +++ b/Sources/LSPLogging/Logging.swift @@ -21,9 +21,6 @@ import Foundation -/// The subsystem that should be used for any logging by default. -public let subsystem = "org.swift.sourcekit-lsp" - #if canImport(os) && !SOURCEKITLSP_FORCE_NON_DARWIN_LOGGER import os // os_log @@ -44,5 +41,5 @@ public typealias Signposter = NonDarwinSignposter /// The logger that is used to log any messages. public var logger: Logger { - Logger(subsystem: subsystem, category: LoggingScope.scope) + Logger(subsystem: LoggingScope.subsystem, category: LoggingScope.scope) } diff --git a/Sources/LSPLogging/LoggingScope.swift b/Sources/LSPLogging/LoggingScope.swift index 644db803e..5ef40a303 100644 --- a/Sources/LSPLogging/LoggingScope.swift +++ b/Sources/LSPLogging/LoggingScope.swift @@ -13,15 +13,50 @@ import Foundation public final class LoggingScope { + /// The name of the current logging subsystem or `nil` if no logging scope is set. + @TaskLocal fileprivate static var _subsystem: String? + /// The name of the current logging scope or `nil` if no logging scope is set. @TaskLocal fileprivate static var _scope: String? + /// The name of the current logging subsystem. + public static var subsystem: String { + return _subsystem ?? "org.swift.sourcekit-lsp" + } + /// The name of the current logging scope. public static var scope: String { return _scope ?? "default" } } +/// Logs all messages created from the operation to the given subsystem. +/// +/// This overrides the current logging subsystem. +/// +/// - Note: Since this stores the logging subsystem in a task-local value, it only works when run inside a task. +/// Outside a task, this is a no-op. +public func withLoggingSubsystemAndScope( + subsystem: String, + scope: String?, + _ operation: () throws -> Result +) rethrows -> Result { + return try LoggingScope.$_subsystem.withValue(subsystem) { + return try LoggingScope.$_scope.withValue(scope, operation: operation) + } +} + +/// Same as `withLoggingSubsystemAndScope` but allows the operation to be `async`. +public func withLoggingSubsystemAndScope( + subsystem: String, + scope: String?, + _ operation: () async throws -> Result +) async rethrows -> Result { + return try await LoggingScope.$_subsystem.withValue(subsystem) { + return try await LoggingScope.$_scope.withValue(scope, operation: operation) + } +} + /// Create a new logging scope, which will be used as the category in any log messages created from the operation. /// /// This overrides the current logging scope. diff --git a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift index 9dfa85e44..7eaea0b34 100644 --- a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift +++ b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift @@ -45,7 +45,7 @@ fileprivate class ClangdStderrLogForwarder { // than 1000 bytes long but if we merge multiple lines into one message, we might easily exceed this limit. // b) It might be confusing why sometimes a single log message contains one line while sometimes it contains // multiple. - let logger = Logger(subsystem: subsystem, category: "clangd-stderr") + let logger = Logger(subsystem: LoggingScope.subsystem, category: "clangd-stderr") logger.info("\(String(data: self.buffer[...newlineIndex], encoding: .utf8) ?? "")") buffer = buffer[buffer.index(after: newlineIndex)...] } diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 5c5dc4859..fce17f037 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -826,7 +826,8 @@ extension SourceKitLSPServer: MessageHandler { let notificationID = notificationIDForLogging.fetchAndIncrement() - let signposter = Logger(subsystem: subsystem, category: "notification-\(notificationID)").makeSignposter() + let signposter = Logger(subsystem: LoggingScope.subsystem, category: "notification-\(notificationID)") + .makeSignposter() let signpostID = signposter.makeSignpostID() let state = signposter.beginInterval("Notification", id: signpostID, "\(type(of: params))") messageHandlingQueue.async(metadata: TaskMetadata(params)) { @@ -875,7 +876,7 @@ extension SourceKitLSPServer: MessageHandler { id: RequestID, reply: @escaping (LSPResult) -> Void ) { - let signposter = Logger(subsystem: subsystem, category: "request-\(id)").makeSignposter() + let signposter = Logger(subsystem: LoggingScope.subsystem, category: "request-\(id)").makeSignposter() let signpostID = signposter.makeSignpostID() let state = signposter.beginInterval("Request", id: signpostID, "\(R.self)") diff --git a/Tests/LSPLoggingTests/LoggingTests.swift b/Tests/LSPLoggingTests/LoggingTests.swift index 710726747..8e596704c 100644 --- a/Tests/LSPLoggingTests/LoggingTests.swift +++ b/Tests/LSPLoggingTests/LoggingTests.swift @@ -25,7 +25,7 @@ fileprivate func assertLogging( // nonisolated(unsafe) because calls of `assertLogging` do not log to `logHandler` concurrently. nonisolated(unsafe) var messages: [String] = [] let logger = NonDarwinLogger( - subsystem: subsystem, + subsystem: LoggingScope.subsystem, category: "test", logLevel: logLevel, privacyLevel: privacyLevel, @@ -75,7 +75,7 @@ final class LoggingTests: XCTestCase { // nonisolated(unsafe) because we only have a single call to `logger.log` and that cannot race. nonisolated(unsafe) var message: String = "" let logger = NonDarwinLogger( - subsystem: subsystem, + subsystem: LoggingScope.subsystem, category: "test", logHandler: { message = $0 From 28d0dd3b9377c281b4425978e92327d657b447f0 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 2 May 2024 11:34:00 -0700 Subject: [PATCH 16/32] Log messages from `TaskScheduler` to a different subsystem --- Sources/SKCore/TaskScheduler.swift | 66 ++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/Sources/SKCore/TaskScheduler.swift b/Sources/SKCore/TaskScheduler.swift index fac5b3365..deb3a3399 100644 --- a/Sources/SKCore/TaskScheduler.swift +++ b/Sources/SKCore/TaskScheduler.swift @@ -20,6 +20,8 @@ public enum TaskDependencyAction { case cancelAndRescheduleDependency(TaskDescription) } +private let taskSchedulerSubsystem = "org.swift.sourcekit-lsp.task-scheduler" + public protocol TaskDescriptionProtocol: Identifiable, Sendable, CustomLogStringConvertible { /// Execute the task. /// @@ -232,9 +234,11 @@ fileprivate actor QueuedTask { /// a new task that depends on it. Otherwise a no-op. nonisolated func elevatePriority(to targetPriority: TaskPriority) { if priority < targetPriority { - logger.debug( - "Elevating priority of \(self.description.forLogging) from \(self.priority.rawValue) to \(targetPriority.rawValue)" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug( + "Elevating priority of \(self.description.forLogging) from \(self.priority.rawValue) to \(targetPriority.rawValue)" + ) + } Task(priority: targetPriority) { await self.resultTask.value } @@ -306,7 +310,9 @@ public actor TaskScheduler { priority: TaskPriority? = nil, _ taskDescription: TaskDescription ) async -> Task { - logger.debug("Scheduling \(taskDescription.forLogging)") + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug("Scheduling \(taskDescription.forLogging)") + } let queuedTask = await QueuedTask(priority: priority, description: taskDescription) pendingTasks.append(queuedTask) Task.detached(priority: priority ?? Task.currentPriority) { @@ -361,13 +367,17 @@ public actor TaskScheduler { case .cancelAndRescheduleDependency(let taskDescription): guard let dependency = self.currentlyExecutingTasks.first(where: { $0.description.id == taskDescription.id }) else { - logger.fault( - "Cannot find task to wait for \(taskDescription.forLogging) in list of currently executing tasks" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.fault( + "Cannot find task to wait for \(taskDescription.forLogging) in list of currently executing tasks" + ) + } return nil } if !taskDescription.isIdempotent { - logger.fault("Cannot reschedule task '\(taskDescription.forLogging)' since it is not idempotent") + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.fault("Cannot reschedule task '\(taskDescription.forLogging)' since it is not idempotent") + } return dependency } if dependency.priority > task.priority { @@ -378,9 +388,11 @@ public actor TaskScheduler { case .waitAndElevatePriorityOfDependency(let taskDescription): guard let dependency = self.currentlyExecutingTasks.first(where: { $0.description.id == taskDescription.id }) else { - logger.fault( - "Cannot find task to wait for '\(taskDescription.forLogging)' in list of currently executing tasks" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.fault( + "Cannot find task to wait for '\(taskDescription.forLogging)' in list of currently executing tasks" + ) + } return nil } return dependency @@ -398,9 +410,11 @@ public actor TaskScheduler { switch taskDependency { case .cancelAndRescheduleDependency(let taskDescription): guard let task = self.currentlyExecutingTasks.first(where: { $0.description.id == taskDescription.id }) else { - logger.fault( - "Cannot find task to reschedule \(taskDescription.forLogging) in list of currently executing tasks" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.fault( + "Cannot find task to reschedule \(taskDescription.forLogging) in list of currently executing tasks" + ) + } return nil } return task @@ -411,7 +425,9 @@ public actor TaskScheduler { if !rescheduleTasks.isEmpty { Task.detached(priority: task.priority) { for task in rescheduleTasks { - logger.debug("Suspending \(task.description.forLogging)") + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug("Suspending \(task.description.forLogging)") + } await task.cancelToBeRescheduled() } } @@ -422,19 +438,25 @@ public actor TaskScheduler { return } - logger.debug("Executing \(task.description.forLogging) with priority \(task.priority.rawValue)") + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug("Executing \(task.description.forLogging) with priority \(task.priority.rawValue)") + } currentlyExecutingTasks.append(task) pendingTasks.removeAll(where: { $0 === task }) Task.detached(priority: task.priority) { - logger.debug( - "Execution of \(task.description.forLogging) started with priority \(Task.currentPriority.rawValue)" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug( + "Execution of \(task.description.forLogging) started with priority \(Task.currentPriority.rawValue)" + ) + } // Await the task's return in a task so that this poker can continue checking if there are more execution // slots that can be filled with queued tasks. let finishStatus = await task.execute() - logger.debug( - "Execution of \(task.description.forLogging) finished with priority \(Task.currentPriority.rawValue)" - ) + withLoggingSubsystemAndScope(subsystem: taskSchedulerSubsystem, scope: nil) { + logger.debug( + "Execution of \(task.description.forLogging) finished with priority \(Task.currentPriority.rawValue)" + ) + } await self.finalizeTaskExecution(task: task, finishStatus: finishStatus) } } From 555df6073053e0291a268660749ab35d3e264c92 Mon Sep 17 00:00:00 2001 From: Rintaro Ishizaki Date: Wed, 1 May 2024 10:00:34 -0700 Subject: [PATCH 17/32] [build-script-helper] Prefer just-built plugins to SDK plugins Add '-Xswiftc -plugin-path -Xswiftc ${toolchain}/lib/swift/host/plugins' to swiftpm invocations. --- Utilities/build-script-helper.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Utilities/build-script-helper.py b/Utilities/build-script-helper.py index 2d6704b99..27f00cbd8 100755 --- a/Utilities/build-script-helper.py +++ b/Utilities/build-script-helper.py @@ -109,7 +109,14 @@ def get_swiftpm_options(swift_exec: str, args: argparse.Namespace, suppress_verb build_target = get_build_target(swift_exec, args, cross_compile=(True if args.cross_compile_config else False)) build_os = build_target.split('-')[2] - if not build_os.startswith('macosx'): + if build_os.startswith('macosx'): + swiftpm_args += [ + # Prefer just-built plugins to SDK plugins. + # This is a workaround for building fat binaries with Xcode build system being old. + '-Xswiftc', '-plugin-path', + '-Xswiftc', os.path.join(args.toolchain, 'lib', 'swift', 'host', 'plugins'), + ] + else: swiftpm_args += [ # Dispatch headers '-Xcxx', '-I', '-Xcxx', From ab32186382e89d289c65054fe1dd54f30ad24b21 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 25 Apr 2024 10:51:49 -0700 Subject: [PATCH 18/32] Generalize SyntaxRefactoringCodeActionProvider to work with EditRefactoringProvider Rather than only adapt refactoring actions that conform to SyntaxRefactoringProvider, which takes a syntax node and produces a syntax node, adapt to the less-constraining EditRefactoringProvider, which takes a syntax node and produces edits. We can map edits over just as effectively. --- .../SyntaxRefactoringCodeActionProvider.swift | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift index 2b725b421..1fb72e0ed 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift @@ -16,7 +16,7 @@ import SwiftSyntax /// Protocol that adapts a SyntaxRefactoringProvider (that comes from /// swift-syntax) into a SyntaxCodeActionProvider. -protocol SyntaxRefactoringCodeActionProvider: SyntaxCodeActionProvider, SyntaxRefactoringProvider { +protocol SyntaxRefactoringCodeActionProvider: SyntaxCodeActionProvider, EditRefactoringProvider { static var title: String { get } } @@ -31,20 +31,23 @@ extension SyntaxRefactoringCodeActionProvider where Self.Context == Void { return [] } - guard let refactored = Self.refactor(syntax: node) else { + let sourceEdits = Self.textRefactor(syntax: node) + if sourceEdits.isEmpty { return [] } - let edit = TextEdit( - range: scope.snapshot.range(of: node), - newText: refactored.description - ) + let textEdits = sourceEdits.map { edit in + TextEdit( + range: scope.snapshot.range(of: edit.range), + newText: edit.replacement + ) + } return [ CodeAction( title: Self.title, kind: .refactorInline, - edit: WorkspaceEdit(changes: [scope.snapshot.uri: [edit]]) + edit: WorkspaceEdit(changes: [scope.snapshot.uri: textEdits]) ) ] } From b628738473d90f712fa2b4f7023f08d3d71798c8 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 25 Apr 2024 19:07:35 -0700 Subject: [PATCH 19/32] Add "Add documentation" code action to stub out documentation for a function This code action takes an undocumented function declaration like func refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? and adds stub documentation for the parameters / result / etc., like this: /// A description /// - Parameters: /// - syntax: /// - context: /// /// - Returns: --- Sources/SourceKitLSP/CMakeLists.txt | 1 + .../Swift/CodeActions/AddDocumentation.swift | 156 +++++++++++ .../Swift/CodeActions/SyntaxCodeActions.swift | 1 + Tests/SourceKitLSPTests/CodeActionTests.swift | 48 +++- .../SyntaxRefactorTests.swift | 265 ++++++++++++++++++ 5 files changed, 470 insertions(+), 1 deletion(-) create mode 100644 Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift create mode 100644 Tests/SourceKitLSPTests/SyntaxRefactorTests.swift diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index 12558497a..3b9202799 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/Sources/SourceKitLSP/CMakeLists.txt @@ -22,6 +22,7 @@ target_sources(SourceKitLSP PRIVATE Clang/ClangLanguageService.swift) target_sources(SourceKitLSP PRIVATE Swift/AdjustPositionToStartOfIdentifier.swift + Swift/CodeActions/AddDocumentation.swift Swift/CodeActions/ConvertIntegerLiteral.swift Swift/CodeActions/PackageManifestEdits.swift Swift/CodeActions/SyntaxCodeActionProvider.swift diff --git a/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift new file mode 100644 index 000000000..3118913fd --- /dev/null +++ b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift @@ -0,0 +1,156 @@ +//===----------------------------------------------------------------------===// +// +// 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 SwiftParser +import SwiftRefactor +import SwiftSyntax + +/// Insert a documentation template associated with a function or macro. +/// +/// ## Before +/// +/// ```swift +/// static func refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? {} +/// ``` +/// +/// ## After +/// +/// ```swift +/// /// +/// /// - Parameters: +/// /// - syntax: +/// /// - context: +/// /// - Returns: +/// static func refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? {} +/// ``` +@_spi(Testing) +public struct AddDocumentation: EditRefactoringProvider { + @_spi(Testing) + public static func textRefactor(syntax: DeclSyntax, in context: Void) -> [SourceEdit] { + let hasDocumentation = syntax.leadingTrivia.contains(where: { trivia in + switch trivia { + case .blockComment(_), .docBlockComment(_), .lineComment(_), .docLineComment(_): + return true + default: + return false + } + }) + + guard !hasDocumentation else { + return [] + } + + let indentation = [.newlines(1)] + syntax.leadingTrivia.lastLineIndentation() + var content: [TriviaPiece] = [] + content.append(contentsOf: indentation) + content.append(.docLineComment("/// A description")) + + if let parameters = syntax.parameters?.parameters { + if let onlyParam = parameters.only { + let paramToken = onlyParam.secondName?.text ?? onlyParam.firstName.text + content.append(contentsOf: indentation) + content.append(.docLineComment("/// - Parameter \(paramToken):")) + } else { + content.append(contentsOf: indentation) + content.append(.docLineComment("/// - Parameters:")) + content.append( + contentsOf: parameters.flatMap({ param in + indentation + [ + .docLineComment("/// - \(param.secondName?.text ?? param.firstName.text):") + ] + }) + ) + content.append(contentsOf: indentation) + content.append(.docLineComment("///")) + } + } + + if syntax.throwsKeyword != nil { + content.append(contentsOf: indentation) + content.append(.docLineComment("/// - Throws:")) + } + + if syntax.returnType != nil { + content.append(contentsOf: indentation) + content.append(.docLineComment("/// - Returns:")) + } + + let insertPos = syntax.position + return [ + SourceEdit( + range: insertPos.. Trivia { + guard let lastNewline = pieces.lastIndex(where: { $0.isNewline }) else { + return self + } + + return Trivia(pieces: pieces[(lastNewline + 1)...]) + } +} diff --git a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift index b4b2ce85c..15c86cf7c 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift @@ -15,6 +15,7 @@ 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] = [ + AddDocumentation.self, AddSeparatorsToIntegerLiteral.self, ConvertIntegerLiteral.self, FormatRawStringLiteral.self, diff --git a/Tests/SourceKitLSPTests/CodeActionTests.swift b/Tests/SourceKitLSPTests/CodeActionTests.swift index 657c5c78a..77763c4f3 100644 --- a/Tests/SourceKitLSPTests/CodeActionTests.swift +++ b/Tests/SourceKitLSPTests/CodeActionTests.swift @@ -348,7 +348,23 @@ final class CodeActionTests: XCTestCase { command: expectedCommand ) - XCTAssertEqual(result, .codeActions([expectedCodeAction])) + guard case .codeActions(var resultActions) = result else { + XCTFail("Result doesn't have code actions: \(String(describing: result))") + return + } + + // Filter out "Add documentation"; we test it elsewhere + if let addDocIndex = resultActions.firstIndex(where: { + $0.title == "Add documentation" + } + ) { + resultActions.remove(at: addDocIndex) + } else { + XCTFail("Missing 'Add documentation'.") + return + } + + XCTAssertEqual(resultActions, [expectedCodeAction]) } func testCodeActionsRemovePlaceholders() async throws { @@ -441,6 +457,36 @@ final class CodeActionTests: XCTestCase { try await fulfillmentOfOrThrow([editReceived]) } + func testAddDocumentationCodeActionResult() async throws { + let testClient = try await TestSourceKitLSPClient(capabilities: clientCapabilitiesWithCodeActionSupport()) + let uri = DocumentURI.for(.swift) + let positions = testClient.openDocument( + """ + 2️⃣func refacto1️⃣r(syntax: DeclSyntax, in context: Void) -> DeclSyntax? { }3️⃣ + """, + uri: uri + ) + + let testPosition = positions["1️⃣"] + let request = CodeActionRequest( + range: Range(testPosition), + context: .init(), + textDocument: TextDocumentIdentifier(uri) + ) + let result = try await testClient.send(request) + + guard case .codeActions(let codeActions) = result else { + XCTFail("Expected code actions") + return + } + + // Make sure we get an add-documentation action. + let addDocAction = codeActions.first { action in + return action.title == "Add documentation" + } + XCTAssertNotNil(addDocAction) + } + func testCodeActionForFixItsProducedBySwiftSyntax() async throws { let project = try await MultiFileTestProject(files: [ "test.swift": "protocol 1️⃣Multi 2️⃣ident 3️⃣{}", diff --git a/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift new file mode 100644 index 000000000..f1207284b --- /dev/null +++ b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift @@ -0,0 +1,265 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2019 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(Testing) import SourceKitLSP +import SwiftParser +import SwiftRefactor +import SwiftSyntax +import XCTest + +final class SyntaxRefactorTests: XCTestCase { + func testAddDocumentationRefactor() throws { + try assertRefactor( + """ + func refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? { } + """, + context: (), + provider: AddDocumentation.self, + expected: [ + SourceEdit( + range: AbsolutePosition(utf8Offset: 0)..( + malformedInput input: String, + context: R.Context, + provider: R.Type, + expected: [SourceEdit], + file: StaticString = #filePath, + line: UInt = #line +) throws where R.Input == Syntax { + var parser = Parser(input) + let syntax = ExprSyntax.parse(from: &parser) + try assertRefactor( + Syntax(syntax), + context: context, + provider: provider, + expected: expected, + file: file, + line: line + ) +} + +// Borrowed from the swift-syntax library's SwiftRefactor tests. + +func assertRefactor( + _ input: R.Input, + context: R.Context, + provider: R.Type, + expected: [SourceEdit], + file: StaticString = #filePath, + line: UInt = #line +) throws { + let edits = R.textRefactor(syntax: input, in: context) + guard !edits.isEmpty else { + if !expected.isEmpty { + XCTFail( + """ + Refactoring produced empty result, expected: + \(expected) + """, + file: file, + line: line + ) + } + return + } + + if edits.count != expected.count { + XCTFail( + """ + Refactoring produced incorrect number of edits, expected \(expected.count) not \(edits.count). + + Actual: + \(edits.map({ $0.debugDescription }).joined(separator: "\n")) + + Expected: + \(expected.map({ $0.debugDescription }).joined(separator: "\n")) + + """, + file: file, + line: line + ) + return + } + + for (actualEdit, expectedEdit) in zip(edits, expected) { + XCTAssertEqual( + actualEdit, + expectedEdit, + "Incorrect edit, expected \(expectedEdit.debugDescription) but actual was \(actualEdit.debugDescription)", + file: file, + line: line + ) + assertStringsEqualWithDiff( + actualEdit.replacement, + expectedEdit.replacement, + file: file, + line: line + ) + } +} + +/// Asserts that the two strings are equal, providing Unix `diff`-style output if they are not. +/// +/// - Parameters: +/// - actual: The actual string. +/// - expected: The expected string. +/// - message: An optional description of the failure. +/// - additionalInfo: Additional information about the failed test case that will be printed after the diff +/// - file: The file in which failure occurred. Defaults to the file name of the test case in +/// which this function was called. +/// - line: The line number on which failure occurred. Defaults to the line number on which this +/// function was called. +public func assertStringsEqualWithDiff( + _ actual: String, + _ expected: String, + _ message: String = "", + additionalInfo: @autoclosure () -> String? = nil, + file: StaticString = #filePath, + line: UInt = #line +) { + if actual == expected { + return + } + failStringsEqualWithDiff( + actual, + expected, + message, + additionalInfo: additionalInfo(), + file: file, + line: line + ) +} + +/// Asserts that the two data are equal, providing Unix `diff`-style output if they are not. +/// +/// - Parameters: +/// - actual: The actual string. +/// - expected: The expected string. +/// - message: An optional description of the failure. +/// - additionalInfo: Additional information about the failed test case that will be printed after the diff +/// - file: The file in which failure occurred. Defaults to the file name of the test case in +/// which this function was called. +/// - line: The line number on which failure occurred. Defaults to the line number on which this +/// function was called. +public func assertDataEqualWithDiff( + _ actual: Data, + _ expected: Data, + _ message: String = "", + additionalInfo: @autoclosure () -> String? = nil, + file: StaticString = #filePath, + line: UInt = #line +) { + if actual == expected { + return + } + + // NOTE: Converting to `Stirng` here looses invalid UTF8 sequence difference, + // but at least we can see something is different. + failStringsEqualWithDiff( + String(decoding: actual, as: UTF8.self), + String(decoding: expected, as: UTF8.self), + message, + additionalInfo: additionalInfo(), + file: file, + line: line + ) +} + +/// `XCTFail` with `diff`-style output. +public func failStringsEqualWithDiff( + _ actual: String, + _ expected: String, + _ message: String = "", + additionalInfo: @autoclosure () -> String? = nil, + file: StaticString = #filePath, + line: UInt = #line +) { + let stringComparison: String + + // Use `CollectionDifference` on supported platforms to get `diff`-like line-based output. On + // older platforms, fall back to simple string comparison. + if #available(macOS 10.15, *) { + let actualLines = actual.components(separatedBy: .newlines) + let expectedLines = expected.components(separatedBy: .newlines) + + let difference = actualLines.difference(from: expectedLines) + + var result = "" + + var insertions = [Int: String]() + var removals = [Int: String]() + + for change in difference { + switch change { + case .insert(let offset, let element, _): + insertions[offset] = element + case .remove(let offset, let element, _): + removals[offset] = element + } + } + + var expectedLine = 0 + var actualLine = 0 + + while expectedLine < expectedLines.count || actualLine < actualLines.count { + if let removal = removals[expectedLine] { + result += "–\(removal)\n" + expectedLine += 1 + } else if let insertion = insertions[actualLine] { + result += "+\(insertion)\n" + actualLine += 1 + } else { + result += " \(expectedLines[expectedLine])\n" + expectedLine += 1 + actualLine += 1 + } + } + + stringComparison = result + } else { + // Fall back to simple message on platforms that don't support CollectionDifference. + stringComparison = """ + Expected: + \(expected) + + Actual: + \(actual) + """ + } + + var fullMessage = """ + \(message.isEmpty ? "Actual output does not match the expected" : message) + \(stringComparison) + """ + if let additional = additionalInfo() { + fullMessage = """ + \(fullMessage) + \(additional) + """ + } + XCTFail(fullMessage, file: file, line: line) +} From c2986269add04f418804673af77a1247562389f3 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Sat, 4 May 2024 17:36:22 -0700 Subject: [PATCH 20/32] Update test for new refactoring action --- .../PullDiagnosticsTests.swift | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift b/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift index c6b412792..b64013a1e 100644 --- a/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift +++ b/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift @@ -93,17 +93,18 @@ final class PullDiagnosticsTests: XCTestCase { return } - XCTAssertEqual(actions.count, 1) - let action = try XCTUnwrap(actions.first) - // Allow the action message to be the one before or after - // https://github.com/apple/swift/pull/67909, ensuring this test passes with - // a sourcekitd that contains the change from that PR as well as older - // toolchains that don't contain the change yet. + XCTAssertEqual(actions.count, 2) XCTAssert( - [ - "Add stubs for conformance", - "Do you want to add protocol stubs?", - ].contains(action.title) + actions.contains { action in + // Allow the action message to be the one before or after + // https://github.com/apple/swift/pull/67909, ensuring this test passes with + // a sourcekitd that contains the change from that PR as well as older + // toolchains that don't contain the change yet. + [ + "Add stubs for conformance", + "Do you want to add protocol stubs?", + ].contains(action.title) + } ) } From 73b2e262ffe7f34cf19d59c6968232a95538e818 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 6 May 2024 07:35:29 -0700 Subject: [PATCH 21/32] Add GitHub issue templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most importantly, the bug report template is now asking for the user’s OS, Swift version and logging. --- .github/ISSUE_TEMPLATE/BUG_REPORT.yml | 53 ++++++++++++++++++++++ .github/ISSUE_TEMPLATE/FEATURE_REQUEST.yml | 13 ++++++ .github/ISSUE_TEMPLATE/config.yml | 12 +++++ 3 files changed, 78 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/BUG_REPORT.yml create mode 100644 .github/ISSUE_TEMPLATE/FEATURE_REQUEST.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml diff --git a/.github/ISSUE_TEMPLATE/BUG_REPORT.yml b/.github/ISSUE_TEMPLATE/BUG_REPORT.yml new file mode 100644 index 000000000..f7d5ff735 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/BUG_REPORT.yml @@ -0,0 +1,53 @@ +name: Bug Report +description: Something isn't working as expected +labels: [bug] +body: + - type: input + id: version + attributes: + label: Swift version + description: Which version of Swift are you using? If you are unsure, insert the output of `path/to/swift --version` + placeholder: Eg. swiftlang-5.10.0.13, swift-DEVELOPMENT-SNAPSHOT-2024-05-01-a + - type: input + id: platform + attributes: + label: Platform + description: What operating system are you seeing the issue on? + placeholder: Eg. Ubuntu 22.04, Windows 11, macOS 14 + - type: input + id: editor + attributes: + label: Editor + description: Which text editor are you using? + placeholder: Eg. Visual Studio Code with Swift plugin 1.9.0, neovim + - type: dropdown + id: reproduces-with-swift-6 + attributes: + label: Does the issue reproduce with Swift 6? + description: | + Does the issue also reproduce using a [recent Swift 6 Development Snapshot](https://www.swift.org/download/#swift-60-development)? + + We have made significant changes to SourceKit-LSP in Swift 6 and the issue might have already been fixed. If you didn’t try, that is fine. + options: + - "Yes" + - "No" + - I didn’t try + - type: textarea + id: description + attributes: + label: Description + description: | + A short description of the incorrect behavior. + If you think this issue has been recently introduced and did not occur in an earlier version, please note that. If possible, include the last version that the behavior was correct in addition to your current version. + - type: textarea + id: steps-to-reproduce + attributes: + label: Steps to Reproduce + description: If you have steps that reproduce the issue, please add them here. If you can share a project that reproduces the issue, please attach it. + - type: textarea + id: logging + attributes: + label: Logging + description: | + If you are using SourceKit-LSP from Swift 6, running `sourcekit-lsp diagnose` in terminal and attaching the generated bundle helps us diagnose the issue. + The generated bundle might contain portions of your source code, so please only attach it if you feel comfortable sharing it. diff --git a/.github/ISSUE_TEMPLATE/FEATURE_REQUEST.yml b/.github/ISSUE_TEMPLATE/FEATURE_REQUEST.yml new file mode 100644 index 000000000..30f157e55 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/FEATURE_REQUEST.yml @@ -0,0 +1,13 @@ +name: Feature Request +description: A suggestion for a new feature +labels: [enhancement] +body: + - type: textarea + id: description + attributes: + label: Description + description: | + A description of your proposed feature. + Examples that show what's missing, or what new capabilities will be possible, are very helpful! + If this feature unlocks new use-cases please describe them. + Provide links to existing issues or external references/discussions, if appropriate. diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 000000000..f689e31e7 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,12 @@ +# This source file is part of the Swift.org open source project + # + # Copyright (c) 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 Swift project authors + + contact_links: + - name: Discussion Forum + url: https://forums.swift.org/c/development/sourcekit-lsp + about: Ask and answer questions about SourceKit-LSP From 9f7e7b03c753d987f8cfcbd4360d9da56e89861f Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 6 May 2024 07:36:11 -0700 Subject: [PATCH 22/32] Move `CODEOWNERS` to `.github` folder Since we now have a `.github` folder for issue templates, we can move `CODEOWNERS` to it. --- CODEOWNERS => .github/CODEOWNERS | 4 ---- 1 file changed, 4 deletions(-) rename CODEOWNERS => .github/CODEOWNERS (86%) diff --git a/CODEOWNERS b/.github/CODEOWNERS similarity index 86% rename from CODEOWNERS rename to .github/CODEOWNERS index 62201d93a..b8e1ca770 100644 --- a/CODEOWNERS +++ b/.github/CODEOWNERS @@ -8,8 +8,4 @@ # Order is important. The last matching pattern has the most precedence. # Owner of anything in SourceKit-LSP not owned by anyone else. -# N: Ben Langmuir -# E: blangmuir@apple.com -# N: Alex Hoppen -# E: ahoppen@apple.com * @benlangmuir @ahoppen From 6a9a95f9fdd380a122f10606f09a1041d224201c Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sun, 21 Apr 2024 08:26:46 -0700 Subject: [PATCH 23/32] Fix test discovery for Objective-C XCTests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were two issues with Objective-C XCTest discovery: 1. We were relying on syntactic test discovery after a document is edited. But since we don't have syntactic test discovery for Objective-C tests, this meant that all tests would disappear as a document got edited. Until we get syntactic test discovery for Objective-C, use the semantic index to discover tests, even if they are out-of-date. 2. We were assuming that the `DocumentSymbols` request returned `[DocumentSymbol]` to find the ranges of tests. But clangd returns the alternative `[SymbolInformation]`, which meant that we were only returning the position of a test function’s name instead of the test function’s range. rdar://126810202 --- Sources/SourceKitLSP/LanguageService.swift | 4 +- Sources/SourceKitLSP/TestDiscovery.swift | 64 ++++++--- .../DocumentTestDiscoveryTests.swift | 134 ++++++++++++++++++ .../WorkspaceTestDiscoveryTests.swift | 132 +++++++++++++++++ 4 files changed, 314 insertions(+), 20 deletions(-) diff --git a/Sources/SourceKitLSP/LanguageService.swift b/Sources/SourceKitLSP/LanguageService.swift index b09d433a4..29ed79fa8 100644 --- a/Sources/SourceKitLSP/LanguageService.swift +++ b/Sources/SourceKitLSP/LanguageService.swift @@ -198,7 +198,9 @@ public protocol LanguageService: AnyObject { /// Perform a syntactic scan of the file at the given URI for test cases and test classes. /// /// This is used as a fallback to show the test cases in a file if the index for a given file is not up-to-date. - func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem] + /// + /// A return value of `nil` indicates that this language service does not support syntactic test discovery. + func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem]? /// Crash the language server. Should be used for crash recovery testing only. func _crash() async diff --git a/Sources/SourceKitLSP/TestDiscovery.swift b/Sources/SourceKitLSP/TestDiscovery.swift index 3bf810876..0a185df1a 100644 --- a/Sources/SourceKitLSP/TestDiscovery.swift +++ b/Sources/SourceKitLSP/TestDiscovery.swift @@ -13,6 +13,7 @@ import IndexStoreDB import LSPLogging import LanguageServerProtocol +import SemanticIndex import SwiftSyntax public enum TestStyle { @@ -41,22 +42,26 @@ fileprivate extension SymbolOccurrence { /// Find the innermost range of a document symbol that contains the given position. private func findInnermostSymbolRange( containing position: Position, - documentSymbols documentSymbolsResponse: DocumentSymbolResponse + documentSymbolsResponse: DocumentSymbolResponse ) -> Range? { - guard case .documentSymbols(let documentSymbols) = documentSymbolsResponse else { - // Both `ClangLanguageService` and `SwiftLanguageService` return `documentSymbols` so we don't need to handle the - // .symbolInformation case. - logger.fault( - """ - Expected documentSymbols response from language service to resolve test ranges but got \ - \(documentSymbolsResponse.forLogging) - """ - ) - return nil + switch documentSymbolsResponse { + case .documentSymbols(let documentSymbols): + return findInnermostSymbolRange(containing: position, documentSymbols: documentSymbols) + case .symbolInformation(let symbolInformation): + return findInnermostSymbolRange(containing: position, symbolInformation: symbolInformation) } +} + +private func findInnermostSymbolRange( + containing position: Position, + documentSymbols: [DocumentSymbol] +) -> Range? { for documentSymbol in documentSymbols where documentSymbol.range.contains(position) { if let children = documentSymbol.children, - let rangeOfChild = findInnermostSymbolRange(containing: position, documentSymbols: .documentSymbols(children)) + let rangeOfChild = findInnermostSymbolRange( + containing: position, + documentSymbolsResponse: .documentSymbols(children) + ) { // If a child contains the position, prefer that because it's more specific. return rangeOfChild @@ -66,6 +71,21 @@ private func findInnermostSymbolRange( return nil } +/// Return the smallest range in `symbolInformation` containing `position`. +private func findInnermostSymbolRange( + containing position: Position, + symbolInformation symbolInformationArray: [SymbolInformation] +) -> Range? { + var bestRange: Range? = nil + for symbolInformation in symbolInformationArray where symbolInformation.location.range.contains(position) { + let range = symbolInformation.location.range + if bestRange == nil || (bestRange!.lowerBound < range.lowerBound && range.upperBound < bestRange!.upperBound) { + bestRange = range + } + } + return bestRange +} + extension SourceKitLSPServer { /// Converts a flat list of test symbol occurrences to a hierarchical `TestItem` array, inferring the hierarchical /// structure from `childOf` relations between the symbol occurrences. @@ -263,9 +283,15 @@ extension SourceKitLSPServer { let syntacticTests = try await languageService.syntacticDocumentTests(for: req.textDocument.uri, in: workspace) - if let index = workspace.index(checkedFor: .inMemoryModifiedFiles(documentManager)) { + // We `syntacticDocumentTests` returns `nil`, it indicates that it doesn't support syntactic test discovery. + // In that case, the semantic index is the only source of tests we have and we thus want to show tests from the + // semantic index, even if they are out-of-date. The alternative would be showing now tests after an edit to a file. + let indexCheckLevel: IndexCheckLevel = + syntacticTests == nil ? .deletedFiles : .inMemoryModifiedFiles(documentManager) + + if let index = workspace.index(checkedFor: indexCheckLevel) { var syntacticSwiftTestingTests: [TestItem] { - syntacticTests.filter { $0.style == TestStyle.swiftTesting } + syntacticTests?.filter { $0.style == TestStyle.swiftTesting } ?? [] } let testSymbols = @@ -283,7 +309,7 @@ extension SourceKitLSPServer { for: testSymbols, resolveLocation: { uri, position in if uri == snapshot.uri, let documentSymbols, - let range = findInnermostSymbolRange(containing: position, documentSymbols: documentSymbols) + let range = findInnermostSymbolRange(containing: position, documentSymbolsResponse: documentSymbols) { return Location(uri: uri, range: range) } @@ -298,7 +324,7 @@ extension SourceKitLSPServer { } } // We don't have any up-to-date semantic index entries for this file. Syntactically look for tests. - return syntacticTests + return syntacticTests ?? [] } } @@ -437,7 +463,7 @@ extension TestItem { } extension SwiftLanguageService { - public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem] { + public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem]? { let snapshot = try documentManager.latestSnapshot(uri) let semanticSymbols = workspace.index(checkedFor: .deletedFiles)?.symbols(inFilePath: snapshot.uri.pseudoPath) let xctestSymbols = await SyntacticSwiftXCTestScanner.findTestSymbols( @@ -453,7 +479,7 @@ extension SwiftLanguageService { } extension ClangLanguageService { - public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async -> [TestItem] { - return [] + public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async -> [TestItem]? { + return nil } } diff --git a/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift b/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift index e0f1e14c0..c1133305b 100644 --- a/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift +++ b/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift @@ -1065,4 +1065,138 @@ final class DocumentTestDiscoveryTests: XCTestCase { XCTAssertFalse(testsAfterEdit.contains { $0.label == "NotQuiteTest" }) XCTAssertTrue(testsAfterEdit.contains { $0.label == "OtherNotQuiteTest" }) } + + func testObjectiveCTestFromSemanticIndex() async throws { + try SkipUnless.platformIsDarwin("Non-Darwin platforms don't support Objective-C") + + let project = try await SwiftPMTestProject( + files: [ + "Tests/MyLibraryTests/Test.m": """ + #import + + @interface MyTests : XCTestCase + @end + + 1️⃣@implementation MyTests + 2️⃣- (void)testSomething { + }3️⃣ + @4️⃣end + """ + ], + manifest: """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyLibrary", + targets: [.testTarget(name: "MyLibraryTests")] + ) + """, + build: true + ) + + let (uri, positions) = try project.openDocument("Test.m") + + let tests = try await project.testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri))) + + XCTAssertEqual( + tests, + [ + TestItem( + id: "MyTests", + label: "MyTests", + disabled: false, + style: TestStyle.xcTest, + location: Location(uri: uri, range: positions["1️⃣"].. + + @interface MyTests : XCTestCase + @end + + 1️⃣@implementation MyTests + 2️⃣- (void)testSomething {}3️⃣ + 0️⃣ + @4️⃣end + """ + ], + manifest: """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyLibrary", + targets: [.testTarget(name: "MyLibraryTests")] + ) + """, + build: true + ) + + let (uri, positions) = try project.openDocument("Test.m") + + project.testClient.send( + DidChangeTextDocumentNotification( + textDocument: VersionedTextDocumentIdentifier(uri, version: 2), + contentChanges: [ + TextDocumentContentChangeEvent( + range: Range(positions["0️⃣"]), + text: """ + - (void)testSomethingElse {} + """ + ) + ] + ) + ) + + let tests = try await project.testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri))) + // Since we don't have syntactic test discovery for clang-languages, we don't discover `testSomethingElse` as a + // test method until we perform a build + XCTAssertEqual( + tests, + [ + TestItem( + id: "MyTests", + label: "MyTests", + disabled: false, + style: TestStyle.xcTest, + location: Location(uri: uri, range: positions["1️⃣"].. + + @interface MyTests : XCTestCase + @end + + @implementation 1️⃣MyTests + - (void)2️⃣testSomething { + } + @end + """ + ], + manifest: """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyLibrary", + targets: [.testTarget(name: "MyLibraryTests")] + ) + """, + build: true + ) + + let tests = try await project.testClient.send(WorkspaceTestsRequest()) + + XCTAssertEqual( + tests, + [ + TestItem( + id: "MyTests", + label: "MyTests", + disabled: false, + style: TestStyle.xcTest, + location: try project.location(from: "1️⃣", to: "1️⃣", in: "Test.m"), + children: [ + TestItem( + id: "MyTests/testSomething", + label: "testSomething", + disabled: false, + style: TestStyle.xcTest, + location: try project.location(from: "2️⃣", to: "2️⃣", in: "Test.m"), + children: [], + tags: [] + ) + ], + tags: [] + ) + ] + ) + } + + func testObjectiveCTestsAfterInMemoryEdit() async throws { + try SkipUnless.platformIsDarwin("Non-Darwin platforms don't support Objective-C") + let project = try await SwiftPMTestProject( + files: [ + "Tests/MyLibraryTests/Test.m": """ + #import + + @interface MyTests : XCTestCase + @end + + 1️⃣@implementation MyTests + 2️⃣- (void)testSomething {}3️⃣ + 0️⃣ + @4️⃣end + """ + ], + manifest: """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyLibrary", + targets: [.testTarget(name: "MyLibraryTests")] + ) + """, + build: true + ) + + let (uri, positions) = try project.openDocument("Test.m") + + project.testClient.send( + DidChangeTextDocumentNotification( + textDocument: VersionedTextDocumentIdentifier(uri, version: 2), + contentChanges: [ + TextDocumentContentChangeEvent( + range: Range(positions["0️⃣"]), + text: """ + - (void)testSomethingElse {} + """ + ) + ] + ) + ) + + let tests = try await project.testClient.send(WorkspaceTestsRequest()) + // Since we don't have syntactic test discovery for clang-languages, we don't discover `testSomethingElse` as a + // test method until we perform a build + XCTAssertEqual( + tests, + [ + TestItem( + id: "MyTests", + label: "MyTests", + disabled: false, + style: TestStyle.xcTest, + location: Location(uri: uri, range: positions["1️⃣"].. Date: Mon, 6 May 2024 08:25:21 -0700 Subject: [PATCH 24/32] Use `TokenSyntax.indentationOfLine` from SwiftBasicFormat instead of duplicating indentation inferring logic --- .../Swift/CodeActions/AddDocumentation.swift | 39 +++++++------------ 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift index 3118913fd..03fcd92c8 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift @@ -10,6 +10,7 @@ // //===----------------------------------------------------------------------===// +import SwiftBasicFormat import SwiftParser import SwiftRefactor import SwiftSyntax @@ -49,38 +50,36 @@ public struct AddDocumentation: EditRefactoringProvider { return [] } - let indentation = [.newlines(1)] + syntax.leadingTrivia.lastLineIndentation() + let newlineAndIndentation = [.newlines(1)] + (syntax.firstToken(viewMode: .sourceAccurate)?.indentationOfLine ?? []) var content: [TriviaPiece] = [] - content.append(contentsOf: indentation) + content += newlineAndIndentation content.append(.docLineComment("/// A description")) if let parameters = syntax.parameters?.parameters { if let onlyParam = parameters.only { let paramToken = onlyParam.secondName?.text ?? onlyParam.firstName.text - content.append(contentsOf: indentation) + content += newlineAndIndentation content.append(.docLineComment("/// - Parameter \(paramToken):")) } else { - content.append(contentsOf: indentation) + content += newlineAndIndentation content.append(.docLineComment("/// - Parameters:")) - content.append( - contentsOf: parameters.flatMap({ param in - indentation + [ - .docLineComment("/// - \(param.secondName?.text ?? param.firstName.text):") - ] - }) - ) - content.append(contentsOf: indentation) + content += parameters.flatMap({ param in + newlineAndIndentation + [ + .docLineComment("/// - \(param.secondName?.text ?? param.firstName.text):") + ] + }) + content += newlineAndIndentation content.append(.docLineComment("///")) } } if syntax.throwsKeyword != nil { - content.append(contentsOf: indentation) + content += newlineAndIndentation content.append(.docLineComment("/// - Throws:")) } if syntax.returnType != nil { - content.append(contentsOf: indentation) + content += newlineAndIndentation content.append(.docLineComment("/// - Returns:")) } @@ -142,15 +141,3 @@ extension DeclSyntax { } } } - -extension Trivia { - /// Produce trivia from the last newline to the end, dropping anything - /// prior to that. - fileprivate func lastLineIndentation() -> Trivia { - guard let lastNewline = pieces.lastIndex(where: { $0.isNewline }) else { - return self - } - - return Trivia(pieces: pieces[(lastNewline + 1)...]) - } -} From 329e3d3297c1b36593118200f4579cfdb236a0d4 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 6 May 2024 08:36:01 -0700 Subject: [PATCH 25/32] Use `as(DeclSyntaxEnum.self)` instead of force-unwrapping --- .../Swift/CodeActions/AddDocumentation.swift | 48 +++++++++---------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift index 03fcd92c8..f37a6b661 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift @@ -99,43 +99,41 @@ extension AddDocumentation: SyntaxRefactoringCodeActionProvider { extension DeclSyntax { fileprivate var parameters: FunctionParameterClauseSyntax? { - switch self.syntaxNodeType { - case is FunctionDeclSyntax.Type: - return self.as(FunctionDeclSyntax.self)!.signature.parameterClause - case is SubscriptDeclSyntax.Type: - return self.as(SubscriptDeclSyntax.self)!.parameterClause - case is InitializerDeclSyntax.Type: - return self.as(InitializerDeclSyntax.self)!.signature.parameterClause - case is MacroDeclSyntax.Type: - return self.as(MacroDeclSyntax.self)!.signature.parameterClause + switch self.as(DeclSyntaxEnum.self) { + case .functionDecl(let functionDecl): + return functionDecl.signature.parameterClause + case .subscriptDecl(let subscriptDecl): + return subscriptDecl.parameterClause + case .initializerDecl(let initializer): + return initializer.signature.parameterClause + case .macroDecl(let macro): + return macro.signature.parameterClause default: return nil } } fileprivate var throwsKeyword: TokenSyntax? { - switch self.syntaxNodeType { - case is FunctionDeclSyntax.Type: - return self.as(FunctionDeclSyntax.self)!.signature.effectSpecifiers? - .throwsClause?.throwsSpecifier - case is InitializerDeclSyntax.Type: - return self.as(InitializerDeclSyntax.self)!.signature.effectSpecifiers? - .throwsClause?.throwsSpecifier + switch self.as(DeclSyntaxEnum.self) { + case .functionDecl(let functionDecl): + return functionDecl.signature.effectSpecifiers?.throwsClause?.throwsSpecifier + case .initializerDecl(let initializer): + return initializer.signature.effectSpecifiers?.throwsClause?.throwsSpecifier default: return nil } } fileprivate var returnType: TypeSyntax? { - switch self.syntaxNodeType { - case is FunctionDeclSyntax.Type: - return self.as(FunctionDeclSyntax.self)!.signature.returnClause?.type - case is SubscriptDeclSyntax.Type: - return self.as(SubscriptDeclSyntax.self)!.returnClause.type - case is InitializerDeclSyntax.Type: - return self.as(InitializerDeclSyntax.self)!.signature.returnClause?.type - case is MacroDeclSyntax.Type: - return self.as(MacroDeclSyntax.self)!.signature.returnClause?.type + switch self.as(DeclSyntaxEnum.self) { + case .functionDecl(let functionDecl): + return functionDecl.signature.returnClause?.type + case .subscriptDecl(let subscriptDecl): + return subscriptDecl.returnClause.type + case .initializerDecl(let initializer): + return initializer.signature.returnClause?.type + case .macroDecl(let macro): + return macro.signature.returnClause?.type default: return nil } From e54c99eebd7e36cadd417bdedcc523e8c0531740 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 25 Apr 2024 16:47:22 -0700 Subject: [PATCH 26/32] Add a syntactic "Add Codable structs from JSON" code action Add a syntactic action that takes JSON pasted into a Swift file or placed in a string literal, then turns it into a set of Codable structs that can represent the JSON. Our typical example starts like this: ``` { "name": "Produce", "shelves": [ { "name": "Discount Produce", "product": { "name": "Banana", "points": 200, "description": "A banana that's perfectly ripe." } } ] } ``` and turns into this: ```swift struct JSONValue: Codable { var name: String var shelves: [Shelves] struct Shelves: Codable { var name: String var product: Product struct Product: Codable { var description: String var name: String var points: Double } } } ``` When converting to JSON, we attempt to reason about multiple JSON objects on the same level to detect when there are optional fields, due to either an explicit null or due to the absence of fields in some of the JSON objects that are conceptually stored together. The refactoring itself would live down in the swift-syntax package if not for its dependency on Foundation. We'll move it when appropriate. --- Sources/SourceKitLSP/CMakeLists.txt | 1 + .../ConvertJSONToCodableStruct.swift | 389 ++++++++++++++++++ .../Swift/CodeActions/SyntaxCodeActions.swift | 1 + Tests/SourceKitLSPTests/CodeActionTests.swift | 42 ++ .../SyntaxRefactorTests.swift | 162 ++++++++ 5 files changed, 595 insertions(+) create mode 100644 Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index 641276d13..e56f172fa 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/Sources/SourceKitLSP/CMakeLists.txt @@ -23,6 +23,7 @@ target_sources(SourceKitLSP PRIVATE Swift/AdjustPositionToStartOfIdentifier.swift Swift/CodeActions/AddDocumentation.swift Swift/CodeActions/ConvertIntegerLiteral.swift + Swift/CodeActions/ConvertJSONToCodableStruct.swift Swift/CodeActions/PackageManifestEdits.swift Swift/CodeActions/SyntaxCodeActionProvider.swift Swift/CodeActions/SyntaxCodeActions.swift diff --git a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift new file mode 100644 index 000000000..0a5cd38b7 --- /dev/null +++ b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift @@ -0,0 +1,389 @@ +//===----------------------------------------------------------------------===// +// +// 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 Foundation +import LanguageServerProtocol +import SwiftBasicFormat +import SwiftRefactor +import SwiftSyntax + +/// Convert JSON literals into corresponding Swift structs that conform to the +/// `Codable` protocol. +/// +/// ## Before +/// +/// ```javascript +/// { +/// "name": "Produce", +/// "shelves": [ +/// { +/// "name": "Discount Produce", +/// "product": { +/// "name": "Banana", +/// "points": 200, +/// "description": "A banana that's perfectly ripe." +/// } +/// } +/// ] +/// } +/// ``` +/// +/// ## After +/// +/// ```swift +/// struct JSONValue: Codable { +/// var name: String +/// var shelves: [Shelves] +/// +/// struct Shelves: Codable { +/// var name: String +/// var product: Product +/// +/// struct Product: Codable { +/// var description: String +/// var name: String +/// var points: Double +/// } +/// } +/// } +/// ``` +@_spi(Testing) +public struct ConvertJSONToCodableStruct: EditRefactoringProvider { + @_spi(Testing) + public static func textRefactor( + syntax: Syntax, + in context: Void + ) -> [SourceEdit] { + // Dig out a syntax node that looks like it might be JSON or have JSON + // in it. + guard let preflight = preflightRefactoring(syntax) else { + return [] + } + + // Dig out the text that we think might be JSON. + let text: String + switch preflight { + case let .closure(closure): + /// The outer structure of the JSON { ... } looks like a closure in the + /// syntax tree, albeit one with lots of ill-formed syntax in the body. + /// We're only going to look at the text of the closure to see if we + /// have JSON in there. + text = closure.trimmedDescription + case .stringLiteral(_, let literalText): + /// A string literal that could contain JSON within it. + text = literalText + } + + // Try to process this as JSON. + guard + let object = try? JSONSerialization.jsonObject(with: text.data(using: .utf8)!), + let dictionary = object as? [String: Any] + else { + return [] + } + + // Create the top-level object. + let topLevelObject = JSONObject(dictionary: dictionary) + + // Render the top-level object as a struct. + let indentation = BasicFormat.inferIndentation(of: syntax) + let format = BasicFormat(indentationWidth: indentation) + let decls = topLevelObject.asDeclSyntax(name: "JSONValue") + .formatted(using: format) + + // Render the change into a set of source edits. + switch preflight { + case .closure(let closure): + // Closures are replaced entirely, since they were invalid code to + // start with. + return [ + SourceEdit(range: closure.trimmedRange, replacement: decls.description) + ] + case .stringLiteral(let literal, _): + /// Leave the string literal in place (it might be there for testing + /// purposes), and put the newly-created structs afterward. + return [ + SourceEdit( + range: literal.endPosition.. Preflight? { + // Preflight a closure. + // + // A blob of JSON dropped into a Swift source file will look like a + // closure due to the curly braces. The internals might be a syntactic + // disaster, but we don't actually care. + if let closure = syntax.as(ClosureExprSyntax.self) { + return .closure(closure) + } + + // We found a string literal; its contents might be JSON. + if let stringLit = syntax.as(StringLiteralExprSyntax.self) { + // Look for an enclosing context and prefer that, because we might have + // a string literal that's inside a closure where the closure itself + // is the JSON. + if let parent = syntax.parent, + let enclosingPreflight = preflightRefactoring(parent) + { + return enclosingPreflight + } + + guard let text = stringLit.representedLiteralValue else { + return nil + } + + return .stringLiteral(stringLit, text) + } + + // Look further up the syntax tree. + if let parent = syntax.parent { + return preflightRefactoring(parent) + } + + return nil + } +} + +extension ConvertJSONToCodableStruct: SyntaxRefactoringCodeActionProvider { + static var title = "Create Codable structs from JSON" +} + +/// A JSON object, which is has a set of fields, each of which has the given +/// type. +fileprivate struct JSONObject { + /// The fields of the JSON object. + var fields: [String: JSONType] = [:] + + /// Form a JSON object from its fields. + private init(fields: [String: JSONType]) { + self.fields = fields + } + + /// Form a JSON object given a dictionary. + init(dictionary: [String: Any]) { + fields = dictionary.mapValues { JSONType(value: $0) } + } + + /// Merge the fields of this JSON object with another JSON object to produce + /// a JSON object + func merging(with other: JSONObject) -> JSONObject { + // Collect the set of all keys from both JSON objects. + var allKeys: Set = [] + allKeys.formUnion(fields.keys) + allKeys.formUnion(other.fields.keys) + + // Form a new JSON object containing the union of the fields + let newFields = allKeys.map { key in + let myValue = fields[key] ?? .null + let otherValue = other.fields[key] ?? .null + return (key, myValue.merging(with: otherValue)) + } + return JSONObject(fields: [String: JSONType](uniqueKeysWithValues: newFields)) + } + + /// Render this JSON object into a struct. + func asDeclSyntax(name: String) -> DeclSyntax { + /// The list of fields in this object, sorted alphabetically. + let sortedFields = fields.sorted(by: { $0.key < $1.key }) + + // Collect the nested types + let nestedTypes: [(String, JSONObject)] = sortedFields.compactMap { (name, type) in + guard let object = type.innerObject else { + return nil + } + + return (name.capitalized, object) + } + + let members = MemberBlockItemListSyntax { + // Print the fields of this type. + for (fieldName, fieldType) in sortedFields { + MemberBlockItemSyntax( + leadingTrivia: .newline, + decl: "var \(raw: fieldName): \(fieldType.asTypeSyntax(name: fieldName))" as DeclSyntax + ) + } + + // Print any nested types. + for (typeName, object) in nestedTypes { + MemberBlockItemSyntax( + leadingTrivia: (typeName == nestedTypes.first?.0) ? .newlines(2) : .newline, + decl: object.asDeclSyntax(name: typeName) + ) + } + } + + return """ + struct \(raw: name): Codable { + \(members.trimmed) + } + """ + } +} + +/// Describes the type of JSON data. +fileprivate enum JSONType { + /// String data + case string + + /// Numeric data + case number + + /// Boolean data + case boolean + + /// A "null", which implies optionality but without any underlying type + /// information. + case null + + /// An array. + indirect case array(JSONType) + + /// An object. + indirect case object(JSONObject) + + /// A value that is optional, for example because it is missing or null in + /// other cases. + indirect case optional(JSONType) + + /// Determine the type of a JSON value. + init(value: Any) { + switch value { + case let string as String: + if string == "true" || string == "false" { + self = .boolean + } else { + self = .string + } + case is NSNumber: + self = .number + case is NSArray: + let array = value as! [Any] + + // Use null as a fallback for an empty array. + guard let firstValue = array.first else { + self = .array(.null) + return + } + + // Merge the array elements. + let elementType: JSONType = array[1...].reduce( + JSONType(value: firstValue) + ) { (result, value) in + result.merging(with: JSONType(value: value)) + } + self = .array(elementType) + + case is NSNull: + self = .null + case is NSDictionary: + self = .object(JSONObject(dictionary: value as! [String: Any])) + default: + self = .string + } + } + + /// Merge this JSON type with another JSON type, producing a new JSON type + /// that abstracts over the two. + func merging(with other: JSONType) -> JSONType { + switch (self, other) { + // Exact matches are easy. + case (.string, .string): return .string + case (.number, .number): return .number + case (.boolean, .boolean): return .boolean + case (.null, .null): return .null + + case (.array(let inner), .array(.null)), (.array(.null), .array(let inner)): + // Merging an array with an array of null leaves the array. + return .array(inner) + + case (.array(let inner), .null), (.null, .array(let inner)): + // Merging an array with a null just leaves an array. + return .array(inner) + + case (.array(let left), .array(let right)): + // Merging two arrays merges the element types + return .array(left.merging(with: right)) + + case (.object(let left), .object(let right)): + // Merging two arrays merges the element types + return .object(left.merging(with: right)) + + // Merging a string with a Boolean means we misinterpreted "true" or + // "false" as Boolean when it was meant as a string. + case (.string, .boolean), (.boolean, .string): return .string + + // Merging 'null' with an optional returns the optional. + case (.optional(let inner), .null), (.null, .optional(let inner)): + return .optional(inner) + + // Merging 'null' with anything else makes it an optional. + case (let inner, .null), (.null, let inner): + return .optional(inner) + + // Merging two optionals merges the underlying types and makes the + // result optional. + case (.optional(let left), .optional(let right)): + return .optional(left.merging(with: right)) + + // Merging an optional with anything else merges the underlying bits and + // makes them optional. + case (let outer, .optional(let inner)), (.optional(let inner), let outer): + return .optional(inner.merging(with: outer)) + + // Fall back to the null case when we don't know. + default: + return .null + } + } + + /// Dig out the JSON inner object referenced by this type. + var innerObject: JSONObject? { + switch self { + case .string, .null, .number, .boolean: nil + case .optional(let inner): inner.innerObject + case .array(let inner): inner.innerObject + case .object(let object): object + } + } + + /// Render this JSON type into type syntax. + func asTypeSyntax(name: String) -> TypeSyntax { + switch self { + case .string: "String" + case .number: "Double" + case .boolean: "Bool" + case .null: "Void" + case .optional(let inner): "\(inner.asTypeSyntax(name: name))?" + case .array(let inner): "[\(inner.asTypeSyntax(name: name))]" + case .object(_): "\(raw: name.capitalized)" + } + } +} diff --git a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift index 15c86cf7c..acdea5dd8 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActions.swift @@ -18,6 +18,7 @@ let allSyntaxCodeActions: [SyntaxCodeActionProvider.Type] = [ AddDocumentation.self, AddSeparatorsToIntegerLiteral.self, ConvertIntegerLiteral.self, + ConvertJSONToCodableStruct.self, FormatRawStringLiteral.self, MigrateToNewIfLetSyntax.self, OpaqueParameterToGeneric.self, diff --git a/Tests/SourceKitLSPTests/CodeActionTests.swift b/Tests/SourceKitLSPTests/CodeActionTests.swift index 77763c4f3..27752e902 100644 --- a/Tests/SourceKitLSPTests/CodeActionTests.swift +++ b/Tests/SourceKitLSPTests/CodeActionTests.swift @@ -299,6 +299,48 @@ final class CodeActionTests: XCTestCase { ) } + func testJSONCodableCodeActionResult() async throws { + let testClient = try await TestSourceKitLSPClient(capabilities: clientCapabilitiesWithCodeActionSupport()) + let uri = DocumentURI.for(.swift) + let positions = testClient.openDocument( + """ + 1️⃣{ + "name": "Produce", + "shelves": [ + { + "name": "Discount Produce", + "product": { + "name": "Banana", + "points": 200, + "description": "A banana that's perfectly ripe." + } + } + ] + } + """, + uri: uri + ) + + let testPosition = positions["1️⃣"] + let request = CodeActionRequest( + range: Range(testPosition), + context: .init(), + textDocument: TextDocumentIdentifier(uri) + ) + let result = try await testClient.send(request) + + guard case .codeActions(let codeActions) = result else { + XCTFail("Expected code actions") + return + } + + // Make sure we get a JSON conversion action. + let codableAction = codeActions.first { action in + return action.title == "Create Codable structs from JSON" + } + XCTAssertNotNil(codableAction) + } + func testSemanticRefactorRangeCodeActionResult() async throws { let testClient = try await TestSourceKitLSPClient(capabilities: clientCapabilitiesWithCodeActionSupport()) let uri = DocumentURI.for(.swift) diff --git a/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift index f1207284b..e60630898 100644 --- a/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift +++ b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift @@ -40,6 +40,168 @@ final class SyntaxRefactorTests: XCTestCase { ] ) } + + func testConvertJSONToCodableStructClosure() throws { + try assertRefactor( + malformedInput: """ + { + "name": "Produce", + "shelves": [ + { + "name": "Discount Produce", + "product": { + "name": "Banana", + "points": 200, + "description": "A banana that's perfectly ripe." + } + } + ] + } + """, + context: (), + provider: ConvertJSONToCodableStruct.self, + expected: [ + SourceEdit( + range: AbsolutePosition(utf8Offset: 0)..( From 037e55e9d6a38a674f51efe5fe533b384dc82d76 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Mon, 6 May 2024 14:32:29 -0700 Subject: [PATCH 27/32] Handle unexpected nodes following a closure --- .../ConvertJSONToCodableStruct.swift | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift index 0a5cd38b7..09f330b65 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift @@ -78,6 +78,9 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider { /// We're only going to look at the text of the closure to see if we /// have JSON in there. text = closure.trimmedDescription + case let .endingClosure(closure, unexpected): + text = closure.trimmedDescription + unexpected.description + case .stringLiteral(_, let literalText): /// A string literal that could contain JSON within it. text = literalText @@ -108,6 +111,15 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider { return [ SourceEdit(range: closure.trimmedRange, replacement: decls.description) ] + case .endingClosure(let closure, let unexpected): + // Closures are replaced entirely, since they were invalid code to + // start with. + return [ + SourceEdit( + range: closure.positionAfterSkippingLeadingTrivia.. Date: Mon, 6 May 2024 17:07:17 -0700 Subject: [PATCH 28/32] Prevent rename of argument labels for enum cases Renaming an enum case currently caused invalid code to be generated because we would rename eg. `myCase(String)` to `myNewCase(_ String)`. Fixing the underlying issue requires changes to `sourcekitd`, that are out-of-scope at the moment. For now, just suppress argument label rename for enum cases in SourceKit-LSP and avoid generating invalid code even if just the base name is modified. rdar://127248157 --- Sources/SourceKitLSP/Rename.swift | 42 +++++++++++++++-- Tests/SourceKitLSPTests/RenameTests.swift | 55 +++++++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index 81c00fed9..2e28a2cd3 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -995,6 +995,24 @@ extension SwiftLanguageService { return nil } + /// Returns `true` if the given position is inside an `EnumCaseDeclSyntax`. + fileprivate func isInsideEnumCaseDecl(position: Position, snapshot: DocumentSnapshot) async -> Bool { + let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot) + var node = Syntax(syntaxTree.token(at: snapshot.absolutePosition(of: position))) + + while let parent = node?.parent { + if parent.is(EnumCaseDeclSyntax.self) { + return true + } + if parent.is(MemberBlockItemSyntax.self) || parent.is(CodeBlockItemSyntax.self) { + // `MemberBlockItemSyntax` and `CodeBlockItemSyntax` can't be nested inside an EnumCaseDeclSyntax. Early exit. + return false + } + node = parent + } + return false + } + /// When the user requested a rename at `position` in `snapshot`, determine the position at which the rename should be /// performed internally, the USR of the symbol to rename and the range to rename that should be returned to the /// editor. @@ -1013,9 +1031,9 @@ extension SwiftLanguageService { /// For example, `position` could point to the definition of a function within the file when rename was initiated on /// a call. /// - /// If a `range` is returned, this is an expanded range that contains both the symbol to rename as well as the - /// position at which the rename was requested. For example, when rename was initiated from the argument label of a - /// function call, the `range` will contain the entire function call from the base name to the closing `)`. + /// If a `functionLikeRange` is returned, this is an expanded range that contains both the symbol to rename as well + /// as the position at which the rename was requested. For example, when rename was initiated from the argument label + /// of a function call, the `range` will contain the entire function call from the base name to the closing `)`. func symbolToRename( at position: Position, in snapshot: DocumentSnapshot @@ -1069,8 +1087,17 @@ extension SwiftLanguageService { try Task.checkCancellation() + var requestedNewName = request.newName + if let openParenIndex = requestedNewName.firstIndex(of: "("), + await isInsideEnumCaseDecl(position: renamePosition, snapshot: snapshot) + { + // We don't support renaming enum parameter labels at the moment + // (https://github.com/apple/sourcekit-lsp/issues/1228) + requestedNewName = String(requestedNewName[.. Date: Mon, 6 May 2024 17:48:35 -0700 Subject: [PATCH 29/32] Log request ID of received requests Useful for correlating request IDs with cancellation notifications. --- Sources/SourceKitLSP/SourceKitLSPServer.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index fce17f037..f5b14807d 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -935,7 +935,7 @@ extension SourceKitLSPServer: MessageHandler { } } - logger.log("Received request: \(params.forLogging)") + logger.log("Received request \(id): \(params.forLogging)") switch request { case let request as RequestAndReply: From d4bbf9ccc1f3a297ab7053aa2cddae914975a388 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Mon, 6 May 2024 17:57:07 -0700 Subject: [PATCH 30/32] Address review comments --- .../ConvertJSONToCodableStruct.swift | 38 ++-- .../SyntaxRefactorTests.swift | 191 ++++++++++-------- 2 files changed, 123 insertions(+), 106 deletions(-) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift index 09f330b65..73f583627 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift @@ -88,7 +88,8 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider { // Try to process this as JSON. guard - let object = try? JSONSerialization.jsonObject(with: text.data(using: .utf8)!), + let data = text.data(using: .utf8), + let object = try? JSONSerialization.jsonObject(with: data), let dictionary = object as? [String: Any] else { return [] @@ -131,12 +132,10 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider { ] } } -} -extension ConvertJSONToCodableStruct { /// The result of preflighting a syntax node to try to find potential JSON /// in it. - enum Preflight { + private enum Preflight { /// A closure, which is what a JSON dictionary looks like when pasted /// into Swift. case closure(ClosureExprSyntax) @@ -150,7 +149,7 @@ extension ConvertJSONToCodableStruct { } /// Look for either a closure or a string literal that might have JSON in it. - static func preflightRefactoring(_ syntax: Syntax) -> Preflight? { + private static func preflightRefactoring(_ syntax: Syntax) -> Preflight? { // Preflight a closure. // // A blob of JSON dropped into a Swift source file will look like a @@ -166,7 +165,7 @@ extension ConvertJSONToCodableStruct { } // We found a string literal; its contents might be JSON. - if let stringLit = syntax.as(StringLiteralExprSyntax.self) { + if let stringLiteral = syntax.as(StringLiteralExprSyntax.self) { // Look for an enclosing context and prefer that, because we might have // a string literal that's inside a closure where the closure itself // is the JSON. @@ -176,11 +175,11 @@ extension ConvertJSONToCodableStruct { return enclosingPreflight } - guard let text = stringLit.representedLiteralValue else { + guard let text = stringLiteral.representedLiteralValue else { return nil } - return .stringLiteral(stringLit, text) + return .stringLiteral(stringLiteral, text) } // Look further up the syntax tree. @@ -216,9 +215,7 @@ fileprivate struct JSONObject { /// a JSON object func merging(with other: JSONObject) -> JSONObject { // Collect the set of all keys from both JSON objects. - var allKeys: Set = [] - allKeys.formUnion(fields.keys) - allKeys.formUnion(other.fields.keys) + let allKeys = Set(fields.keys).union(other.fields.keys) // Form a new JSON object containing the union of the fields let newFields = allKeys.map { key in @@ -235,7 +232,7 @@ fileprivate struct JSONObject { let sortedFields = fields.sorted(by: { $0.key < $1.key }) // Collect the nested types - let nestedTypes: [(String, JSONObject)] = sortedFields.compactMap { (name, type) in + let nestedTypes: [(name: String, type: JSONObject)] = sortedFields.compactMap { (name, type) in guard let object = type.innerObject else { return nil } @@ -255,7 +252,7 @@ fileprivate struct JSONObject { // Print any nested types. for (typeName, object) in nestedTypes { MemberBlockItemSyntax( - leadingTrivia: (typeName == nestedTypes.first?.0) ? .newlines(2) : .newline, + leadingTrivia: (typeName == nestedTypes.first?.name) ? .newlines(2) : .newline, decl: object.asDeclSyntax(name: typeName) ) } @@ -298,16 +295,13 @@ fileprivate enum JSONType { init(value: Any) { switch value { case let string as String: - if string == "true" || string == "false" { - self = .boolean - } else { - self = .string + switch string { + case "true", "false": self = .boolean + default: self = .string } case is NSNumber: self = .number - case is NSArray: - let array = value as! [Any] - + case let array as [Any]: // Use null as a fallback for an empty array. guard let firstValue = array.first else { self = .array(.null) @@ -324,8 +318,8 @@ fileprivate enum JSONType { case is NSNull: self = .null - case is NSDictionary: - self = .object(JSONObject(dictionary: value as! [String: Any])) + case let dictionary as [String: Any]: + self = .object(JSONObject(dictionary: dictionary)) default: self = .string } diff --git a/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift index e60630898..f908385c4 100644 --- a/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift +++ b/Tests/SourceKitLSPTests/SyntaxRefactorTests.swift @@ -10,6 +10,7 @@ // //===----------------------------------------------------------------------===// +import SKTestSupport @_spi(Testing) import SourceKitLSP import SwiftParser import SwiftRefactor @@ -20,7 +21,7 @@ final class SyntaxRefactorTests: XCTestCase { func testAddDocumentationRefactor() throws { try assertRefactor( """ - func refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? { } + 1️⃣func 2️⃣refactor(syntax: DeclSyntax, in context: Void) -> DeclSyntax? { } """, context: (), provider: AddDocumentation.self, @@ -43,21 +44,21 @@ final class SyntaxRefactorTests: XCTestCase { func testConvertJSONToCodableStructClosure() throws { try assertRefactor( - malformedInput: """ - { - "name": "Produce", - "shelves": [ - { - "name": "Discount Produce", - "product": { - "name": "Banana", - "points": 200, - "description": "A banana that's perfectly ripe." - } - } - ] - } - """, + """ + 4️⃣{1️⃣ + 2️⃣"name": "Produce", + "shelves": [ + { + "name": "Discount Produce", + "product": { + "name": "Banana", + "points": 200, + "description": "A banana that's perfectly ripe." + } + } + ] + } + """, context: (), provider: ConvertJSONToCodableStruct.self, expected: [ @@ -87,23 +88,23 @@ final class SyntaxRefactorTests: XCTestCase { func testConvertJSONToCodableStructLiteral() throws { try assertRefactor( - malformedInput: #""" + #""" + """ + { + "name": "Produce", + "shelves": [ + { + "name": "Discount Produce", + "product": { + "name": "Banana", + "points": 200, + "description": "A banana that's perfectly ripe." + } + } + ] + } """ - { - "name": "Produce", - "shelves": [ - { - "name": "Discount Produce", - "product": { - "name": "Banana", - "points": 200, - "description": "A banana that's perfectly ripe." - } - } - ] - } - """ - """#, + """#, context: (), provider: ConvertJSONToCodableStruct.self, expected: [ @@ -134,44 +135,44 @@ final class SyntaxRefactorTests: XCTestCase { func testConvertJSONToCodableStructClosureMerging() throws { try assertRefactor( - malformedInput: """ - { - "name": "Store", - "shelves": [ - { - "name": "Discount Produce", - "product": { - "name": "Banana", - "points": 200, - "description": "A banana that's perfectly ripe.", - "healthy": "true", - "delicious": "true", - "categories": [ "fruit", "yellow" ] - } - }, - { - "name": "Meat", - "product": { - "name": "steak", - "points": 200, - "healthy": "false", - "delicious": "true", - "categories": [ ] - } - }, - { - "name": "Cereal aisle", - "product": { - "name": "Sugarydoos", - "points": 0.5, - "healthy": "false", - "delicious": "maybe", - "description": "More sugar than you can imagine." - } - } - ] - } - """, + """ + { + "name": "Store", + "shelves": [ + { + "name": "Discount Produce", + "product": { + "name": "Banana", + "points": 200, + "description": "A banana that's perfectly ripe.", + "healthy": "true", + "delicious": "true", + "categories": [ "fruit", "yellow" ] + } + }, + { + "name": "Meat", + "product": { + "name": "steak", + "points": 200, + "healthy": "false", + "delicious": "true", + "categories": [ ] + } + }, + { + "name": "Cereal aisle", + "product": { + "name": "Sugarydoos", + "points": 0.5, + "healthy": "false", + "delicious": "maybe", + "description": "More sugar than you can imagine." + } + } + ] + } + """, context: (), provider: ConvertJSONToCodableStruct.self, expected: [ @@ -205,23 +206,44 @@ final class SyntaxRefactorTests: XCTestCase { } func assertRefactor( - malformedInput input: String, + _ input: String, context: R.Context, provider: R.Type, expected: [SourceEdit], file: StaticString = #filePath, line: UInt = #line -) throws where R.Input == Syntax { - var parser = Parser(input) - let syntax = ExprSyntax.parse(from: &parser) - try assertRefactor( - Syntax(syntax), - context: context, - provider: provider, - expected: expected, - file: file, - line: line - ) +) throws { + let (markers, textWithoutMarkers) = extractMarkers(input) + + var parser = Parser(textWithoutMarkers) + let sourceFile = SourceFileSyntax.parse(from: &parser) + + let markersToCheck = markers.isEmpty ? [("1️⃣", 0)] : markers.sorted { $0.key < $1.key } + + for (marker, location) in markersToCheck { + guard let token = sourceFile.token(at: AbsolutePosition(utf8Offset: location)) else { + XCTFail("Could not find token at location \(marker)") + continue + } + + let input: R.Input + if let parentMatch = token.parent?.as(R.Input.self) { + input = parentMatch + } else { + XCTFail("token at \(marker) did not match expected input: \(token)") + continue + } + + try assertRefactor( + input, + context: context, + provider: provider, + expected: expected, + at: marker, + file: file, + line: line + ) + } } // Borrowed from the swift-syntax library's SwiftRefactor tests. @@ -231,6 +253,7 @@ func assertRefactor( context: R.Context, provider: R.Type, expected: [SourceEdit], + at marker: String, file: StaticString = #filePath, line: UInt = #line ) throws { @@ -239,7 +262,7 @@ func assertRefactor( if !expected.isEmpty { XCTFail( """ - Refactoring produced empty result, expected: + Refactoring at \(marker) produced empty result, expected: \(expected) """, file: file, @@ -252,7 +275,7 @@ func assertRefactor( if edits.count != expected.count { XCTFail( """ - Refactoring produced incorrect number of edits, expected \(expected.count) not \(edits.count). + Refactoring at \(marker) produced incorrect number of edits, expected \(expected.count) not \(edits.count). Actual: \(edits.map({ $0.debugDescription }).joined(separator: "\n")) From 30459c7b5481539a8c219706f13df069bfa752ac Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 6 May 2024 17:44:15 -0700 Subject: [PATCH 31/32] =?UTF-8?q?Don=E2=80=99t=20report=20no-op=20rename?= =?UTF-8?q?=20edits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When renaming `func test(foo: Int) {}` to `test2(foo:)`, rename used to report an edit from `foo` to `foo`, which clutters the refactor preview view. We shouldn’t report edits if no text is actually changed. rdar://127291815 --- Sources/SourceKitLSP/Rename.swift | 15 +++++++++++++++ Tests/SourceKitLSPTests/RenameTests.swift | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index 2e28a2cd3..a8bb0b15d 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -817,6 +817,7 @@ extension SourceKitLSPServer { newName: newName ) } + edits = edits.filter { !$0.isNoOp(in: snapshot) } return (uri, edits) }.compactMap { $0 } for (uri, editsForUri) in urisAndEdits { @@ -1116,7 +1117,11 @@ extension SwiftLanguageService { ) } } + edits = edits.filter { !$0.isNoOp(in: snapshot) } + if edits.isEmpty { + return (edits: WorkspaceEdit(changes: [:]), usr: usr) + } return (edits: WorkspaceEdit(changes: [snapshot.uri: edits]), usr: usr) } @@ -1485,3 +1490,13 @@ fileprivate extension RelatedIdentifiersResponse { } } } + +fileprivate extension TextEdit { + /// Returns `true` the replaced text is the same as the new text + func isNoOp(in snapshot: DocumentSnapshot) -> Bool { + if snapshot.text[snapshot.indexRange(of: range)] == newText { + return true + } + return false + } +} diff --git a/Tests/SourceKitLSPTests/RenameTests.swift b/Tests/SourceKitLSPTests/RenameTests.swift index b053b9957..75cc5ad68 100644 --- a/Tests/SourceKitLSPTests/RenameTests.swift +++ b/Tests/SourceKitLSPTests/RenameTests.swift @@ -1236,4 +1236,26 @@ final class RenameTests: XCTestCase { """ ) } + + func testRenameDoesNotReportEditsIfNoActualChangeIsMade() async throws { + try await SkipUnless.sourcekitdSupportsRename() + let project = try await SwiftPMTestProject( + files: [ + "FileA.swift": """ + func 1️⃣foo(x: Int) {} + """, + "FileB.swift": """ + func test() { + foo(x: 1) + } + """, + ], + build: true + ) + let (uri, positions) = try project.openDocument("FileA.swift") + let result = try await project.testClient.send( + RenameRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"], newName: "foo(x:)") + ) + XCTAssertEqual(result?.changes, [:]) + } } From 714ff2a620e11ea0f2052d568472abb0badff79e Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 7 May 2024 11:45:39 -0700 Subject: [PATCH 32/32] Adjustment because `trimmedRange` is not available in swift-syntax `release/6.0` --- .../Swift/CodeActions/ConvertJSONToCodableStruct.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift index 73f583627..82c35ade4 100644 --- a/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift +++ b/Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift @@ -110,7 +110,10 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider { // Closures are replaced entirely, since they were invalid code to // start with. return [ - SourceEdit(range: closure.trimmedRange, replacement: decls.description) + SourceEdit( + range: closure.positionAfterSkippingLeadingTrivia..