Disallow upgrading exceptions in 3.x

sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs

@@ -219,6 +219,7 @@ data UnwarnableError
   | EForbiddenNewImplementation !TypeConName !TypeConName
   | EUpgradeDependenciesFormACycle ![(PackageId, PackageMetadata)]
   | EUpgradeMultiplePackagesWithSameNameAndVersion !PackageName !RawPackageVersion ![PackageId]
+  | EUpgradeTriedToUpgradeException !TypeConName
   deriving (Show)
 
 data WarnableError
@@ -227,6 +228,7 @@ data WarnableError
   | WEUpgradeShouldDefineTplInSeparatePackage !TypeConName !TypeConName
   | WEDependencyHasUnparseableVersion !PackageName !PackageVersion !PackageUpgradeOrigin
   | WEDependencyHasNoMetadataDespiteUpgradeability !PackageId !PackageUpgradeOrigin
+  | WEUpgradeShouldDefineExceptionsAndTemplatesSeparately
   deriving (Eq, Show)
 
 instance Pretty WarnableError where
@@ -255,6 +257,12 @@ instance Pretty WarnableError where
       "Dependency " <> pPrint pkgName <> " of " <> pPrint packageOrigin <> " has a version which cannot be parsed: '" <> pPrint version <> "'"
     WEDependencyHasNoMetadataDespiteUpgradeability pkgId packageOrigin ->
       "Dependency with package ID " <> pPrint pkgId <> " of " <> pPrint packageOrigin <> " has no metadata, despite being compiled with an SDK version that supports metadata."
+    WEUpgradeShouldDefineExceptionsAndTemplatesSeparately ->
+      vsep
+        [ "This package defines both exceptions and templates. This may make this package and its dependents not upgradeable."
+        , "It is recommended that exceptions are defined in their own package separate from their implementations."
+        --, "Ignore this error message with the --warn-bad-exceptions=yes flag."
+        ]
 
 data PackageUpgradeOrigin = UpgradingPackage | UpgradedPackage
   deriving (Eq, Ord, Show)
@@ -687,6 +695,8 @@ instance Pretty UnwarnableError where
       where
       pprintDep (pkgId, meta) = pPrint pkgId <> "(" <> pPrint (packageName meta) <> ", " <> pPrint (packageVersion meta) <> ")"
     EUpgradeMultiplePackagesWithSameNameAndVersion name version ids -> "Multiple packages with name " <> pPrint name <> " and version " <> pPrint (show version) <> ": " <> hcat (L.intersperse ", " (map pPrint ids))
+    EUpgradeTriedToUpgradeException exception ->
+      "Tried to upgrade exception " <> pPrint exception <> ", but exceptions cannot be upgraded. They should be removed in any upgrading package."
 
 instance Pretty UpgradedRecordOrigin where
   pPrint = \case

sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs

@@ -130,7 +130,7 @@ checkPackageSingle mbContext pkg =
   withReaderT (\(version, upgradeInfo) -> mkGamma version upgradeInfo presentWorld) $
     withMbContext $ do
       checkNewInterfacesAreUnused pkg
-      checkNewInterfacesHaveNoTemplates
+      checkInterfacesAndExceptionsHaveNoTemplates
 
 type UpgradedPkgWithNameAndVersion = (LF.PackageId, LF.Package, LF.PackageName, Maybe LF.PackageVersion)
 
@@ -145,7 +145,7 @@ checkModule world0 module_ deps version upgradeInfo mbUpgradedPkg =
       let world = extendWorldSelf module_ world0
       withReaderT (\(version, upgradeInfo) -> mkGamma version upgradeInfo world) $ do
         checkNewInterfacesAreUnused module_
-        checkNewInterfacesHaveNoTemplates
+        checkInterfacesAndExceptionsHaveNoTemplates
       case mbUpgradedPkg of
         Nothing -> pure ()
         Just (upgradedPkgWithId@(upgradedPkgIdRaw, upgradedPkg, _, _), upgradingDeps) -> do
@@ -410,42 +410,50 @@ checkModuleM upgradedPackageId module_ = do
 
     let ifaceDts :: Upgrading (HMS.HashMap LF.TypeConName (DefDataType, DefInterface))
         unownedDts :: Upgrading (HMS.HashMap LF.TypeConName DefDataType)
-        (ifaceDts, unownedDts) =
+        exceptionDts :: Upgrading (HMS.HashMap LF.TypeConName (DefDataType, DefException))
+        (ifaceDts, exceptionDts, unownedDts) =
             let Upgrading
-                    { _past = (pastIfaceDts, pastUnownedDts)
-                    , _present = (presentIfaceDts, presentUnownedDts)
+                    { _past = (pastIfaceDts, pastExceptionDts, pastUnownedDts)
+                    , _present = (presentIfaceDts, presentExceptionDts, presentUnownedDts)
                     } = fmap splitModuleDts module_
             in
             ( Upgrading pastIfaceDts presentIfaceDts
+            , Upgrading pastExceptionDts presentExceptionDts
             , Upgrading pastUnownedDts presentUnownedDts
             )
 
         splitModuleDts
             :: Module
             -> ( HMS.HashMap LF.TypeConName (DefDataType, DefInterface)
+               , HMS.HashMap LF.TypeConName (DefDataType, DefException)
                , HMS.HashMap LF.TypeConName DefDataType)
         splitModuleDts module_ =
-            let (ifaceDtsList, unownedDtsList) =
-                    partitionEithers
-                        $ map (\(tcon, def) -> lookupInterface module_ tcon def)
+            let (ifaceDtsList, (exceptionDtsList, unownedDtsList)) =
+                    fmap partitionEithers $ partitionEithers
+                        $ map (\(tcon, def) -> lookupInterfaceOrException module_ tcon def)
                         $ HMS.toList $ NM.toHashMap $ moduleDataTypes module_
             in
-            (HMS.fromList ifaceDtsList, HMS.fromList unownedDtsList)
+            (HMS.fromList ifaceDtsList, HMS.fromList exceptionDtsList, HMS.fromList unownedDtsList)
 
-        lookupInterface
+        lookupInterfaceOrException
             :: Module -> LF.TypeConName -> DefDataType
-            -> Either (LF.TypeConName, (DefDataType, DefInterface)) (LF.TypeConName, DefDataType)
-        lookupInterface module_ tcon datatype =
+            -> Either (LF.TypeConName, (DefDataType, DefInterface)) (Either (LF.TypeConName, (DefDataType, DefException)) (LF.TypeConName, DefDataType))
+        lookupInterfaceOrException module_ tcon datatype =
             case NM.name datatype `NM.lookup` moduleInterfaces module_ of
-              Nothing -> Right (tcon, datatype)
+              Nothing -> Right $
+                case NM.name datatype `NM.lookup` moduleExceptions module_ of
+                  Nothing -> Right (tcon, datatype)
+                  Just exception -> Left (tcon, (datatype, exception))
               Just iface -> Left (tcon, (datatype, iface))
 
     -- Check that no interfaces have been deleted, nor propagated
-    -- New interface checks are handled by `checkNewInterfacesHaveNoTemplates`,
+    -- New interface checks are handled by `checkInterfacesAndExceptionsHaveNoTemplates`,
     -- invoked in `singlePkgDiagnostics` above
     -- Interface deletion is the correct behaviour so we ignore that
     let (_ifaceDel, ifaceExisting, _ifaceNew) = extractDelExistNew ifaceDts
     checkContinuedIfaces module_ ifaceExisting
+    let (_exceptionDel, exceptionExisting, _exceptionNew) = extractDelExistNew exceptionDts
+    checkContinuedExceptions module_ exceptionExisting
 
     let flattenInstances
             :: Module
@@ -515,6 +523,17 @@ checkContinuedIfaces module_ ifaces =
         withContextF present' (ContextDefInterface (_present module_) iface IPWhole) $
             throwWithContextF present' $ EUpgradeTriedToUpgradeIface (NM.name iface)
 
+checkContinuedExceptions
+    :: Upgrading Module
+    -> HMS.HashMap LF.TypeConName (Upgrading (DefDataType, DefException))
+    -> TcUpgradeM ()
+checkContinuedExceptions module_ exceptions =
+    forM_ exceptions $ \upgradedDtException ->
+        let (_dt, exception) = _present upgradedDtException
+        in
+        withContextF present' (ContextDefException (_present module_) exception) $
+            throwWithContextF present' $ EUpgradeTriedToUpgradeException (NM.name exception)
+
 class HasModules a where
   getModules :: a -> NM.NameMap LF.Module
 
@@ -530,13 +549,16 @@ instance HasModules [LF.Module] where
 -- Check that a module or package does not define both interfaces and templates.
 -- This warning should trigger even when no previous version DAR is specified in
 -- the `upgrades:` field.
-checkNewInterfacesHaveNoTemplates :: TcM ()
-checkNewInterfacesHaveNoTemplates = do
+checkInterfacesAndExceptionsHaveNoTemplates :: TcM ()
+checkInterfacesAndExceptionsHaveNoTemplates = do
     modules <- NM.toList . getWorldSelfPkgModules <$> getWorld
     let templateDefined = filter (not . NM.null . moduleTemplates) modules
         interfaceDefined = filter (not . NM.null . moduleInterfaces) modules
+        exceptionDefined = filter (not . NM.null . moduleExceptions) modules
     when (not (null templateDefined) && not (null interfaceDefined)) $
         diagnosticWithContext WEUpgradeShouldDefineIfacesAndTemplatesSeparately
+    when (not (null templateDefined) && not (null exceptionDefined)) $
+        diagnosticWithContext WEUpgradeShouldDefineExceptionsAndTemplatesSeparately
 
 -- Check that any interfaces defined in this package or module do not also have
 -- an instance. Interfaces defined in other packages are allowed to have

sdk/compiler/damlc/tests/BUILD.bazel

@@ -463,6 +463,7 @@ da_haskell_test(
         "//test-common:upgrades-FailsWhenAnInstanceIsAddedUpgradedPackage-files",
         "//test-common:upgrades-FailsWhenAnInstanceIsDropped-files",
         "//test-common:upgrades-FailsWhenAnInterfaceIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-files",
+        "//test-common:upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-files",
         "//test-common:upgrades-FailsWhenDatatypeChangesVariety-files",
         "//test-common:upgrades-FailsWhenDependencyIsNotAValidUpgrade-files",
         "//test-common:upgrades-FailsWhenDepsDowngradeVersionsWhileUsingDatatypes-files",
@@ -499,6 +500,7 @@ da_haskell_test(
         "//test-common:upgrades-SucceedsWhenAnInstanceIsAddedToNewTemplateSeparateDep-files",
         "//test-common:upgrades-SucceedsWhenAnInstanceIsAddedToNewTemplateUpgradedPackage-files",
         "//test-common:upgrades-SucceedsWhenAnInterfaceIsOnlyDefinedInTheInitialPackage-files",
+        "//test-common:upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-files",
         "//test-common:upgrades-SucceedsWhenDepsDowngradeVersionsWithoutUsingDatatypes-files",
         "//test-common:upgrades-SucceedsWhenNewFieldWithOptionalTypeIsAddedToTemplate-files",
         "//test-common:upgrades-SucceedsWhenNewFieldWithOptionalTypeIsAddedToTemplateChoice-files",

sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs

@@ -509,6 +509,20 @@ tests damlc =
                 --      (SeparateDeps False)
                 --      False
                 --      setUpgradeField
+                , test
+                      "SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage"
+                      Succeed
+                      versionDefault
+                      NoDependencies
+                      False
+                      setUpgradeField
+                , test
+                      "FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage"
+                      (FailWithError "\ESC\\[0;91merror type checking exception Main.E:\n  Tried to upgrade exception E, but exceptions cannot be upgraded. They should be removed in any upgrading package.")
+                      versionDefault
+                      NoDependencies
+                      False
+                      setUpgradeField
                 ]
             | setUpgradeField <- [True, False]
             ] ++

sdk/daml-lf/validation/BUILD.bazel

@@ -277,6 +277,11 @@ da_scala_test_suite(
             "//test-common:upgrades-SucceedsWhenDepsDowngradeVersionsWithoutUsingDatatypes-dep-v2.dar",
             "//test-common:upgrades-SucceedsWhenDepsDowngradeVersionsWithoutUsingDatatypes-v1.dar",
             "//test-common:upgrades-SucceedsWhenDepsDowngradeVersionsWithoutUsingDatatypes-v2.dar",
+
+            "//test-common:upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-v1.dar",
+            "//test-common:upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-v2.dar",
+            "//test-common:upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-v1.dar",
+            "//test-common:upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-v2.dar",
         ],
         flaky = True,
         scala_deps = [

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

@@ -201,6 +201,11 @@ object UpgradeError {
     override def message: String =
       s"Implementation of interface $iface by template $tpl appears in this package, but does not appear in package that is being upgraded."
   }
+
+  final case class TriedToUpgradeException(exception: Ref.DottedName) extends Error {
+    override def message: String =
+      s"Tried to upgrade exception $exception, but exceptions cannot be upgraded. They should be removed in any upgrading package."
+  }
 }
 
 sealed abstract class UpgradedRecordOrigin
@@ -567,27 +572,35 @@ case class TypecheckUpgrades(
       module: Ast.Module
   ): (
       Map[Ref.DottedName, (Ast.DDataType, Ast.DefInterface)],
+      Map[Ref.DottedName, (Ast.DDataType, Ast.DefException)],
       Map[Ref.DottedName, Ast.DDataType],
   ) = {
     val datatypes: Map[Ref.DottedName, Ast.DDataType] = module.definitions.collect({
       case (name, dt: Ast.DDataType) => (name, dt)
     })
-    val (ifaces, other) = datatypes.partitionMap({ case (tcon, dt) =>
-      lookupInterface(module, tcon, dt)
+    val (ifaces, other1) = datatypes.partitionMap({ case (tcon, dt) =>
+      lookupInterfaceOrException(module, tcon, dt)
     })
-    (ifaces.toMap, other.filter(_._2.serializable).toMap)
+    val (exceptions, other) = other1.partitionMap(identity)
+    (ifaces.toMap, exceptions.toMap, other.filter(_._2.serializable).toMap)
   }
 
-  private def lookupInterface(
+  private def lookupInterfaceOrException(
       module: Ast.Module,
       tcon: Ref.DottedName,
       dt: Ast.DDataType,
   ): Either[
     (Ref.DottedName, (Ast.DDataType, Ast.DefInterface)),
-    (Ref.DottedName, Ast.DDataType),
+    Either[
+      (Ref.DottedName, (Ast.DDataType, Ast.DefException)),
+      (Ref.DottedName, Ast.DDataType),
+    ]
   ] = {
     module.interfaces.get(tcon) match {
-      case None => Right((tcon, dt))
+      case None => Right(module.exceptions.get(tcon) match {
+        case None => Right((tcon, dt))
+        case Some(exception) => Left((tcon, (dt, exception)))
+      })
       case Some(iface) => Left((tcon, (dt, iface)))
     }
   }
@@ -602,9 +615,10 @@ case class TypecheckUpgrades(
   }
 
   private def checkModule(module: Upgrading[Ast.Module]): Try[Unit] = {
-    val (pastIfaceDts, pastUnownedDts) = splitModuleDts(module.past)
-    val (presentIfaceDts, presentUnownedDts) = splitModuleDts(module.present)
+    val (pastIfaceDts, pastExceptionDts, pastUnownedDts) = splitModuleDts(module.past)
+    val (presentIfaceDts, presentExceptionDts, presentUnownedDts) = splitModuleDts(module.present)
     val ifaceDts = Upgrading(past = pastIfaceDts, present = presentIfaceDts)
+    val exceptionDts = Upgrading(past = pastExceptionDts, present = presentExceptionDts)
     val unownedDts = Upgrading(past = pastUnownedDts, present = presentUnownedDts)
 
     val moduleWithMetadata = module.map(ModuleWithMetadata)
@@ -618,6 +632,9 @@ case class TypecheckUpgrades(
       (_ifaceDel, ifaceExisting, _ifaceNew) = extractDelExistNew(ifaceDts)
       _ <- checkContinuedIfaces(ifaceExisting)
 
+      (_exceptionDel, exceptionExisting, _exceptionNew) = extractDelExistNew(exceptionDts)
+      _ <- checkContinuedExceptions(exceptionExisting)
+
       (instanceDel, _instanceExisting, instanceNew) = extractDelExistNew(
         module.map(flattenInstances)
       )
@@ -646,6 +663,18 @@ case class TypecheckUpgrades(
     ).map(_ => ())
   }
 
+  private def checkContinuedExceptions(
+      exceptions: Map[Ref.DottedName, Upgrading[(Ast.DDataType, Ast.DefException)]]
+  ): Try[Unit] = {
+    tryAll(
+      exceptions,
+      (arg: (Ref.DottedName, Upgrading[(Ast.DDataType, Ast.DefException)])) => {
+        val (name, _) = arg
+        fail(UpgradeError.TriedToUpgradeException(name))
+      },
+    ).map(_ => ())
+  }
+
   private def checkDeletedInstances(
       deletedInstances: Map[(Ref.DottedName, TypeConName), (Ast.Template, Ast.TemplateImplements)]
   ): Try[Unit] =

sdk/daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/upgrade/UpgradesSpecBase.scala

@@ -619,6 +619,28 @@ trait LongTests { this: UpgradesSpec =>
       )
     }
 
+    "Succeeds when an exception is only defined in the initial package." in {
+      testPackagePair(
+        "test-common/upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-v1.dar",
+        "test-common/upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-v2.dar",
+        assertPackageUpgradeCheck(
+          None
+        ),
+      )
+    }
+
+    "Fails when an exception is defined in an upgrading package when it was already in the prior package." in {
+      testPackagePair(
+        "test-common/upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-v1.dar",
+        "test-common/upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-v2.dar",
+        assertPackageUpgradeCheck(
+          Some(
+            "Tried to upgrade exception E, but exceptions cannot be upgraded. They should be removed in any upgrading package."
+          )
+        ),
+      )
+    }
+
     def mkTrivialPkg(pkgName: String, pkgVersion: String, lfVersion: LanguageVersion) = {
       import com.digitalasset.daml.lf.testing.parser._
       import com.digitalasset.daml.lf.testing.parser.Implicits._

sdk/test-common/BUILD.bazel

@@ -316,6 +316,8 @@ da_scala_dar_resources_library(
         # More more more tests ported from DamlcUpgrades.hs
         ("FailsWhenAnInterfaceIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage", {}, {}),
         ("SucceedsWhenAnInterfaceIsOnlyDefinedInTheInitialPackage", {}, {}),
+        ("FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage", {}, {}),
+        ("SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage", {}, {}),
         ("SucceedWhenATopLevelEnumAddsAField", {}, {}),
         ("SucceedsWhenATopLevelVariantAddsAnOptionalFieldToAVariantsType", {}, {}),
         ("WarnsWhenAnInterfaceAndATemplateAreDefinedInTheSamePackage", {}, {}),

sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v1/Main.daml

@@ -0,0 +1,11 @@
+-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+module Main where
+
+exception E with
+    field1 : Text
+  where
+    message field1
+
+data IsSchemaPackage = IsSchemaPackage { isSchemaPackage : Text }

sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v2/Main.daml

@@ -0,0 +1,11 @@
+-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+module Main where
+
+exception E with
+    field1 : Text
+  where
+    message field1
+
+data IsSchemaPackage = IsSchemaPackage { isSchemaPackage : Text }

sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v1/Main.daml

@@ -0,0 +1,11 @@
+-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+module Main where
+
+exception E with
+    field1 : Text
+  where
+    message field1
+
+data IsSchemaPackage = IsSchemaPackage { isSchemaPackage : Text }

sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v2/Main.daml

@@ -0,0 +1,6 @@
+-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+module Main where
+
+data IsSchemaPackage = IsSchemaPackage { isSchemaPackage : Text }