From ca5b2a4a5527d96386fce132a4e36a7427277e36 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sat, 10 Feb 2024 21:47:41 -0800 Subject: [PATCH] fixup! GenericErrorConstructor adaptor --- packages/common/throw-labeled.js | 16 ++++- packages/errors/index.js | 4 +- packages/marshal/src/marshal-justin.js | 5 +- packages/marshal/src/marshal.js | 39 +++++++--- packages/pass-style/src/error.js | 61 +++++++++++++--- .../pass-style/test/test-extended-errors.js | 23 ++++++ packages/ses/src/commons.js | 1 + packages/ses/src/error/assert.js | 43 ++++++++--- packages/ses/src/error/types.js | 72 ++++++++++++------- packages/ses/types.d.ts | 20 ++++-- 10 files changed, 220 insertions(+), 64 deletions(-) create mode 100644 packages/pass-style/test/test-extended-errors.js diff --git a/packages/common/throw-labeled.js b/packages/common/throw-labeled.js index b7457ecf4f..b8835405a4 100644 --- a/packages/common/throw-labeled.js +++ b/packages/common/throw-labeled.js @@ -7,14 +7,24 @@ import { X, makeError, annotateError } from '@endo/errors'; * * @param {Error} innerErr * @param {string|number} label - * @param {ErrorConstructor=} ErrorConstructor + * @param {import('ses/src/error/types.js').GenericErrorConstructor} [errConstructor] + * @param {import('ses/src/error/types.js').AssertMakeErrorOptions} [options] * @returns {never} */ -export const throwLabeled = (innerErr, label, ErrorConstructor = undefined) => { +export const throwLabeled = ( + innerErr, + label, + errConstructor = undefined, + options = undefined, +) => { if (typeof label === 'number') { label = `[${label}]`; } - const outerErr = makeError(`${label}: ${innerErr.message}`, ErrorConstructor); + const outerErr = makeError( + `${label}: ${innerErr.message}`, + errConstructor, + options, + ); annotateError(outerErr, X`Caused by ${innerErr}`); throw outerErr; }; diff --git a/packages/errors/index.js b/packages/errors/index.js index 15320cbe1e..f3ae3d8cc2 100644 --- a/packages/errors/index.js +++ b/packages/errors/index.js @@ -57,8 +57,8 @@ const { } = globalAssert; /** @type {import("ses").AssertionFunctions } */ // @ts-expect-error missing properties assigned next -const assert = (value, optDetails, optErrorContructor) => - globalAssert(value, optDetails, optErrorContructor); +const assert = (value, optDetails, errContructor, options) => + globalAssert(value, optDetails, errContructor, options); Object.assign(assert, assertions); export { diff --git a/packages/marshal/src/marshal-justin.js b/packages/marshal/src/marshal-justin.js index 240328774f..e09a2279b7 100644 --- a/packages/marshal/src/marshal-justin.js +++ b/packages/marshal/src/marshal-justin.js @@ -217,8 +217,9 @@ const decodeToJustin = (encoding, shouldIndent = false, slots = []) => { } case 'error': { const { name, message } = rawTree; - typeof name === 'string' || - Fail`invalid error name typeof ${q(typeof name)}`; + if (typeof name !== 'string') { + throw Fail`invalid error name typeof ${q(typeof name)}`; + } getErrorConstructor(name) !== undefined || Fail`Must be the name of an Error constructor ${name}`; typeof message === 'string' || diff --git a/packages/marshal/src/marshal.js b/packages/marshal/src/marshal.js index fe2b419e3b..d31c6eb884 100644 --- a/packages/marshal/src/marshal.js +++ b/packages/marshal/src/marshal.js @@ -112,8 +112,9 @@ export const makeMarshal = ( assert.typeof(message, 'string'); const name = encodeRecur(`${err.name}`); assert.typeof(name, 'string'); - // Must encode `cause`, `errors`. - // nested non-passable errors must be ok from here. + // TODO Must encode `cause`, `errors`, but + // only once all possible counterparty decoders are tolerant of + // receiving them. if (errorTagging === 'on') { // We deliberately do not share the stack, but it would // be useful to log the stack locally so someone who has @@ -254,12 +255,25 @@ export const makeMarshal = ( }; /** - * @param {{errorId?: string, message: string, name: string}} errData + * @param {{ + * errorId?: string, + * message: string, + * name: string, + * cause: unknown, + * errors: unknown, + * }} errData * @param {(e: unknown) => Passable} decodeRecur * @returns {Error} */ const decodeErrorCommon = (errData, decodeRecur) => { - const { errorId = undefined, message, name, ...rest } = errData; + const { + errorId = undefined, + message, + name, + cause = undefined, + errors = undefined, + ...rest + } = errData; ownKeys(rest).length === 0 || Fail`unexpected encoded error properties ${q(ownKeys(rest))}`; // TODO Must decode `cause` and `errors` properties @@ -272,13 +286,22 @@ export const makeMarshal = ( Fail`invalid error name typeof ${q(typeof dName)}`; typeof dMessage === 'string' || Fail`invalid error message typeof ${q(typeof dMessage)}`; - const EC = getErrorConstructor(dName) || Error; + const errConstructor = getErrorConstructor(dName) || Error; // errorId is a late addition so be tolerant of its absence. const errorName = dErrorId === undefined - ? `Remote${EC.name}` - : `Remote${EC.name}(${dErrorId})`; - const error = makeError(dMessage, EC, { errorName }); + ? `Remote${errConstructor.name}` + : `Remote${errConstructor.name}(${dErrorId})`; + const options = { + errorName, + }; + if (cause) { + options.cause = decodeRecur(cause); + } + if (errors) { + options.errors = decodeRecur(errors); + } + const error = makeError(dMessage, errConstructor, options); return harden(error); }; diff --git a/packages/pass-style/src/error.js b/packages/pass-style/src/error.js index 9171d43991..7e55b5e0ff 100644 --- a/packages/pass-style/src/error.js +++ b/packages/pass-style/src/error.js @@ -1,6 +1,6 @@ /// -import { X, Fail, annotateError } from '@endo/errors'; +import { X, Fail, annotateError, makeError } from '@endo/errors'; import { assertChecker } from './passStyle-helpers.js'; /** @typedef {import('./internal-types.js').PassStyleHelper} PassStyleHelper */ @@ -14,7 +14,7 @@ const { ownKeys } = Reflect; const errorConstructors = new Map( // Cast because otherwise TS is confused by AggregateError // See https://github.com/endojs/endo/pull/2042#discussion_r1484933028 - /** @type {Array<[string, ErrorConstructor]>} */ + /** @type {Array<[string, import('ses/src/error/types.js').GenericErrorConstructor]>} */ ([ ['Error', Error], ['EvalError', EvalError], @@ -29,6 +29,16 @@ const errorConstructors = new Map( ]), ); +/** + * Because the error constructor returned by this function might be + * `AggregateError`, which has different construction parameters + * from the other error constructors, do not use it directly to try + * to make an error instance. Rather, use `makeError` which encapsulates + * this non-uniformity. + * + * @param {string} name + * @returns {import('ses/src/error/types.js').GenericErrorConstructor | undefined} + */ export const getErrorConstructor = name => errorConstructors.get(name); harden(getErrorConstructor); @@ -46,6 +56,7 @@ const checkErrorLike = (candidate, check = undefined) => { ); }; harden(checkErrorLike); +/// /** * Validating error objects are passable raises a tension between security @@ -76,18 +87,20 @@ export const ErrorHelper = harden({ canBeValid: checkErrorLike, - assertValid: candidate => { + assertValid: (candidate, passStyleOfRecur) => { ErrorHelper.canBeValid(candidate, assertChecker); const proto = getPrototypeOf(candidate); const { name } = proto; - const EC = getErrorConstructor(name); - (EC && EC.prototype === proto) || + const errConstructor = getErrorConstructor(name); + (errConstructor && errConstructor.prototype === proto) || Fail`Errors must inherit from an error class .prototype ${candidate}`; const { // TODO Must allow `cause`, `errors` message: mDesc, stack: stackDesc, + cause: causeDesc = undefined, + errors: errorsDesc = undefined, ...restDescs } = getOwnPropertyDescriptors(candidate); ownKeys(restDescs).length < 1 || @@ -104,6 +117,22 @@ export const ErrorHelper = harden({ !stackDesc.enumerable || Fail`Passed Error "stack" ${stackDesc} must not be enumerable`; } + if (causeDesc) { + ErrorHelper.assertValid(causeDesc.value, passStyleOfRecur); + !causeDesc.enumerable || + Fail`Passed Error "cause" ${causeDesc} must not be enumerable`; + } + if (errorsDesc) { + const errors = errorsDesc.value; + passStyleOfRecur(errors) === 'copyArray' || + Fail`Passed Error "errors" must be an array`; + for (const subErr of errors) { + ErrorHelper.assertValid(subErr, passStyleOfRecur); + } + !errorsDesc.enumerable || + Fail`Passed Error "errors" ${errorsDesc} must not be enumerable`; + } + return true; }, }); @@ -112,14 +141,28 @@ export const ErrorHelper = harden({ * Return a new passable error that propagates the diagnostic info of the * original, and is linked to the original as a note. * - * @param {Error} err + * @param {Error | AggregateError} err * @returns {Error} */ export const toPassableError = err => { - const { name, message } = err; + const { + name, + message, + cause = undefined, + // @ts-expect-error err might be an AggregateError, which would + // have an `errors` property. In addition, we tolerate `errors` on + // other errors, just like we do `cause`. + errors = undefined, + } = err; - const EC = getErrorConstructor(`${name}`) || Error; - const newError = harden(new EC(`${message}`)); + const errConstructor = getErrorConstructor(`${name}`) || Error; + const newError = harden( + makeError(`${message}`, errConstructor, { + // @ts-ignore Assuming cause is Error | undefined + cause, + errors, + }), + ); // Even the cleaned up error copy, if sent to the console, should // cause hidden diagnostic information of the original error // to be logged. diff --git a/packages/pass-style/test/test-extended-errors.js b/packages/pass-style/test/test-extended-errors.js new file mode 100644 index 0000000000..0f653bf39a --- /dev/null +++ b/packages/pass-style/test/test-extended-errors.js @@ -0,0 +1,23 @@ +/* eslint-disable max-classes-per-file */ +import { test } from './prepare-test-env-ava.js'; + +// eslint-disable-next-line import/order +import { passStyleOf } from '../src/passStyleOf.js'; + +test('style of extended errors', t => { + const e1 = Error('e1'); + t.throws(() => passStyleOf(e1), { + message: 'Cannot pass non-frozen objects like "[Error: e1]". Use harden()', + }); + harden(e1); + t.is(passStyleOf(e1), 'error'); + + const e2 = harden(Error('e2', { cause: e1 })); + t.is(passStyleOf(e2), 'error'); + + const u3 = harden(URIError('u3', { cause: e1 })); + t.is(passStyleOf(u3), 'error'); + + const a4 = harden(AggregateError([e2, u3], 'a4', { cause: e1 })); + t.is(passStyleOf(a4), 'error'); +}); diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 2d53725267..2ef01d7425 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -47,6 +47,7 @@ export const { ReferenceError, SyntaxError, TypeError, + AggregateError, } = globalThis; export const { diff --git a/packages/ses/src/error/assert.js b/packages/ses/src/error/assert.js index ada53abe86..6dae9e3ae4 100644 --- a/packages/ses/src/error/assert.js +++ b/packages/ses/src/error/assert.js @@ -21,6 +21,7 @@ import { arrayPush, assign, freeze, + defineProperty, globalThis, is, isError, @@ -33,6 +34,7 @@ import { weakmapGet, weakmapHas, weakmapSet, + AggregateError, } from '../commons.js'; import { an, bestEffortStringify } from './stringify-utils.js'; import './types.js'; @@ -257,8 +259,8 @@ const tagError = (err, optErrorName = err.name) => { */ const makeError = ( optDetails = redactedDetails`Assert failed`, - ErrorConstructor = globalThis.Error, - { errorName = undefined } = {}, + errConstructor = globalThis.Error, + { errorName = undefined, cause = undefined, errors = undefined } = {}, ) => { if (typeof optDetails === 'string') { // If it is a string, use it as the literal part of the template so @@ -270,7 +272,26 @@ const makeError = ( throw TypeError(`unrecognized details ${quote(optDetails)}`); } const messageString = getMessageString(hiddenDetails); - const error = new ErrorConstructor(messageString); + const opts = cause && { cause }; + let error; + if (errConstructor === AggregateError) { + error = AggregateError(errors || [], messageString, opts); + } else { + error = /** @type {ErrorConstructor} */ (errConstructor)( + messageString, + opts, + ); + if (errors !== undefined) { + // Since we need to tolerate `errors` on an AggregateError, may as + // well tolerate it on all errors. + defineProperty(error, 'errors', { + value: errors, + writable: true, + enumerable: false, + configurable: true, + }); + } + } weakmapSet(hiddenMessageLogArgs, error, getLogArgs(hiddenDetails)); if (errorName !== undefined) { tagError(error, errorName); @@ -382,9 +403,10 @@ const makeAssert = (optRaise = undefined, unredacted = false) => { /** @type {AssertFail} */ const fail = ( optDetails = assertFailedDetails, - ErrorConstructor = globalThis.Error, + errConstructor = undefined, + options = undefined, ) => { - const reason = makeError(optDetails, ErrorConstructor); + const reason = makeError(optDetails, errConstructor, options); if (optRaise !== undefined) { optRaise(reason); } @@ -402,9 +424,10 @@ const makeAssert = (optRaise = undefined, unredacted = false) => { function baseAssert( flag, optDetails = undefined, - ErrorConstructor = undefined, + errConstructor = undefined, + options = undefined, ) { - flag || fail(optDetails, ErrorConstructor); + flag || fail(optDetails, errConstructor, options); } /** @type {AssertEqual} */ @@ -412,12 +435,14 @@ const makeAssert = (optRaise = undefined, unredacted = false) => { actual, expected, optDetails = undefined, - ErrorConstructor = undefined, + errConstructor = undefined, + options = undefined, ) => { is(actual, expected) || fail( optDetails || details`Expected ${actual} is same as ${expected}`, - ErrorConstructor || RangeError, + errConstructor || RangeError, + options, ); }; freeze(equal); diff --git a/packages/ses/src/error/types.js b/packages/ses/src/error/types.js index 4038cc5168..d3ecdfaa52 100644 --- a/packages/ses/src/error/types.js +++ b/packages/ses/src/error/types.js @@ -1,19 +1,35 @@ // @ts-check +/** + * TypeScript does not treat `AggregateErrorConstructor` as a subtype of + * `ErrorConstructor`, which makes sense because their constructors + * have incompatible signatures. However, we want to parameterize some + * operations by any error constructor, including possible `AggregateError`. + * So we introduce `GenericErrorConstructor` as a common supertype. Any call + * to it to make an instance must therefore first case split on whether the + * constructor is an AggregateErrorConstructor or a normal ErrorConstructor. + * + * @typedef {ErrorConstructor | AggregateErrorConstructor} GenericErrorConstructor + */ + /** * @callback BaseAssert * The `assert` function itself. * * @param {any} flag The truthy/falsy value - * @param {Details=} optDetails The details to throw - * @param {ErrorConstructor=} ErrorConstructor An optional alternate error - * constructor to use. + * @param {Details} [optDetails] The details to throw + * @param {GenericErrorConstructor} [errConstructor] + * An optional alternate error constructor to use. + * @param {AssertMakeErrorOptions} [options] * @returns {asserts flag} */ /** * @typedef {object} AssertMakeErrorOptions - * @property {string=} errorName + * @property {string} [errorName] + * @property {Error} [cause] + * @property {Error[]} [errors] + * Normally only used when the ErrorConstuctor is `AggregateError` */ /** @@ -22,10 +38,10 @@ * The `assert.error` method, recording details for the console. * * The optional `optDetails` can be a string. - * @param {Details=} optDetails The details of what was asserted - * @param {ErrorConstructor=} ErrorConstructor An optional alternate error - * constructor to use. - * @param {AssertMakeErrorOptions=} options + * @param {Details} [optDetails] The details of what was asserted + * @param {GenericErrorConstructor} [errConstructor] + * An optional alternate error constructor to use. + * @param {AssertMakeErrorOptions} [options] * @returns {Error} */ @@ -40,9 +56,10 @@ * * The optional `optDetails` can be a string for backwards compatibility * with the nodejs assertion library. - * @param {Details=} optDetails The details of what was asserted - * @param {ErrorConstructor=} ErrorConstructor An optional alternate error - * constructor to use. + * @param {Details} [optDetails] The details of what was asserted + * @param {GenericErrorConstructor} [errConstructor] + * An optional alternate error constructor to use. + * @param {AssertMakeErrorOptions} [options] * @returns {never} */ @@ -53,9 +70,10 @@ * Assert that two values must be `Object.is`. * @param {any} actual The value we received * @param {any} expected What we wanted - * @param {Details=} optDetails The details to throw - * @param {ErrorConstructor=} ErrorConstructor An optional alternate error - * constructor to use. + * @param {Details} [optDetails] The details to throw + * @param {GenericErrorConstructor} [errConstructor] + * An optional alternate error constructor to use. + * @param {AssertMakeErrorOptions} [options] * @returns {void} */ @@ -66,7 +84,7 @@ * @callback AssertTypeofBigint * @param {any} specimen * @param {'bigint'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is bigint} */ @@ -74,7 +92,7 @@ * @callback AssertTypeofBoolean * @param {any} specimen * @param {'boolean'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is boolean} */ @@ -82,7 +100,7 @@ * @callback AssertTypeofFunction * @param {any} specimen * @param {'function'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is Function} */ @@ -90,7 +108,7 @@ * @callback AssertTypeofNumber * @param {any} specimen * @param {'number'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is number} */ @@ -98,7 +116,7 @@ * @callback AssertTypeofObject * @param {any} specimen * @param {'object'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is Record | null} */ @@ -106,7 +124,7 @@ * @callback AssertTypeofString * @param {any} specimen * @param {'string'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is string} */ @@ -114,7 +132,7 @@ * @callback AssertTypeofSymbol * @param {any} specimen * @param {'symbol'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is symbol} */ @@ -122,7 +140,7 @@ * @callback AssertTypeofUndefined * @param {any} specimen * @param {'undefined'} typename - * @param {Details=} optDetails + * @param {Details} [optDetails] * @returns {asserts specimen is undefined} */ @@ -141,7 +159,7 @@ * * Assert an expected typeof result. * @param {any} specimen The value to get the typeof - * @param {Details=} optDetails The details to throw + * @param {Details} [optDetails] The details to throw * @returns {asserts specimen is string} */ @@ -202,7 +220,7 @@ * * @callback AssertQuote * @param {any} payload What to declassify - * @param {(string|number)=} spaces + * @param {(string|number)} [spaces] * @returns {StringablePayload} The declassified payload */ @@ -235,8 +253,8 @@ * `optRaise` returns normally, which would be unusual, the throw following * `optRaise(reason)` would still happen. * - * @param {Raise=} optRaise - * @param {boolean=} unredacted + * @param {Raise} [optRaise] + * @param {boolean} [unredacted] * @returns {Assert} */ @@ -400,6 +418,6 @@ * @callback FilterConsole * @param {VirtualConsole} baseConsole * @param {ConsoleFilter} filter - * @param {string=} topic + * @param {string} [topic] * @returns {VirtualConsole} */ diff --git a/packages/ses/types.d.ts b/packages/ses/types.d.ts index 79fec88bcf..c198edd1ac 100644 --- a/packages/ses/types.d.ts +++ b/packages/ses/types.d.ts @@ -119,6 +119,8 @@ export type Details = string | DetailsToken; export interface AssertMakeErrorOptions { errorName?: string; + cause?: Error; + errors?: Error[]; } type AssertTypeofBigint = ( @@ -175,6 +177,10 @@ interface ToStringable { toString(): string; } +export type GenericErrorConstructor = + | ErrorConstructor + | AggregateErrorConstructor; + export type Raise = (reason: Error) => void; // Behold: recursion. // eslint-disable-next-line no-use-before-define @@ -184,23 +190,29 @@ export interface AssertionFunctions { ( value: any, details?: Details, - errorConstructor?: ErrorConstructor, + errConstructor?: GenericErrorConstructor, + options?: AssertMakeErrorOptions, ): asserts value; typeof: AssertTypeof; equal( left: any, right: any, details?: Details, - errorConstructor?: ErrorConstructor, + errConstructor?: GenericErrorConstructor, + options?: AssertMakeErrorOptions, ): void; string(specimen: any, details?: Details): asserts specimen is string; - fail(details?: Details, errorConstructor?: ErrorConstructor): never; + fail( + details?: Details, + errConstructor?: GenericErrorConstructor, + options?: AssertMakeErrorOptions, + ): never; } export interface AssertionUtilities { error( details?: Details, - errorConstructor?: ErrorConstructor, + errConstructor?: GenericErrorConstructor, options?: AssertMakeErrorOptions, ): Error; note(error: Error, details: Details): void;