From 46f26aaa667fcbcf6390b1cf8bb386834ef192e4 Mon Sep 17 00:00:00 2001 From: dylant-da <106664681+dylant-da@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:19:18 +0100 Subject: [PATCH] Disallow exceptions repetition for 2.x (#20003) * Disallow upgrading exceptions in 2.x * lint * Replace setUpgradeField with True --- .../src/DA/Daml/LF/TypeChecker/Error.hs | 10 +++ .../src/DA/Daml/LF/TypeChecker/Upgrade.hs | 65 ++++++++++++++----- .../daml-opts-types/DA/Daml/Options/Types.hs | 6 +- sdk/compiler/damlc/lib/DA/Cli/Options.hs | 9 +++ sdk/compiler/damlc/tests/BUILD.bazel | 2 + .../Daml3ScriptTrySubmitConcurrently.daml | 3 +- .../daml-test-files/ExceptionSemantics.daml | 9 +-- .../daml-test-files/ExceptionSyntax.daml | 1 + .../daml-test-files/InterfaceGuarded.daml | 11 ++-- .../RestrictedNameWarnings.daml | 9 +-- .../tests/src/DA/Test/DamlcIntegration.hs | 2 +- .../damlc/tests/src/DA/Test/DamlcUpgrades.hs | 14 ++++ .../src/DA/Daml/Assistant/IntegrationTests.hs | 2 +- sdk/daml-lf/validation/BUILD.bazel | 4 ++ .../daml/lf/validation/Upgrading.scala | 46 ++++++++++--- .../validation/upgrade/UpgradesSpecBase.scala | 22 +++++++ sdk/docs/BUILD.bazel | 31 +++++++-- sdk/test-common/BUILD.bazel | 2 + .../v1/Main.daml | 11 ++++ .../v2/Main.daml | 11 ++++ .../v1/Main.daml | 11 ++++ .../v2/Main.daml | 6 ++ 22 files changed, 238 insertions(+), 49 deletions(-) create mode 100644 sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v1/Main.daml create mode 100644 sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v2/Main.daml create mode 100644 sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v1/Main.daml create mode 100644 sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v2/Main.daml diff --git a/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs b/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs index 0d9a1ee0a381..192d6ccbb217 100644 --- a/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs +++ b/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs @@ -221,6 +221,7 @@ data UnwarnableError | EForbiddenNewImplementation !TypeConName !TypeConName | EUpgradeDependenciesFormACycle ![(PackageId, Maybe PackageMetadata)] | EUpgradeMultiplePackagesWithSameNameAndVersion !PackageName !RawPackageVersion ![PackageId] + | EUpgradeTriedToUpgradeException !TypeConName | EUpgradeDifferentParamsCount !UpgradedRecordOrigin | EUpgradeDifferentParamsKinds !UpgradedRecordOrigin deriving (Show) @@ -231,6 +232,7 @@ data WarnableError | WEUpgradeShouldDefineTplInSeparatePackage !TypeConName !TypeConName | WEDependencyHasUnparseableVersion !PackageName !PackageVersion !PackageUpgradeOrigin | WEDependencyHasNoMetadataDespiteUpgradeability !PackageId !PackageUpgradeOrigin + | WEUpgradeShouldDefineExceptionsAndTemplatesSeparately deriving (Eq, Show) instance Pretty WarnableError where @@ -259,6 +261,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) @@ -696,6 +704,8 @@ instance Pretty UnwarnableError where pprintDep (pkgId, Just meta) = pPrint pkgId <> "(" <> pPrint (packageName meta) <> ", " <> pPrint (packageVersion meta) <> ")" pprintDep (pkgId, Nothing) = pPrint pkgId 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." EUpgradeDifferentParamsCount origin -> "The upgraded " <> pPrint origin <> " has changed the number of type variables it has." EUpgradeDifferentParamsKinds origin -> "The upgraded " <> pPrint origin <> " has changed the kind of one of its type variables." diff --git a/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs b/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs index 23826cb8ca91..a8a5488a0017 100644 --- a/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs +++ b/sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs @@ -66,7 +66,7 @@ shouldTypecheckM = asks (uncurry shouldTypecheck) mkGamma :: Version -> UpgradeInfo -> World -> Gamma mkGamma version upgradeInfo world = - let addBadIfaceSwapIndicator :: Gamma -> Gamma + let addBadIfaceSwapIndicator, addBadExceptionSwapIndicator :: Gamma -> Gamma addBadIfaceSwapIndicator = if uiWarnBadInterfaceInstances upgradeInfo then @@ -76,8 +76,15 @@ mkGamma version upgradeInfo world = Left WEUpgradeShouldDefineIfacesAndTemplatesSeparately {} -> Just True _ -> Nothing) else id + addBadExceptionSwapIndicator = + if uiWarnBadExceptions upgradeInfo + then + addDiagnosticSwapIndicator (\case + Left WEUpgradeShouldDefineExceptionsAndTemplatesSeparately {} -> Just True + _ -> Nothing) + else id in - addBadIfaceSwapIndicator $ emptyGamma world version + addBadExceptionSwapIndicator $ addBadIfaceSwapIndicator $ emptyGamma world version gammaM :: World -> TcPreUpgradeM Gamma gammaM world = asks (flip (uncurry mkGamma) world) @@ -131,7 +138,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) @@ -146,7 +153,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 @@ -411,42 +418,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 @@ -516,6 +531,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 @@ -531,13 +557,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 . packageModules . getWorldSelf <$> 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 diff --git a/sdk/compiler/damlc/daml-opts/daml-opts-types/DA/Daml/Options/Types.hs b/sdk/compiler/damlc/daml-opts/daml-opts-types/DA/Daml/Options/Types.hs index 64c23ec8d336..5a7d8383f1ef 100644 --- a/sdk/compiler/damlc/daml-opts/daml-opts-types/DA/Daml/Options/Types.hs +++ b/sdk/compiler/damlc/daml-opts/daml-opts-types/DA/Daml/Options/Types.hs @@ -37,6 +37,7 @@ module DA.Daml.Options.Types , UpgradeInfo (..) , defaultUiTypecheckUpgrades , defaultUiWarnBadInterfaceInstances + , defaultUiWarnBadExceptions , defaultUpgradeInfo ) where @@ -141,6 +142,7 @@ data UpgradeInfo = UpgradeInfo { uiUpgradedPackagePath :: Maybe FilePath , uiTypecheckUpgrades :: Bool , uiWarnBadInterfaceInstances :: Bool + , uiWarnBadExceptions :: Bool } newtype IncrementalBuild = IncrementalBuild { getIncrementalBuild :: Bool } @@ -291,11 +293,13 @@ defaultUpgradeInfo = UpgradeInfo { uiUpgradedPackagePath = Nothing , uiTypecheckUpgrades = defaultUiTypecheckUpgrades , uiWarnBadInterfaceInstances = defaultUiWarnBadInterfaceInstances + , uiWarnBadExceptions = defaultUiWarnBadExceptions } -defaultUiTypecheckUpgrades, defaultUiWarnBadInterfaceInstances :: Bool +defaultUiTypecheckUpgrades, defaultUiWarnBadInterfaceInstances, defaultUiWarnBadExceptions :: Bool defaultUiTypecheckUpgrades = True defaultUiWarnBadInterfaceInstances = False +defaultUiWarnBadExceptions = False pkgNameVersion :: LF.PackageName -> Maybe LF.PackageVersion -> UnitId pkgNameVersion (LF.PackageName n) mbV = diff --git a/sdk/compiler/damlc/lib/DA/Cli/Options.hs b/sdk/compiler/damlc/lib/DA/Cli/Options.hs index b29c8f515e82..9bded69eb6a9 100644 --- a/sdk/compiler/damlc/lib/DA/Cli/Options.hs +++ b/sdk/compiler/damlc/lib/DA/Cli/Options.hs @@ -587,11 +587,20 @@ optionsParser numProcessors enableScenarioService parsePkgName parseDlintUsage = "Convert errors about bad, non-upgradeable interface instances into warnings." idm + optWarnBadExceptions :: Parser Bool + optWarnBadExceptions = + flagYesNoAuto + "warn-bad-exceptions" + defaultUiWarnBadExceptions + "Convert errors about bad, non-upgradeable exceptions into warnings." + idm + optUpgradeInfo :: Parser UpgradeInfo optUpgradeInfo = do uiTypecheckUpgrades <- optTypecheckUpgrades uiUpgradedPackagePath <- optUpgradeDar uiWarnBadInterfaceInstances <- optWarnBadInterfaceInstances + uiWarnBadExceptions <- optWarnBadExceptions pure UpgradeInfo {..} optGhcCustomOptions :: Parser [String] diff --git a/sdk/compiler/damlc/tests/BUILD.bazel b/sdk/compiler/damlc/tests/BUILD.bazel index eba1e2ffea95..634c00171146 100644 --- a/sdk/compiler/damlc/tests/BUILD.bazel +++ b/sdk/compiler/damlc/tests/BUILD.bazel @@ -457,6 +457,7 @@ da_haskell_test( "//test-common:upgrades-FailsWhenATopLevelVariantAddsAFieldToAVariantsType-files", "//test-common:upgrades-FailsWhenATopLevelVariantAddsAVariant-files", "//test-common:upgrades-FailsWhenATopLevelVariantRemovesAVariant-files", + "//test-common:upgrades-FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage-files", "//test-common:upgrades-FailsWhenAnInstanceIsAddedSeparateDep-files", "//test-common:upgrades-FailsWhenAnInstanceIsAddedUpgradedPackage-files", "//test-common:upgrades-FailsWhenAnInstanceIsDropped-files", @@ -496,6 +497,7 @@ da_haskell_test( "//test-common:upgrades-SucceedsWhenATopLevelTypeSynonymChanges-files", "//test-common:upgrades-SucceedsWhenATopLevelVariantAddsAVariant-files", "//test-common:upgrades-SucceedsWhenATopLevelVariantAddsAnOptionalFieldToAVariantsType-files", + "//test-common:upgrades-SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage-files", "//test-common:upgrades-SucceedsWhenAnInstanceIsAddedToNewTemplateSeparateDep-files", "//test-common:upgrades-SucceedsWhenAnInstanceIsAddedToNewTemplateUpgradedPackage-files", "//test-common:upgrades-SucceedsWhenAnInterfaceIsOnlyDefinedInTheInitialPackage-files", diff --git a/sdk/compiler/damlc/tests/daml-test-files/Daml3ScriptTrySubmitConcurrently.daml b/sdk/compiler/damlc/tests/daml-test-files/Daml3ScriptTrySubmitConcurrently.daml index 817e3c6a2186..1501496ee67a 100644 --- a/sdk/compiler/damlc/tests/daml-test-files/Daml3ScriptTrySubmitConcurrently.daml +++ b/sdk/compiler/damlc/tests/daml-test-files/Daml3ScriptTrySubmitConcurrently.daml @@ -3,7 +3,8 @@ -- @ SCRIPT-V2 --- @ WARN range=19:1-19:52; Import of internal module Daml.Script.Internal of package daml3-script is discouraged, as this module will change without warning. +-- @ WARN range=20:1-20:52; Import of internal module Daml.Script.Internal of package daml3-script is discouraged, as this module will change without warning. +-- @ WARN warn-bad-exceptions {-# LANGUAGE ApplicativeDo #-} diff --git a/sdk/compiler/damlc/tests/daml-test-files/ExceptionSemantics.daml b/sdk/compiler/damlc/tests/daml-test-files/ExceptionSemantics.daml index a7669e9678cf..3da8a57f73bc 100644 --- a/sdk/compiler/damlc/tests/daml-test-files/ExceptionSemantics.daml +++ b/sdk/compiler/damlc/tests/daml-test-files/ExceptionSemantics.daml @@ -2,10 +2,11 @@ -- SPDX-License-Identifier: Apache-2.0 -- @SUPPORTS-LF-FEATURE DAML_EXCEPTIONS --- @ERROR range=132:1-132:23; Unhandled exception: ExceptionSemantics:E --- @ERROR range=147:1-147:25; Unhandled exception: DA.Exception.ArithmeticError:ArithmeticError@XXXXXX with message = "ArithmeticError while evaluating (DIV_INT64 1 0)." --- @WARN range=181:3-181:12; Use of divulged contracts is deprecated --- @ERROR range=184:1-184:11; Attempt to exercise a consumed contract +-- @ERROR range=133:1-133:23; Unhandled exception: ExceptionSemantics:E +-- @ERROR range=148:1-148:25; Unhandled exception: DA.Exception.ArithmeticError:ArithmeticError@XXXXXX with message = "ArithmeticError while evaluating (DIV_INT64 1 0)." +-- @WARN range=182:3-182:12; Use of divulged contracts is deprecated +-- @ERROR range=185:1-185:11; Attempt to exercise a consumed contract +-- @WARN since-lf=1.17 2.0; warn-bad-exceptions module ExceptionSemantics where import Daml.Script diff --git a/sdk/compiler/damlc/tests/daml-test-files/ExceptionSyntax.daml b/sdk/compiler/damlc/tests/daml-test-files/ExceptionSyntax.daml index 34a6d7f755b7..6fbbc2ca4b05 100644 --- a/sdk/compiler/damlc/tests/daml-test-files/ExceptionSyntax.daml +++ b/sdk/compiler/damlc/tests/daml-test-files/ExceptionSyntax.daml @@ -2,6 +2,7 @@ -- SPDX-License-Identifier: Apache-2.0 -- @SUPPORTS-LF-FEATURE DAML_EXCEPTIONS +-- @WARN since-lf=1.17 2.0; warn-bad-exceptions -- @QUERY-LF [ $pkg.modules[].exceptions[] ] | length == 1 -- | Test that exception syntax is correctly handled. diff --git a/sdk/compiler/damlc/tests/daml-test-files/InterfaceGuarded.daml b/sdk/compiler/damlc/tests/daml-test-files/InterfaceGuarded.daml index cd1306a4911f..56a315670f07 100644 --- a/sdk/compiler/damlc/tests/daml-test-files/InterfaceGuarded.daml +++ b/sdk/compiler/damlc/tests/daml-test-files/InterfaceGuarded.daml @@ -6,6 +6,7 @@ -- @WARN since-lf=1.16 2.0; warn-bad-interface-instances -- @WARN since-lf=1.16 2.0; warn-bad-interface-instances -- @WARN since-lf=1.16 2.0; warn-bad-interface-instances +-- @WARN since-lf=1.16 2.0; warn-bad-exceptions module InterfaceGuarded where @@ -146,17 +147,17 @@ exerciseTest c = script do trueGuard = exerciseTest TrueGuard --- @ERROR range=150:1-150:11; failing guard +-- @ERROR range=151:1-151:11; failing guard falseGuard = exerciseTest FalseGuard --- @ERROR range=153:1-153:11; failing guard +-- @ERROR range=154:1-154:11; failing guard errorGuard = exerciseTest ErrorGuard --- @ERROR range=156:1-156:17; failing guard +-- @ERROR range=157:1-157:17; failing guard customErrorGuard = exerciseTest CustomErrorGuard --- @ERROR range=159:1-159:14; failing guard +-- @ERROR range=160:1-160:14; failing guard tryErrorGuard = exerciseTest TryErrorGuard --- @ERROR range=162:1-162:20; failing guard +-- @ERROR range=163:1-163:20; failing guard tryCustomErrorGuard = exerciseTest TryCustomErrorGuard diff --git a/sdk/compiler/damlc/tests/daml-test-files/RestrictedNameWarnings.daml b/sdk/compiler/damlc/tests/daml-test-files/RestrictedNameWarnings.daml index 18c4a831e708..36cf15641407 100644 --- a/sdk/compiler/damlc/tests/daml-test-files/RestrictedNameWarnings.daml +++ b/sdk/compiler/damlc/tests/daml-test-files/RestrictedNameWarnings.daml @@ -1,7 +1,8 @@ --- @WARN range=9:5-9:9; `self' is an unsupported field name, and may break without warning in future versions. Please use something else. --- @WARN range=14:5-14:8; `arg' is an unsupported field name, and may break without warning in future versions. Please use something else. --- @WARN range=19:5-19:9; `self' is an unsupported field name, and may break without warning in future versions. Please use something else. --- @WARN range=24:5-24:8; `arg' is an unsupported field name, and may break without warning in future versions. Please use something else. +-- @WARN range=10:5-10:9; `self' is an unsupported field name, and may break without warning in future versions. Please use something else. +-- @WARN range=15:5-15:8; `arg' is an unsupported field name, and may break without warning in future versions. Please use something else. +-- @WARN range=20:5-20:9; `self' is an unsupported field name, and may break without warning in future versions. Please use something else. +-- @WARN range=25:5-25:8; `arg' is an unsupported field name, and may break without warning in future versions. Please use something else. +-- @WARN since-lf=1.17 2.0; warn-bad-exceptions module RestrictedNameWarnings where diff --git a/sdk/compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs b/sdk/compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs index 9ffc322665fe..17a5a51b52f9 100644 --- a/sdk/compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs +++ b/sdk/compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs @@ -329,7 +329,7 @@ getIntegrationTests registerTODO scenarioService (packageDbPath, packageFlags) = , dlintHintFiles = NoDlintHintFiles } , optPackageImports = packageFlags - , optUpgradeInfo = (optUpgradeInfo opts0) { uiWarnBadInterfaceInstances = True } + , optUpgradeInfo = (optUpgradeInfo opts0) { uiWarnBadInterfaceInstances = True, uiWarnBadExceptions = True } } mkIde options = do diff --git a/sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs b/sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs index c43a96bcc859..7571faa87b26 100644 --- a/sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs +++ b/sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs @@ -618,6 +618,20 @@ tests damlc = "my-package" "0.0.2" version1_dev + , test + "SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage" + Succeed + versionDefault + NoDependencies + False + True + , 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 + True , test "FailWhenParamCountChanges" (FailWithError "\ESC\\[0;91merror type checking data type Main.MyStruct:\n The upgraded data type MyStruct has changed the number of type variables it has.") diff --git a/sdk/daml-assistant/integration-tests/src/DA/Daml/Assistant/IntegrationTests.hs b/sdk/daml-assistant/integration-tests/src/DA/Daml/Assistant/IntegrationTests.hs index 36ffecf870d8..d25e751b428c 100644 --- a/sdk/daml-assistant/integration-tests/src/DA/Daml/Assistant/IntegrationTests.hs +++ b/sdk/daml-assistant/integration-tests/src/DA/Daml/Assistant/IntegrationTests.hs @@ -123,7 +123,7 @@ damlStart tmpDir disableUpgradeValidation = do , " npm-scope: daml.js" , " java:" , " output-directory: ui/java" - ] ++ [ "build-options:\n- --warn-bad-interface-instances=yes" | disableUpgradeValidation ] + ] ++ [ "build-options:\n- --warn-bad-interface-instances=yes\n- --warn-bad-exceptions=yes" | disableUpgradeValidation ] writeFileUTF8 (projDir "daml/Main.daml") $ unlines [ "module Main where" diff --git a/sdk/daml-lf/validation/BUILD.bazel b/sdk/daml-lf/validation/BUILD.bazel index 02c34b42b796..a476977d284e 100644 --- a/sdk/daml-lf/validation/BUILD.bazel +++ b/sdk/daml-lf/validation/BUILD.bazel @@ -284,6 +284,10 @@ 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 = False, scala_deps = [ diff --git a/sdk/daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/Upgrading.scala b/sdk/daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/Upgrading.scala index d3a5dfcc3954..c5e8fc69eb99 100644 --- a/sdk/daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/Upgrading.scala +++ b/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 @@ -406,27 +411,36 @@ 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))) } } @@ -441,9 +455,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) @@ -457,6 +472,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) ) @@ -485,6 +503,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] = diff --git a/sdk/daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/upgrade/UpgradesSpecBase.scala b/sdk/daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/upgrade/UpgradesSpecBase.scala index fa9e9bf31257..24f0499463f2 100644 --- a/sdk/daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/upgrade/UpgradesSpecBase.scala +++ b/sdk/daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/upgrade/UpgradesSpecBase.scala @@ -772,6 +772,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." + ) + ), + ) + } } } diff --git a/sdk/docs/BUILD.bazel b/sdk/docs/BUILD.bazel index 8d2e5096f6fc..a29a8d8373ba 100644 --- a/sdk/docs/BUILD.bazel +++ b/sdk/docs/BUILD.bazel @@ -422,14 +422,20 @@ java_binary( daml_test( name = "ledger-api-daml-test", srcs = glob(["source/app-dev/code-snippets/**/*.daml"]), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], deps = ["//daml-script/daml:daml-script.dar"], ) daml_test( name = "bindings-java-daml-test", srcs = glob(["source/app-dev/bindings-java/code-snippets/**/*.daml"]), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], # FIXME: https://github.com/digital-asset/daml/issues/12051 # remove target, once interfaces are stable. target = lf_version_configuration.get("latest"), @@ -458,7 +464,10 @@ daml_test( name = "daml-ref-daml-test", timeout = "long", srcs = glob(["source/daml/code-snippets/**/*.daml"]), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], deps = ["//daml-script/daml:daml-script.dar"], ) @@ -467,7 +476,10 @@ daml_test( name = "daml-ref-daml-test-{}".format(version), timeout = "long", srcs = glob(["source/daml/code-snippets-dev/**/*.daml"]), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], target = version, ) for version in LF_DEV_VERSIONS @@ -527,7 +539,10 @@ daml_test( srcs = glob( ["source/daml/intro/daml/daml-intro-12-part1/**/*.daml"], ), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], target = "1.15", deps = ["//daml-script/daml:daml-script-1.15.dar"], ) @@ -546,7 +561,10 @@ daml_test( srcs = glob( ["source/daml/intro/daml/daml-intro-12-part2/**/*.daml"], ), - additional_compiler_flags = ["--warn-bad-interface-instances=yes"], + additional_compiler_flags = [ + "--warn-bad-interface-instances=yes", + "--warn-bad-exceptions=yes", + ], data_deps = ["daml-intro12-part1.dar"], target = "1.15", deps = ["//daml-script/daml:daml-script-1.15.dar"], @@ -572,6 +590,7 @@ daml_test( daml_test( name = "daml-intro-8-daml-test-{}".format(version), srcs = glob(["source/daml/intro/daml/daml-intro-8/**/*.daml"]), + additional_compiler_flags = ["--warn-bad-exceptions=yes"], target = version, deps = ["//daml-script/daml:daml-script-{}.dar".format(version)], ) diff --git a/sdk/test-common/BUILD.bazel b/sdk/test-common/BUILD.bazel index 3cdea57310d8..48de5df37c4f 100644 --- a/sdk/test-common/BUILD.bazel +++ b/sdk/test-common/BUILD.bazel @@ -451,6 +451,8 @@ da_scala_dar_resources_library( # More more more tests ported from DamlcUpgrades.hs ("FailsWhenAnInterfaceIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage", {}, {}), ("SucceedsWhenAnInterfaceIsOnlyDefinedInTheInitialPackage", {}, {}), + ("FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage", {}, {}), + ("SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage", {}, {}), ( "FailsWhenAnInstanceIsAddedUpgradedPackage", {}, diff --git a/sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v1/Main.daml b/sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v1/Main.daml new file mode 100644 index 000000000000..65facf6ad9a2 --- /dev/null +++ b/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 } diff --git a/sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v2/Main.daml b/sdk/test-common/src/main/daml/upgrades/FailsWhenAnExceptionIsDefinedInAnUpgradingPackageWhenItWasAlreadyInThePriorPackage/v2/Main.daml new file mode 100644 index 000000000000..65facf6ad9a2 --- /dev/null +++ b/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 } diff --git a/sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v1/Main.daml b/sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v1/Main.daml new file mode 100644 index 000000000000..65facf6ad9a2 --- /dev/null +++ b/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 } diff --git a/sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v2/Main.daml b/sdk/test-common/src/main/daml/upgrades/SucceedsWhenAnExceptionIsOnlyDefinedInTheInitialPackage/v2/Main.daml new file mode 100644 index 000000000000..c431dc708459 --- /dev/null +++ b/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 }