Apply Paul's suggested changes

- Change _package to literal identifier `package`
- Use type synonym PackageMap where possible
- Fix binding of _new and _deleted in wrong order
- Refactor `splitPackageVersion` and `comparePackageVersion`

sdk/canton/community/ledger/ledger-api-core/src/main/scala/com/digitalasset/canton/platform/apiserver/services/admin/PackageUpgradeValidator.scala

@@ -193,7 +193,7 @@ class PackageUpgradeValidator(
       typecheckPhase: TypecheckUpgrades.UploadPhaseCheck,
       optNewDar1: Option[(Ref.PackageId, Ast.Package)],
       optOldDar2: Option[(Ref.PackageId, Ast.Package)],
-      packageMap: Map[Ref.PackageId, (Ref.PackageName, Ref.PackageVersion)],
+      packageMap: PackageMap,
   )(implicit
       loggingContext: LoggingContextWithTrace
   ): Future[Unit] = {

sdk/compiler/daml-lf-ast/src/DA/Daml/LF/Ast/Util.hs

@@ -327,11 +327,15 @@ splitUnitId (unitIdString -> unitId) = fromMaybe (PackageName (T.pack unitId), N
 
 -- | Take a package version of regex "(0|[1-9][0-9]*)(\.(0|[1-9][0-9]*))*" into
 -- a list of integers [Integer]
-splitPackageVersion :: PackageVersion -> Maybe [Integer]
+splitPackageVersion
-splitPackageVersion (PackageVersion raw) =
+  :: (PackageVersion -> a) -> PackageVersion
+  -> Either a [Integer]
+splitPackageVersion mkError version@(PackageVersion raw) =
   let pieces = T.split (== '.') raw
   in
-  traverse (readMaybe . T.unpack) pieces
+  case traverse (readMaybe . T.unpack) pieces of
+    Nothing -> Left (mkError version)
+    Just versions -> Right versions
 
 data ComparePackageVersionError
   = FirstVersionUnparseable PackageVersion
@@ -339,15 +343,11 @@ data ComparePackageVersionError
   deriving (Show, Eq, Ord)
 
 comparePackageVersion :: PackageVersion -> PackageVersion -> Either ComparePackageVersionError Ordering
-comparePackageVersion v1 v2 =
+comparePackageVersion v1 v2 = do
-  case splitPackageVersion v1 of
+  v1Pieces <- splitPackageVersion FirstVersionUnparseable v1
-    Nothing -> Left $ FirstVersionUnparseable v1
+  v2Pieces <- splitPackageVersion SecondVersionUnparseable v2
-    Just v1Pieces ->
+  let pad xs target =
-      case splitPackageVersion v2 of
+        take
-        Nothing -> Left $ SecondVersionUnparseable v2
+          (length target `max` length xs)
-        Just v2Pieces ->
+          (xs ++ repeat 0)
-          let pad xs target
+  pure $ compare (pad v1Pieces v2Pieces) (pad v2Pieces v1Pieces)
-                | length xs < length target = xs ++ replicate (length target - length xs) 0
-                | otherwise = xs
-          in
-          pure $ compare (pad v1Pieces v2Pieces) (pad v2Pieces v1Pieces)

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

@@ -343,7 +343,7 @@ case class TypecheckUpgrades(
 ) {
   import TypecheckUpgrades._
 
-  private lazy val _package: Upgrading[Ast.Package] = packages.map(_._2)
+  private lazy val `package`: Upgrading[Ast.Package] = packages.map(_._2)
   private lazy val dependencies
       : Upgrading[Map[Ref.PackageId, (Ref.PackageName, Ref.PackageVersion)]] = packages.map(_._3)
 
@@ -351,7 +351,7 @@ case class TypecheckUpgrades(
     for {
       (upgradedModules, newModules @ _) <-
         checkDeleted(
-          _package.map(_.modules),
+          `package`.map(_.modules),
           (name: Ref.ModuleName, _: Ast.Module) => UpgradeError.MissingModule(name),
         )
       _ <- checkDeps()
@@ -360,7 +360,7 @@ case class TypecheckUpgrades(
   }
 
   private def checkDeps(): Try[Unit] = {
-    val (_new @ _, existing, _deleted @ _) = extractDelExistNew(dependencies.map(_.values.toMap))
+    val (_deleted @ _, existing, _new @ _) = extractDelExistNew(dependencies.map(_.values.toMap))
     tryAll(existing, checkDep).map(_ => ())
   }