Merge pull request #1170 from aciidb0mb3r/unedit-resolve

[Workspace] Resolve post unediting

Author: aciidgh

Sources/Commands/SwiftPackageTool.swift

@@ -123,7 +123,12 @@ public class SwiftPackageTool: SwiftTool<PackageToolOptions> {
             try resolve()
             let workspace = try getActiveWorkspace()
 
-            try workspace.unedit(packageName: packageName, forceRemove: options.editOptions.shouldForceRemove)
+            try workspace.unedit(
+                packageName: packageName,
+                forceRemove: options.editOptions.shouldForceRemove,
+                root: getWorkspaceRoot(),
+                diagnostics: diagnostics
+            )
 
         case .showDependencies:
             let graph = try loadPackageGraph()

Sources/Workspace/Workspace.swift

@@ -323,17 +323,26 @@ extension Workspace {
         }
     }
 
-    /// Ends the edit mode of a dependency which is in edit mode.
+    /// Ends the edit mode of an edited dependency.
+    ///
+    /// This will re-resolve the dependencies after ending edit as the original
+    /// checkout may be outdated.
     ///
     /// - Parameters:
     ///     - packageName: The name of the package to edit.
-    ///     - forceRemove: If true, the dependency will be unedited even if has
-    /// unpushed and uncommited changes. Otherwise will throw respective errors.
-    ///
-    /// - throws: WorkspaceError
-    public func unedit(packageName: String, forceRemove: Bool) throws {
+    ///     - forceRemove: If true, the dependency will be unedited even if has unpushed
+    ///           or uncommited changes. Otherwise will throw respective errors.
+    ///     - root: The workspace root. This is used to resolve the dependencies post unediting.
+    ///     - diagnostics: The diagnostics engine that reports errors, warnings
+    ///           and notes.
+    public func unedit(
+        packageName: String,
+        forceRemove: Bool,
+        root: WorkspaceRoot,
+        diagnostics: DiagnosticsEngine
+    ) throws {
         let dependency = try managedDependencies.dependency(forName: packageName)
-        try unedit(dependency: dependency, forceRemove: forceRemove)
+        try unedit(dependency: dependency, forceRemove: forceRemove, root: root, diagnostics: diagnostics)
     }
 
     /// Resolve a package at the given state.
@@ -367,27 +376,22 @@ extension Workspace {
             return diagnostics.emit(error)
         }
 
-        // Compute the checkout state.
-        //
-        // We use a dummy revision in case of version and branch because we
-        // might not know the needed revision at this point.
-        let checkoutState: CheckoutState
+        // Compute the custom or extra constraint we need to impose.
+        let requirement: RepositoryPackageConstraint.Requirement
         if let version = version {
-            checkoutState = CheckoutState(revision: Revision(identifier: ""), version: version)
+            requirement = .versionSet(.exact(version))
         } else if let branch = branch {
-            checkoutState = CheckoutState(revision: Revision(identifier: ""), branch: branch)
+            requirement = .revision(branch)
         } else if let revision = revision {
-            checkoutState = CheckoutState(revision: Revision(identifier: revision))
+            requirement = .revision(revision)
         } else {
-            checkoutState = currentState
+            requirement = currentState.requirement()
         }
-
-        // Create a pin with above checkout state.
-        let pin = PinsStore.Pin(
-            package: dependency.name, repository: dependency.repository, state: checkoutState)
+        let constraint = RepositoryPackageConstraint(
+                container: dependency.repository, requirement: requirement)
 
         // Run the resolution.
-        _resolve(root: root, extraPins: [pin], diagnostics: diagnostics)
+        _resolve(root: root, extraConstraints: [constraint], diagnostics: diagnostics)
     }
 
     /// Cleans the build artefacts from workspace data.
@@ -666,7 +670,12 @@ extension Workspace {
     }
 
     /// Unedit a managed dependency. See public API unedit(packageName:forceRemove:).
-    fileprivate func unedit(dependency: ManagedDependency, forceRemove: Bool) throws {
+    fileprivate func unedit(
+        dependency: ManagedDependency,
+        forceRemove: Bool,
+        root: WorkspaceRoot? = nil,
+        diagnostics: DiagnosticsEngine
+    ) throws {
 
         // Compute if we need to force remove.
         var forceRemove = forceRemove
@@ -709,6 +718,12 @@ extension Workspace {
         managedDependencies[dependency.repository] = dependency.basedOn
         // Save the state.
         try managedDependencies.saveState()
+
+        // Resolve the dependencies if workspace root is provided. We do this to
+        // ensure the unedited version of this dependency is resolved properly.
+        if let root = root {
+            resolve(root: root, diagnostics: diagnostics)
+        }
     }
 
 }
@@ -866,14 +881,14 @@ extension Workspace {
 
     /// Implementation of resolve(root:diagnostics:).
     ///
-    /// The extra pins will override the pin of the same package in the
-    /// pinsStore.  It is useful in situations where a requirement is being
+    /// The extra constraints will be added to the main requirements.
+    /// It is useful in situations where a requirement is being
     /// imposed outside of manifest and pins file. E.g., when using a command
     /// like `$ swift package resolve foo --version 1.0.0`.
     @discardableResult
     fileprivate func _resolve(
         root: WorkspaceRoot,
-        extraPins: [PinsStore.Pin] = [],
+        extraConstraints: [RepositoryPackageConstraint] = [],
         diagnostics: DiagnosticsEngine
     ) -> DependencyManifests {
 
@@ -898,17 +913,18 @@ extension Workspace {
         // The pins to use in case we need to run the resolution.
         var validPins = pinsStore.createConstraints()
 
-        // Compute if we need to run the resolver.
+        // Compute if we need to run the resolver. We always run the resolver if
+        // there are extra constraints.
         if missingURLs.isEmpty {
             // Use root constraints, dependency manifest constraints and extra
-            // pins to compute if a new resolution is required.
-            let dependencies = graphRoot.constraints + currentManifests.dependencyConstraints()
-            extraPins.forEach(pinsStore.add)
+            // constraints to compute if a new resolution is required.
+            let dependencies = graphRoot.constraints + currentManifests.dependencyConstraints() + extraConstraints
 
             let result = isResolutionRequired(dependencies: dependencies, pinsStore: pinsStore)
 
-            // If we don't need resolution, just validate pinsStore and return.
-            if !result.resolve {
+            // If we don't need resolution and there are no extra constraints,
+            // just validate pinsStore and return.
+            if !result.resolve && extraConstraints.isEmpty {
                 validatePinsStore(with: diagnostics)
                 return currentManifests
             }
@@ -918,13 +934,23 @@ extension Workspace {
 
         // Create the constraints.
         var constraints = [RepositoryPackageConstraint]()
-        constraints += graphRoot.constraints
+        constraints += graphRoot.constraints + extraConstraints
         constraints += currentManifests.editedPackagesConstraints()
 
         // Perform dependency resolution.
-        let result = resolveDependencies(dependencies: constraints, pins: validPins, diagnostics: diagnostics)
-        guard !diagnostics.hasErrors else {
-            return currentManifests
+        let resolverDiagnostics = DiagnosticsEngine()
+        var result = resolveDependencies(dependencies: constraints, pins: validPins, diagnostics: resolverDiagnostics)
+
+        // If we fail, we just try again without any pins because the pins might
+        // be completely incompatible.
+        //
+        // FIXME: We should only do this if resolver emits "unresolvable" error.
+        // FIXME: We should merge the engine in case we get no errors but warnings or notes.
+        if resolverDiagnostics.hasErrors {
+            result = resolveDependencies(dependencies: constraints, pins: [], diagnostics: diagnostics)
+            guard !diagnostics.hasErrors else {
+                return currentManifests
+            }
         }
 
         // Update the checkouts with dependency resolution result.
@@ -1171,18 +1197,26 @@ extension Workspace {
     fileprivate func fixManagedDependencies(with diagnostics: DiagnosticsEngine) {
         for dependency in managedDependencies.values {
             diagnostics.wrap {
+
+                // If the dependency is present, we're done.
                 let dependencyPath = path(for: dependency)
-                if !fileSystem.isDirectory(dependencyPath) {
-                    switch dependency.state {
-                    case .checkout(let checkoutState):
-                        // If some checkout dependency has been removed, clone it again.
-                        _ = try clone(repository: dependency.repository, at: checkoutState)
-                        diagnostics.emit(WorkspaceDiagnostics.CheckedOutDependencyMissing(packageName: dependency.name))
-                    case .edited:
-                        // If some edited dependency has been removed, mark it as unedited.
-                        try unedit(dependency: dependency, forceRemove: true)
-                        diagnostics.emit(WorkspaceDiagnostics.EditedDependencyMissing(packageName: dependency.name))
-                    }
+                guard !fileSystem.isDirectory(dependencyPath) else { return }
+
+                switch dependency.state {
+                case .checkout(let checkoutState):
+                    // If some checkout dependency has been removed, clone it again.
+                    _ = try clone(repository: dependency.repository, at: checkoutState)
+                    diagnostics.emit(WorkspaceDiagnostics.CheckedOutDependencyMissing(packageName: dependency.name))
+
+                case .edited:
+                    // If some edited dependency has been removed, mark it as unedited.
+                    //
+                    // Note: We don't resolve the dependencies when unediting
+                    // here because we expect this method to be called as part
+                    // of some other resolve operation (i.e. resolve, update, etc).
+                    try unedit(dependency: dependency, forceRemove: true, diagnostics: diagnostics)
+
+                    diagnostics.emit(WorkspaceDiagnostics.EditedDependencyMissing(packageName: dependency.name))
                 }
             }
         }