[Workspace] Improve how we handle changes to Package.resolved

This improves our resolution logic when we already have state and there
are updates to Package.resolved file. Previously, we were restoring the
Package.resolved file with our current state if we saw that there is
a missing entry in there. This is not correct since dependencies can
disappear as part of an update performed on some other machine (or just
switching between commits/branches without removing state).

<rdar://problem/51080676> [SR-10718]

Sources/Workspace/PinsStore.swift

@@ -99,6 +99,10 @@ public final class PinsStore {
         pinsMap[pin.packageRef.identity] = pin
     }
 
+    func remove(identity: String) {
+        pinsMap[identity] = nil
+    }
+
     /// Pin a managed dependency at its checkout state.
     ///
     /// This method does nothing if the dependency is in edited state.

Sources/Workspace/Workspace.swift

@@ -1236,21 +1236,27 @@ extension Workspace {
             return currentManifests
         }
 
-        validatePinsStore(dependencyManifests: currentManifests, diagnostics: diagnostics)
-
-        // Abort if pinsStore is unloadable or if diagnostics has errors.
-        guard !diagnostics.hasErrors, let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else {
+        // Load the pinstore and check if its outdated.
+        guard let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else {
             return currentManifests
         }
+        let isPinStoreOutdated = pinsStore.isOutdated(
+            dependencyManifests: currentManifests,
+            managedDependencies: managedDependencies
+        )
 
-        // Compute the missing package identities.
+        // Compute the missing package identities and scrub any out-of-date entry from the pin store..
         let missingPackageURLs = currentManifests.missingPackageURLs()
+        for missingURL in missingPackageURLs {
+            pinsStore.remove(identity: missingURL.identity)
+            try? pinsStore.saveState()
+        }
 
         // The pins to use in case we need to run the resolution.
         var validPins = pinsStore.createConstraints()
 
         // Compute if we need to run the resolver. We always run the resolver if
-        // there are extra constraints.
+        // there are missing packages.
         if missingPackageURLs.isEmpty {
             // Use root constraints, dependency manifest constraints and extra
             // constraints to compute if a new resolution is required.
@@ -1263,9 +1269,11 @@ extension Workspace {
 
             let result = isResolutionRequired(root: graphRoot, dependencies: dependencies, pinsStore: pinsStore)
 
-            // If we don't need resolution and there are no extra constraints,
-            // just validate pinsStore and return.
-            if !result.resolve && extraConstraints.isEmpty {
+            // We don't need to resolve if:
+            // 1. Nothing in the manifest changed.
+            // 2. PinStore is valid.
+            // 3. We have no extra constraints.
+            if !result.resolve && extraConstraints.isEmpty && !isPinStoreOutdated {
                 return currentManifests
             }
 
@@ -1442,38 +1450,7 @@ extension Workspace {
             }
         }
 
-        return (false, [])
-    }
-
-    /// Validates that each checked out managed dependency has an entry in pinsStore.
-    private func validatePinsStore(dependencyManifests: DependencyManifests, diagnostics: DiagnosticsEngine) {
-        guard let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else {
-            return
-        }
-
-		let pins = pinsStore.pinsMap.keys
-        let requiredURLs = dependencyManifests.computePackageURLs().required
-
-        for dependency in managedDependencies.values {
-            switch dependency.state {
-            case .checkout: break
-            case .edited, .local: continue
-            }
-
-            let identity = dependency.packageRef.identity
-
-            if requiredURLs.contains(where: { $0.path == dependency.packageRef.path }) {
-                // If required identity contains this dependency, it should be in the pins store.
-                if let pin = pinsStore.pinsMap[identity], pin.packageRef.path == dependency.packageRef.path {
-                    continue
-                }
-            } else if !pins.contains(identity) {
-                // Otherwise, it should *not* be in the pins store.
-                continue
-            }
-
-            return self.pinAll(dependencyManifests: dependencyManifests, pinsStore: pinsStore, diagnostics: diagnostics)
-        }
+        return (false, validPins)
     }
 
     /// This enum represents state of an external package.
@@ -1994,3 +1971,36 @@ public final class LoadableResult<Value> {
         return try loadResult().dematerialize()
     }
 }
+
+extension PinsStore {
+    /// Returns true if the pin store is outdated with the content in the managed dependencies.
+    func isOutdated(
+        dependencyManifests: Workspace.DependencyManifests,
+        managedDependencies: ManagedDependencies
+    ) -> Bool {
+        let pins = pinsMap.keys
+        let requiredURLs = dependencyManifests.computePackageURLs().required
+
+        for dependency in managedDependencies.values {
+            switch dependency.state {
+            case .checkout: break
+            case .edited, .local: continue
+            }
+
+            let identity = dependency.packageRef.identity
+
+            if requiredURLs.contains(where: { $0.path == dependency.packageRef.path }) {
+                // If required identity contains this dependency, it should be in the pins store.
+                if let pin = pinsMap[identity], pin.packageRef.path == dependency.packageRef.path {
+                    continue
+                }
+            } else if !pins.contains(identity) {
+                // Otherwise, it should *not* be in the pins store.
+                continue
+            }
+
+            return true
+        }
+        return false
+    }
+}

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -861,7 +861,7 @@ final class WorkspaceTests: XCTestCase {
             ], pinsStore: pinsStore)
 
             XCTAssertEqual(result.resolve, false)
-            XCTAssertEqual(result.validPins, [])
+            XCTAssertEqual(result.validPins.map({$0.identifier.repository.url}).sorted(), ["/A", "/B", "/C"])
         }
     }
 
@@ -2575,7 +2575,7 @@ final class WorkspaceTests: XCTestCase {
             XCTAssertNoDiagnostics(diagnostics)
         }
         workspace.checkManagedDependencies() { result in
-            result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
+            result.check(notPresent: "foo")
         }
         workspace.checkResolved() { result in
             result.check(notPresent: "foo")
@@ -3048,6 +3048,107 @@ final class WorkspaceTests: XCTestCase {
             }
         }
     }
+
+	func testUpdateToResolvedFileResolvesCorrectly() throws {
+         let sandbox = AbsolutePath("/tmp/ws/")
+         let fs = InMemoryFileSystem()
+
+         let workspace = try TestWorkspace(
+             sandbox: sandbox,
+             fs: fs,
+             roots: [
+                 TestPackage(
+                     name: "Root",
+                     targets: [
+                         TestTarget(name: "Root", dependencies: ["Bar"]),
+                     ],
+                     products: [],
+                     dependencies: [
+                        TestDependency(name: "Bar", requirement: .upToNextMajor(from: "1.0.0")),
+                     ]
+                 ),
+             ],
+             packages: [
+                 TestPackage(
+                     name: "Bar",
+                     targets: [
+                         TestTarget(name: "Bar", dependencies: ["Foo"]),
+                     ],
+                     products: [
+                         TestProduct(name: "Bar", targets: ["Bar"]),
+                     ],
+                     dependencies: [
+                        TestDependency(name: "Foo", requirement: .upToNextMajor(from: "1.0.0")),
+                     ],
+                     versions: ["1.0.0"]
+                 ),
+                 TestPackage(
+                     name: "Bar",
+                     targets: [
+                         TestTarget(name: "Bar", dependencies: []),
+                     ],
+                     products: [
+                         TestProduct(name: "Bar", targets: ["Bar"]),
+                     ],
+                     dependencies: [
+                     ],
+                     versions: ["1.1.0"]
+                 ),
+                 TestPackage(
+                     name: "Foo",
+                     targets: [
+                         TestTarget(name: "Foo"),
+                     ],
+                     products: [
+                         TestProduct(name: "Foo", targets: ["Foo"]),
+                     ],
+                     versions: ["1.0.0"]
+                 ),
+             ]
+         )
+
+         let deps: [TestWorkspace.PackageDependency] = [
+             .init(name: "Bar", requirement: .exact("1.0.0")),
+         ]
+         workspace.checkPackageGraph(roots: ["Root"], deps: deps) { (graph, diagnostics) in
+             PackageGraphTester(graph) { result in
+                 result.check(roots: "Root")
+                 result.check(packages: "Bar", "Foo", "Root")
+             }
+             XCTAssertNoDiagnostics(diagnostics)
+         }
+         workspace.checkManagedDependencies() { result in
+            result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
+            result.check(dependency: "bar", at: .checkout(.version("1.0.0")))
+         }
+
+        // Emulate a complete change in Package.resolved file.
+        do {
+            let ws = workspace.createWorkspace()
+            let pinsStore = try ws.pinsStore.load()
+            let barPin = pinsStore.pins.first(where: { $0.packageRef.identity == "bar" })!
+
+            let barRepo = workspace.repoProvider.specifierMap[RepositorySpecifier(url: barPin.packageRef.path)]!
+            let revision = try barRepo.resolveRevision(tag: "1.1.0")
+            let newState = CheckoutState(revision: revision, version: "1.1.0")
+
+            pinsStore.unpinAll()
+            pinsStore.pin(packageRef: barPin.packageRef, state: newState)
+            try pinsStore.saveState()
+        }
+
+         workspace.checkPackageGraph(roots: ["Root"]) { (graph, diagnostics) in
+             PackageGraphTester(graph) { result in
+                 result.check(roots: "Root")
+                 result.check(packages: "Bar", "Root")
+             }
+             XCTAssertNoDiagnostics(diagnostics)
+         }
+         workspace.checkManagedDependencies() { result in
+            result.check(notPresent: "foo")
+            result.check(dependency: "bar", at: .checkout(.version("1.1.0")))
+         }
+     }
 }
 
 extension PackageGraph {

Tests/WorkspaceTests/XCTestManifests.swift

@@ -85,6 +85,7 @@ extension WorkspaceTests {
         ("testToolsVersionRootPackages", testToolsVersionRootPackages),
         ("testTransitiveDependencySwitchWithSameIdentity", testTransitiveDependencySwitchWithSameIdentity),
         ("testUpdate", testUpdate),
+        ("testUpdateToResolvedFileResolvesCorrectly", testUpdateToResolvedFileResolvesCorrectly),
     ]
 }