fix(marshal): add Data marker, tolerate its presence

Test that it can be added to empty and non-empty data, and that it doesn't
cause changes to the serialization of non-empty objects. We do not yet change
the handling of empty objects.

Data precludes Remotable: anything marked as Data cannot also be treated as
Remotable. This is mostly to guard against confusion inside passStyleOf.

packages/marshal/index.js

@@ -9,6 +9,7 @@ export {
   makeMarshal,
   Remotable,
   Far,
+  Data,
 } from './src/marshal';
 
 export { stringify, parse } from './src/marshal-stringify';

packages/marshal/src/marshal.js

@@ -14,6 +14,7 @@ const {
   setPrototypeOf,
   getOwnPropertyDescriptors,
   defineProperties,
+  getOwnPropertyNames,
   is,
   isFrozen,
   fromEntries,
@@ -262,7 +263,23 @@ function isPassByCopyRecord(val) {
     // See /~https://github.com/Agoric/agoric-sdk/issues/2018
     return false;
   }
+
+  function ignorePassStyle(descKey) {
+    const desc = descs[descKey];
+    return (
+      descKey === PASS_STYLE &&
+      !desc.enumerable &&
+      desc.value === 'copyRecord' &&
+      !('get' in desc)
+    );
+  }
+
   for (const descKey of descKeys) {
+    // we tolerate and ignore a non-enumerable PASS_STYLE symbol-named key, the Data marker
+    if (ignorePassStyle(descKey)) {
+      // eslint-disable-next-line no-continue
+      continue;
+    }
     if (typeof descKey === 'symbol') {
       return false;
     }
@@ -272,6 +289,10 @@ function isPassByCopyRecord(val) {
     }
   }
   for (const descKey of descKeys) {
+    if (ignorePassStyle(descKey)) {
+      // eslint-disable-next-line no-continue
+      continue;
+    }
     assert.typeof(
       descKey,
       'string',
@@ -351,6 +372,7 @@ function assertCanBeRemotable(val) {
   assert.typeof(val, 'object', X`cannot serialize non-objects like ${val}`);
   assert(!Array.isArray(val), X`Arrays cannot be pass-by-remote`);
   assert(val !== null, X`null cannot be pass-by-remote`);
+  assert(val[PASS_STYLE] !== 'copyRecord', X`object already marked as Data`);
 
   const descs = getOwnPropertyDescriptors(val);
   const keys = ownKeys(descs); // enumerable-and-not, string-or-Symbol
@@ -759,7 +781,7 @@ export function makeMarshal(
               // Currently copyRecord allows only string keys so this will
               // work. If we allow sortable symbol keys, this will need to
               // become more interesting.
-              const names = ownKeys(val).sort();
+              const names = getOwnPropertyNames(val).sort();
               return fromEntries(names.map(name => [name, encode(val[name])]));
             }
             case 'copyArray': {
@@ -1089,3 +1111,17 @@ const Far = (farName, remotable = {}) =>
 
 harden(Far);
 export { Far };
+
+function Data(props) {
+  assert(
+    ownKeys(props).length === 0 || isPassByCopyRecord(props),
+    X`Data() can only be applied to otherwise pass-by-copy records`,
+  );
+  Object.defineProperty(props, PASS_STYLE, {
+    enumerable: false,
+    value: 'copyRecord',
+  });
+  return harden(props);
+}
+harden(Data);
+export { Data };

packages/marshal/test/test-marshal.js

@@ -3,7 +3,7 @@ import test from 'ava';
 import {
   Remotable,
   Far,
-  // Data,
+  Data,
   getInterfaceOf,
   makeMarshal,
   passStyleOf,
@@ -415,6 +415,8 @@ test('records', t => {
   }
   const m = makeMarshal(convertValToSlot, convertSlotToVal);
   const ser = val => m.serialize(val);
+  const unser = capdata => m.unserialize(capdata);
+
   const noIface = {
     body: JSON.stringify({ '@qclass': 'slot', index: 0 }),
     slots: ['slot'],
@@ -473,9 +475,9 @@ test('records', t => {
       }
     }
     const o = create(objectPrototype, props);
-    // if (mark === 'data') {
-    //   return Data(o);
-    // }
+    if (mark === 'data') {
+      return Data(o);
+    }
     if (mark === 'far') {
       return Far('iface', o);
     }
@@ -489,6 +491,7 @@ test('records', t => {
   const NOACC = /Records must not contain accessors/;
   const RECENUM = /Record fields must be enumerable/;
   const NOMETH = /cannot serialize objects with non-methods/;
+  const NODATA = /Data\(\) can only be applied to otherwise pass-by-copy records/;
 
   // empty objects
 
@@ -508,6 +511,22 @@ test('records', t => {
   // t.throws(() => ser(harden({})), { message: /??/ }, 'unmarked empty object rejected'); // int2
   // t.deepEqual(ser(build()), emptyData); // final
 
+  // Data({key1: 'data'})
+  // old: not applicable, Data() not yet added
+  // interim1: pass-by-copy without warning, but Data() is not necessary
+  // final: not applicable, Data() removed
+  const key1Data = { body: JSON.stringify({ key1: 'data' }), slots: [] };
+  t.deepEqual(ser(build('enumStringData', 'data')), key1Data);
+
+  // Serialized data should roundtrip properly. The first case here will
+  // trigger a warning during interim1.
+  t.deepEqual(unser(ser(harden({}))), {});
+  t.deepEqual(unser(ser(harden({ key1: 'data' }))), { key1: 'data' });
+
+  // unserialized data can be serialized again
+  // t.deepEqual(ser(unser(emptyData)), emptyData);
+  t.deepEqual(ser(unser(key1Data)), key1Data);
+
   // Data({})
   // old: not applicable, Data() not yet added
   // interim1: pass-by-copy without warning
@@ -544,7 +563,7 @@ test('records', t => {
   t.deepEqual(ser(build('nonenumSymbolFunc')), noIface);
 
   // Data({ key: data, key: func }) : rejected
-  // shouldThrow('data', 'enumStringData', 'enumStringFunc');
+  shouldThrow(['data', 'enumStringData', 'enumStringFunc'], NODATA);
 
   // Far('iface', { key: data, key: func }) : rejected
   // (some day this might add auxilliary data, but not now