Visitor and logging again (#437)

This PR fixes some logging stuff, which also necessitated some
`BaseValueVisitor` tweakage.

CHANGELOG.md

@@ -17,12 +17,18 @@ Other notable changes:
 * general:
   * Allow node version 23.
 * `loggy-intf` / `loggy`:
-  * Minor tweaks to "human" (non-JSON) log rendering.
+  * Made several improvements to "human" (non-JSON) log rendering, including
+    fixing it to be able to log values with reference cycles.
 * `structy`:
   * Started allowing any object (plain or not) to be used as the argument to the
     `BaseStruct` constructor.
   * Added the option to allow undeclared properties to be allowed and
     dynamically vetted, via two additional `_impl*` methods.
+* `valvis`:
+  * `BaseValueVisitor`:
+    * Simplified detection of reference cycles.
+    * Added argument `isCycleHead` to `_impl_shouldRef()`, so client code can
+      choose to be more cycle-aware.
 * `webapp-builtins`:
   * Simplified naming scheme for preserved log files: Names now always include
     a `-<num>` suffix after the date.

src/loggy-intf/export/LogPayload.js

@@ -2,8 +2,8 @@
 // SPDX-License-Identifier: Apache-2.0
 
 import { EventPayload, EventSource } from '@this/async';
-import { IntfDeconstructable, Sexp } from '@this/sexp';
 import { Moment } from '@this/quant';
+import { IntfDeconstructable, Sexp } from '@this/sexp';
 import { MustBe } from '@this/typey';
 import { StackTrace } from '@this/valvis';
 

src/loggy-intf/export/LoggedValueEncoder.js

@@ -33,17 +33,27 @@ export class LoggedValueEncoder extends BaseValueVisitor {
   }
 
   /** @override */
-  _impl_shouldRef(value) {
-    if (typeof value === 'object') {
-      if (Array.isArray(value)) {
-        return (value.length > 10);
-      } else if (AskIf.plainObject(value)) {
-        return (Object.entries(value).length > 10);
-      } else {
+  _impl_shouldRef(value, isCycleHead) {
+    switch (typeof value) {
+      case 'function': {
         return true;
       }
-    } else {
-      return false;
+
+      case 'object': {
+        if (isCycleHead) {
+          return true;
+        } else if (Array.isArray(value)) {
+          return (value.length > 10);
+        } else if (AskIf.plainObject(value)) {
+          return (Object.entries(value).length > 10);
+        } else {
+          return true;
+        }
+      }
+
+      default: {
+        return false;
+      }
     }
   }
 
@@ -99,7 +109,10 @@ export class LoggedValueEncoder extends BaseValueVisitor {
       const visitedArray = this._prot_visitProperties(sexpArray);
       return new Sexp(...visitedArray);
     } else {
-      return this._prot_labelFromValue(node);
+      const constructor = Reflect.getPrototypeOf(node).constructor;
+      return constructor
+        ? new Sexp(this._prot_nameFromValue(constructor), '...')
+        : new Sexp('Object', this._prot_labelFromValue(node), '...');
     }
   }
 
@@ -110,7 +123,7 @@ export class LoggedValueEncoder extends BaseValueVisitor {
 
   /** @override */
   _impl_visitProxy(node, isFunction_unused) {
-    return this._prot_labelFromValue(node);
+    return new Sexp('Proxy', this._prot_nameFromValue(node));
   }
 
   /** @override */
@@ -186,12 +199,22 @@ export class LoggedValueEncoder extends BaseValueVisitor {
       return this.#makeDefIfAppropriate(node, result);
     }
 
+    /** @override */
+    _impl_visitInstance(node) {
+      return this.#makeDefIfAppropriate(node, node);
+    }
+
     /** @override */
     _impl_visitPlainObject(node) {
       const result = this._prot_visitProperties(node);
       return this.#makeDefIfAppropriate(node, result);
     }
 
+    /** @override */
+    _impl_visitString(node) {
+      return this.#makeDefIfAppropriate(node, node);
+    }
+
     /**
      * Wraps a result in a "def" if it is in fact a defining value occurrence.
      *

src/loggy-intf/tests/LogPayload.test.js

@@ -3,9 +3,9 @@
 
 import stripAnsi from 'strip-ansi';
 
-import { Sexp } from '@this/sexp';
 import { LogPayload, LogTag } from '@this/loggy-intf';
 import { Moment } from '@this/quant';
+import { Sexp } from '@this/sexp';
 import { StackTrace } from '@this/valvis';
 
 

src/loggy-intf/tests/LogTag.test.js

@@ -3,8 +3,8 @@
 
 import stripAnsi from 'strip-ansi';
 
-import { Sexp } from '@this/sexp';
 import { LogTag } from '@this/loggy-intf';
+import { Sexp } from '@this/sexp';
 import { StyledText } from '@this/texty';
 
 

src/loggy-intf/tests/LoggedValueEncoder.test.js

@@ -0,0 +1,147 @@
+// Copyright 2022-2024 the Lactoserv Authors (Dan Bornstein et alia).
+// SPDX-License-Identifier: Apache-2.0
+
+import { LoggedValueEncoder } from '@this/loggy-intf';
+import { Duration } from '@this/quant';
+import { Sexp } from '@this/sexp';
+import { VisitDef, VisitRef } from '@this/valvis';
+
+
+function withNullObjectProtos(value) {
+  if (!(value && (typeof value === 'object'))) {
+    return value;
+  } else if (Array.isArray(value)) {
+    const result = [];
+    for (const v of value) {
+      result.push(withNullObjectProtos(v));
+    }
+    return result;
+  } else if (Reflect.getPrototypeOf(value) === Object.prototype) {
+    const result = Object.create(null);
+    for (const [k, v] of Object.entries(value)) {
+      result[k] = withNullObjectProtos(v);
+    }
+    return result;
+  } else {
+    return value;
+  }
+}
+
+function sexp(type, ...args) {
+  return new Sexp(type, ...args);
+}
+
+describe('encode()', () => {
+  // Cases where the result should be equal to the input.
+  test.each`
+  value
+  ${null}
+  ${false}
+  ${true}
+  ${'florp'}
+  ${123.456}
+  ${[]}
+  ${[1, 2, 3, 'four']}
+  ${[[null], [false], [[55]]]}
+  `('($#) self-encodes $value', ({ value }) => {
+    const got = LoggedValueEncoder.encode(value);
+    expect(got).toStrictEqual(value);
+  });
+
+  // Plain objects are expected to get converted to null-prototype objects.
+  test.each`
+  value
+  ${{}}
+  ${{ a: 10, b: 'twenty' }}
+  ${{ abc: { x: [{ d: 'efg' }] } }}
+  `('($#) self-encodes $value except with a `null` object prototypes', ({ value }) => {
+    const got = LoggedValueEncoder.encode(value);
+    const expected = withNullObjectProtos(value);
+    expect(got).toStrictEqual(expected);
+  });
+
+  class SomeClass {
+    // @defaultConstructor
+  }
+
+  const someFunc = () => null;
+
+  // Stuff that isn't JSON-encodable should end up in the form of a sexp.
+  test.each`
+  value                  | expected
+  ${undefined}           | ${sexp('Undefined')}}
+  ${[undefined]}         | ${[sexp('Undefined')]}}
+  ${321123n}             | ${sexp('BigInt', '321123')}}
+  ${Symbol('xyz')}       | ${sexp('Symbol', 'xyz')}}
+  ${Symbol.for('blorp')} | ${sexp('Symbol', 'blorp', true)}}
+  ${new Duration(12.34)} | ${sexp('Duration', 12.34, '12.340 sec')}
+  ${new Map()}           | ${sexp('Map', '...')}
+  ${new Proxy({}, {})}   | ${sexp('Proxy', '<anonymous>')}
+  ${new Proxy([], {})}   | ${sexp('Proxy', '<anonymous>')}
+  ${new Proxy(new SomeClass(), {})} | ${sexp('Proxy', '<anonymous>')}
+  ${new Proxy(someFunc, {})} | ${sexp('Proxy', 'someFunc')}
+  `('($#) correctly encodes $value', ({ value, expected }) => {
+    const got = LoggedValueEncoder.encode(value);
+    expect(got).toStrictEqual(expected);
+  });
+
+  test('does not def-ref a small-enough array', () => {
+    const value = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+    const got   = LoggedValueEncoder.encode([value, value]);
+    expect(got).toStrictEqual([value, value]);
+  });
+
+  test('does not def-ref a small-enough plain object', () => {
+    const value    = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, g: 7, h: 8, i: 9, j: 10 };
+    const expected = withNullObjectProtos(value);
+    const got      = LoggedValueEncoder.encode([value, value]);
+    expect(got).toStrictEqual([expected, expected]);
+  });
+
+  test('def-refs a large-enough array', () => {
+    const value    = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];
+    const def      = new VisitDef(0, value);
+    const expected = [def, new VisitRef(def)];
+    const got      = LoggedValueEncoder.encode([value, value]);
+    expect(got).toStrictEqual(expected);
+  });
+
+  test('def-refs a large-enough plain object', () => {
+    const value    = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, g: 7, h: 8, i: 9, j: 10, k: 11 };
+    const def      = new VisitDef(0, withNullObjectProtos(value));
+    const expected = [def, new VisitRef(def)];
+    const got      = LoggedValueEncoder.encode([value, value]);
+
+    expect(got).toBeArrayOfSize(2);
+    expect(got[0]).toStrictEqual(expected[0]);
+    expect(got[1]).toStrictEqual(expected[1]);
+    expect(got).toStrictEqual(expected);
+  });
+
+  test('def-refs the sexp from an instance', () => {
+    const value    = new Map();
+    const def      = new VisitDef(0, sexp('Map', '...'));
+    const expected = [def, new VisitRef(def)];
+    const got      = LoggedValueEncoder.encode([value, value]);
+    expect(got).toStrictEqual(expected);
+  });
+
+  test('def-refs an array with a self-reference', () => {
+    const value = [123];
+    value.push(value);
+
+    const got = LoggedValueEncoder.encode(value);
+
+    // Jest can't compare self-referential structures (it recurses forever), so
+    // we have to do it "manually."
+
+    expect(got).toBeInstanceOf(VisitDef);
+    expect(got.index).toBe(0);
+
+    const gotValue = got.value;
+    expect(gotValue).toBeArrayOfSize(2);
+    expect(gotValue[0]).toBe(123);
+    expect(gotValue[1]).toBeInstanceOf(VisitRef);
+    expect(gotValue[1].def).toBe(got);
+  });
+});

src/main-tester/index.js

@@ -1,6 +1,9 @@
 // Copyright 2022-2024 the Lactoserv Authors (Dan Bornstein et alia).
 // SPDX-License-Identifier: Apache-2.0
 
+import { inspect } from 'node:util';
+
+
 process.on('warning', (warning) => {
   if (warning.name === 'ExperimentalWarning') {
     if (/VM Modules/.test(warning.message)) {
@@ -13,3 +16,51 @@ process.on('warning', (warning) => {
   // eslint-disable-next-line no-console
   console.log('%s: %s\n', warning.name, warning.message);
 });
+
+// This extends Jest's equality checks to deal reasonably with a few more cases
+// than it does by default, by leaning on common patterns in this codebase.
+
+function lactoservEquals(a, b, customTesters) {
+  // Note: `return undefined` means, "The comparison is not handled by this
+  // tester."
+
+  if ((typeof a !== 'object') || (typeof b !== 'object')) {
+    return undefined;
+  } else if ((a === null) || (b === null)) {
+    return undefined;
+  } else if (Array.isArray(a) || Array.isArray(b)) {
+    return undefined;
+  }
+
+  const aProto = Reflect.getPrototypeOf(a);
+  const bProto = Reflect.getPrototypeOf(b);
+
+  if ((aProto === null) || (aProto !== bProto)) {
+    return undefined;
+  }
+
+  // At this point, we're looking at two non-array objects of the same class
+  // (that is, with the same prototype).
+
+  if (typeof aProto.deconstruct === 'function') {
+    // They (effectively) implement `IntfDeconstructable`. Note: There is no
+    // need to check the `functor` of the deconstructed result, since it's
+    // going to be `aProto` (which is also `bProto`).
+    const aDecon = a.deconstruct(true).args;
+    const bDecon = b.deconstruct(true).args;
+    return this.equals(aDecon, bDecon, customTesters);
+  } else if (typeof aProto[inspect.custom] === 'function') {
+    // They have a custom inspector.
+    return this.equals(inspect(a), inspect(b));
+  } else if (typeof aProto.toJSON === 'function') {
+    // They have a custom JSON encoder. This is the least-preferable option
+    // because it's the most likely to end up losing information.
+    return this.equals(a.toJSON(), b.toJSON(), customTesters);
+  }
+
+  return undefined;
+}
+
+// The linter doesn't realize this file is loaded in the context of testing, so
+// it would complain that `expect` is undefined without the disable directive.
+expect.addEqualityTesters([lactoservEquals]); // eslint-disable-line no-undef

src/net-util/export/DispatchInfo.js

@@ -2,8 +2,8 @@
 // SPDX-License-Identifier: Apache-2.0
 
 import { PathKey } from '@this/collections';
-import { IntfDeconstructable, Sexp } from '@this/sexp';
 import { IntfLogger } from '@this/loggy-intf';
+import { IntfDeconstructable, Sexp } from '@this/sexp';
 import { MustBe } from '@this/typey';
 
 import { IncomingRequest } from '#x/IncomingRequest';

src/sexp/tests/Sexp.test.js

@@ -181,3 +181,27 @@ describe('.toArray()', () => {
     expect(sexp.toArray()).toStrictEqual(expected);
   });
 });
+
+// This validates that it's safe to use `expect(sexp).toStrictEqual(otherSexp)`
+// in test cases throughout the system.
+describe('validating Jest usage', () => {
+  test('can use `expect().toStrictEqual()` to check `functor`', () => {
+    const sexp1a = new Sexp('x', 1, 2, 3);
+    const sexp1b = new Sexp('x', 1, 2, 3);
+    const sexp2  = new Sexp('y', 1, 2, 3);
+
+    expect(sexp1a).toStrictEqual(sexp1a);
+    expect(sexp1a).toStrictEqual(sexp1b);
+    expect(sexp1a).not.toStrictEqual(sexp2);
+  });
+
+  test('can use `expect().toStrictEqual()` to check `args`', () => {
+    const sexp1a = new Sexp('x', 1, 2, 3);
+    const sexp1b = new Sexp('x', 1, 2, 3);
+    const sexp2  = new Sexp('x', 1, 2, 3, 'floop');
+
+    expect(sexp1a).toStrictEqual(sexp1a);
+    expect(sexp1a).toStrictEqual(sexp1b);
+    expect(sexp1a).not.toStrictEqual(sexp2);
+  });
+});