identity improvments

motivation: improve correctlness by using identity more broadly throughout the code

changes:
* remove name from package-reference, replace with alternate-identity for secondary (manifest driven) identity
* use identity to lookup managed-dependencies and pins instead of urls
* refactor pins map serialization
* adjust tests

Author: tomerd

Sources/Commands/SwiftPackageTool.swift

@@ -910,11 +910,11 @@ fileprivate func logPackageChanges(changes: [(PackageReference, Workspace.Packag
         let currentVersion = pins.pinsMap[package.identity]?.state.description ?? ""
         switch change {
         case let .added(state):
-            stream <<< "+ \(package.name) \(state.requirement.prettyPrinted)"
+            stream <<< "+ \(package.identity) \(state.requirement.prettyPrinted)"
         case let .updated(state):
-            stream <<< "~ \(package.name) \(currentVersion) -> \(package.name) \(state.requirement.prettyPrinted)"
+            stream <<< "~ \(package.identity) \(currentVersion) -> \(package.identity) \(state.requirement.prettyPrinted)"
         case .removed:
-            stream <<< "- \(package.name) \(currentVersion)"
+            stream <<< "- \(package.identity) \(currentVersion)"
         case .unchanged:
             continue
         }

Sources/Commands/SwiftTool.swift

@@ -141,7 +141,7 @@ private class ToolWorkspaceDelegate: WorkspaceDelegate {
                 let dependencies = packages.lazy.map({ "'\($0.path)'" }).joined(separator: ", ")
                 self.stdoutStream <<< "the following dependencies were added: \(dependencies)"
             case .packageRequirementChange(let package, let state, let requirement):
-                self.stdoutStream <<< "dependency '\(package.name)' was "
+                self.stdoutStream <<< "dependency '\(package.identity)' was "
 
                 switch state {
                 case .checkout(let checkoutState)?:

Sources/PackageGraph/DependencyResolutionNode.swift

@@ -134,6 +134,6 @@ extension DependencyResolutionNode: Hashable {
 
 extension DependencyResolutionNode: CustomStringConvertible {
     public var description: String {
-        return "\(package.name)\(productFilter)"
+        return "\(self.package.identity)\(productFilter)"
     }
 }

Sources/PackageGraph/DependencyResolver.swift

@@ -13,7 +13,7 @@ import PackageModel
 
 public enum DependencyResolverError: Error, Equatable {
      /// A revision-based dependency contains a local package dependency.
-    case revisionDependencyContainsLocalPackage(dependency: String, localPackage: String)
+    case revisionDependencyContainsLocalPackage(dependency: PackageIdentity, localPackage: PackageIdentity)
 }
 
 public class DependencyResolver {

Sources/PackageGraph/LocalPackageContainer.swift

@@ -66,7 +66,7 @@ public final class LocalPackageContainer: PackageContainer {
     public func getUpdatedIdentifier(at boundVersion: BoundVersion) throws -> PackageReference {
         assert(boundVersion == .unversioned, "Unexpected bound version \(boundVersion)")
         let manifest = try loadManifest()
-        return identifier.with(newName: manifest.name)
+        return identifier.with(alternateIdentity: PackageIdentity(name: manifest.name))
     }
 
     public init(

Sources/PackageGraph/PinsStore.swift

@@ -8,10 +8,11 @@
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
+import Basics
+import PackageModel
+import SourceControl
 import TSCBasic
 import TSCUtility
-import SourceControl
-import PackageModel
 
 public final class PinsStore {
     public typealias PinsMap = [PackageIdentity: PinsStore.Pin]
@@ -133,28 +134,54 @@ extension PinsStore: JSONSerializable {
 extension PinsStore.Pin: JSONMappable, JSONSerializable {
     /// Create an instance from JSON data.
     public init(json: JSON) throws {
-        let name: String? = json.get("package")
-        let url: String = try json.get("repositoryURL")
-        let identity = PackageIdentity(url: url)
-        let ref = PackageReference(identity: identity, path: url)
-        self.packageRef = name.flatMap(ref.with(newName:)) ?? ref
+        // backwards compatibility 12/2020
+        let location: String
+        if let value: String = json.get("location") {
+            location = value
+        } else if let value: String = json.get("repositoryURL") {
+            location = value
+        } else {
+            throw InternalError("unknown location")
+        }
+
+        // backwards compatibility 12/2020
+        let identity: PackageIdentity
+        if let value: PackageIdentity = json.get("identity") {
+            identity = value
+        } else {
+            identity = PackageIdentity(url: location)
+        }
+
+        // backwards compatibility 12/2020
+        var alternateIdentity: PackageIdentity? = nil
+        if let value: PackageIdentity = json.get("alternate_identity") {
+            alternateIdentity = value
+        } else if let value: String = json.get("name") {
+            alternateIdentity = PackageIdentity(name: value)
+        }
+        let package = PackageReference(identity: identity, path: location)
+        self.packageRef = alternateIdentity.map{ package.with(alternateIdentity: $0) } ?? package
         self.state = try json.get("state")
     }
 
     /// Convert the pin to JSON.
     public func toJSON() -> JSON {
-        return .init([
-            "package": packageRef.name.toJSON(),
-            "repositoryURL": packageRef.path,
+        var map: [String: JSONSerializable] = [
+            "identity": packageRef.identity,
+            "location": packageRef.path,
             "state": state
-        ])
+        ]
+        if let alternateIdentity = packageRef.alternateIdentity {
+            map["alternate_identity"] = alternateIdentity
+        }
+        return .init(map)
     }
 }
 
 // MARK: - SimplePersistanceProtocol
 
 extension PinsStore: SimplePersistanceProtocol {
     public func restore(from json: JSON) throws {
-        self.pinsMap = try Dictionary(json.get("pins").map({ ($0.packageRef.identity, $0) }), uniquingKeysWith: { first, _ in throw StringError("duplicated entry for package \"\(first.packageRef.name)\"") })
+        self.pinsMap = try Dictionary(json.get("pins").map({ ($0.packageRef.identity, $0) }), uniquingKeysWith: { first, _ in throw StringError("duplicated entry for package \"\(first.packageRef.identity)\"") })
     }
 }

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -152,11 +152,9 @@ public struct PubgrubDependencyResolver {
 
     /// Execute the resolution algorithm to find a valid assignment of versions.
     public func solve(constraints: [Constraint]) -> Result<[DependencyResolver.Binding], Error> {
-        let root = DependencyResolutionNode.root(package: PackageReference(
+        let root = DependencyResolutionNode.root(package: .root(
             identity: PackageIdentity(url: "<synthesized-root>"),
-            path: "<synthesized-root-path>",
-            name: nil,
-            kind: .root
+            path: AbsolutePath("/synthesized-root-path")
         ))
 
         do {
@@ -248,7 +246,7 @@ public struct PubgrubDependencyResolver {
             }
         }
         var finalAssignments: [DependencyResolver.Binding]
-            = flattenedAssignments.keys.sorted(by: { $0.name < $1.name }).map { package in
+            = flattenedAssignments.keys.sorted(by: { $0.identity < $1.identity }).map { package in
                 let details = flattenedAssignments[package]!
                 return (container: package, binding: details.binding, products: details.products)
             }
@@ -400,8 +398,8 @@ public struct PubgrubDependencyResolver {
                         constraints.append(dependency)
                     case .unversioned:
                         throw DependencyResolverError.revisionDependencyContainsLocalPackage(
-                            dependency: package.name,
-                            localPackage: dependency.identifier.name
+                            dependency: package.identity,
+                            localPackage: dependency.identifier.identity
                         )
                     }
                 }
@@ -1364,7 +1362,7 @@ private final class PubGrubPackageContainer {
         let versions: [Version] = try self.underlying.versionsAscending()
 
         guard let idx = versions.firstIndex(of: firstVersion) else {
-            throw InternalError("from version \(firstVersion) not found in \(node.package.name)")
+            throw InternalError("from version \(firstVersion) not found in \(node.package.identity)")
         }
 
         let sync = DispatchGroup()
@@ -1380,7 +1378,7 @@ private final class PubGrubPackageContainer {
         }
 
         guard case .success = sync.wait(timeout: .now() + timeout) else {
-            throw StringError("timeout computing \(node.package.name) bounds")
+            throw StringError("timeout computing \(node.package.identity) bounds")
         }
 
         return (lowerBounds, upperBounds)
@@ -1419,7 +1417,7 @@ private final class ContainerProvider {
     /// Get a cached container for the given identifier, asserting / throwing if not found.
     func getCachedContainer(for package: PackageReference) throws -> PubGrubPackageContainer {
         guard let container = self.containersCache[package] else {
-            throw InternalError("container for \(package.name) expected to be cached")
+            throw InternalError("container for \(package.identity) expected to be cached")
         }
         return container
     }
@@ -1564,6 +1562,6 @@ private extension PackageRequirement {
 
 private extension DependencyResolutionNode {
     var nameForDiagnostics: String {
-        return "'\(package.name)'"
+        return "'\(self.package.identity)'"
     }
 }

Sources/PackageGraph/RepositoryPackageContainer.swift

@@ -248,7 +248,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
         }
 
         let manifest = try self.loadManifest(at: revision, version: version)
-        return self.identifier.with(newName: manifest.name)
+        return self.identifier.with(alternateIdentity: PackageIdentity(name: manifest.name))
     }
 
     /// Returns true if the tools version is valid and can be used by this

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

@@ -30,10 +30,12 @@ public struct PackageDependencyDescription: Equatable, Codable {
         }
     }
 
+    // FIXME: remove this and use identity instead
     /// The name of the package dependency explicitly defined in the manifest, if any.
     public let explicitName: String?
 
-    /// The name of the packagedependency,
+    // FIXME: remove this and use identity instead
+    /// The name of the package dependency,
     /// either explicitly defined in the manifest,
     /// or derived from the URL.
     ///
@@ -57,6 +59,7 @@ public struct PackageDependencyDescription: Equatable, Codable {
         requirement: Requirement,
         productFilter: ProductFilter = .everything
     ) {
+        // FIXME: remove this / move elsewhere. need a better model for location instead of string URL for both AbsolutePath and URLs
         // FIXME: SwiftPM can't handle file URLs with file:// scheme so we need to
         // strip that. We need to design a URL data structure for SwiftPM.
         let filePrefix = "file://"
@@ -68,7 +71,7 @@ public struct PackageDependencyDescription: Equatable, Codable {
         }
 
         self.explicitName = name
-        self.name = name ?? PackageReference.computeDefaultName(fromURL: normalizedURL)
+        self.name = name ?? LegacyPackageIdentity.computeDefaultName(fromURL: normalizedURL)
         self.url = normalizedURL
         self.requirement = requirement
         self.productFilter = productFilter

Sources/PackageModel/PackageIdentity.swift

@@ -51,6 +51,27 @@ public struct PackageIdentity: Hashable, CustomStringConvertible {
     public init(path: AbsolutePath) {
         self.init(path.pathString)
     }
+
+    /// Creates a package identity from a string
+    /// - Parameter name: An identity for the package.
+    public init(name: String) {
+        // use as-is!
+        self.init(name)
+    }
+}
+
+extension PackageIdentity {
+    public var mangledToC99ExtendedIdentifier: String {
+        get {
+            self.description.spm_mangledToC99ExtendedIdentifier()
+        }
+    }
+
+    public var mangledToBundleIdentifier: String {
+        get {
+            self.description.spm_mangledToBundleIdentifier()
+        }
+    }
 }
 
 extension PackageIdentity: Comparable {
@@ -94,7 +115,34 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
 
     /// Instantiates an instance of the conforming type from a string representation.
     public init(_ string: String) {
-        self.description = PackageReference.computeDefaultName(fromURL: string).lowercased()
+        self.description = Self.computeDefaultName(fromURL: string).lowercased()
+    }
+
+    /// Compute the default name of a package given its URL.
+    public static func computeDefaultName(fromURL url: String) -> String {
+        #if os(Windows)
+        let isSeparator : (Character) -> Bool = { $0 == "/" || $0 == "\\" }
+        #else
+        let isSeparator : (Character) -> Bool = { $0 == "/" }
+        #endif
+
+        // Get the last path component of the URL.
+        // Drop the last character in case it's a trailing slash.
+        var endIndex = url.endIndex
+        if let lastCharacter = url.last, isSeparator(lastCharacter) {
+            endIndex = url.index(before: endIndex)
+        }
+
+        let separatorIndex = url[..<endIndex].lastIndex(where: isSeparator)
+        let startIndex = separatorIndex.map { url.index(after: $0) } ?? url.startIndex
+        var lastComponent = url[startIndex..<endIndex]
+
+        // Strip `.git` suffix if present.
+        if lastComponent.hasSuffix(".git") {
+            lastComponent = lastComponent.dropLast(4)
+        }
+
+        return String(lastComponent)
     }
 }