From 4d90f40f7219100cbcf46916f581178012acaa7a Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen@apple.com>
Date: Fri, 21 Jun 2024 07:19:57 -0700
Subject: [PATCH] Watch for changes to `Package.resolved`

We need to watch for changes to `Package.resolved` so that we can update the dependency checkouts in `.index-build` when the user runs `swift package update`.
---
 .../SwiftPMBuildSystem.swift                  |   2 +-
 .../RepeatUntilExpectedResult.swift           |  38 ++++++
 .../SwiftPMDependencyProject.swift            |  11 +-
 Sources/SourceKitLSP/SourceKitLSPServer.swift |   1 +
 .../BackgroundIndexingTests.swift             | 126 +++++++++++++++++-
 .../SourceKitLSPTests/BuildSystemTests.swift  |   9 +-
 .../MainFilesProviderTests.swift              |  12 +-
 7 files changed, 179 insertions(+), 20 deletions(-)
 create mode 100644 Sources/SKTestSupport/RepeatUntilExpectedResult.swift

diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
index 1267add64..53cbc7e83 100644
--- a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
+++ b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
@@ -691,7 +691,7 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
         packageGraph: self.modulesGraph
       )
     case .changed:
-      return fileURL.lastPathComponent == "Package.swift"
+      return fileURL.lastPathComponent == "Package.swift" || fileURL.lastPathComponent == "Package.resolved"
     default:  // Unknown file change type
       return false
     }
diff --git a/Sources/SKTestSupport/RepeatUntilExpectedResult.swift b/Sources/SKTestSupport/RepeatUntilExpectedResult.swift
new file mode 100644
index 000000000..1f2c52045
--- /dev/null
+++ b/Sources/SKTestSupport/RepeatUntilExpectedResult.swift
@@ -0,0 +1,38 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2020 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 LSPTestSupport
+import XCTest
+
+/// Runs the body repeatedly once per second until it returns `true`, giving up after `timeout`.
+///
+/// This is useful to test some request that requires global state to be updated but will eventually converge on the
+/// correct result.
+///
+/// If `bodyHasOneSecondDelay` is true, it is assume that the body already has a one-second delay between iterations.
+public func repeatUntilExpectedResult(
+  _ body: () async throws -> Bool,
+  bodyHasOneSecondDelay: Bool = false,
+  timeout: TimeInterval = defaultTimeout,
+  file: StaticString = #filePath,
+  line: UInt = #line
+) async throws {
+  for _ in 0..<Int(timeout) {
+    if try await body() {
+      return
+    }
+    if !bodyHasOneSecondDelay {
+      try await Task.sleep(for: .seconds(1))
+    }
+  }
+  XCTFail("Failed to get expected result", file: file, line: line)
+}
diff --git a/Sources/SKTestSupport/SwiftPMDependencyProject.swift b/Sources/SKTestSupport/SwiftPMDependencyProject.swift
index e1ef7b6b1..ec7bfecec 100644
--- a/Sources/SKTestSupport/SwiftPMDependencyProject.swift
+++ b/Sources/SKTestSupport/SwiftPMDependencyProject.swift
@@ -86,15 +86,20 @@ public class SwiftPMDependencyProject {
     }
 
     try await runGitCommand(["init"], workingDirectory: packageDirectory)
+    try await tag(changedFiles: files.keys.map { $0.url(relativeTo: packageDirectory) }, version: "1.0.0")
+  }
+
+  public func tag(changedFiles: [URL], version: String) async throws {
     try await runGitCommand(
-      ["add"] + files.keys.map { $0.url(relativeTo: packageDirectory).path },
+      ["add"] + changedFiles.map(\.path),
       workingDirectory: packageDirectory
     )
     try await runGitCommand(
-      ["-c", "user.name=Dummy", "-c", "user.email=noreply@swift.org", "commit", "-m", "Initial commit"],
+      ["-c", "user.name=Dummy", "-c", "user.email=noreply@swift.org", "commit", "-m", "Version \(version)"],
       workingDirectory: packageDirectory
     )
-    try await runGitCommand(["tag", "1.0.0"], workingDirectory: packageDirectory)
+
+    try await runGitCommand(["tag", version], workingDirectory: self.packageDirectory)
   }
 
   deinit {
diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift
index 4add2a90e..8feb64e60 100644
--- a/Sources/SourceKitLSP/SourceKitLSPServer.swift
+++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift
@@ -1189,6 +1189,7 @@ extension SourceKitLSPServer {
       return FileSystemWatcher(globPattern: "**/*.\(fileExtension)", kind: [.create, .change, .delete])
     }
     watchers.append(FileSystemWatcher(globPattern: "**/Package.swift", kind: [.change]))
+    watchers.append(FileSystemWatcher(globPattern: "**/Package.resolved", kind: [.change]))
     watchers.append(FileSystemWatcher(globPattern: "**/compile_commands.json", kind: [.create, .change, .delete]))
     watchers.append(FileSystemWatcher(globPattern: "**/compile_flags.txt", kind: [.create, .change, .delete]))
     // Watch for changes to `.swiftmodule` files to detect updated modules during a build.
diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift
index bd657aab5..255f60dd6 100644
--- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift
+++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift
@@ -12,12 +12,14 @@
 
 import LSPTestSupport
 import LanguageServerProtocol
-import SKCore
+@_spi(Testing) import SKCore
 import SKTestSupport
 import SemanticIndex
 import SourceKitLSP
 import XCTest
 
+import class TSCBasic.Process
+
 final class BackgroundIndexingTests: XCTestCase {
   func testBackgroundIndexingOfSingleFile() async throws {
     let project = try await SwiftPMTestProject(
@@ -1095,4 +1097,126 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual(response, .locations([try project.location(from: "1️⃣", to: "1️⃣", in: "LibB.swift")]))
   }
+
+  func testUpdatePackageDependency() async throws {
+    try SkipUnless.longTestsEnabled()
+
+    let dependencyProject = try await SwiftPMDependencyProject(files: [
+      "Sources/MyDependency/Dependency.swift": """
+      /// Do something v1.0.0
+      public func doSomething() {}
+      """
+    ])
+    let dependencySwiftURL = dependencyProject.packageDirectory
+      .appendingPathComponent("Sources")
+      .appendingPathComponent("MyDependency")
+      .appendingPathComponent("Dependency.swift")
+    defer { dependencyProject.keepAlive() }
+
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Test.swift": """
+        import MyDependency
+
+        func test() {
+          1️⃣doSomething()
+        }
+        """
+      ],
+      manifest: """
+        let package = Package(
+          name: "MyLibrary",
+          dependencies: [.package(url: "\(dependencyProject.packageDirectory)", from: "1.0.0")],
+          targets: [
+            .target(
+              name: "MyLibrary",
+              dependencies: [.product(name: "MyDependency", package: "MyDependency")]
+            )
+          ]
+        )
+        """,
+      enableBackgroundIndexing: true
+    )
+    let packageResolvedURL = project.scratchDirectory.appendingPathComponent("Package.resolved")
+
+    let originalPackageResolvedContents = try String(contentsOf: packageResolvedURL)
+
+    // First check our setup to see that we get the expected hover response before changing the dependency project.
+    let (uri, positions) = try project.openDocument("Test.swift")
+    let hoverBeforeUpdate = try await project.testClient.send(
+      HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
+    )
+    XCTAssert(
+      hoverBeforeUpdate?.contents.markupContent?.value.contains("Do something v1.0.0") ?? false,
+      "Did not contain expected string: \(String(describing: hoverBeforeUpdate))"
+    )
+
+    // Just committing a new version of the dependency shouldn't change anything because we didn't update the package
+    // dependencies.
+    try """
+    /// Do something v1.1.0
+    public func doSomething() {}
+    """.write(to: dependencySwiftURL, atomically: true, encoding: .utf8)
+    try await dependencyProject.tag(changedFiles: [dependencySwiftURL], version: "1.1.0")
+
+    let hoverAfterNewVersionCommit = try await project.testClient.send(
+      HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
+    )
+    XCTAssert(
+      hoverAfterNewVersionCommit?.contents.markupContent?.value.contains("Do something v1.0.0") ?? false,
+      "Did not contain expected string: \(String(describing: hoverBeforeUpdate))"
+    )
+
+    // Updating Package.swift causes a package reload but should not cause dependencies to be updated.
+    project.testClient.send(
+      DidChangeWatchedFilesNotification(changes: [
+        FileEvent(uri: DocumentURI(project.scratchDirectory.appendingPathComponent("Package.resolved")), type: .changed)
+      ])
+    )
+    _ = try await project.testClient.send(PollIndexRequest())
+    XCTAssertEqual(try String(contentsOf: packageResolvedURL), originalPackageResolvedContents)
+
+    // Simulate a package update which goes as follows:
+    //  - The user runs `swift package update`
+    //  - This updates `Package.resolved`, which we watch
+    //  - We reload the package, which updates `Dependency.swift` in `.index-build/checkouts`, which we also watch.
+    try await Process.run(
+      arguments: [
+        unwrap(ToolchainRegistry.forTesting.default?.swift?.pathString),
+        "package", "update",
+        "--package-path", project.scratchDirectory.path,
+      ],
+      workingDirectory: nil
+    )
+    XCTAssertNotEqual(try String(contentsOf: packageResolvedURL), originalPackageResolvedContents)
+    project.testClient.send(
+      DidChangeWatchedFilesNotification(changes: [
+        FileEvent(uri: DocumentURI(project.scratchDirectory.appendingPathComponent("Package.resolved")), type: .changed)
+      ])
+    )
+    _ = try await project.testClient.send(PollIndexRequest())
+    project.testClient.send(
+      DidChangeWatchedFilesNotification(
+        changes: FileManager.default.findFiles(named: "Dependency.swift", in: project.scratchDirectory).map {
+          FileEvent(uri: DocumentURI($0), type: .changed)
+        }
+      )
+    )
+
+    try await repeatUntilExpectedResult {
+      let hoverAfterPackageUpdate = try await project.testClient.send(
+        HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])
+      )
+      return hoverAfterPackageUpdate?.contents.markupContent?.value.contains("Do something v1.1.0") ?? false
+    }
+  }
+}
+
+extension HoverResponseContents {
+  var markupContent: MarkupContent? {
+    switch self {
+    case .markupContent(let markupContent): return markupContent
+    default: return nil
+    }
+  }
 }
diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift
index 53a79893b..2d7983866 100644
--- a/Tests/SourceKitLSPTests/BuildSystemTests.swift
+++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift
@@ -187,15 +187,10 @@ final class BuildSystemTests: XCTestCase {
 
     await buildSystem.delegate?.fileBuildSettingsChanged([doc])
 
-    var receivedCorrectDiagnostic = false
-    for _ in 0..<Int(defaultTimeout) {
+    try await repeatUntilExpectedResult {
       let refreshedDiags = try await testClient.nextDiagnosticsNotification(timeout: .seconds(1))
-      if refreshedDiags.diagnostics.count == 0, try text == documentManager.latestSnapshot(doc).text {
-        receivedCorrectDiagnostic = true
-        break
-      }
+      return try text == documentManager.latestSnapshot(doc).text && refreshedDiags.diagnostics.count == 0
     }
-    XCTAssert(receivedCorrectDiagnostic)
   }
 
   func testSwiftDocumentUpdatedBuildSettings() async throws {
diff --git a/Tests/SourceKitLSPTests/MainFilesProviderTests.swift b/Tests/SourceKitLSPTests/MainFilesProviderTests.swift
index 4c71b7842..f2f13405a 100644
--- a/Tests/SourceKitLSPTests/MainFilesProviderTests.swift
+++ b/Tests/SourceKitLSPTests/MainFilesProviderTests.swift
@@ -201,16 +201,12 @@ final class MainFilesProviderTests: XCTestCase {
     // `clangd` may return diagnostics from the old build settings sometimes (I believe when it's still building the
     // preamble for shared.h when the new build settings come in). Check that it eventually returns the correct
     // diagnostics.
-    var receivedCorrectDiagnostic = false
-    for _ in 0..<Int(defaultTimeout) {
+    try await repeatUntilExpectedResult {
       let refreshedDiags = try await project.testClient.nextDiagnosticsNotification(timeout: .seconds(1))
-      if let diagnostic = refreshedDiags.diagnostics.only,
-        diagnostic.message == "Unused variable 'fromMyFancyLibrary'"
-      {
-        receivedCorrectDiagnostic = true
-        break
+      guard let diagnostic = refreshedDiags.diagnostics.only else {
+        return false
       }
+      return diagnostic.message == "Unused variable 'fromMyFancyLibrary'"
     }
-    XCTAssert(receivedCorrectDiagnostic)
   }
 }