refactor(exo): shorten exo-call fastpath

packages/exo/src/exo-tools.js

@@ -18,7 +18,7 @@ import { GET_INTERFACE_GUARD } from './get-interface.js';
 
 /**
  * @import {InterfaceGuard, Method, MethodGuard, MethodGuardPayload} from '@endo/patterns'
- * @import {ContextProvider, FacetName, KitContextProvider, MatchConfig, Methods} from './types.js'
+ * @import {ClassContext, ContextProvider, FacetName, KitContext, KitContextProvider, MatchConfig, Methods} from './types.js'
  */
 
 const { apply, ownKeys } = Reflect;
@@ -146,21 +146,28 @@ const buildMatchConfig = methodGuardPayload => {
 };
 
 /**
- * @param {Method} method
+ * @param {(representative: any) => ClassContext | KitContext} getContext
+ * @param {CallableFunction} behaviorMethod
  * @param {MethodGuardPayload} methodGuardPayload
  * @param {string} label
  * @returns {Method}
  */
-const defendSyncMethod = (method, methodGuardPayload, label) => {
+const defendSyncMethod = (
+  getContext,
+  behaviorMethod,
+  methodGuardPayload,
+  label,
+) => {
   const { returnGuard } = methodGuardPayload;
   const isRawReturn = isRawGuard(returnGuard);
   const matchConfig = buildMatchConfig(methodGuardPayload);
   const { syncMethod } = {
     // Note purposeful use of `this` and concise method syntax
     syncMethod(...syncArgs) {
+      const context = getContext(this);
       // Only harden args and return value if not dealing with a raw value guard.
       const realArgs = defendSyncArgs(syncArgs, matchConfig, label);
-      const result = apply(method, this, realArgs);
+      const result = apply(behaviorMethod, context, realArgs);
       if (!isRawReturn) {
         mustMatch(harden(result), returnGuard, `${label}: result`);
       }
@@ -202,11 +209,17 @@ const desync = methodGuardPayload => {
 };
 
 /**
- * @param {(...args: unknown[]) => any} method
+ * @param {(representative: any) => ClassContext | KitContext} getContext
+ * @param {CallableFunction} behaviorMethod
  * @param {MethodGuardPayload} methodGuardPayload
  * @param {string} label
  */
-const defendAsyncMethod = (method, methodGuardPayload, label) => {
+const defendAsyncMethod = (
+  getContext,
+  behaviorMethod,
+  methodGuardPayload,
+  label,
+) => {
   const { returnGuard } = methodGuardPayload;
   const isRawReturn = isRawGuard(returnGuard);
 
@@ -231,8 +244,11 @@ const defendAsyncMethod = (method, methodGuardPayload, label) => {
           for (let j = 0; j < awaitedArgs.length; j += 1) {
             syncArgs[awaitIndexes[j]] = awaitedArgs[j];
           }
+          // Get the context after all waiting in case we ever do revocation
+          // by removing the context entry. Avoid TOCTTOU!
+          const context = getContext(this);
           const realArgs = defendSyncArgs(syncArgs, matchConfig, label);
-          return apply(method, this, realArgs);
+          return apply(behaviorMethod, context, realArgs);
         },
       );
       if (isRawReturn) {
@@ -249,93 +265,73 @@ const defendAsyncMethod = (method, methodGuardPayload, label) => {
 
 /**
  *
- * @param {Method} method
+ * @param {(representative: any) => ClassContext | KitContext} getContext
+ * @param {CallableFunction} behaviorMethod
  * @param {MethodGuard} methodGuard
  * @param {string} label
  */
-const defendMethod = (method, methodGuard, label) => {
+const defendMethod = (getContext, behaviorMethod, methodGuard, label) => {
   const methodGuardPayload = getMethodGuardPayload(methodGuard);
   const { callKind } = methodGuardPayload;
   if (callKind === 'sync') {
-    return defendSyncMethod(method, methodGuardPayload, label);
+    return defendSyncMethod(
+      getContext,
+      behaviorMethod,
+      methodGuardPayload,
+      label,
+    );
   } else {
     assert(callKind === 'async');
-    return defendAsyncMethod(method, methodGuardPayload, label);
+    return defendAsyncMethod(
+      getContext,
+      behaviorMethod,
+      methodGuardPayload,
+      label,
+    );
   }
 };
 
 /**
  * @param {string} methodTag
  * @param {ContextProvider} contextProvider
  * @param {CallableFunction} behaviorMethod
- * @param {boolean} [thisfulMethods]
- * @param {MethodGuard} [methodGuard]
- * @param {import('@endo/patterns').DefaultGuardType} [defaultGuards]
+ * @param {MethodGuard} methodGuard
  */
 const bindMethod = (
   methodTag,
   contextProvider,
   behaviorMethod,
-  thisfulMethods = false,
-  methodGuard = undefined,
-  defaultGuards = undefined,
+  methodGuard,
 ) => {
   assert.typeof(behaviorMethod, 'function');
 
-  const getContext = self => {
-    const context = contextProvider(self);
-    context ||
-      Fail`${q(methodTag)} may only be applied to a valid instance: ${self}`;
+  /**
+   * @param {any} representative
+   * @returns {ClassContext | KitContext}
+   */
+  const getContext = representative => {
+    representative ||
+      // separate line to ease breakpointing
+      Fail`Method ${methodTag} called without 'this' object`;
+    const context = contextProvider(representative);
+    if (context === undefined) {
+      throw Fail`${q(
+        methodTag,
+      )} may only be applied to a valid instance: ${representative}`;
+    }
     return context;
   };
 
-  // Violating all Jessie rules to create representatives that inherit
-  // methods from a shared prototype. The bound method therefore needs
-  // to mention `this`. We define it using concise method syntax
-  // so that it will be `this` sensitive but not constructable.
-  //
-  // We normally consider `this` unsafe because of the hazard of a
-  // method of one abstraction being applied to an instance of
-  // another abstraction. To prevent that attack, the bound method
-  // checks that its `this` is in the map in which its representatives
-  // are registered.
-  let { method } = thisfulMethods
-    ? {
-        method(...args) {
-          this ||
-            Fail`thisful method ${methodTag} called without 'this' object`;
-          const context = getContext(this);
-          return apply(behaviorMethod, context, args);
-        },
-      }
-    : {
-        method(...args) {
-          const context = getContext(this);
-          return apply(behaviorMethod, null, [context, ...args]);
-        },
-      };
-  if (!methodGuard && thisfulMethods) {
-    switch (defaultGuards) {
-      case undefined:
-      case 'passable':
-        methodGuard = PassableMethodGuard;
-        break;
-      case 'raw':
-        methodGuard = RawMethodGuard;
-        break;
-      default:
-        throw Fail`Unrecognized defaultGuards ${q(defaultGuards)}`;
-    }
-  }
-  if (methodGuard) {
-    method = defendMethod(method, methodGuard, methodTag);
-  }
+  const method = defendMethod(
+    getContext,
+    behaviorMethod,
+    methodGuard,
+    methodTag,
+  );
 
   defineProperties(method, {
     name: { value: methodTag },
-    length: {
-      value: thisfulMethods ? behaviorMethod.length : behaviorMethod.length - 1,
-    },
+    length: { value: behaviorMethod.length },
   });
   return method;
 };
@@ -402,14 +398,44 @@ export const defendPrototype = (
   }
 
   for (const prop of methodNames) {
+    const originalMethod = behaviorMethods[prop];
+    const { shiftedMethod } = {
+      shiftedMethod(...args) {
+        return originalMethod(this, ...args);
+      },
+    };
+    const behaviorMethod = thisfulMethods ? originalMethod : shiftedMethod;
+    // TODO some tool does not yet understand the `?.[` syntax
+    // See https://github.com/endojs/endo/pull/2247#discussion_r1583724424
+    let methodGuard = methodGuards && methodGuards[prop];
+    if (!methodGuard) {
+      switch (defaultGuards) {
+        case undefined: {
+          if (thisfulMethods) {
+            methodGuard = PassableMethodGuard;
+          } else {
+            methodGuard = RawMethodGuard;
+          }
+          break;
+        }
+        case 'passable': {
+          methodGuard = PassableMethodGuard;
+          break;
+        }
+        case 'raw': {
+          methodGuard = RawMethodGuard;
+          break;
+        }
+        default: {
+          throw Fail`Unrecognized defaultGuards ${q(defaultGuards)}`;
+        }
+      }
+    }
     prototype[prop] = bindMethod(
       `In ${q(prop)} method of (${tag})`,
       contextProvider,
-      behaviorMethods[prop],
-      thisfulMethods,
-      // TODO some tool does not yet understand the `?.[` syntax
-      methodGuards && methodGuards[prop],
-      defaultGuards,
+      behaviorMethod,
+      methodGuard,
     );
   }
 
@@ -424,8 +450,7 @@ export const defendPrototype = (
       `In ${q(GET_INTERFACE_GUARD)} method of (${tag})`,
       contextProvider,
       getInterfaceGuardMethod,
-      thisfulMethods,
-      undefined,
+      PassableMethodGuard,
     );
   }
 

packages/exo/test/test-heap-classes.js

@@ -1,3 +1,4 @@
+/* eslint-disable @endo/no-optional-chaining */
 // @ts-check
 import test from '@endo/ses-ava/prepare-endo.js';
 
@@ -77,7 +78,7 @@ test('test defineExoClass', t => {
     message:
       'In "incr" method of (UpCounter): arg 0?: string "foo" - Must be a number',
   });
-  t.deepEqual(upCounter[GET_INTERFACE_GUARD](), UpCounterI);
+  t.deepEqual(upCounter[GET_INTERFACE_GUARD]?.(), UpCounterI);
   t.deepEqual(getInterfaceMethodKeys(UpCounterI), ['incr']);
 
   const symbolic = Symbol.for('symbolic');
@@ -91,7 +92,7 @@ test('test defineExoClass', t => {
     [symbolic]() {},
   });
   const foo = makeFoo();
-  t.deepEqual(foo[GET_INTERFACE_GUARD](), FooI);
+  t.deepEqual(foo[GET_INTERFACE_GUARD]?.(), FooI);
   // @ts-expect-error intentional for test
   t.throws(() => foo[symbolic]('invalid arg'), {
     message:
@@ -143,8 +144,8 @@ test('test defineExoClassKit', t => {
   t.throws(() => upCounter.decr(3), {
     message: 'upCounter.decr is not a function',
   });
-  t.deepEqual(upCounter[GET_INTERFACE_GUARD](), UpCounterI);
-  t.deepEqual(downCounter[GET_INTERFACE_GUARD](), DownCounterI);
+  t.deepEqual(upCounter[GET_INTERFACE_GUARD]?.(), UpCounterI);
+  t.deepEqual(downCounter[GET_INTERFACE_GUARD]?.(), DownCounterI);
 });
 
 test('test makeExo', t => {
@@ -165,7 +166,7 @@ test('test makeExo', t => {
     message:
       'In "incr" method of (upCounter): arg 0?: string "foo" - Must be a number',
   });
-  t.deepEqual(upCounter[GET_INTERFACE_GUARD](), UpCounterI);
+  t.deepEqual(upCounter[GET_INTERFACE_GUARD]?.(), UpCounterI);
 });
 
 // For code sharing with defineKind which does not support an interface
@@ -186,7 +187,7 @@ test('missing interface', t => {
     message:
       'In "makeSayHello" method of (greeterMaker): result: "[Symbol(passStyle)]" property expected: "[Function <anon>]"',
   });
-  t.is(greeterMaker[GET_INTERFACE_GUARD](), undefined);
+  t.is(greeterMaker[GET_INTERFACE_GUARD]?.(), undefined);
 });
 
 const SloppyGreeterI = M.interface('greeter', {}, { sloppy: true });
@@ -199,7 +200,7 @@ test('sloppy option', t => {
     },
   });
   t.is(greeter.sayHello(), 'hello');
-  t.deepEqual(greeter[GET_INTERFACE_GUARD](), SloppyGreeterI);
+  t.deepEqual(greeter[GET_INTERFACE_GUARD]?.(), SloppyGreeterI);
 
   t.throws(
     () =>
@@ -262,7 +263,7 @@ test('raw guards', t => {
       return 'hello';
     },
   });
-  t.deepEqual(greeter[GET_INTERFACE_GUARD](), RawGreeterI);
+  t.deepEqual(greeter[GET_INTERFACE_GUARD]?.(), RawGreeterI);
   testGreeter(t, greeter, 'raw defaultGuards');
 
   const Greeter2I = M.interface('greeter2', {
@@ -302,7 +303,7 @@ test('raw guards', t => {
       return {};
     },
   });
-  t.deepEqual(greeter2[GET_INTERFACE_GUARD](), Greeter2I);
+  t.deepEqual(greeter2[GET_INTERFACE_GUARD]?.(), Greeter2I);
   testGreeter(t, greeter, 'explicit raw');
 
   t.is(Object.isFrozen(greeter2.rawIn({})), true);
@@ -340,15 +341,15 @@ test('naked function call', t => {
   const { sayHello, [GET_INTERFACE_GUARD]: gigm } = greeter;
   t.throws(() => sayHello(), {
     message:
-      'thisful method "In \\"sayHello\\" method of (greeter)" called without \'this\' object',
+      'Method "In \\"sayHello\\" method of (greeter)" called without \'this\' object',
   });
   t.is(sayHello.bind(greeter)(), 'hello');
 
-  t.throws(() => gigm(), {
+  t.throws(() => gigm?.(), {
     message:
-      'thisful method "In \\"__getInterfaceGuard__\\" method of (greeter)" called without \'this\' object',
+      'Method "In \\"__getInterfaceGuard__\\" method of (greeter)" called without \'this\' object',
   });
-  t.deepEqual(gigm.bind(greeter)(), GreeterI);
+  t.deepEqual(gigm?.bind(greeter)(), GreeterI);
 });
 
 // needn't run. we just don't have a better place to write these.