refactor CheckoutState (#3666)

motivation: when working on configuration, CheckoutState data model stood out as one that can be improved

changes:
* transition CheckoutState from struct with optionals to enum representing the finite states
* adust call sites and tests

Author: tomerd

Sources/PackageGraph/CheckoutState.swift

@@ -15,50 +15,44 @@ import TSCUtility
 /// A checkout state represents the current state of a repository.
 ///
 /// A state will always has a revision. It can also have a branch or a version but not both.
-public struct CheckoutState: Equatable, Hashable {
+public enum CheckoutState: Equatable, Hashable {
 
-    /// The revision of the checkout.
-    public let revision: Revision
-
-    /// The version of the checkout, if known.
-    public let version: Version?
-
-    /// The branch of the checkout, if known.
-    public let branch: String?
-
-    /// Create a checkout state with given data. It is invalid to provide both version and branch.
-    ///
-    /// This is deliberately fileprivate so CheckoutState is not initialized
-    /// with both version and branch. All other initializers should delegate to it.
-    fileprivate init(revision: Revision, version: Version?, branch: String?) {
-        assert(version == nil || branch == nil, "Can't set both branch and version.")
-        self.revision = revision
-        self.version = version
-        self.branch = branch
-    }
-
-    /// Create a checkout state with given revision and branch.
-    public init(revision: Revision, branch: String? = nil) {
-        self.init(revision: revision, version: nil, branch: branch)
-    }
+    case revision(Revision)
+    case version(Version, revision: Revision)
+    case branch(name: String, revision: Revision)
 
-    /// Create a checkout state with given revision and version.
-    public init(revision: Revision, version: Version) {
-        self.init(revision: revision, version: version, branch: nil)
+    /// The revision of the checkout.
+    public var revision: Revision {
+        get {
+            switch self {
+            case .revision(let revision):
+                return revision
+            case .version(_, let revision):
+                return revision
+            case .branch(_, let revision):
+                return revision
+            }
+        }
     }
 
     public var isBranchOrRevisionBased: Bool {
-        return version == nil
+        switch self {
+        case .revision, .branch:
+            return true
+        case .version:
+            return false
+        }
     }
 
     /// Returns requirement induced by this state.
     public var requirement: PackageRequirement {
-        if let version = version {
+        switch self {
+        case .revision(let revision):
+            return .revision(revision.identifier)
+        case .version(let version, _):
             return .versionSet(.exact(version))
-        } else if let branch = branch {
+        case .branch(let branch, _):
             return .revision(branch)
-        } else {
-            return .revision(revision.identifier)
         }
     }
 }
@@ -67,27 +61,62 @@ public struct CheckoutState: Equatable, Hashable {
 
 extension CheckoutState: CustomStringConvertible {
     public var description: String {
-        return version?.description ?? branch ?? revision.identifier
+        switch self {
+        case .revision(let revision):
+            return revision.identifier
+        case .version(let version, _):
+            return version.description
+        case .branch(let branch, _):
+            return branch
+        }
     }
 }
 
 // MARK: - JSON
 
 extension CheckoutState: JSONMappable, JSONSerializable {
     public init(json: JSON) throws {
-       self.init(
-           revision: try json.get("revision"),
-           version: json.get("version"),
-           branch: json.get("branch")
-        )
+        let revision: Revision = try json.get("revision")
+        let version: Version? = json.get("version")
+        let branch: String? = json.get("branch")
+
+        switch (version, branch) {
+        case (.none, .none):
+            self = .revision(revision)
+        case (.some(let version), .none):
+            self = .version(version, revision: revision)
+        case (.none, .some(let branch)):
+            self = .branch(name: branch, revision: revision)
+        case (.some(_), .some(_)):
+            preconditionFailure("Can't set both branch and version.")
+        }
     }
 
     public func toJSON() -> JSON {
-       return .init([
-           "revision": revision.identifier,
-           "version": version.toJSON(),
-           "branch": branch.toJSON(),
-       ])
+        let revision: Revision
+        let version: Version?
+        let branch: String?
+
+        switch self {
+        case .revision(let _revision):
+            revision = _revision
+            version = nil
+            branch = nil
+        case .version(let _version, let _revision):
+            revision = _revision
+            version = _version
+            branch = nil
+        case .branch(let _branch, let _revision):
+            revision = _revision
+            version = nil
+            branch = _branch
+        }
+
+        return .init([
+            "revision": revision.identifier,
+            "version": version.toJSON(),
+            "branch": branch.toJSON(),
+        ])
     }
 }
 

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -346,8 +346,8 @@ public struct PubgrubDependencyResolver {
             // latest commit on that branch. Note that if this revision-based dependency is
             // already a commit, then its pin entry doesn't matter in practice.
             let revisionForDependencies: String
-            if let pin = pinsMap[package.identity], pin.state.branch == revision {
-                revisionForDependencies = pin.state.revision.identifier
+            if case .branch(revision, let pinRevision) = pinsMap[package.identity]?.state {
+                revisionForDependencies = pinRevision.identifier
 
                 // Mark the package as overridden with the pinned revision and record the branch as well.
                 overriddenPackages[package] = (version: .revision(revisionForDependencies, branch: revision), products: constraint.products)
@@ -1054,7 +1054,12 @@ private final class PubGrubPackageContainer {
 
     /// Returns the pinned version for this package, if any.
     var pinnedVersion: Version? {
-        return self.pinsMap[self.underlying.package.identity]?.state.version
+        switch self.pinsMap[self.underlying.package.identity]?.state {
+        case .version(let version, _):
+            return version
+        default:
+            return .none
+        }
     }
 
     /// Returns the numbers of versions that are satisfied by the given version requirement.

Sources/SPMTestSupport/MockWorkspace.swift

@@ -689,3 +689,31 @@ public final class MockWorkspaceDelegate: WorkspaceDelegate {
         }
     }
 }
+
+extension CheckoutState {
+    public var version: Version? {
+        get {
+            switch self {
+            case .revision:
+                return .none
+            case .version(let version, _):
+                return version
+            case .branch:
+                return .none
+            }
+        }
+    }
+
+    public var branch: String? {
+        get {
+            switch self {
+            case .revision:
+                return .none
+            case .version:
+                return .none
+            case .branch(let branch, _):
+                return branch
+            }
+        }
+    }
+}

Sources/Workspace/ResolverPrecomputationProvider.swift

@@ -8,12 +8,13 @@
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
  */
 
+import Basics
 import Dispatch
-import PackageModel
 import PackageGraph
+import PackageModel
+import SourceControl
 import TSCBasic
 import TSCUtility
-import SourceControl
 
 /// Enumeration of the different errors that can arise from the `ResolverPrecomputationProvider` provider.
 enum ResolverPrecomputationError: Error {
@@ -95,7 +96,7 @@ private struct LocalPackageContainer: PackageContainer {
     let currentToolsVersion: ToolsVersion
 
     func versionsAscending() throws -> [Version] {
-        if let version = dependency?.state.checkout?.version {
+        if case .version(let version, revision: _) = dependency?.state.checkout {
             return [version]
         } else {
             return []
@@ -121,23 +122,25 @@ private struct LocalPackageContainer: PackageContainer {
 
     func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
         // Because of the implementation of `reversedVersions`, we should only get the exact same version.
-        precondition(dependency?.checkoutState?.version == version)
+        guard case .version(version, revision: _) = dependency?.checkoutState else {
+            throw InternalError("expected version, but checkout state was \(String(describing: dependency?.checkoutState))")
+        }
         return try manifest.dependencyConstraints(productFilter: productFilter)
     }
 
-    func getDependencies(at revision: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
+    func getDependencies(at revisionString: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
         // Return the dependencies if the checkout state matches the revision.
-        if let checkoutState = dependency?.checkoutState,
-            checkoutState.version == nil,
-            checkoutState.revision.identifier == revision {
+        let revision = Revision(identifier: revisionString)
+        switch dependency?.checkoutState {
+        case .branch(_, revision: revision), .revision(revision):
             return try manifest.dependencyConstraints(productFilter: productFilter)
+        default:
+            throw ResolverPrecomputationError.differentRequirement(
+                package: self.package,
+                state: self.dependency?.state,
+                requirement: .revision(revisionString)
+            )
         }
-
-        throw ResolverPrecomputationError.differentRequirement(
-            package: self.package,
-            state: self.dependency?.state,
-            requirement: .revision(revision)
-        )
     }
 
     func getUnversionedDependencies(productFilter: ProductFilter) throws -> [PackageContainerConstraint] {

Sources/Workspace/Workspace.swift

@@ -1513,10 +1513,15 @@ extension Workspace {
         switch managedDependency.state {
         case .checkout(let checkoutState):
             packageKind = .remote
-            version = checkoutState.version
+            switch checkoutState {
+            case .version(let checkoutVersion, _):
+                version = checkoutVersion
+            default:
+                version = .none
+            }
         case .edited, .local:
             packageKind = .local
-            version = nil
+            version = .none
         }
 
         // Get the path of the package.
@@ -2413,19 +2418,22 @@ extension Workspace {
 
                 // If we have a branch and we shouldn't be updating the
                 // branches, use the revision from pin instead (if present).
-                if branch != nil {
-                    if let pin = pinsStore.pins.first(where: { $0.packageRef == packageRef }),
-                        !updateBranches,
-                        pin.state.branch == branch {
-                        revision = pin.state.revision
+                if branch != nil, !updateBranches {
+                    if case .branch(branch, let pinRevision) = pinsStore.pins.first(where: { $0.packageRef == packageRef })?.state {
+                        revision = pinRevision
                     }
                 }
 
                 // First check if we have this dependency.
                 if let currentDependency = currentDependency {
                     // If current state and new state are equal, we don't need
                     // to do anything.
-                    let newState = CheckoutState(revision: revision, branch: branch)
+                    let newState: CheckoutState
+                    if let branch = branch {
+                        newState = .branch(name: branch, revision: revision)
+                    } else {
+                        newState = .revision(revision)
+                    }
                     if case .checkout(let checkoutState) = currentDependency.state, checkoutState == newState {
                         packageStateChanges[packageRef.location] = (packageRef, .unchanged)
                     } else {
@@ -2440,7 +2448,7 @@ extension Workspace {
 
             case .version(let version):
                 if let currentDependency = currentDependency {
-                    if case .checkout(let checkoutState) = currentDependency.state, checkoutState.version == version {
+                    if case .checkout(let checkoutState) = currentDependency.state, case .version(version, _) = checkoutState {
                         packageStateChanges[packageRef.location] = (packageRef, .unchanged)
                     } else {
                         let newState = PackageStateChange.State(requirement: .version(version), products: products)
@@ -2722,10 +2730,13 @@ extension Workspace {
                 throw InternalError("unable to get tag for \(package) \(version); available versions \(try container.versionsDescending())")
             }
             let revision = try container.getRevision(forTag: tag)
-            checkoutState = CheckoutState(revision: revision, version: version)
+            checkoutState = .version(version, revision: revision)
+
+        case .revision(let revision, .none):
+            checkoutState = .revision(revision)
 
-        case .revision(let revision, let branch):
-            checkoutState = CheckoutState(revision: revision, branch: branch)
+        case .revision(let revision, .some(let branch)):
+            checkoutState = .branch(name: branch, revision: revision)
 
         case .unversioned:
             state.dependencies.add(ManagedDependency.local(packageRef: package))

Tests/CommandsTests/PackageToolTests.swift

@@ -723,7 +723,7 @@ final class PackageToolTests: XCTestCase {
             do {
                 try execute("resolve", "bar", "--branch", "YOLO")
                 let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
-                let state = CheckoutState(revision: yoloRevision, branch: "YOLO")
+                let state = CheckoutState.branch(name: "YOLO", revision: yoloRevision)
                 let identity = PackageIdentity(path: barPath)
                 XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
             }
@@ -732,7 +732,7 @@ final class PackageToolTests: XCTestCase {
             do {
                 try execute("resolve", "bar", "--revision", yoloRevision.identifier)
                 let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
-                let state = CheckoutState(revision: yoloRevision)
+                let state = CheckoutState.revision(yoloRevision)
                 let identity = PackageIdentity(path: barPath)
                 XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
             }

Tests/PackageGraphTests/PubgrubTests.swift

@@ -1902,11 +1902,11 @@ final class PubGrubBacktrackTests: XCTestCase {
 fileprivate extension CheckoutState {
     /// Creates a checkout state with the given version and a mocked revision.
     static func version(_ version: Version) -> CheckoutState {
-        CheckoutState(revision: Revision(identifier: "<fake-ident>"), version: version)
+        .version(version, revision: Revision(identifier: "<fake-ident>"))
     }
 
     static func branch(_ branch: String, revision: String) -> CheckoutState {
-        CheckoutState(revision: Revision(identifier: revision), branch: branch)
+        .branch(name: branch, revision: Revision(identifier: revision))
     }
 }