Merge pull request #212 from DavidGoldman/dependencyhandling

Work on implementing dependenciesUpdated part of Build System integration

Author: benlangmuir

Sources/SKCore/BuildSystemDelegate.swift

@@ -21,9 +21,16 @@ public protocol BuildSystemDelegate: AnyObject {
   /// interest.
   func buildTargetsChanged(_ changes: [BuildTargetEvent])
 
-  /// Notify the delegate that the given files' build settings have changed.
+  /// Notify the delegate that the given files' build settings have changed. If the given set is
+  /// empty, assume that all open files are affected.
   ///
   /// The callee should request new build settings for any of the given files
   /// that they are interested in.
   func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>)
+
+  /// Notify the delegate that the dependencies of the given files have changed and that ASTs
+  /// may need to be refreshed. If the given set is empty, assume that all open files are affected.
+  ///
+  /// The callee should refresh ASTs unless it is able to determine that a refresh is not necessary.
+  func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>)
 }

Sources/SKTestSupport/SKTibsTestWorkspace.swift

@@ -121,6 +121,34 @@ extension XCTestCase {
 
     return workspace
   }
+
+  /// Constructs a mutable SKTibsTestWorkspace for the given test from INPUTS.
+  ///
+  /// The resulting workspace allow edits and is not persistent.
+  public func mutableSourceKitTibsTestWorkspace(
+    name: String,
+    clientCapabilities: ClientCapabilities = .init(),
+    tmpDir: URL? = nil,
+    testFile: String = #file
+  ) throws -> SKTibsTestWorkspace? {
+    let testDirName = testDirectoryName
+    let workspace = try SKTibsTestWorkspace(
+      projectDir: inputsDirectory(testFile: testFile)
+        .appendingPathComponent(name, isDirectory: true),
+      tmpDir: tmpDir ?? URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true)
+        .appendingPathComponent("sk-test-data/\(testDirName)", isDirectory: true),
+      toolchain: ToolchainRegistry.shared.default!,
+      clientCapabilities: clientCapabilities)
+
+    if workspace.builder.targets.contains(where: { target in !target.clangTUs.isEmpty })
+      && !workspace.builder.toolchain.clangHasIndexSupport {
+      fputs("warning: skipping test because '\(workspace.builder.toolchain.clang.path)' does not " +
+            "have indexstore support; use swift-clang\n", stderr)
+      return nil
+    }
+
+    return workspace
+  }
 }
 
 extension TestLocation {

Sources/SourceKit/SourceKitServer.swift

@@ -245,24 +245,44 @@ extension SourceKitServer: BuildSystemDelegate {
     // TODO: do something with these changes once build target support is in place
   }
 
+  private func affectedOpenDocumentsForChangeSet(
+    _ changes: Set<DocumentURI>,
+    _ documentManager: DocumentManager
+  ) -> Set<DocumentURI> {
+    // An empty change set is treated as if all open files have been modified.
+    guard !changes.isEmpty else {
+      return documentManager.openDocuments
+    }
+    return documentManager.openDocuments.intersection(changes)
+  }
+
   public func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) {
     guard let workspace = self.workspace else {
       return
     }
     let documentManager = workspace.documentManager
-    let openDocuments = documentManager.openDocuments
-    for uri in changedFiles {
-      guard openDocuments.contains(uri) else {
-        continue
-      }
-
+    for uri in self.affectedOpenDocumentsForChangeSet(changedFiles, documentManager) {
       log("Build settings changed for opened file \(uri)")
       if let snapshot = documentManager.latestSnapshot(uri),
         let service = languageService(for: uri, snapshot.document.language, in: workspace) {
         service.documentUpdatedBuildSettings(uri, language: snapshot.document.language)
       }
     }
   }
+
+  public func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {
+    guard let workspace = self.workspace else {
+      return
+    }
+    let documentManager = workspace.documentManager
+    for uri in self.affectedOpenDocumentsForChangeSet(changedFiles, documentManager) {
+      log("Dependencies updated for opened file \(uri)")
+      if let snapshot = documentManager.latestSnapshot(uri),
+        let service = languageService(for: uri, snapshot.document.language, in: workspace) {
+        service.documentDependenciesUpdated(uri, language: snapshot.document.language)
+      }
+    }
+  }
 }
 
 // MARK: - Request and notification handling

Sources/SourceKit/ToolchainLanguageServer.swift

@@ -28,7 +28,11 @@ public protocol ToolchainLanguageServer: AnyObject {
   func changeDocument(_ note: DidChangeTextDocumentNotification)
   func willSaveDocument(_ note: WillSaveTextDocumentNotification)
   func didSaveDocument(_ note: DidSaveTextDocumentNotification)
+
+  // MARK: - Build System Integration
+
   func documentUpdatedBuildSettings(_ uri: DocumentURI, language: Language)
+  func documentDependenciesUpdated(_ uri: DocumentURI, language: Language)
 
   // MARK: - Text Document
 

Sources/SourceKit/clangd/ClangLanguageServer.swift

@@ -106,6 +106,8 @@ extension ClangLanguageServerShim {
 
   }
 
+  // MARK: - Build System Integration
+
   public func documentUpdatedBuildSettings(_ uri: DocumentURI, language: Language) {
     guard let url = uri.fileURL else {
       // FIXME: The clang workspace can probably be reworked to support non-file URIs.
@@ -126,6 +128,15 @@ extension ClangLanguageServerShim {
     }
   }
 
+  public func documentDependenciesUpdated(_ uri: DocumentURI, language: Language) {
+    // In order to tell clangd to reload an AST, we send it an empty `didChangeTextDocument`. This
+    // works well for us as the moment since clangd ignores the document version.
+    let note = DidChangeTextDocumentNotification(
+      textDocument: VersionedTextDocumentIdentifier(uri, version: nil),
+      contentChanges: [])
+    clangd.send(note)
+  }
+
   // MARK: - Text Document
 
   func completion(_ req: Request<CompletionRequest>) {

Sources/SourceKit/sourcekitd/SwiftLanguageServer.swift

@@ -177,7 +177,32 @@ extension SwiftLanguageServer {
     onExit()
   }
 
-  // MARK: - Workspace
+  // MARK: - Build System Integration
+
+  /// Should be called on self.queue.
+  private func reopenDocument(_ snapshot: DocumentSnapshot, _ settings: FileBuildSettings?) {
+    let keys = self.keys
+    let path = snapshot.document.uri.pseudoPath
+
+    let closeReq = SKRequestDictionary(sourcekitd: self.sourcekitd)
+    closeReq[keys.request] = self.requests.editor_close
+    closeReq[keys.name] = path
+    _ = self.sourcekitd.sendSync(closeReq)
+
+    let openReq = SKRequestDictionary(sourcekitd: self.sourcekitd)
+    openReq[keys.request] = self.requests.editor_open
+    openReq[keys.name] = path
+    openReq[keys.sourcetext] = snapshot.text
+    if let settings = settings {
+      openReq[keys.compilerargs] = settings.compilerArguments
+    }
+
+    guard let dict = self.sourcekitd.sendSync(openReq).success else {
+      // Already logged failure.
+      return
+    }
+    self.publishDiagnostics(response: dict, for: snapshot)
+  }
 
   public func documentUpdatedBuildSettings(_ uri: DocumentURI, language: Language) {
     self.queue.async {
@@ -193,28 +218,21 @@ extension SwiftLanguageServer {
       }
       self.buildSettingsByFile[uri] = newSettings
 
-      let keys = self.keys
-
       // Close and re-open the document internally to inform sourcekitd to
       // update the settings. At the moment there's no better way to do this.
-      let closeReq = SKRequestDictionary(sourcekitd: self.sourcekitd)
-      closeReq[keys.request] = self.requests.editor_close
-      closeReq[keys.name] = uri.pseudoPath
-      _ = self.sourcekitd.sendSync(closeReq)
-
-      let openReq = SKRequestDictionary(sourcekitd: self.sourcekitd)
-      openReq[keys.request] = self.requests.editor_open
-      openReq[keys.name] = uri.pseudoPath
-      openReq[keys.sourcetext] = snapshot.text
-      if let settings = newSettings {
-        openReq[keys.compilerargs] = settings.compilerArguments
-      }
+      self.reopenDocument(snapshot, newSettings)
+    }
+  }
 
-      guard let dict = self.sourcekitd.sendSync(openReq).success else {
-        // Already logged failure.
+  public func documentDependenciesUpdated(_ uri: DocumentURI, language: Language) {
+    self.queue.async {
+      guard let snapshot = self.documentManager.latestSnapshot(uri) else {
         return
       }
-      self.publishDiagnostics(response: dict, for: snapshot)
+
+      // Forcefully reopen the document since the `BuildSystem` has informed us
+      // that the dependencies have changed and the AST needs to be reloaded.
+      self.reopenDocument(snapshot, self.buildSettingsByFile[uri])
     }
   }
 

Tests/SKCoreTests/BuildServerBuildSystemTests.swift

@@ -57,7 +57,7 @@ final class BuildServerBuildSystemTests: XCTestCase {
 
     let fileUrl = URL(fileURLWithPath: "/some/file/path")
     let expectation = XCTestExpectation(description: "\(fileUrl) settings updated")
-    let buildSystemDelegate = TestDelegate(fileExpectations: [DocumentURI(fileUrl): expectation])
+    let buildSystemDelegate = TestDelegate(settingsExpectations: [DocumentURI(fileUrl): expectation])
     buildSystem.delegate = buildSystemDelegate
     buildSystem.registerForChangeNotifications(for: DocumentURI(fileUrl))
 
@@ -185,13 +185,16 @@ final class BuildServerBuildSystemTests: XCTestCase {
 
 final class TestDelegate: BuildSystemDelegate {
 
-  let fileExpectations: [DocumentURI:XCTestExpectation]
+  let settingsExpectations: [DocumentURI:XCTestExpectation]
   let targetExpectations: [BuildTargetEvent:XCTestExpectation]
+  let dependenciesUpdatedExpectations: [DocumentURI:XCTestExpectation]
 
-  public init(fileExpectations: [DocumentURI:XCTestExpectation] = [:],
-              targetExpectations: [BuildTargetEvent:XCTestExpectation] = [:]) {
-    self.fileExpectations = fileExpectations
+  public init(settingsExpectations: [DocumentURI:XCTestExpectation] = [:],
+              targetExpectations: [BuildTargetEvent:XCTestExpectation] = [:],
+              dependenciesUpdatedExpectations: [DocumentURI:XCTestExpectation] = [:]) {
+    self.settingsExpectations = settingsExpectations
     self.targetExpectations = targetExpectations
+    self.dependenciesUpdatedExpectations = dependenciesUpdatedExpectations
   }
 
   func buildTargetsChanged(_ changes: [BuildTargetEvent]) {
@@ -202,7 +205,13 @@ final class TestDelegate: BuildSystemDelegate {
 
   func fileBuildSettingsChanged(_ changedFiles: Set<DocumentURI>) {
     for uri in changedFiles {
-      fileExpectations[uri]?.fulfill()
+      settingsExpectations[uri]?.fulfill()
+    }
+  }
+
+  public func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {
+    for uri in changedFiles {
+      dependenciesUpdatedExpectations[uri]?.fulfill()
     }
   }
 }

Tests/SourceKitTests/SourceKitTests.swift

@@ -182,4 +182,57 @@ final class SKTests: XCTestCase {
         deprecated: false),
     ]))
   }
+
+  func testDependenciesUpdatedSwiftTibs() throws {
+    guard let ws = try mutableSourceKitTibsTestWorkspace(name: "SwiftModules") else { return }
+    guard let server = ws.testServer.server else {
+      XCTFail("Unable to fetch SourceKitServer to notify for build system events.")
+      return
+    }
+
+    let moduleRef = ws.testLoc("aaa:call:c")
+    let startExpectation = XCTestExpectation(description: "initial diagnostics")
+    startExpectation.expectedFulfillmentCount = 2
+    ws.sk.handleNextNotification { (note: Notification<PublishDiagnosticsNotification>) in
+      // Semantic analysis: no errors expected here.
+      XCTAssertEqual(note.params.diagnostics.count, 0)
+      startExpectation.fulfill()
+    }
+    ws.sk.appendOneShotNotificationHandler  { (note: Notification<PublishDiagnosticsNotification>) in
+      // Semantic analysis: expect module import error.
+      XCTAssertEqual(note.params.diagnostics.count, 1)
+      if let diagnostic = note.params.diagnostics.first {
+        XCTAssert(diagnostic.message.contains("no such module"),
+                  "expected module import error but found \"\(diagnostic.message)\"")
+      }
+      startExpectation.fulfill()
+    }
+
+    try ws.openDocument(moduleRef.url, language: .swift)
+    let started = XCTWaiter.wait(for: [startExpectation], timeout: 3)
+    if started != .completed {
+      fatalError("error \(started) waiting for initial diagnostics notification")
+    }
+
+    try ws.buildAndIndex()
+
+    let finishExpectation = XCTestExpectation(description: "post-build diagnostics")
+    finishExpectation.expectedFulfillmentCount = 2
+    ws.sk.handleNextNotification { (note: Notification<PublishDiagnosticsNotification>) in
+      // Semantic analysis - SourceKit currently caches diagnostics so we still see an error.
+      XCTAssertEqual(note.params.diagnostics.count, 1)
+      finishExpectation.fulfill()
+    }
+    ws.sk.appendOneShotNotificationHandler  { (note: Notification<PublishDiagnosticsNotification>) in
+      // Semantic analysis: no more errors expected, import should resolve since we built.
+      XCTAssertEqual(note.params.diagnostics.count, 0)
+      finishExpectation.fulfill()
+    }
+    server.filesDependenciesUpdated([DocumentURI(moduleRef.url)])
+
+    let finished = XCTWaiter.wait(for: [finishExpectation], timeout: 3)
+    if finished != .completed {
+      fatalError("error \(finished) waiting for post-build diagnostics notification")
+    }
+  }
 }

Tests/SourceKitTests/XCTestManifests.swift

@@ -122,6 +122,7 @@ extension SKTests {
     // to regenerate.
     static let __allTests__SKTests = [
         ("testCodeCompleteSwiftTibs", testCodeCompleteSwiftTibs),
+        ("testDependenciesUpdatedSwiftTibs", testDependenciesUpdatedSwiftTibs),
         ("testIndexShutdown", testIndexShutdown),
         ("testIndexSwiftModules", testIndexSwiftModules),
         ("testInitJSON", testInitJSON),