From 2d511b449d219cec65814b9eba36100eb6738968 Mon Sep 17 00:00:00 2001 From: nickchapman-da <49153372+nickchapman-da@users.noreply.github.com> Date: Wed, 16 Dec 2020 14:38:25 +0000 Subject: [PATCH] Make choice observers mandatory when available. (#8316) * Make choice observers mandatory when available. This is an invariant of the DAML-LF proto. The haskell AST allows missing choice observers for any version, because the AST represents what was written in the syntax. However, if we are targeting a new DAML-LF version (>= `featureChoiceObservers`) then `Nothing` is converted to `Just (ENil TParty)` by the haskell encoder (`EncodeV1.hs`) to satisfy the proto invariant. On the scala side, the decoder (`DecodeV1.scala`) now insists that choice observers are present when_ the version is new. (>= `LV.Features.choiceObservers`). changelog_begin changelog_end * LF: LF encoder generate always choice observer if lf >= 1.dev (#8318) This is a follow up of #8316. This advances the state of #7709 CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Remy --- .../src/DA/Daml/LF/Proto3/EncodeV1.hs | 12 ++++++++- .../daml-test-files/ChoiceObservers.daml | 4 +-- .../com/daml/daml_lf_dev/daml_lf_1.proto | 2 +- .../daml/lf/archive/DecodeV1.scala | 1 + .../daml/lf/archive/DecodeV1Spec.scala | 16 +++++++++--- .../daml/lf/archive/testing/EncodeV1.scala | 13 ++++++++++ ...plate_all_.lf => Template_1.6_1.7_1.8_.lf} | 0 .../encoder/src/test/lf/Template_1.dev_.lf | 26 +++++++++++++++++++ .../lf/archive/testing/EncodeV1Spec.scala | 3 +-- 9 files changed, 67 insertions(+), 10 deletions(-) rename daml-lf/encoder/src/test/lf/{Template_all_.lf => Template_1.6_1.7_1.8_.lf} (100%) create mode 100644 daml-lf/encoder/src/test/lf/Template_1.dev_.lf diff --git a/compiler/daml-lf-proto/src/DA/Daml/LF/Proto3/EncodeV1.hs b/compiler/daml-lf-proto/src/DA/Daml/LF/Proto3/EncodeV1.hs index 2223f0aedb..4a8d79459c 100644 --- a/compiler/daml-lf-proto/src/DA/Daml/LF/Proto3/EncodeV1.hs +++ b/compiler/daml-lf-proto/src/DA/Daml/LF/Proto3/EncodeV1.hs @@ -20,6 +20,7 @@ import Data.Functor.Identity import qualified Data.HashMap.Strict as HMS import qualified Data.List as L import qualified Data.Map.Strict as Map +import Data.Maybe (fromMaybe) import qualified Data.NameMap as NM import qualified Data.Text as T import qualified Data.Text.Lazy as TL @@ -878,12 +879,21 @@ encodeTemplateKey TemplateKey{..} = do defTemplate_DefKeyMaintainers <- encodeExpr tplKeyMaintainers pure P.DefTemplate_DefKey{..} +encodeChoiceObservers :: Maybe Expr -> Encode (Just P.Expr) +encodeChoiceObservers chcObservers = do + EncodeEnv{..} <- get + -- The choice-observers field is mandatory when supported. So, when choice-observers are + -- not syntactically explicit, we generate an empty-party-list expression here. + if version `supports` featureChoiceObservers + then encodeExpr (fromMaybe (ENil TParty) chcObservers) + else traverse encodeExpr' chcObservers + encodeTemplateChoice :: TemplateChoice -> Encode P.TemplateChoice encodeTemplateChoice TemplateChoice{..} = do templateChoiceName <- encodeName unChoiceName chcName let templateChoiceConsuming = chcConsuming templateChoiceControllers <- encodeExpr chcControllers - templateChoiceObservers <- traverse encodeExpr' chcObservers + templateChoiceObservers <- encodeChoiceObservers chcObservers templateChoiceSelfBinder <- encodeName unExprVarName chcSelfBinder templateChoiceArgBinder <- Just <$> encodeExprVarWithType chcArgBinder templateChoiceRetType <- encodeType chcReturnType diff --git a/compiler/damlc/tests/daml-test-files/ChoiceObservers.daml b/compiler/damlc/tests/daml-test-files/ChoiceObservers.daml index 61a8a918c0..4f371962df 100644 --- a/compiler/damlc/tests/daml-test-files/ChoiceObservers.daml +++ b/compiler/damlc/tests/daml-test-files/ChoiceObservers.daml @@ -5,7 +5,7 @@ module ChoiceObservers where -- @SINCE-LF 1.dev --- @QUERY-LF [ .modules[] | .templates[] | select(lf::get_template_name($pkg) == ["TheTemplate"]) | .choices[] | select(lf::get_name($pkg) == "C1") | .observers ] == [ null ] +-- @QUERY-LF [ .modules[] | .templates[] | select(lf::get_template_name($pkg) == ["TheTemplate"]) | .choices[] | select(lf::get_name($pkg) == "C1") | .observers | has("nil") ] == [ true ] -- @QUERY-LF [ .modules[] | .templates[] | select(lf::get_template_name($pkg) == ["TheTemplate"]) | .choices[] | select(lf::get_name($pkg) == "C2") | .observers | has("app") ] == [ true ] @@ -30,7 +30,7 @@ template TheTemplate do return () choice C3 : () with ys : [Party] - observer ([]:[Party]) -- explicit empty choice observers; should be distinguishable from no observer clause + observer ([]:[Party]) -- explicit empty choice observers; semantically indistinguishable from no observer clause controller ys do return () diff --git a/daml-lf/archive/src/main/protobuf/com/daml/daml_lf_dev/daml_lf_1.proto b/daml-lf/archive/src/main/protobuf/com/daml/daml_lf_dev/daml_lf_1.proto index e3bee64cb4..52547dcf83 100644 --- a/daml-lf/archive/src/main/protobuf/com/daml/daml_lf_dev/daml_lf_1.proto +++ b/daml-lf/archive/src/main/protobuf/com/daml/daml_lf_dev/daml_lf_1.proto @@ -1281,7 +1281,7 @@ message TemplateChoice { Expr controllers = 3; // The additional informees of the choice. They have type `List Party`. - Expr observers = 11; // *Available in versions >= 1.dev* + Expr observers = 11; // *Mandatory in versions >= 1.dev* // Name to which the choice argument is bound and its type. VarWithType arg_binder = 4; diff --git a/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala b/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala index c94d9567d0..198e609157 100644 --- a/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala +++ b/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala @@ -583,6 +583,7 @@ private[archive] class DecodeV1(minor: LV.Minor) extends Decode.OfPackage[PLF.Pa assertSince(LV.Features.choiceObservers, "TemplateChoice.observers") Some(decodeExpr(lfChoice.getObservers, s"$tpl:$chName:observers")) } else { + assertUntil(LV.Features.choiceObservers, "missing TemplateChoice.observers") None }, selfBinder = selfBinder, diff --git a/daml-lf/archive/src/test/scala/com/digitalasset/daml/lf/archive/DecodeV1Spec.scala b/daml-lf/archive/src/test/scala/com/digitalasset/daml/lf/archive/DecodeV1Spec.scala index b15342bf90..623b395df0 100644 --- a/daml-lf/archive/src/test/scala/com/digitalasset/daml/lf/archive/DecodeV1Spec.scala +++ b/daml-lf/archive/src/test/scala/com/digitalasset/daml/lf/archive/DecodeV1Spec.scala @@ -946,13 +946,17 @@ class DecodeV1Spec val decoder = moduleDecoder(version, stringTable) decoder.decodeChoice(templateName, protoChoiceWithoutObservers) - a[ParseError] should be thrownBy decoder - .decodeChoice(templateName, protoChoiceWithObservers) + a[ParseError] should be thrownBy ( + decoder + .decodeChoice( + templateName, + protoChoiceWithObservers) + ) } } - "accept choice with or without observers if lv version >= 1.dev" in { + "reject choice without observers if lv version >= 1.dev" in { val unitTyp = DamlLf1.Type.newBuilder().setInterned(0).build() @@ -976,7 +980,11 @@ class DecodeV1Spec val decoder = moduleDecoder(version, stringTable, ImmArraySeq.empty, typeTable) - decoder.decodeChoice(templateName, protoChoiceWithoutObservers) + a[ParseError] should be thrownBy ( + decoder.decodeChoice( + templateName, + protoChoiceWithoutObservers) + ) decoder.decodeChoice(templateName, protoChoiceWithObservers) } } diff --git a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1.scala b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1.scala index 98fc9ac6da..9a6824963b 100644 --- a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1.scala +++ b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1.scala @@ -7,6 +7,7 @@ package testing import com.daml.lf.data.Ref._ import com.daml.lf.data._ import com.daml.lf.language.Ast._ +import com.daml.lf.language.{Util => AstUtil} import com.daml.lf.language.{LanguageVersion => LV} import com.daml.daml_lf_dev.{DamlLf1 => PLF} @@ -663,6 +664,14 @@ private[daml] class EncodeV1(minor: LV.Minor) { setString(name, b.setNameStr, b.setNameInternedStr) b.setConsuming(choice.consuming) b.setControllers(choice.controllers) + choice.choiceObservers match { + case Some(value) => + assertUntil(LV.Features.choiceObservers, "TemplateChoice.observer") + b.setObservers(value) + case None if languageVersion >= LV.Features.choiceObservers => + b.setObservers(ENil(AstUtil.TParty)) + case _ => + } b.setArgBinder(choice.argBinder._1 -> choice.argBinder._2) b.setRetType(choice.returnType) b.setUpdate(choice.update) @@ -733,6 +742,10 @@ private[daml] class EncodeV1(minor: LV.Minor) { if (versionIsOlderThan(minVersion)) throw EncodeError(s"$description is not supported by DAML-LF 1.$minor") + private def assertUntil(minVersion: LV, description: String): Unit = + if (versionIsOlderThan(minVersion)) + throw EncodeError(s"non empty $description is not supported by DAML-LF 1.$minor") + } object EncodeV1 { diff --git a/daml-lf/encoder/src/test/lf/Template_all_.lf b/daml-lf/encoder/src/test/lf/Template_1.6_1.7_1.8_.lf similarity index 100% rename from daml-lf/encoder/src/test/lf/Template_all_.lf rename to daml-lf/encoder/src/test/lf/Template_1.6_1.7_1.8_.lf diff --git a/daml-lf/encoder/src/test/lf/Template_1.dev_.lf b/daml-lf/encoder/src/test/lf/Template_1.dev_.lf new file mode 100644 index 0000000000..d71b3ac48d --- /dev/null +++ b/daml-lf/encoder/src/test/lf/Template_1.dev_.lf @@ -0,0 +1,26 @@ +// Copyright (c) 2020 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + + +module TemplateMod { + + record @serializable Person = { person: Party, name: Text } ; + + template (this : Person) = { + precondition True, + signatories Cons @Party [TemplateMod:Person {person} this] (Nil @Party), + observers (Nil @Party), + agreement "Agreement", + choices { + choice Sleep (self) (u: Unit) : ContractId TemplateMod:Person + , controllers Cons @Party [TemplateMod:Person {person} this] (Nil @Party) + , observers Cons @Party [TemplateMod:Person {person} this] (Nil @Party) + to upure @(ContractId TemplateMod:Person) self, + choice @nonConsuming Nap (self) (i : Int64) : Int64 + , controllers Cons @Party [TemplateMod:Person {person} this] (Nil @Party) + , observers Cons @Party [TemplateMod:Person {person} this] (Nil @Party) + to upure @Int64 i + } + } ; + +} diff --git a/daml-lf/encoder/src/test/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1Spec.scala b/daml-lf/encoder/src/test/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1Spec.scala index 4d35993d75..037202b2e2 100644 --- a/daml-lf/encoder/src/test/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1Spec.scala +++ b/daml-lf/encoder/src/test/scala/com/digitalasset/daml/lf/archive/testing/EncodeV1Spec.scala @@ -7,7 +7,6 @@ import com.daml.lf.archive.Decode import com.daml.lf.archive.testing.Encode import com.daml.lf.data.Ref._ import com.daml.lf.language.Ast._ -import com.daml.lf.language.LanguageMajorVersion.V1 import com.daml.lf.language.{Ast, LanguageVersion} import com.daml.lf.testing.parser.Implicits.SyntaxHelper import com.daml.lf.testing.parser.{AstRewriter, ParserParameters} @@ -25,7 +24,7 @@ class EncodeV1Spec extends AnyWordSpec with Matchers with TableDrivenPropertyChe val defaultParserParameters: ParserParameters[this.type] = ParserParameters( pkgId, - LanguageVersion(V1, LanguageVersion.Minor("8")) + LanguageVersion.StableVersions.max ) "Encode and Decode" should {