From e23e300f7c57764fb5d6413ff683f638be4fd3bc Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 22 Jun 2024 08:00:30 -0700 Subject: [PATCH] Add a request to re-index all files in SourceKit-LSP Users should not need to rely on this request. The index should always be updated automatically in the background. Having to invoke this request manes there is a bug in SourceKit-LSP's automatic re-indexing. It does, however, offer a workaround to re-index files when such a bug occurs where otherwise there would be no workaround. rdar://127476221 Resolves #1263 --- Documentation/LSP Extensions.md | 14 +++ Sources/LanguageServerProtocol/CMakeLists.txt | 1 + .../Requests/TriggerReindexRequest.swift | 25 +++++ .../SemanticIndex/SemanticIndexManager.swift | 32 ++++-- .../UpdateIndexStoreTaskDescription.swift | 16 ++- .../MessageHandlingDependencyTracker.swift | 2 + Sources/SourceKitLSP/SourceKitLSPServer.swift | 9 ++ .../BackgroundIndexingTests.swift | 98 +++++++++++++++++++ 8 files changed, 185 insertions(+), 12 deletions(-) create mode 100644 Sources/LanguageServerProtocol/Requests/TriggerReindexRequest.swift diff --git a/Documentation/LSP Extensions.md b/Documentation/LSP Extensions.md index bdfde470c..e3a2852bc 100644 --- a/Documentation/LSP Extensions.md +++ b/Documentation/LSP Extensions.md @@ -431,3 +431,17 @@ New request that returns symbols for all the test classes and test methods withi ```ts export interface WorkspaceTestsParams {} ``` + +## `workspace/triggerReindex` + +New request to re-index all files open in the SourceKit-LSP server. + +Users should not need to rely on this request. The index should always be updated automatically in the background. Having to invoke this request means there is a bug in SourceKit-LSP's automatic re-indexing. It does, however, offer a workaround to re-index files when such a bug occurs where otherwise there would be no workaround. + + +- params: `TriggerReindexParams` +- result: `void` + +```ts +export interface TriggerReindexParams {} +``` diff --git a/Sources/LanguageServerProtocol/CMakeLists.txt b/Sources/LanguageServerProtocol/CMakeLists.txt index 83a1aabb3..bd06c0f5e 100644 --- a/Sources/LanguageServerProtocol/CMakeLists.txt +++ b/Sources/LanguageServerProtocol/CMakeLists.txt @@ -78,6 +78,7 @@ add_library(LanguageServerProtocol STATIC Requests/ShutdownRequest.swift Requests/SignatureHelpRequest.swift Requests/SymbolInfoRequest.swift + Requests/TriggerReindexRequest.swift Requests/TypeDefinitionRequest.swift Requests/TypeHierarchyPrepareRequest.swift Requests/TypeHierarchySubtypesRequest.swift diff --git a/Sources/LanguageServerProtocol/Requests/TriggerReindexRequest.swift b/Sources/LanguageServerProtocol/Requests/TriggerReindexRequest.swift new file mode 100644 index 000000000..9f99614db --- /dev/null +++ b/Sources/LanguageServerProtocol/Requests/TriggerReindexRequest.swift @@ -0,0 +1,25 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +/// Re-index all files open in the SourceKit-LSP server. +/// +/// Users should not need to rely on this request. The index should always be updated automatically in the background. +/// Having to invoke this request means there is a bug in SourceKit-LSP's automatic re-indexing. It does, however, offer +/// a workaround to re-index files when such a bug occurs where otherwise there would be no workaround. +/// +/// **LSP Extension** +public struct TriggerReindexRequest: RequestType { + public static let method: String = "workspace/triggerReindex" + public typealias Response = VoidResponse + + public init() {} +} diff --git a/Sources/SemanticIndex/SemanticIndexManager.swift b/Sources/SemanticIndex/SemanticIndexManager.swift index c0a1979d6..d44071191 100644 --- a/Sources/SemanticIndex/SemanticIndexManager.swift +++ b/Sources/SemanticIndex/SemanticIndexManager.swift @@ -231,15 +231,15 @@ public final actor SemanticIndexManager { /// Returns immediately after scheduling that task. /// /// Indexing is being performed with a low priority. - private func scheduleBackgroundIndex(files: some Collection) async { - _ = await self.scheduleIndexing(of: files, priority: .low) + private func scheduleBackgroundIndex(files: some Collection, indexFilesWithUpToDateUnit: Bool) async { + _ = await self.scheduleIndexing(of: files, indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit, priority: .low) } /// Regenerate the build graph (also resolving package dependencies) and then index all the source files known to the /// build system that don't currently have a unit with a timestamp that matches the mtime of the file. /// /// This method is intended to initially update the index of a project after it is opened. - public func scheduleBuildGraphGenerationAndBackgroundIndexAllFiles() async { + public func scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(indexFilesWithUpToDateUnit: Bool = false) async { generateBuildGraphTask = Task(priority: .low) { await withLoggingSubsystemAndScope(subsystem: indexLoggingSubsystem, scope: "build-graph-generation") { logger.log( @@ -263,16 +263,26 @@ public final actor SemanticIndexManager { // potentially not knowing about unit files, which causes the corresponding source files to be re-indexed. index.pollForUnitChangesAndWait() await testHooks.buildGraphGenerationDidFinish?() - let index = index.checked(for: .modifiedFiles) - let filesToIndex = await self.buildSystemManager.sourceFiles().lazy.map(\.uri) - .filter { !index.hasUpToDateUnit(for: $0) } - await scheduleBackgroundIndex(files: filesToIndex) + var filesToIndex: any Collection = await self.buildSystemManager.sourceFiles().lazy.map(\.uri) + if !indexFilesWithUpToDateUnit { + let index = index.checked(for: .modifiedFiles) + filesToIndex = filesToIndex.filter { !index.hasUpToDateUnit(for: $0) } + } + await scheduleBackgroundIndex(files: filesToIndex, indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit) generateBuildGraphTask = nil } } indexProgressStatusDidChange() } + /// Causes all files to be re-indexed even if the unit file for the source file is up to date. + /// See `TriggerReindexRequest`. + public func scheduleReindex() async { + await indexStoreUpToDateTracker.markAllKnownOutOfDate() + await preparationUpToDateTracker.markAllKnownOutOfDate() + await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(indexFilesWithUpToDateUnit: true) + } + /// Wait for all in-progress index tasks to finish. public func waitForUpToDateIndex() async { logger.info("Waiting for up-to-date index") @@ -312,7 +322,7 @@ public final actor SemanticIndexManager { // Create a new index task for the files that aren't up-to-date. The newly scheduled index tasks will // - Wait for the existing index operations to finish if they have the same number of files. // - Reschedule the background index task in favor of an index task with fewer source files. - await self.scheduleIndexing(of: uris, priority: nil).value + await self.scheduleIndexing(of: uris, indexFilesWithUpToDateUnit: false, priority: nil).value index.pollForUnitChangesAndWait() logger.debug("Done waiting for up-to-date index") } @@ -347,7 +357,7 @@ public final actor SemanticIndexManager { await preparationUpToDateTracker.markOutOfDate(inProgressPreparationTasks.keys) } - await scheduleBackgroundIndex(files: changedFiles) + await scheduleBackgroundIndex(files: changedFiles, indexFilesWithUpToDateUnit: false) } /// Returns the files that should be indexed to get up-to-date index information for the given files. @@ -500,6 +510,7 @@ public final actor SemanticIndexManager { /// Update the index store for the given files, assuming that their targets have already been prepared. private func updateIndexStore( for filesAndTargets: [FileAndTarget], + indexFilesWithUpToDateUnit: Bool, preparationTaskID: UUID, priority: TaskPriority? ) async { @@ -509,6 +520,7 @@ public final actor SemanticIndexManager { buildSystemManager: self.buildSystemManager, index: index, indexStoreUpToDateTracker: indexStoreUpToDateTracker, + indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit, logMessageToIndexLog: logMessageToIndexLog, testHooks: testHooks ) @@ -545,6 +557,7 @@ public final actor SemanticIndexManager { /// The returned task finishes when all files are indexed. private func scheduleIndexing( of files: some Collection, + indexFilesWithUpToDateUnit: Bool, priority: TaskPriority? ) async -> Task { // Perform a quick initial check to whether the files is up-to-date, in which case we don't need to schedule a @@ -619,6 +632,7 @@ public final actor SemanticIndexManager { taskGroup.addTask { await self.updateIndexStore( for: fileBatch.map { FileAndTarget(file: $0, target: target) }, + indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit, preparationTaskID: preparationTaskID, priority: priority ) diff --git a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift index a42200d06..80a68f83b 100644 --- a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift +++ b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift @@ -105,12 +105,18 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { /// The build system manager that is used to get the toolchain and build settings for the files to index. private let buildSystemManager: BuildSystemManager - private let indexStoreUpToDateTracker: UpToDateTracker - /// A reference to the underlying index store. Used to check if the index is already up-to-date for a file, in which /// case we don't need to index it again. private let index: UncheckedIndex + private let indexStoreUpToDateTracker: UpToDateTracker + + /// Whether files that have an up-to-date unit file should be indexed. + /// + /// In general, this should be `false`. The only situation when this should be set to `true` is when the user + /// explicitly requested a re-index of all files. + private let indexFilesWithUpToDateUnit: Bool + /// See `SemanticIndexManager.logMessageToIndexLog`. private let logMessageToIndexLog: @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void @@ -139,6 +145,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { buildSystemManager: BuildSystemManager, index: UncheckedIndex, indexStoreUpToDateTracker: UpToDateTracker, + indexFilesWithUpToDateUnit: Bool, logMessageToIndexLog: @escaping @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void, testHooks: IndexTestHooks ) { @@ -146,6 +153,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { self.buildSystemManager = buildSystemManager self.index = index self.indexStoreUpToDateTracker = indexStoreUpToDateTracker + self.indexFilesWithUpToDateUnit = indexFilesWithUpToDateUnit self.logMessageToIndexLog = logMessageToIndexLog self.testHooks = testHooks } @@ -206,7 +214,9 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { // If we know that the file is up-to-date without having ot hit the index, do that because it's fastest. return } - guard !index.checked(for: .modifiedFiles).hasUpToDateUnit(for: file.sourceFile, mainFile: file.mainFile) + guard + indexFilesWithUpToDateUnit + || !index.checked(for: .modifiedFiles).hasUpToDateUnit(for: file.sourceFile, mainFile: file.mainFile) else { logger.debug("Not indexing \(file.forLogging) because index has an up-to-date unit") // We consider a file's index up-to-date if we have any up-to-date unit. Changing build settings does not diff --git a/Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift b/Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift index 9d33a6bc0..cfa730b39 100644 --- a/Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift +++ b/Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift @@ -199,6 +199,8 @@ enum MessageHandlingDependencyTracker: DependencyTracker { self = .freestanding case is ShutdownRequest: self = .globalConfigurationChange + case is TriggerReindexRequest: + self = .globalConfigurationChange case is TypeHierarchySubtypesRequest: self = .freestanding case is TypeHierarchySupertypesRequest: diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index 378641ef6..a061d1252 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -747,6 +747,8 @@ extension SourceKitLSPServer: MessageHandler { await request.reply { try await shutdown(request.params) } case let request as RequestAndReply: await self.handleRequest(for: request, requestHandler: self.symbolInfo) + case let request as RequestAndReply: + await request.reply { try await triggerReindex(request.params) } case let request as RequestAndReply: await self.handleRequest(for: request, requestHandler: self.prepareTypeHierarchy) case let request as RequestAndReply: @@ -2338,6 +2340,13 @@ extension SourceKitLSPServer { } return VoidResponse() } + + func triggerReindex(_ req: TriggerReindexRequest) async throws -> VoidResponse { + for workspace in workspaces { + await workspace.semanticIndexManager?.scheduleReindex() + } + return VoidResponse() + } } private func languageClass(for language: Language) -> [Language] { diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift index ef8d59fbf..354fa1de7 100644 --- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift +++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift @@ -1246,6 +1246,104 @@ final class BackgroundIndexingTests: XCTestCase { ) _ = try await project.testClient.send(PollIndexRequest()) } + + func testManualReindex() async throws { + // This test relies on the issue described in https://github.com/apple/sourcekit-lsp/issues/1264 that we don't + // re-index dependent files if a function of a low-level module gains a new default parameter, which changes the + // function's USR but is API compatible with all dependencies. + // Check that after running the re-index request, the index gets updated. + + let project = try await SwiftPMTestProject( + files: [ + "LibA/LibA.swift": """ + public func 1️⃣getInt() -> Int { + return 1 + } + """, + "LibB/LibB.swift": """ + import LibA + + public func 2️⃣test() -> Int { + return 3️⃣getInt() + } + """, + ], + manifest: """ + let package = Package( + name: "MyLibrary", + targets: [ + .target(name: "LibA"), + .target(name: "LibB", dependencies: ["LibA"]), + ] + ) + """, + enableBackgroundIndexing: true + ) + + let expectedCallHierarchyItem = CallHierarchyIncomingCall( + from: CallHierarchyItem( + name: "test()", + kind: .function, + tags: nil, + uri: try project.uri(for: "LibB.swift"), + range: try project.range(from: "2️⃣", to: "2️⃣", in: "LibB.swift"), + selectionRange: try project.range(from: "2️⃣", to: "2️⃣", in: "LibB.swift"), + data: .dictionary([ + "usr": .string("s:4LibB4testSiyF"), + "uri": .string(try project.uri(for: "LibB.swift").stringValue), + ]) + ), + fromRanges: [try project.range(from: "3️⃣", to: "3️⃣", in: "LibB.swift")] + ) + + /// Start by making a call hierarchy request to check that we get the expected results without any edits. + let (uri, positions) = try project.openDocument("LibA.swift") + let prepareBeforeUpdate = try await project.testClient.send( + CallHierarchyPrepareRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) + ) + let callHierarchyBeforeUpdate = try await project.testClient.send( + CallHierarchyIncomingCallsRequest(item: XCTUnwrap(prepareBeforeUpdate?.only)) + ) + XCTAssertEqual(callHierarchyBeforeUpdate, [expectedCallHierarchyItem]) + + // Now add a new default parameter to `getInt`. + project.testClient.send(DidCloseTextDocumentNotification(textDocument: TextDocumentIdentifier(uri))) + let newLibAContents = """ + public func getInt(value: Int = 1) -> Int { + return value + } + """ + try newLibAContents.write(to: XCTUnwrap(uri.fileURL), atomically: true, encoding: .utf8) + project.testClient.send( + DidOpenTextDocumentNotification( + textDocument: TextDocumentItem(uri: uri, language: .swift, version: 0, text: newLibAContents) + ) + ) + project.testClient.send(DidChangeWatchedFilesNotification(changes: [FileEvent(uri: uri, type: .changed)])) + _ = try await project.testClient.send(PollIndexRequest()) + + // The USR of `getInt` has changed but LibB.swift has not been re-indexed due to + // https://github.com/apple/sourcekit-lsp/issues/1264. We expect to get an empty call hierarchy. + let prepareAfterUpdate = try await project.testClient.send( + CallHierarchyPrepareRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) + ) + let callHierarchyAfterUpdate = try await project.testClient.send( + CallHierarchyIncomingCallsRequest(item: XCTUnwrap(prepareAfterUpdate?.only)) + ) + XCTAssertEqual(callHierarchyAfterUpdate, []) + + // After re-indexing, we expect to get a full call hierarchy again. + _ = try await project.testClient.send(TriggerReindexRequest()) + _ = try await project.testClient.send(PollIndexRequest()) + + let prepareAfterReindex = try await project.testClient.send( + CallHierarchyPrepareRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) + ) + let callHierarchyAfterReindex = try await project.testClient.send( + CallHierarchyIncomingCallsRequest(item: XCTUnwrap(prepareAfterReindex?.only)) + ) + XCTAssertEqual(callHierarchyAfterReindex, [expectedCallHierarchyItem]) + } } extension HoverResponseContents {