LF: Kill ValueStruct (#7457)

Struct is not serializable and therefore should not appear as Value. CHANGELOG_BEGIN CHANGELOG_END
digital-asset · Sep 22, 2020 · f5694ee · f5694ee
1 parent a899e5e
commit f5694ee
Show file tree

Hide file tree

Showing 23 changed files with 20 additions and 120 deletions.
diff --git a/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs b/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs
@@ -681,8 +681,6 @@ prettyValue' showRecordType prec world (Value (Just vsum)) = case vsum of
        then \fs -> prettyMay "" (prettyDefName world) mbRecordId <-> keyword_ "with" $$ nest 2 fs
        else id)
       (sep (punctuate ";" (mapV prettyField fields)))
-  ValueSumTuple (Tuple fields) ->
-    braces (sep (punctuate ";" (mapV prettyField fields)))
   ValueSumVariant (Variant mbVariantId ctor mbValue) ->
         prettyMay "" (\v -> prettyDefName world v <> ":") mbVariantId <> ltext ctor
     <-> prettyMay "<missing value>" (prettyValue' True precHighest world) mbValue

diff --git a/compiler/scenario-service/protos/scenario_service.proto b/compiler/scenario-service/protos/scenario_service.proto
@@ -266,10 +266,6 @@ message Record {
   repeated Field fields = 2;
 }
 
-message Tuple {
-  repeated Field fields = 1;
-}
-
 message Variant {
   Identifier variant_id = 1;
   string constructor = 2;
@@ -298,25 +294,24 @@ message Environment {
 message Value {
   oneof Sum {
     Record record = 1;
-    Tuple tuple = 2;
-    Variant variant = 3;
-    string contract_id = 4;
-    List list = 5;
-    sint64 int64 = 6;
-    string decimal = 7;
-    string text = 8;
-    sfixed64 timestamp = 9;
-    string party = 10;
-    bool bool = 11;
-    Empty unit = 12;
-    int32 date = 13;
-    Optional optional = 15;
-    Map map = 16;
-    Enum enum = 17;
-    GenMap gen_map = 18;
+    Variant variant = 2;
+    string contract_id = 3;
+    List list = 4;
+    sint64 int64 = 5;
+    string decimal = 6;
+    string text = 7;
+    sfixed64 timestamp = 8;
+    string party = 9;
+    bool bool = 10;
+    Empty unit = 11;
+    int32 date = 12;
+    Optional optional = 13;
+    Map map = 14;
+    Enum enum = 15;
+    GenMap gen_map = 16;
 
     // An unserializable value, e.g. closure. Contains a description of the value.
-    string unserializable = 14;
+    string unserializable = 17;
   }
 }
 

diff --git a/...cenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Conversions.scala b/...cenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Conversions.scala
@@ -624,22 +624,6 @@ final class Conversions(
             )
             .build,
         )
-      case V.ValueStruct(fields) =>
-        builder.setTuple(
-          proto.Tuple.newBuilder
-            .addAllFields(
-              fields.iterator
-                .map { field =>
-                  proto.Field.newBuilder
-                    .setLabel(field._1)
-                    .setValue(convertValue(field._2))
-                    .build
-                }
-                .toSeq
-                .asJava,
-            )
-            .build,
-        )
       case V.ValueVariant(tycon, variant, value) =>
         val vbuilder = proto.Variant.newBuilder
         tycon.foreach(x => vbuilder.setVariantId(convertIdentifier(x)))

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/scenario/ScenarioLedger.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/scenario/ScenarioLedger.scala
@@ -311,8 +311,6 @@ object ScenarioLedger {
           fs.foreach {
             case (_, v) => collect(v)
           }
-        case ValueStruct(fs) =>
-          fs.values.foreach(collect)
         case ValueVariant(_, _, arg) => collect(arg)
         case _: ValueEnum => ()
         case ValueList(vs) =>

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala
@@ -347,12 +347,6 @@ private[lf] object Pretty {
               text("<no-label>") & char('=') & prettyValue(true)(v)
           }) &
           char('}')
-      case ValueStruct(fs) =>
-        char('{') &
-          fill(text(", "), fs.toImmArray.toSeq.map {
-            case (k, v) => text(k) & char('=') & prettyValue(true)(v)
-          }) &
-          char('}')
       case ValueVariant(mbId, variant, value) =>
         (mbId match {
           case None => text("")

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SValue.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SValue.scala
@@ -32,9 +32,7 @@ sealed trait SValue {
       case SBool(x) => V.ValueBool(x)
       case SUnit => V.ValueUnit
       case SDate(x) => V.ValueDate(x)
-      case SStruct(fields, svalues) =>
-        val valuesIterator = svalues.iterator()
-        V.ValueStruct(fields.mapValues(_ => valuesIterator.next().toValue))
+
       case SRecord(id, fields, svalues) =>
         V.ValueRecord(
           Some(id),
@@ -61,6 +59,8 @@ sealed trait SValue {
         V.ValueGenMap(ImmArray(entries.map { case (k, v) => k.toValue -> v.toValue }))
       case SContractId(coid) =>
         V.ValueContractId(coid)
+      case SStruct(_, _) =>
+        throw SErrorCrash("SValue.toValue: unexpected SStruct")
       case SAny(_, _) =>
         throw SErrorCrash("SValue.toValue: unexpected SAny")
       case STypeRep(_) =>

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala
@@ -586,13 +586,6 @@ private[lf] object Speedy {
             SRecord(id, names, values)
           case V.ValueRecord(None, _) =>
             crash("SValue.fromValue: record missing identifier")
-          case V.ValueStruct(fs) =>
-            val values = new util.ArrayList[SValue](fs.size)
-            val fieldNames = fs.mapValues { v =>
-              values.add(go(v))
-              ()
-            }
-            SStruct(fieldNames, values)
           case V.ValueVariant(None, _variant @ _, _value @ _) =>
             crash("SValue.fromValue: variant without identifier")
           case V.ValueEnum(None, constructor @ _) =>

diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala
@@ -221,8 +221,6 @@ object Hash {
         case Value.ValueGenMap(_) =>
           error("Hashing of generic map not implemented")
         // Struct: should never be encountered
-        case Value.ValueStruct(_) =>
-          error("Hashing of struct values is not supported")
       }
   }
 

diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala
@@ -51,8 +51,6 @@ sealed abstract class Value[+Cid] extends CidContainer[Value[Cid]] with Product
         val newNesting = nesting + 1
 
         v match {
-          case tpl: ValueStruct[Cid] =>
-            go(exceededNesting, errs :+ s"contains struct $tpl", vs)
           case ValueRecord(_, flds) =>
             if (newNesting > MAXIMUM_NESTING) {
               if (exceededNesting) {
@@ -156,8 +154,6 @@ object Value extends CidContainer1[Value] {
           ValueRecord(id, fs.map({
             case (lbl, value) => (lbl, go(value))
           }))
-        case ValueStruct(fs) =>
-          ValueStruct(fs.mapValues(go))
         case ValueVariant(id, variant, value) =>
           ValueVariant(id, variant, go(value))
         case x: ValueCidlessLeaf => x
@@ -180,8 +176,6 @@ object Value extends CidContainer1[Value] {
           f(coid)
         case ValueRecord(id @ _, fs) =>
           fs.foreach { case (_, value) => go(value) }
-        case ValueStruct(fs) =>
-          fs.values.foreach(go)
         case ValueVariant(id @ _, variant @ _, value) =>
           go(value)
         case _: ValueCidlessLeaf =>
@@ -280,10 +274,6 @@ object Value extends CidContainer1[Value] {
   final case class ValueOptional[+Cid](value: Option[Value[Cid]]) extends Value[Cid]
   final case class ValueTextMap[+Cid](value: SortedLookupList[Value[Cid]]) extends Value[Cid]
   final case class ValueGenMap[+Cid](entries: ImmArray[(Value[Cid], Value[Cid])]) extends Value[Cid]
-  // this is present here just because we need it in some internal code --
-  // specifically the scenario interpreter converts committed values to values and
-  // currently those can be structs, although we should probably ban that.
-  final case class ValueStruct[+Cid](fields: Struct[Value[Cid]]) extends Value[Cid]
 
   /** The data constructors of a variant or enum, if defined. */
   type LookupVariantEnum = Identifier => Option[ImmArray[Name]]
@@ -496,7 +486,6 @@ private final class `Value Order instance`[Cid: Order](Scope: Value.LookupVarian
           ctorOrder(idA, idB, a, b)
       })
     case ValueRecord(_, a) => (140, k { case ValueRecord(_, b) => _2.T.subst(a) ?|? _2.T.subst(b) })
-    case ValueStruct(a) => (150, k { case ValueStruct(b) => a ?|? b })
     case ValueVariant(idA, conA, a) =>
       (160, k {
         case ValueVariant(idB, conB, b) =>
@@ -572,10 +561,6 @@ private final class `Value Equal instance`[Cid: Equal] extends Equal[Value[Cid]]
         case ValueOptional(value2) =>
           value === value2
       }
-      case ValueStruct(fields) => {
-        case ValueStruct(fields2) =>
-          fields === fields2
-      }
       case ValueTextMap(map1) => {
         case ValueTextMap(map2) =>
           map1 === map2

diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala
@@ -525,8 +525,6 @@ object ValueCoder {
             }
             builder.setGenMap(protoMap).build()
 
-          case ValueStruct(fields) =>
-            throw Err(s"Trying to serialize struct, which are not serializable. Fields: $fields")
         }
       }
     }

diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueVersion.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueVersion.scala
@@ -82,9 +82,6 @@ private[lf] object ValueVersions
                 go(maxVV(minGenMap, currentVersion), newValues)
               case ValueEnum(_, _) =>
                 go(maxVV(minEnum, currentVersion), values)
-              // structs are a no-no
-              case ValueStruct(fields) =>
-                Left(s"Got struct when trying to assign version. Fields: $fields")
             }
         }
       }

diff --git a/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueCoderSpec.scala b/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueCoderSpec.scala
@@ -173,14 +173,6 @@ class ValueCoderSpec extends WordSpec with Matchers with EitherAssertions with P
       }
     }
 
-    "don't struct" in {
-      val fields = List(Ref.Name.assertFromString("foo") -> ValueInt64(42))
-      val struct = ValueStruct(Struct.assertFromSeq(fields))
-      val res =
-        ValueCoder.encodeValue[ContractId](ValueCoder.CidEncoder, defaultValueVersion, struct)
-      res.left.get.errorMessage should include("serializable")
-    }
-
     "do unit" in {
       val recovered = ValueCoder.decodeValue(
         ValueCoder.CidDecoder,

diff --git a/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala b/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala
@@ -4,7 +4,7 @@
 package com.daml.lf
 package value
 
-import data.{Bytes, FrontStack, ImmArray, Ref, Struct}
+import data.{Bytes, ImmArray, Ref}
 import Value._
 import Ref.{Identifier, Name}
 import test.ValueGenerators.{coidGen, idGen, nameGen}
@@ -31,8 +31,6 @@ class ValueSpec
   import ValueSpec._
 
   "serialize" - {
-    val emptyStruct = ValueStruct(Struct.Empty)
-    val emptyStructError = "contains struct ValueStruct(Struct())"
     val exceedsNesting = (1 to MAXIMUM_NESTING + 1).foldRight[Value[Nothing]](ValueInt64(42)) {
       case (_, v) => ValueVariant(None, Ref.Name.assertFromString("foo"), v)
     }
@@ -41,26 +39,13 @@ class ValueSpec
       case (_, v) => ValueVariant(None, Ref.Name.assertFromString("foo"), v)
     }
 
-    "rejects struct" in {
-      emptyStruct.serializable shouldBe ImmArray(emptyStructError)
-    }
-
-    "rejects nested struct" in {
-      ValueList(FrontStack(emptyStruct)).serializable shouldBe ImmArray(emptyStructError)
-    }
-
     "rejects excessive nesting" in {
       exceedsNesting.serializable shouldBe ImmArray(exceedsNestingError)
     }
 
     "accepts just right nesting" in {
       matchesNesting.serializable shouldBe ImmArray.empty
     }
-
-    "outputs both error messages, without duplication" in {
-      ValueList(FrontStack(exceedsNesting, ValueStruct(Struct.Empty), exceedsNesting)).serializable shouldBe
-        ImmArray(exceedsNestingError, emptyStructError)
-    }
   }
 
   "VersionedValue" - {

diff --git a/extractor/src/main/scala/com/digitalasset/extractor/writers/postgresql/Queries.scala b/extractor/src/main/scala/com/digitalasset/extractor/writers/postgresql/Queries.scala
@@ -305,9 +305,6 @@ object Queries {
           Fragment("?::jsonb", toJsonString(value))
         case V.ValueGenMap(entries) =>
           Fragment("?::jsonb", toJsonString(entries))
-        case struct @ V.ValueStruct(_) =>
-          throw new IllegalArgumentException(
-            s"struct should not be present in contract, as raw structs are not serializable: $struct")
       }
     }
   }

diff --git a/.../http-json/src/test/scala/com/digitalasset/http/util/ApiValueToLfValueConverterTest.scala b/.../http-json/src/test/scala/com/digitalasset/http/util/ApiValueToLfValueConverterTest.scala
@@ -61,7 +61,6 @@ object ApiValueToLfValueConverterTest {
       case _: V.ValueCidlessLeaf | V.ValueContractId(_) => fa
       case r @ V.ValueRecord(_, fields) => r copy (fields = fields map (_ rightMap go))
       case v @ V.ValueVariant(_, _, value) => v copy (value = go(value))
-      case s @ V.ValueStruct(fields) => s copy (fields = fields mapValues go)
       case V.ValueList(fs) => V.ValueList(fs map go)
       case V.ValueOptional(o) => V.ValueOptional(o map go)
       case V.ValueTextMap(m) => V.ValueTextMap(m mapValue go)

diff --git a/...lf-value-json/src/main/scala/com/digitalasset/daml/lf/value/json/ApiCodecCompressed.scala b/...lf-value-json/src/main/scala/com/digitalasset/daml/lf/value/json/ApiCodecCompressed.scala
@@ -64,7 +64,6 @@ class ApiCodecCompressed[Cid](val encodeDecimalAsString: Boolean, val encodeInt6
       apiMapToJsValue(textMap)
     case genMap: V.ValueGenMap[Cid] =>
       apiGenMapToJsValue(genMap)
-    case _: V.ValueStruct[Cid] => serializationError("impossible! structs are not serializable")
   }
 
   @throws[SerializationException]

diff --git a/...value-json/src/main/scala/com/digitalasset/daml/lf/value/json/NavigatorModelAliases.scala b/...value-json/src/main/scala/com/digitalasset/daml/lf/value/json/NavigatorModelAliases.scala
@@ -77,7 +77,6 @@ trait NavigatorModelAliases[Cid] {
   type ApiOptional = OfCid[V.ValueOptional]
   type ApiMap = OfCid[V.ValueTextMap]
   type ApiGenMap = OfCid[V.ValueGenMap]
-  type ApiImpossible = OfCid[V.ValueStruct]
 }
 
 object NavigatorModelAliases extends NavigatorModelAliases[String]
diff --git a/...-api-common/src/main/scala/com/digitalasset/platform/participant/util/LfEngineToApi.scala b/...-api-common/src/main/scala/com/digitalasset/platform/participant/util/LfEngineToApi.scala
@@ -114,7 +114,6 @@ object LfEngineToApi {
               } yield api.GenMap.Entry(Some(key), Some(value)) :: tail
           }
           .map(list => api.Value(api.Value.Sum.GenMap(api.GenMap(list))))
-      case Lf.ValueStruct(_) => Left("structs not allowed")
       case Lf.ValueList(vs) =>
         vs.toImmArray.toSeq.traverseEitherStrictly(lfValueToApiValue(verbose, _)) map { xs =>
           api.Value(api.Value.Sum.List(api.List(xs)))

diff --git a/...r-api-common/src/main/scala/com/digitalasset/platform/store/serialization/KeyHasher.scala b/...r-api-common/src/main/scala/com/digitalasset/platform/store/serialization/KeyHasher.scala
@@ -116,10 +116,6 @@ object KeyHasher extends KeyHasher {
         val z1 = op(z, HashTokenCollectionBegin(entries.length))
         val z2 = entries.foldLeft[T](z1) { case (t, (k, v)) => foldLeft(k, foldLeft(v, t, op), op) }
         op(z2, HashTokenCollectionEnd())
-
-      // Struct: should never be encountered
-      case ValueStruct(_) =>
-        sys.error("Hashing of struct values is not supported")
     }
   }
 

diff --git a/...src/benchmark/scala/ledger/participant/state/kvutils/tools/engine/benchmark/Adapter.scala b/...src/benchmark/scala/ledger/participant/state/kvutils/tools/engine/benchmark/Adapter.scala
@@ -87,8 +87,6 @@ private[benchmark] final class Adapter(
         Value.ValueTextMap(value.mapValue(adapt))
       case Value.ValueGenMap(entries) =>
         Value.ValueGenMap(entries.map { case (k, v) => adapt(k) -> adapt(v) })
-      case Value.ValueStruct(fields) =>
-        Value.ValueStruct(fields.mapValues(adapt))
       case _: Value.ValueCidlessLeaf | _: Value.ValueContractId[ContractId] =>
         value
     }

diff --git a/navigator/backend/src/main/scala/com/digitalasset/navigator/console/Pretty.scala b/navigator/backend/src/main/scala/com/digitalasset/navigator/console/Pretty.scala
@@ -252,7 +252,6 @@ object Pretty {
             PrettyField("value", argument(value))
           )
       }: _*)
-    case _: model.ApiImpossible => sys.error("impossible! structs are not serializable")
   }
 
   /** Outputs an object in YAML format */

diff --git a/navigator/backend/src/main/scala/com/digitalasset/navigator/json/ApiCodecVerbose.scala b/navigator/backend/src/main/scala/com/digitalasset/navigator/json/ApiCodecVerbose.scala
@@ -73,7 +73,6 @@ object ApiCodecVerbose {
       JsObject(propType -> JsString(tagOptional), propValue -> apiValueToJsValue(v))
     case v: Model.ApiMap => apiTextMapToJsValue(v)
     case v: Model.ApiGenMap => apiGenMapToJsValue(v)
-    case _: Model.ApiImpossible => serializationError("impossible! structs are not serializable")
   }
 
   def apiListToJsValue(value: Model.ApiList): JsValue =

diff --git a/...gator/backend/src/main/scala/com/digitalasset/navigator/model/converter/LedgerApiV1.scala b/...gator/backend/src/main/scala/com/digitalasset/navigator/model/converter/LedgerApiV1.scala
@@ -426,8 +426,6 @@ case object LedgerApiV1 {
       case v: Model.ApiList => fillInListTI(v, typ, ctx)
       case v: Model.ApiRecord => fillInRecordTI(v, typ, ctx)
       case v: Model.ApiVariant => fillInVariantTI(v, typ, ctx)
-      case _: Model.ApiImpossible =>
-        Left(GenericConversionError("unserializable Struct appeared of serializable type"))
     }
 
   def readCompletion(completion: V1.completion.Completion): Result[Option[Model.CommandStatus]] = {
-Original file line number
+Diff line change
@@ Expand Up / @@ -525,8 +525,6 @@ object ValueCoder { @@
                 }
                 builder.setGenMap(protoMap).build()
-              case ValueStruct(fields) =>
-                throw Err(s"Trying to serialize struct, which are not serializable. Fields: $fields")
             }
           }
         }
@@ Expand Down @@