Improve package reloading logic when manifest is changed

Two improvements to the logic that determines if we need to reload the package on a file change:
1. Only reload a package if the modified package manifest or Package.resolved is in the root of the package folder. This is relevant if we have a multi-root workspace, in which currently a single change to a package manifest or Package.resolved causes all packages to get reloaded.
2. While I was at it, I noticed that we didn’t properly handle version-specific package manifests. Also trigger a package reload if a version-specific package manifest is modified, created or deleted.

Author: ahoppen

Sources/BuildServerIntegration/SwiftPMBuildServer.swift

@@ -137,8 +137,12 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
 
   private var targetDependencies: [BuildTargetIdentifier: Set<BuildTargetIdentifier>] = [:]
 
-  static package func searchForConfig(in path: URL, options: SourceKitLSPOptions) -> BuildServerSpec? {
+  /// Regular expression that matches version-specific package manifest file names such as [email protected]
+  private static var versionSpecificPackageManifestNameRegex: Regex<(Substring, Substring, Substring?, Substring?)> {
+    #/^Package@swift-(\d+)(?:\.(\d+))?(?:\.(\d+))?.swift$/#
+  }
 
+  static package func searchForConfig(in path: URL, options: SourceKitLSPOptions) -> BuildServerSpec? {
     let packagePath = path.appendingPathComponent("Package.swift")
     if (try? String(contentsOf: packagePath, encoding: .utf8))?.contains("PackageDescription") ?? false {
       return BuildServerSpec(kind: .swiftPM, projectRoot: path, configPath: packagePath)
@@ -543,12 +547,11 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
     // (https://github.com/swiftlang/sourcekit-lsp/issues/1267)
     for target in request.targets {
       if target == .forPackageManifest {
-        let packageManifestName = #/^Package@swift-(\d+)(?:\.(\d+))?(?:\.(\d+))?.swift$/#
         let versionSpecificManifests = try? FileManager.default.contentsOfDirectory(
           at: projectRoot,
           includingPropertiesForKeys: nil
         ).compactMap { (url) -> SourceItem? in
-          guard (try? packageManifestName.wholeMatch(in: url.lastPathComponent)) != nil else {
+          guard (try? Self.versionSpecificPackageManifestNameRegex.wholeMatch(in: url.lastPathComponent)) != nil else {
             return nil
           }
           return SourceItem(
@@ -806,11 +809,28 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
     }
   }
 
+  private func isPackageManifestOrPackageResolved(_ url: URL) -> Bool {
+    guard url.lastPathComponent.contains("Package") else {
+      // Fast check to early exit for files that don't like a package manifest or Package.resolved
+      return false
+    }
+    guard
+      url.lastPathComponent == "Package.resolved" || url.lastPathComponent == "Package.swift"
+        || (try? Self.versionSpecificPackageManifestNameRegex.wholeMatch(in: url.lastPathComponent)) != nil
+    else {
+      return false
+    }
+    return url.deletingLastPathComponent() == self.projectRoot
+  }
+
   /// An event is relevant if it modifies a file that matches one of the file rules used by the SwiftPM workspace.
   private func fileEventShouldTriggerPackageReload(event: FileEvent) -> Bool {
     guard let fileURL = event.uri.fileURL else {
       return false
     }
+    if isPackageManifestOrPackageResolved(fileURL) {
+      return true
+    }
     switch event.type {
     case .created, .deleted:
       guard let buildDescription else {
@@ -819,7 +839,8 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
 
       return buildDescription.fileAffectsSwiftOrClangBuildSettings(fileURL)
     case .changed:
-      return fileURL.lastPathComponent == "Package.swift" || fileURL.lastPathComponent == "Package.resolved"
+      // Only modified package manifests should trigger a package reload and that's handled above.
+      return false
     default:  // Unknown file change type
       return false
     }

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -1325,7 +1325,7 @@ final class BackgroundIndexingTests: XCTestCase {
     // 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)
+        FileEvent(uri: DocumentURI(project.scratchDirectory.appendingPathComponent("Package.swift")), type: .changed)
       ])
     )
     try await project.testClient.send(SynchronizeRequest(index: true))
@@ -1360,14 +1360,19 @@ final class BackgroundIndexingTests: XCTestCase {
       ])
     )
     try await project.testClient.send(SynchronizeRequest(index: true))
+    let dependencyCheckoutFile = try XCTUnwrap(
+      FileManager.default.findFiles(
+        named: "Dependency.swift",
+        in: project.scratchDirectory
+          .appendingPathComponent(".build")
+          .appendingPathComponent("index-build")
+          .appendingPathComponent("checkouts")
+      ).only
+    )
+    // Check that modifying Package.resolved actually modified the dependency checkout inside the package
+    assertContains(try String(contentsOf: dependencyCheckoutFile, encoding: .utf8), "Do something v1.1.0")
     project.testClient.send(
-      DidChangeWatchedFilesNotification(
-        changes: FileManager.default.findFiles(
-          named: "Dependency.swift",
-          in: project.scratchDirectory.appendingPathComponent(".build").appendingPathComponent("index-build")
-            .appendingPathComponent("checkouts")
-        ).map { FileEvent(uri: DocumentURI($0), type: .changed) }
-      )
+      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: DocumentURI(dependencyCheckoutFile), type: .changed)])
     )
 
     try await repeatUntilExpectedResult {

Tests/SourceKitLSPTests/SwiftPMIntegrationTests.swift

@@ -561,4 +561,156 @@ final class SwiftPMIntegrationTests: XCTestCase {
       return location.range.lowerBound == Position(line: 3, utf16index: 4)
     }
   }
+
+  func testChangePackageManifestFile() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Lib.swift": """
+        #if MY_FLAG
+        #error("MY_FLAG set")
+        #else
+        #error("MY_FLAG not set")
+        #endif
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary")]
+        )
+        """
+    )
+
+    let (uri, _) = try project.openDocument("Lib.swift")
+    let initialDiagnostics = try await project.testClient.send(
+      DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+    )
+    XCTAssertEqual(initialDiagnostics.fullReport?.items.map(\.message), ["MY_FLAG not set"])
+
+    try await project.changeFileOnDisk(
+      "Package.swift",
+      newMarkedContents: """
+        // swift-tools-version: 5.7
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary", swiftSettings: [.define("MY_FLAG")])]
+        )
+        """
+    )
+    try await repeatUntilExpectedResult {
+      let diagnosticsAfterUpdate = try await project.testClient.send(
+        DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+      )
+      return diagnosticsAfterUpdate.fullReport?.items.map(\.message) == ["MY_FLAG set"]
+    }
+  }
+
+  func testChangeVersionSpecificPackageManifestFile() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Lib.swift": """
+        #if MY_FLAG
+        #error("MY_FLAG set")
+        #elseif MY_OTHER_FLAG
+        #error("MY_OTHER_FLAG set")
+        #else
+        #error("no flag set")
+        #endif
+        """,
+        "/[email protected]": """
+        // swift-tools-version: 6.1
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary", swiftSettings: [.define("MY_FLAG")])]
+        )
+        """,
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary")]
+        )
+        """
+    )
+
+    let (uri, _) = try project.openDocument("Lib.swift")
+    let initialDiagnostics = try await project.testClient.send(
+      DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+    )
+    XCTAssertEqual(initialDiagnostics.fullReport?.items.map(\.message), ["MY_FLAG set"])
+
+    try await project.changeFileOnDisk(
+      "[email protected]",
+      newMarkedContents: """
+        // swift-tools-version: 6.1
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary", swiftSettings: [.define("MY_OTHER_FLAG")])]
+        )
+        """
+    )
+    try await repeatUntilExpectedResult {
+      let diagnosticsAfterUpdate = try await project.testClient.send(
+        DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+      )
+      return diagnosticsAfterUpdate.fullReport?.items.map(\.message) == ["MY_OTHER_FLAG set"]
+    }
+  }
+
+  func testAddVersionSpecificPackageManifestFile() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Lib.swift": """
+        #if MY_FLAG
+        #error("MY_FLAG set")
+        #else
+        #error("MY_FLAG not set")
+        #endif
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+        import PackageDescription
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.target(name: "MyLibrary")]
+        )
+        """
+    )
+
+    let (uri, _) = try project.openDocument("Lib.swift")
+    let initialDiagnostics = try await project.testClient.send(
+      DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+    )
+    XCTAssertEqual(initialDiagnostics.fullReport?.items.map(\.message), ["MY_FLAG not set"])
+
+    let versionSpecificManifestUrl = project.scratchDirectory.appending(component: "[email protected]")
+    try await """
+    // swift-tools-version: 6.1
+    import PackageDescription
+    let package = Package(
+      name: "MyLibrary",
+      targets: [.target(name: "MyLibrary", swiftSettings: [.define("MY_FLAG")])]
+    )
+    """.writeWithRetry(to: versionSpecificManifestUrl)
+
+    project.testClient.send(
+      DidChangeWatchedFilesNotification(changes: [
+        FileEvent(uri: DocumentURI(versionSpecificManifestUrl), type: .created)
+      ])
+    )
+    try await repeatUntilExpectedResult {
+      let diagnosticsAfterUpdate = try await project.testClient.send(
+        DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri))
+      )
+      return diagnosticsAfterUpdate.fullReport?.items.map(\.message) == ["MY_FLAG set"]
+    }
+  }
 }