feat(import-bundle): accept symbol-named properties on inescapableGlo…

…balProperties (#2377)

Previously, the `inescapableGlobalProperties` option to `importBundle()`
only accepted enumerable string-named properties. This commit changes
that to accept all properties (even unenumerable ones), and to accept
symbol-named properties (instead of only string-named ones).

We'll use this to provide `passStyleOf` to the vat environment built by
liveslots, so it can use a real WeakMap, instead of the (slower)
"virtual object -aware WeakMap".

closes #2376

### Documentation Considerations

This modifies the behavior of `options.inescapableGlobalProperties`,
which is undocumented. We should probably write docs for it at some
point.

### Upgrade Considerations

See #2376 for notes about compatibility considerations, probably nothing
worth calling out in NEWS.md .

---------

Co-authored-by: Mark S. Miller <erights@gmail.com>

packages/import-bundle/NEWS.md

@@ -1,3 +1,11 @@
+# User-visible changes to `@endo/import-bundle`
+
+## Next release
+
+The `inescapableGlobalProperties` option is changed from supporting only
+string-named enumerable own properties to supporting all own properties
+whether string-named or symbol-named, and whether enumerable or not. But
+see https://github.com/endojs/endo/blob/master/packages/import-bundle/src/compartment-wrapper.md for the longer term plan.
 
 ## 0.4.0 (2023-08-07)
 

packages/import-bundle/src/compartment-wrapper.js

@@ -42,10 +42,32 @@ export function wrapInescapableCompartment(
     // there are details to work out.
     c.globalThis.Compartment = NewCompartment;
 
-    for (const prop of Object.keys(inescapableGlobalProperties)) {
+    // Use Reflect.ownKeys, not Object.keys, because we want both
+    // string-named and symbol-named properties. Note that
+    // Reflect.ownKeys also includes non-enumerable keys.
+    // This differs from the longer term agreement discussed at
+    // https://www.youtube.com/watch?v=xlR21uDigGE in these ways:
+    // - The option in question should be named `inescapableGlobals` since
+    //   we want to reserve `*Properties` for descriptor copying rather
+    //   than value copying.
+    // - We don't plan to support such `*Properties` options at this time.
+    //   Rather, we should deprecate (and eventually remove) this one
+    //   once we introduce`inescapableGlobals`.
+    // - We plan to move these options to the `Compartment` constructor itself,
+    //   in which case their support in import-bundle will just forward
+    //   to `Compartment`.
+    // - Following the `assign`-like semantics agree on in that meeting,
+    //   this should only copy enumerable own properties, whereas the loop
+    //   below copies all own properties.
+    // The loop here does follow this agreement by differing from `assign` in
+    // making all the target properties non-enumerable. The agreement would
+    // further align the normal `Compartment` endowments argument to also
+    // make the target properties non-enumerable.
+    for (const prop of Reflect.ownKeys(inescapableGlobalProperties)) {
       Object.defineProperty(c.globalThis, prop, {
         value: inescapableGlobalProperties[prop],
         writable: true,
+        // properties of globalThis are generally non-enumerable
         enumerable: false,
         configurable: true,
       });

packages/import-bundle/src/compartment-wrapper.md

@@ -44,3 +44,27 @@ c2.evaluate(`new WeakMap()`);
 
   [Compartments]: ../../ses/README.md#compartment
   [SES]: ../../ses/README.md
+
+## Note expected semantic changes
+
+This `inescapableGlobalProperties` currently copies the value of all
+own properties, whether string-named or symbol-named, and whether enumerable
+or not. This
+differs from the longer term agreement discussed at
+https://www.youtube.com/watch?v=xlR21uDigGE in these ways:
+- The option in question should be named `inescapableGlobals` since
+  we want to reserve `*Properties` for descriptor copying rather
+  than value copying.
+- We don't plan to support such `*Properties` options at this time.
+  Rather, we should deprecate (and eventually remove) this one
+  once we introduce`inescapableGlobals`.
+- We plan to move these options to the `Compartment` constructor itself,
+  in which case their support in import-bundle will just forward
+  to `Compartment`.
+- Following the `assign`-like semantics agree on in that meeting,
+  this should only copy enumerable own properties, whereas the loop
+  below copies all own properties.
+The loop here does follow this agreement by differing from `assign` in
+making all the target properties non-enumerable. The agreement would
+further align the normal `Compartment` endowments argument to also
+make the target properties non-enumerable.

packages/import-bundle/test/compartment-wrapper.test.js

@@ -5,6 +5,10 @@ import { wrapInescapableCompartment } from '../src/compartment-wrapper.js';
 
 const createChild = `() => new Compartment({console})`;
 
+// simulate what pass-style wants to give a liveslots environment
+const symbolEndowment = () => 0;
+const endowmentSymbol = Symbol.for('endowmentSymbol');
+
 function check(t, c, n) {
   t.is(c.evaluate('Compartment.name'), 'Compartment', `${n}.Compartment.name`);
 
@@ -21,11 +25,31 @@ function check(t, c, n) {
 
   t.is(c.evaluate('WeakMap'), 'replaced');
   t.is(c.evaluate('globalThis.WeakMap'), 'replaced');
+  t.is(c.evaluate('globalThis.unenumerable'), 'yep');
+  t.is(
+    c.evaluate(`globalThis[Symbol.for('endowmentSymbol')]`),
+    symbolEndowment,
+  );
 }
 
+// match what @endo/pass-style does, but note that import-bundle
+// does/should not depend upon on pass-style in any way
+//
+// alternative: have one of pass-style and import-bundle have a
+// devDependencies on the other (avoid a cycle, avoid strong/"normal"
+// `dependencies`), to allow a more thorough test, which is currently
+// only really exercised by the agoric-sdk/swingset-liveslots test
+
 test('wrap', t => {
   const inescapableTransforms = [];
-  const inescapableGlobalProperties = { WeakMap: 'replaced' };
+  const inescapableGlobalProperties = {
+    WeakMap: 'replaced',
+    [endowmentSymbol]: symbolEndowment,
+  };
+  Object.defineProperty(inescapableGlobalProperties, 'unenumerable', {
+    value: 'yep',
+    enumerable: false,
+  });
   const WrappedCompartment = wrapInescapableCompartment(
     Compartment,
     inescapableTransforms,