diff --git a/Contributor Documentation/BSP Extensions.md b/Contributor Documentation/BSP Extensions.md index 37265f3f3..6f7cb6c3f 100644 --- a/Contributor Documentation/BSP Extensions.md +++ b/Contributor Documentation/BSP Extensions.md @@ -100,14 +100,14 @@ To do so, the build server should perform any work that is necessary to typechec The server communicates during the initialize handshake whether this method is supported or not by setting `prepareProvider: true` in `SourceKitInitializeBuildResponseData`. - method: `sourcekit/buildTarget/prepare` -- params: `PrepareParams` -- result: `void` +- params: `BuildTargetPrepareRequest` +- result: `BuildTargetPrepareResponse` > [!NOTE] > This request was previously named `buildTarget/prepare`. The old name is still accepted for backward compatibility. ```ts -export interface PrepareParams { +export interface BuildTargetPrepareRequest { /** A list of build targets to prepare. */ targets: BuildTargetIdentifier[]; @@ -115,6 +115,17 @@ export interface PrepareParams { * The server may include this id in triggered notifications or responses. **/ originId?: OriginId; } + +export interface BuildTargetPrepareResponse { + /** + * Targets that were implicitly prepared by the prepare request. + * + * For example, a target may be implicitly prepared if one if its dependents gets prepared. + * + * When omitted, this is the same as an empty array. + */ + implicitlyPreparedTargets?: BuildTargetIdentifier[] +} ``` ## `buildTarget/sources` diff --git a/Sources/BuildServerIntegration/BuildServerManager.swift b/Sources/BuildServerIntegration/BuildServerManager.swift index cb982efcc..72e8a33f1 100644 --- a/Sources/BuildServerIntegration/BuildServerManager.swift +++ b/Sources/BuildServerIntegration/BuildServerManager.swift @@ -1466,24 +1466,55 @@ package actor BuildServerManager: QueueBasedMessageHandler { return (depths, dependents) } - /// Sort the targets so that low-level targets occur before high-level targets. - /// - /// This sorting is best effort but allows the indexer to prepare and index low-level targets first, which allows - /// index data to be available earlier. - package func topologicalSort(of targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] { + /// Sort the targets in the order they should be indexed in for best performance. + package func targetsSortedForIndexing(_ targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] { guard let buildTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() }) else { return targets.sorted { $0.uri.stringValue < $1.uri.stringValue } } - return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in - let lhsDepth = buildTargets[lhs]?.depth ?? 0 - let rhsDepth = buildTargets[rhs]?.depth ?? 0 + // Generate a preliminary work list of targets to index in which we prefer top-level targets over low-level targets + // and targets of the root package over targets in dependencies. + // We want to index targets in the root package first because those are likely the files that the user is interested + // in editing. We want to index top-level targets first, because preparing those likely implies preparation of the + // low-level targets. + let workList = targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in + let lhsTarget = buildTargets[lhs] + let rhsTarget = buildTargets[rhs] + + switch (lhsTarget?.target.tags.contains(.dependency), rhsTarget?.target.tags.contains(.dependency)) { + case (true, false): return false + case (false, true): return true + default: break + } + + let lhsDepth = lhsTarget?.depth ?? 0 + let rhsDepth = rhsTarget?.depth ?? 0 if lhsDepth != rhsDepth { - return lhsDepth > rhsDepth + return lhsDepth < rhsDepth } + // Use the target's name as a tie-breaker return lhs.uri.stringValue < rhs.uri.stringValue } + + // Now walk through the list of targets in the work list. For each target from the work list, index all of its + // transitive dependencies next. We do this because preparing a top-level target likely also prepared all of its + // dependencies, so we should be able to index all files in the target's dependencies without needing to perform any + // target preparation. + var sorted: [BuildTargetIdentifier] = [] + // A `Set` representation of `sorted` to efficiently check if `target` is already in `sorted` and should be skipped. + var visited: Set = [] + for target in workList where !visited.contains(target) { + sorted.append(target) + visited.insert(target) + + let transitiveDependencies = transitiveClosure(of: [target]) { Set(buildTargets[$0]?.target.dependencies ?? []) } + let dependenciesInWorkList = workList.filter { transitiveDependencies.contains($0) } + sorted += dependenciesInWorkList + visited.formUnion(dependenciesInWorkList) + } + + return sorted } /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in @@ -1499,14 +1530,18 @@ package actor BuildServerManager: QueueBasedMessageHandler { .sorted { $0.uri.stringValue < $1.uri.stringValue } } - package func prepare(targets: Set) async throws { - let _: BuildTargetPrepareResponse? = try await buildServerAdapterAfterInitialized?.send( + package func prepare(targets: Set) async throws -> BuildTargetPrepareResponse { + guard let buildServerAdapterAfterInitialized = try await buildServerAdapterAfterInitialized else { + throw ResponseError.unknown("No connection to build server") + } + let response = try await buildServerAdapterAfterInitialized.send( BuildTargetPrepareRequest(targets: targets.sorted { $0.uri.stringValue < $1.uri.stringValue }) ) await orLog("Calling fileDependenciesUpdated") { let filesInPreparedTargets = try await self.sourceFiles(in: targets).flatMap(\.sources).map(\.uri) await filesDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets)) } + return response } package func registerForChangeNotifications(for uri: DocumentURI, language: Language) async { diff --git a/Sources/BuildServerIntegration/SwiftPMBuildServer.swift b/Sources/BuildServerIntegration/SwiftPMBuildServer.swift index 71c817dbd..cbb28c235 100644 --- a/Sources/BuildServerIntegration/SwiftPMBuildServer.swift +++ b/Sources/BuildServerIntegration/SwiftPMBuildServer.swift @@ -781,10 +781,15 @@ package actor SwiftPMBuildServer: BuiltInBuildServer { package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse { // TODO: Support preparation of multiple targets at once. (https://github.com/swiftlang/sourcekit-lsp/issues/1262) + var implicitlyPreparedTargets: [BuildTargetIdentifier] = [] for target in request.targets { - await orLog("Preparing") { try await prepare(singleTarget: target) } + await orLog("Preparing") { + try await prepare(singleTarget: target) + implicitlyPreparedTargets += transitiveClosure(of: [target], successors: { targetDependencies[$0] ?? [] }) + .filter { !request.targets.contains($0) } + } } - return BuildTargetPrepareResponse() + return BuildTargetPrepareResponse(implicitlyPreparedTargets: implicitlyPreparedTargets) } private func prepare(singleTarget target: BuildTargetIdentifier) async throws { diff --git a/Sources/SemanticIndex/PreparationTaskDescription.swift b/Sources/SemanticIndex/PreparationTaskDescription.swift index c861a6dce..7626443b9 100644 --- a/Sources/SemanticIndex/PreparationTaskDescription.swift +++ b/Sources/SemanticIndex/PreparationTaskDescription.swift @@ -111,14 +111,19 @@ package struct PreparationTaskDescription: IndexTaskDescription { ) signposter.endInterval("Preparing", state) } + let prepareResponse: BuildTargetPrepareResponse? do { - try await buildServerManager.prepare(targets: Set(targetsToPrepare)) + prepareResponse = try await buildServerManager.prepare(targets: Set(targetsToPrepare)) } catch { logger.error("Preparation failed: \(error.forLogging)") + prepareResponse = nil } await hooks.preparationTaskDidFinish?(self) if !Task.isCancelled { - await preparationUpToDateTracker.markUpToDate(targetsToPrepare, updateOperationStartDate: startDate) + await preparationUpToDateTracker.markUpToDate( + targetsToPrepare + (prepareResponse?.implicitlyPreparedTargets ?? []), + updateOperationStartDate: startDate + ) } } } diff --git a/Sources/SemanticIndex/SemanticIndexManager.swift b/Sources/SemanticIndex/SemanticIndexManager.swift index 555a0bfa2..e8d2c7086 100644 --- a/Sources/SemanticIndex/SemanticIndexManager.swift +++ b/Sources/SemanticIndex/SemanticIndexManager.swift @@ -957,10 +957,12 @@ package final actor SemanticIndexManager { indexTasksWereScheduled(newIndexTasks) } - // The targets sorted in reverse topological order, low-level targets before high-level targets. If topological - // sorting fails, sorted in another deterministic way where the actual order doesn't matter. + // The targets sorted in topological order, high-level targets before low-level targets. If topological sorting + // fails, sorted in another deterministic way where the actual order doesn't matter. var sortedTargets: [BuildTargetIdentifier] = - await orLog("Sorting targets") { try await buildServerManager.topologicalSort(of: Array(filesByTarget.keys)) } + await orLog("Sorting targets") { + try await buildServerManager.targetsSortedForIndexing(Array(filesByTarget.keys)) + } ?? Array(filesByTarget.keys).sorted { $0.uri.stringValue < $1.uri.stringValue } if Set(sortedTargets) != Set(filesByTarget.keys) { diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift index 7f2b54384..f4c23a60a 100644 --- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift +++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift @@ -528,13 +528,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase { func testPrepareTargetAfterEditToDependency() async throws { var testHooks = Hooks() let expectedPreparationTracker = ExpectedIndexTaskTracker(expectedPreparations: [ - [ - try ExpectedPreparation(targetName: "LibA"), - try ExpectedPreparation(targetName: "LibB"), - ], - [ - try ExpectedPreparation(targetName: "LibB") - ], + [try ExpectedPreparation(targetName: "LibB")], + [try ExpectedPreparation(targetName: "LibB")], ]) testHooks.indexHooks = expectedPreparationTracker.testHooks @@ -626,9 +621,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase { var testHooks = Hooks() let expectedPreparationTracker = ExpectedIndexTaskTracker(expectedPreparations: [ - // Preparation of targets during the initial of the target + // Preparation of targets during the initial indexing of the target [ - try ExpectedPreparation(targetName: "LibA"), try ExpectedPreparation(targetName: "LibB"), try ExpectedPreparation(targetName: "LibC"), try ExpectedPreparation(targetName: "LibD"), @@ -2635,35 +2629,60 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase { } }) ) - let project = try await SwiftPMTestProject( + let project = try await MultiFileTestProject( files: [ - "LibA/LibA.swift": "", - "LibB/LibB.swift": "", - ], - manifest: """ + "MyDependency/Sources/DependencyA/DependencyA.swift": "", + "MyDependency/Package.swift": """ + // swift-tools-version: 5.7 + + import PackageDescription + let package = Package( - name: "MyLibrary", + name: "MyDependency", + products: [.library(name: "DependencyA", targets: ["DependencyA"])], targets: [ - .target(name: "LibA"), - .target(name: "LibB", dependencies: ["LibA"]) + .target(name: "DependencyA"), ] ) """, + + "MyPackage/Sources/LibA/LibA.swift": "", + "MyPackage/Sources/LibB/LibB.swift": "", + "MyPackage/Sources/LibC/LibC.swift": "", + "MyPackage/Package.swift": """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyPackage", + dependencies: [.package(path: "../MyDependency")], + targets: [ + .target(name: "LibA", dependencies: [.product(name: "DependencyA", package: "MyDependency")]), + .target(name: "LibB", dependencies: ["LibA"]), + .target(name: "LibC") + ] + ) + """, + ], + workspaces: { scratchDirectory in + [WorkspaceFolder(uri: DocumentURI(scratchDirectory.appending(component: "MyPackage")))] + }, hooks: testHooks, - enableBackgroundIndexing: true, - pollIndex: false + enableBackgroundIndexing: true ) // We can't poll the index using `workspace/synchronize` because that elevates the priority of the indexing requests - // in a non-deterministic order (due to the way ). If LibB's priority gets elevated before LibA's, then LibB will - // get prepared first, which is contrary to the background behavior we want to check here. + // in a non-deterministic order (due to the way `withTaskPriorityChangedHandler` is implemented). If LibB's priority + // gets elevated before LibA's, then LibB will get prepared first, which is contrary to the background behavior we + // want to check here. try await fulfillmentOfOrThrow(twoPreparationRequestsReceived) let preparedTargets = preparationRequests.value.flatMap(\.targets) guard preparedTargets.count == 2 else { XCTFail("expected 2 prepared targets, but got: \(preparedTargets)") return } - XCTAssert(preparedTargets[0].matchesTargetName("LibA")) - XCTAssert(preparedTargets[1].matchesTargetName("LibB")) + XCTAssert(preparedTargets[0].matchesTargetName("LibB")) + XCTAssert(preparedTargets[1].matchesTargetName("LibC")) withExtendedLifetime(project) {} } @@ -2715,8 +2734,8 @@ final class BackgroundIndexingTests: SourceKitLSPTestCase { return TextDocumentSourceKitOptionsResponse(compilerArguments: arguments) } - func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> VoidResponse { - return VoidResponse() + func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse { + return BuildTargetPrepareResponse() } }