From 6253be0a4a34b0314ee3fc876ece368eefb21c32 Mon Sep 17 00:00:00 2001 From: Sofia Faro Date: Mon, 1 Nov 2021 11:52:28 +0000 Subject: [PATCH 1/2] Check fixed choices in name collision checker This is a follow up to #11364. This PR checks that the list of inherited choice names is correct in the type checker, and moves the "fixed choice name collision" check into the name collision checker using this data. Haskell side only for now. Part of #11137. changelog_begin changelog_end --- .../src/DA/Daml/LF/TypeChecker/Check.hs | 22 ++++++------------- .../src/DA/Daml/LF/TypeChecker/Error.hs | 7 ++++++ .../DA/Daml/LF/TypeChecker/NameCollision.hs | 8 +++++++ .../InterfaceChoiceCollision.daml | 2 +- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs index 77b4bb8f32e1..bc72a4d5fe90 100644 --- a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs +++ b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs @@ -885,27 +885,18 @@ checkTemplate m t@(Template _loc tpl param precond signatories observers text ch whenJust mbKey $ checkTemplateKey param tcon forM_ implements $ checkIfaceImplementation tcon - -- Check template choice and interface fixed choice name collisions. - foldM_ checkFixedChoiceCollision (S.fromList (NM.names choices)) implements - -- ^ We don't use NM.namesSet here because Data.HashSet is assymptotically - -- slower than Data.Set when it comes to unions and checking for disjointness. - where withPart p = withContext (ContextTemplate m t p) - checkFixedChoiceCollision :: S.Set ChoiceName -> TemplateImplements -> m (S.Set ChoiceName) - checkFixedChoiceCollision !accum ifaceImpl = do - iface <- inWorld $ lookupInterface (tpiInterface ifaceImpl) - let newNames = S.fromList (NM.names (intFixedChoices iface)) - unless (S.disjoint accum newNames) $ do - let choiceName = head (S.toList (S.intersection accum newNames)) - throwWithContext (EDuplicateTemplateChoiceViaInterfaces tpl choiceName) - pure (S.union accum newNames) - checkIfaceImplementation :: MonadGamma m => Qualified TypeConName -> TemplateImplements -> m () checkIfaceImplementation tplTcon TemplateImplements{..} = do let tplName = qualObject tplTcon - DefInterface {intVirtualChoices, intMethods} <- inWorld $ lookupInterface tpiInterface + DefInterface {intFixedChoices, intVirtualChoices, intMethods} <- inWorld $ lookupInterface tpiInterface + + -- check fixed choices + let inheritedChoices = S.fromList (NM.names intFixedChoices) + unless (inheritedChoices == tpiInheritedChoiceNames) $ + throwWithContext $ EBadInheritedChoices tpiInterface (S.toList inheritedChoices) (S.toList tpiInheritedChoiceNames) -- check virtual choices forM_ intVirtualChoices $ \InterfaceChoice {ifcName, ifcConsuming, ifcArgType, ifcRetType} -> do @@ -929,6 +920,7 @@ checkIfaceImplementation tplTcon TemplateImplements{..} = do Just InterfaceMethod{ifmType} -> checkExpr tpiMethodExpr (TCon tplTcon :-> ifmType) + _checkFeature :: MonadGamma m => Feature -> m () _checkFeature feature = do version <- getLfVersion diff --git a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs index c28cafd9513e..6958d8136d4f 100644 --- a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs +++ b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs @@ -135,6 +135,7 @@ data Error | EDuplicateInterfaceChoiceName !TypeConName !ChoiceName | EDuplicateInterfaceMethodName !TypeConName !MethodName | EUnknownInterface !TypeConName + | EBadInheritedChoices { ebicInterface :: !(Qualified TypeConName), ebicExpected :: ![ChoiceName], ebicGot :: ![ChoiceName] } | EMissingInterfaceChoice !ChoiceName | EBadInterfaceChoiceImplConsuming !ChoiceName !Bool !Bool | EBadInterfaceChoiceImplArgType !ChoiceName !Type !Type @@ -389,6 +390,12 @@ instance Pretty Error where EDuplicateInterfaceMethodName iface method -> "Duplicate method name '" <> pretty method <> "' in interface definition for " <> pretty iface EUnknownInterface tcon -> "Unknown interface: " <> pretty tcon + EBadInheritedChoices {ebicInterface, ebicExpected, ebicGot} -> + vcat + [ "List of inherited choices does not match interface definition for " <> pretty ebicInterface + , "Expected: " <> pretty ebicExpected + , "But got: " <> pretty ebicGot + ] EMissingInterfaceChoice ch -> "Missing interface choice implementation for " <> pretty ch EBadInterfaceChoiceImplConsuming ch ifaceConsuming tplConsuming -> vcat diff --git a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/NameCollision.hs b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/NameCollision.hs index 6f1f1cb19f7c..2131964a8fb8 100644 --- a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/NameCollision.hs +++ b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/NameCollision.hs @@ -38,6 +38,7 @@ data Name | NEnumCon ModuleName TypeConName VariantConName | NField ModuleName TypeConName FieldName | NChoice ModuleName TypeConName ChoiceName + | NChoiceViaInterface ModuleName TypeConName ChoiceName (Qualified TypeConName) | NInterface ModuleName TypeConName -- | Helper method so we can turn collisions with virtual modules into warnings @@ -69,6 +70,8 @@ displayName = \case T.concat ["field ", dot m, ":", dot t, ".", f] NChoice (ModuleName m) (TypeConName t) (ChoiceName c) -> T.concat ["choice ", dot m, ":", dot t, ".", c] + NChoiceViaInterface (ModuleName m) (TypeConName t) (ChoiceName c) (Qualified _ (ModuleName imod) (TypeConName ityp)) -> + T.concat ["choice ", dot m, ":", dot t, ".", c, " (via interface ", dot imod, ":", dot ityp, ")"] NInterface (ModuleName m) (TypeConName t) -> T.concat ["interface ", dot m, ":", dot t] where @@ -125,6 +128,8 @@ fullyResolve = FRName . map T.toLower . \case m ++ t ++ [f] NChoice (ModuleName m) (TypeConName t) (ChoiceName c) -> m ++ t ++ [c] + NChoiceViaInterface (ModuleName m) (TypeConName t) (ChoiceName c) _ -> + m ++ t ++ [c] NInterface (ModuleName m) (TypeConName t) -> m ++ t @@ -199,6 +204,9 @@ checkTemplate :: ModuleName -> Template -> NCMonad () checkTemplate moduleName Template{..} = do forM_ tplChoices $ \TemplateChoice{..} -> checkName (NChoice moduleName tplTypeCon chcName) + forM_ tplImplements $ \TemplateImplements{..} -> + forM_ tpiInheritedChoiceNames $ \choiceName -> + checkName (NChoiceViaInterface moduleName tplTypeCon choiceName tpiInterface) checkSynonym :: ModuleName -> DefTypeSyn -> NCMonad () checkSynonym moduleName DefTypeSyn{..} = diff --git a/compiler/damlc/tests/daml-test-files/InterfaceChoiceCollision.daml b/compiler/damlc/tests/daml-test-files/InterfaceChoiceCollision.daml index d63aec8f058c..26ea7febe62c 100644 --- a/compiler/damlc/tests/daml-test-files/InterfaceChoiceCollision.daml +++ b/compiler/damlc/tests/daml-test-files/InterfaceChoiceCollision.daml @@ -2,7 +2,7 @@ -- SPDX-License-Identifier: Apache-2.0 -- @SINCE-LF-FEATURE DAML_INTERFACE --- @ERROR Duplicate choice name 'MyArchive' in template T via interfaces. +-- @ERROR name collision between choice InterfaceChoiceCollision:T.MyArchive (via interface InterfaceChoiceCollision:InterfaceA) and choice InterfaceChoiceCollision:T.MyArchive (via interface InterfaceChoiceCollision:InterfaceB) module InterfaceChoiceCollision where interface InterfaceA where From 172d91473167b85238682d4542666e4cbcf82375 Mon Sep 17 00:00:00 2001 From: Sofia Faro Date: Mon, 1 Nov 2021 13:59:50 +0000 Subject: [PATCH 2/2] Update compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs Co-authored-by: Moritz Kiefer --- compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs index bc72a4d5fe90..50430f76c3c9 100644 --- a/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs +++ b/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Check.hs @@ -920,7 +920,6 @@ checkIfaceImplementation tplTcon TemplateImplements{..} = do Just InterfaceMethod{ifmType} -> checkExpr tpiMethodExpr (TCon tplTcon :-> ifmType) - _checkFeature :: MonadGamma m => Feature -> m () _checkFeature feature = do version <- getLfVersion