fix: some review comments

packages/assert/src/assert.js

@@ -58,7 +58,6 @@ const cycleTolerantStringify = payload => {
   return JSON.stringify(payload, replacer);
 };
 freeze(cycleTolerantStringify);
-export { cycleTolerantStringify };
 
 const declassifiers = new WeakMap();
 

packages/console/src/console.js

@@ -1,10 +1,17 @@
 // @ts-check
 
+// To ensure that this module operates without special privilege, it should
+// not reference the free variable "console" except for its own internal
+// debugging purposes in the declaration of "originalConsole", which is
+// normally commented out.
+
 // FIXME: We currently need to deep-link to the ESM version of the module.
 // Typescript ignores the package.json exports['.'].import and so uses "main",
 // which points to CJS.  That prevents Typescript from loading '@agoric/assert'
 // correctly.
-import { ErrorInfo } from '@agoric/assert/src/assert';
+// TODO FIXME BEFORE MERGING
+// @ts-ignore
+import { ErrorInfo } from '@agoric/assert';
 import '@agoric/assert/exported';
 import './types.js';
 
@@ -228,14 +235,14 @@ const makeCausalConsole = (baseConsole, options = {}) => {
    */
   const logSubErrors = (label, subErrors) => {
     if (subErrors.length >= 1) {
-      console.groupCollapsed(label);
+      baseConsole.groupCollapsed(label);
       try {
         for (const subError of subErrors) {
           // eslint-disable-next-line no-use-before-define
           logError(subError);
         }
       } finally {
-        console.groupEnd();
+        baseConsole.groupEnd();
       }
     }
   };
@@ -262,7 +269,7 @@ const makeCausalConsole = (baseConsole, options = {}) => {
     if (messageRecords.length === 0) {
       // If there are no message records, then just show the message that
       // the error itself carries.
-      baseConsole[BASE_CONSOLE_LEVEL](`${errorTag}: ${error.message}`);
+      baseConsole[BASE_CONSOLE_LEVEL](`${errorTag}:`, error.message);
     } else {
       // If there are message records (typically just one), then we take
       // these to be strictly more informative than the message string
@@ -280,7 +287,7 @@ const makeCausalConsole = (baseConsole, options = {}) => {
     ) {
       stackString += '\n';
     }
-    baseConsole[BASE_CONSOLE_LEVEL](stackString);
+    baseConsole[BASE_CONSOLE_LEVEL]('', stackString);
     // Show the other annotations on error
     for (const { kind, getLogArgs } of otherInfoRecords) {
       logErrorInfo(error, kind, getLogArgs(), subErrors);

packages/console/test/assert-log.test.js

@@ -36,8 +36,8 @@ test('throwsAndLogs with data', t => {
       ['error', 'what', obj],
       ['log', 'Caught', '(TypeError#1)'],
       ['groupCollapsed', ''],
-      ['info', 'TypeError#1: foo'],
-      ['info', 'stack of TypeError\n'],
+      ['info', 'TypeError#1:', 'foo'],
+      ['info', '', 'stack of TypeError\n'],
       ['groupEnd'],
     ],
     { wrapWithCausal: true },
@@ -77,7 +77,7 @@ test('assert', t => {
       ['log', 'Caught', '(RangeError#1)'],
       ['groupCollapsed', ''],
       ['info', 'RangeError#1:', 'Check failed'],
-      ['info', 'stack of RangeError\n'],
+      ['info', '', 'stack of RangeError\n'],
       ['groupEnd'],
     ],
     { wrapWithCausal: true },
@@ -136,13 +136,13 @@ test('causal tree', t => {
       ['log', 'Caught', '(RangeError#1)'],
       ['groupCollapsed', ''],
       ['info', 'RangeError#1:', 'because', '(RangeError#2)'],
-      ['info', 'stack of RangeError\n'],
+      ['info', '', 'stack of RangeError\n'],
       ['groupCollapsed', 'RangeError#1'],
       ['info', 'RangeError#2:', 'synful', '(SyntaxError#3)'],
-      ['info', 'stack of RangeError\n'],
+      ['info', '', 'stack of RangeError\n'],
       ['groupCollapsed', 'RangeError#2'],
-      ['info', 'SyntaxError#3: foo'],
-      ['info', 'stack of SyntaxError\n'],
+      ['info', 'SyntaxError#3:', 'foo'],
+      ['info', '', 'stack of SyntaxError\n'],
       ['groupEnd'],
       ['groupEnd'],
       ['groupEnd'],

packages/console/test/throws-and-logs.js

@@ -1,12 +1,44 @@
-import { cycleTolerantStringify } from '@agoric/assert';
 import { makeLoggingConsoleKit, makeCausalConsole } from '../src/console.js';
 
 const { freeze, getPrototypeOf, is: isSame } = Object;
 
 // For our internal debugging purposes
 // const originalConsole = console;
 
+/**
+ * Like `JSON.stringify` but does not blow up if given a cycle. This is not
+ * intended to be a serialization to support any useful unserialization,
+ * or any programmatic use of the resulting string. The string is intended
+ * only for showing a human, in order to be informative enough for some
+ * logging purposes. As such, this `cycleTolerantStringify` has an
+ * imprecise specification and may change over time.
+ *
+ * The current `cycleTolerantStringify` possibly emits too many "seen"
+ * markings: Not only for cycles, but also for repeated subtrees by
+ * object identity.
+ *
+ * (Started off as a duplicate of the cycleTolerantStringify internal
+ * to the assert module.)
+ */
+const cycleTolerantStringify = payload => {
+  const seenSet = new Set();
+  const replacer = (_, val) => {
+    if (typeof val === 'object' && val !== null) {
+      if (seenSet.has(val)) {
+        return '<**seen**>';
+      }
+      seenSet.add(val);
+    }
+    return val;
+  };
+  return JSON.stringify(payload, replacer);
+};
+freeze(cycleTolerantStringify);
+
 const compareLogs = freeze((t, log, goldenLog) => {
+  // For our internal debugging purposes
+  // originalConsole.log('LOG', log);
+
   t.is(log.length, goldenLog.length, 'wrong log length');
   log.forEach((logRecord, i) => {
     const goldenRecord = goldenLog[i];

packages/ses/package.json

@@ -36,8 +36,8 @@
     "demo": "http-server -o /demos"
   },
   "dependencies": {
-    "@agoric/assert": "^0.1.0-dev",
-    "@agoric/console": "^0.1.0-dev",
+    "@agoric/assert": "^0.1.0",
+    "@agoric/console": "^0.1.0",
     "@agoric/babel-standalone": "^7.9.5",
     "@agoric/make-hardener": "^0.1.0",
     "@agoric/transform-module": "^0.4.1"

packages/ses/src/module-load.js

@@ -6,11 +6,9 @@
 // module's "imports" with the more specific "resolvedImports" as inferred from
 // the particular compartment's "resolveHook".
 
+import { assert, details as d, quote as q } from '@agoric/assert';
 import { create, values, freeze } from './commons.js';
 
-// q, for quoting strings.
-const q = JSON.stringify;
-
 // `makeAlias` constructs compartment specifier tuples for the `aliases`
 // private field of compartments.
 // These aliases allow a compartment to alias an internal module specifier to a
@@ -92,16 +90,18 @@ const loadWithoutErrorAnnotation = async (
     aliasNamespace = moduleMapHook(moduleSpecifier);
   }
   if (typeof aliasNamespace === 'string') {
-    throw new TypeError(
-      `Cannot map module ${q(moduleSpecifier)} to ${q(
+    // TODO Prefer TypeError if we can ask assert to do that.
+    assert.fail(
+      d`Cannot map module ${q(moduleSpecifier)} to ${q(
         aliasNamespace,
       )} in parent compartment, not yet implemented`,
     );
   } else if (aliasNamespace !== undefined) {
     const alias = moduleAliases.get(aliasNamespace);
     if (alias === undefined) {
-      throw new ReferenceError(
-        `Cannot map module ${q(
+      // TODO Prefer ReferenceError if we can ask assert to do that.
+      assert.fail(
+        d`Cannot map module ${q(
           moduleSpecifier,
         )} because the key is not a module exports namespace, or is from another realm`,
       );
@@ -172,13 +172,12 @@ export const load = async (
     moduleSpecifier,
   ).catch(error => {
     const { name } = compartmentPrivateFields.get(compartment);
-    // TODO The following drops the causes' stacks.
-    // In the future, we should capture the causal chain.
-    // https://github.com/Agoric/SES-shim/issues/440
-    throw new error.constructor(
-      `${error.message}, loading ${q(moduleSpecifier)} in compartment ${q(
+    assert.note(
+      error,
+      d`${error.message}, loading ${q(moduleSpecifier)} in compartment ${q(
         name,
       )}`,
     );
+    throw error;
   });
 };