From 1dea84d316eb412d864042ffb08b4b6420092a7c Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Tue, 22 Oct 2024 02:47:58 -0400 Subject: [PATCH] fix(marshal)!: Update compareRank to terminate comparability at the first remotable (#2597) Fixes #2588 To be refinable into a total order that distinguishes remotables, `compareRank` must consider `[r1, 'x']` vs. `[r2, 'y']` as a tie rather than as equivalent to `[0, 'x']` vs. `[0, 'y']`, in case r1 vs. r2 ends up comparing differently than 'x' vs. 'y'. --- packages/marshal/NEWS.md | 4 + packages/marshal/src/encodePassable.js | 3 + packages/marshal/src/rankOrder.js | 64 +++-- packages/marshal/src/types.js | 81 ++++-- packages/marshal/test/_marshal-test-data.js | 6 + packages/marshal/test/encodePassable.test.js | 265 ++++++++++-------- packages/pass-style/tools.js | 2 + packages/pass-style/tools/arb-passable.js | 32 ++- packages/patterns/package.json | 2 + packages/patterns/src/keys/copyBag.js | 4 +- packages/patterns/src/keys/copySet.js | 4 +- .../src/keys/keycollection-operators.js | 4 +- .../patterns/src/keys/merge-bag-operators.js | 4 +- .../patterns/src/keys/merge-set-operators.js | 4 +- packages/patterns/src/types.js | 57 +--- packages/patterns/test/copySet.test.js | 47 +++- yarn.lock | 2 + 17 files changed, 351 insertions(+), 234 deletions(-) diff --git a/packages/marshal/NEWS.md b/packages/marshal/NEWS.md index 4b5c8ac543..ae49279801 100644 --- a/packages/marshal/NEWS.md +++ b/packages/marshal/NEWS.md @@ -1,5 +1,9 @@ User-visible changes in `@endo/marshal`: +# Next release + +- `compareRank` now short-circuits upon encountering remotables to compare, considering the inputs to be tied for the same rank regardless of what would otherwise be visited later in their respective data structures. This ensures that a `fullCompare` which does distinguish remotables will be a refinement of `compareRank`, rather than disagreeing about whether or not two values share a rank ([#2588](https://github.com/endojs/endo/issues/2588)). + # v1.5.1 (2024-07-30) - `deeplyFulfilled` moved from @endo/marshal to @endo/pass-style. @endo/marshal still reexports it, to avoid breaking old importers. But importers should be upgraded to import `deeplyFulfilled` directly from @endo/pass-style. diff --git a/packages/marshal/src/encodePassable.js b/packages/marshal/src/encodePassable.js index d5863af1ec..0935900749 100644 --- a/packages/marshal/src/encodePassable.js +++ b/packages/marshal/src/encodePassable.js @@ -877,6 +877,9 @@ export const passStylePrefixes = { string: 's', null: 'v', symbol: 'y', + // Because Array.prototype.sort puts undefined values at the end without + // passing them to a comparison function, undefined MUST be the last + // category. undefined: 'z', }; Object.setPrototypeOf(passStylePrefixes, null); diff --git a/packages/marshal/src/rankOrder.js b/packages/marshal/src/rankOrder.js index 2018833417..05ef1503b2 100644 --- a/packages/marshal/src/rankOrder.js +++ b/packages/marshal/src/rankOrder.js @@ -8,7 +8,7 @@ import { /** * @import {Passable, PassStyle} from '@endo/pass-style' - * @import {FullCompare, RankCompare, RankCover} from './types.js' + * @import {FullCompare, PartialCompare, PartialComparison, RankCompare, RankCover} from './types.js' */ const { entries, fromEntries, setPrototypeOf, is } = Object; @@ -101,15 +101,16 @@ const memoOfSorted = new WeakMap(); const comparatorMirrorImages = new WeakMap(); /** - * @param {RankCompare=} compareRemotables - * An option to create a comparator in which an internal order is - * assigned to remotables. This defaults to a comparator that - * always returns `0`, meaning that all remotables are tied - * for the same rank. + * @param {PartialCompare} [compareRemotables] + * A comparator for assigning an internal order to remotables. + * It defaults to a function that always returns `NaN`, meaning that all + * remotables are incomparable and should tie for the same rank by + * short-circuiting without further refinement (e.g., not only are `r1` and `r2` + * tied, but so are `[r1, 0]` and `[r2, "x"]`). * @returns {RankComparatorKit} */ -export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => { - /** @type {RankCompare} */ +export const makeComparatorKit = (compareRemotables = (_x, _y) => NaN) => { + /** @type {PartialCompare} */ const comparator = (left, right) => { if (sameValueZero(left, right)) { return 0; @@ -191,10 +192,9 @@ export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => { if (result !== 0) { return result; } - return comparator( - recordValues(left, leftNames), - recordValues(right, rightNames), - ); + const leftValues = recordValues(left, leftNames); + const rightValues = recordValues(right, rightNames); + return comparator(leftValues, rightValues); } case 'copyArray': { // Lexicographic @@ -225,14 +225,20 @@ export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => { }; /** @type {RankCompare} */ - const antiComparator = (x, y) => comparator(y, x); + const outerComparator = (x, y) => + // When the inner comparator returns NaN to indicate incomparability, + // replace that with 0 to indicate a tie. + /** @type {Exclude} */ (comparator(x, y) || 0); + + /** @type {RankCompare} */ + const antiComparator = (x, y) => outerComparator(y, x); - memoOfSorted.set(comparator, new WeakSet()); + memoOfSorted.set(outerComparator, new WeakSet()); memoOfSorted.set(antiComparator, new WeakSet()); - comparatorMirrorImages.set(comparator, antiComparator); - comparatorMirrorImages.set(antiComparator, comparator); + comparatorMirrorImages.set(outerComparator, antiComparator); + comparatorMirrorImages.set(antiComparator, outerComparator); - return harden({ comparator, antiComparator }); + return harden({ comparator: outerComparator, antiComparator }); }; /** * @param {RankCompare} comparator @@ -275,15 +281,6 @@ export const assertRankSorted = (sorted, compare) => harden(assertRankSorted); /** - * TODO SECURITY BUG: https://github.com/Agoric/agoric-sdk/issues/4260 - * sortByRank currently uses `Array.prototype.sort` directly, and - * so only works correctly when given a `compare` function that considers - * `undefined` strictly bigger (`>`) than everything else. This is - * because `Array.prototype.sort` bizarrely moves all `undefined`s to - * the end of the array regardless, without consulting the `compare` - * function. This is a genuine bug for us NOW because sometimes we sort - * in reverse order by passing a reversed rank comparison function. - * * @template {Passable} T * @param {Iterable} passables * @param {RankCompare} compare @@ -301,7 +298,20 @@ export const sortByRank = (passables, compare) => { } const unsorted = [...passables]; unsorted.forEach(harden); - const sorted = harden(unsorted.sort(compare)); + const sorted = unsorted.sort(compare); + // For reverse comparison, move `undefined` values from the end to the start. + // Note that passStylePrefixes (@see {@link ./encodePassable.js}) MUST NOT + // sort any category after `undefined`. + if (compare(true, undefined) > 0) { + let i = sorted.length - 1; + while (i >= 0 && sorted[i] === undefined) i -= 1; + const n = sorted.length - i - 1; + if (n > 0 && n < sorted.length) { + sorted.copyWithin(n, 0); + sorted.fill(/** @type {T} */ (undefined), 0, n); + } + } + harden(sorted); const subMemoOfSorted = memoOfSorted.get(compare); assert(subMemoOfSorted !== undefined); subMemoOfSorted.add(sorted); diff --git a/packages/marshal/src/types.js b/packages/marshal/src/types.js index c70818bf34..8f73c6c936 100644 --- a/packages/marshal/src/types.js +++ b/packages/marshal/src/types.js @@ -156,28 +156,27 @@ export {}; * Returns `-1`, `0`, or `1` depending on whether the rank of `left` * is respectively before, tied-with, or after the rank of `right`. * - * This comparison function is valid as argument to - * `Array.prototype.sort`. This is sometimes described as a "total order" - * but, depending on your definitions, this is technically incorrect because - * it may return `0` to indicate that two distinguishable elements such as - * `-0` and `0` are tied (i.e., are in the same equivalence class - * for the purposes of this ordering). If each such equivalence class is - * a *rank* and ranks are disjoint, then this "rank order" is a - * true total order over these ranks. In mathematics this goes by several - * other names such as "total preorder". + * As a total preorder, this comparison function is valid as an argument to + * `Array.prototype.sort` but may return `0` to indicate that two + * distinguishable elements such as `-0` and `0` are tied (i.e., are in the same + * equivalence class for the purposes of this ordering). If each such + * equivalence class is + * a *rank* and ranks are disjoint, then this "rank order" is a true total order + * over these ranks. * * This function establishes a total rank order over all passables. * To do so it makes arbitrary choices, such as that all strings - * are after all numbers. Thus, this order is not intended to be - * used directly as a comparison with useful semantics. However, it must be - * closely enough related to such comparisons to aid in implementing - * lookups based on those comparisons. For example, in order to get a total - * order among ranks, we put `NaN` after all other JavaScript "number" values - * (i.e., IEEE 754 floating-point values). But otherwise, we rank JavaScript - * numbers by signed magnitude, with `0` and `-0` tied. A semantically useful - * ordering would also compare magnitudes, and so agree with the rank ordering - * of all values other than `NaN`. An array sorted by rank would enable range - * queries by magnitude. + * are after all numbers, and thus is not intended to be used directly as a + * comparison with useful semantics. + * However, it must be closely enough related to such comparisons to aid in + * implementing lookups based on those comparisons. + * For example, in order to get a total order over ranks, we put `NaN` after all + * other JavaScript "number" values (i.e., IEEE 754 floating-point values) but + * otherwise rank JavaScript numbers by signed magnitude, with `0` and `-0` + * tied, as would a semantically useful ordering such as `KeyCompare` in + * {@link ../../patterns}. + * Likewise, an array sorted by rank would enable range queries by magnitude. + * * @param {any} left * @param {any} right * @returns {RankComparison} @@ -185,16 +184,38 @@ export {}; /** * @typedef {RankCompare} FullCompare - * A `FullCompare` function satisfies all the invariants stated below for - * `RankCompare`'s relation with KeyCompare. - * In addition, its equality is as precise as the `KeyCompare` - * comparison defined below, in that, for all Keys `x` and `y`, - * `FullCompare(x, y) === 0` iff `KeyCompare(x, y) === 0`. + * A function that refines `RankCompare` into a total order over its inputs by + * making arbitrary choices about the relative ordering of values within the + * same rank. + * Like `RankCompare` but even more strongly, it is expected to agree with a + * `KeyCompare` (@see {@link ../../patterns}) where they overlap --- + * `FullCompare(key1, key2) === 0` iff `KeyCompare(key1, key2) === 0`. + */ + +/** + * @typedef {-1 | 0 | 1 | NaN} PartialComparison + * The result of a `PartialCompare` function that defines a meaningful and + * meaningfully precise partial order in which incomparable values are + * represented by `NaN`. See `PartialCompare`. + */ + +/** + * @template [T=any] + * @callback PartialCompare + * A function that implements a partial order --- defining relative position + * between values but leaving some pairs incomparable (for example, subsets over + * sets is a partial order in which {} precedes {x} and {y}, which are mutually + * incomparable but both precede {x, y}). As with the rank ordering produced by + * `RankCompare`, -1, 0, and 1 respectively mean "less than", "equivalent to", + * and "greater than". NaN means "incomparable" --- the first value is not less, + * equivalent, or greater than the second. + * + * By using NaN for "incomparable", the normal equivalence for using + * the return value in a comparison is preserved. + * `PartialCompare(left, right) >= 0` iff `left` is greater than or equivalent + * to `right` in the partial ordering. * - * For non-keys a `FullCompare` should be exactly as imprecise as - * `RankCompare`. For example, both will treat all errors as in the same - * equivalence class. Both will treat all promises as in the same - * equivalence class. Both will order taggeds the same way, which is admittedly - * weird, as some taggeds will be considered keys and other taggeds will be - * considered non-keys. + * @param {T} left + * @param {T} right + * @returns {PartialComparison} */ diff --git a/packages/marshal/test/_marshal-test-data.js b/packages/marshal/test/_marshal-test-data.js index b4be1d07a5..fcdc680a55 100644 --- a/packages/marshal/test/_marshal-test-data.js +++ b/packages/marshal/test/_marshal-test-data.js @@ -271,6 +271,9 @@ export const unsortedSample = harden([ Symbol.for(''), false, exampleCarol, + [exampleCarol, 'm'], + [exampleAlice, 'a'], + [exampleBob, 'z'], -0, {}, [5, undefined], @@ -350,6 +353,9 @@ export const sortedSample = harden([ [5, { foo: 4, bar: undefined }], [5, null], [5, undefined], + [exampleAlice, 'a'], + [exampleCarol, 'm'], + [exampleBob, 'z'], false, true, diff --git a/packages/marshal/test/encodePassable.test.js b/packages/marshal/test/encodePassable.test.js index f0fbd63053..dbb8967ca7 100644 --- a/packages/marshal/test/encodePassable.test.js +++ b/packages/marshal/test/encodePassable.test.js @@ -5,91 +5,68 @@ import test from '@endo/ses-ava/prepare-endo.js'; import { fc } from '@fast-check/ava'; import { Remotable } from '@endo/pass-style'; import { arbPassable } from '@endo/pass-style/tools.js'; -import { Fail, q } from '@endo/errors'; +import { assert, Fail, q, b } from '@endo/errors'; import { makePassableKit, makeEncodePassable, makeDecodePassable, } from '../src/encodePassable.js'; -import { compareRank, makeComparatorKit } from '../src/rankOrder.js'; +import { compareRank, makeFullOrderComparatorKit } from '../src/rankOrder.js'; import { unsortedSample } from './_marshal-test-data.js'; -const buffers = { - __proto__: null, - r: [], - '?': [], - '!': [], -}; -const resetBuffers = () => { - buffers.r = []; - buffers['?'] = []; - buffers['!'] = []; -}; -const cursors = { - __proto__: null, - r: 0, - '?': 0, - '!': 0, -}; -const resetCursors = () => { - cursors.r = 0; - cursors['?'] = 0; - cursors['!'] = 0; -}; - -const encodeThing = (prefix, r) => { - buffers[prefix].push(r); - // With this encoding, all things with the same prefix have the same rank - return prefix; -}; - -const decodeThing = (prefix, e) => { - prefix === e || - Fail`expected encoding ${q(e)} to simply be the prefix ${q(prefix)}`; - (cursors[prefix] >= 0 && cursors[prefix] < buffers[prefix].length) || - Fail`while decoding ${q(e)}, expected cursors[${q(prefix)}], i.e., ${q( - cursors[prefix], - )} <= ${q(buffers[prefix].length)}`; - const thing = buffers[prefix][cursors[prefix]]; - cursors[prefix] += 1; - return thing; -}; - -const compareRemotables = (x, y) => - compareRank(encodeThing('r', x), encodeThing('r', y)); - -const encodePassableInternal = makeEncodePassable({ - encodeRemotable: r => encodeThing('r', r), - encodePromise: p => encodeThing('?', p), - encodeError: er => encodeThing('!', er), -}); -const encodePassableInternal2 = makeEncodePassable({ - encodeRemotable: r => encodeThing('r', r), - encodePromise: p => encodeThing('?', p), - encodeError: er => encodeThing('!', er), - format: 'compactOrdered', -}); - -const encodePassable = passable => { - resetBuffers(); - return encodePassableInternal(passable); -}; -const encodePassable2 = passable => { - resetBuffers(); - return encodePassableInternal2(passable); -}; +const statelessEncodePassableLegacy = makeEncodePassable(); + +const makeSimplePassableKit = ({ statelessSuffix } = {}) => { + let count = 0n; + const encodingFromVal = new Map(); + const valFromEncoding = new Map(); + + const encodeSpecial = (prefix, val) => { + const foundEncoding = encodingFromVal.get(val); + if (foundEncoding) return foundEncoding; + count += 1n; + const template = statelessEncodePassableLegacy(count); + const encoding = prefix + template.substring(1); + encodingFromVal.set(val, encoding); + valFromEncoding.set(encoding, val); + return encoding; + }; + const decodeSpecial = (prefix, e) => { + e.startsWith(prefix) || + Fail`expected encoding ${q(e)} to start with ${b(prefix)}`; + const val = + valFromEncoding.get(e) || Fail`no object found while decoding ${q(e)}`; + return val; + }; -const decodePassableInternal = makeDecodePassable({ - decodeRemotable: e => decodeThing('r', e), - decodePromise: e => decodeThing('?', e), - decodeError: e => decodeThing('!', e), -}); + const encoders = + statelessSuffix !== undefined + ? { + encodeRemotable: r => `r${statelessSuffix}`, + encodePromise: p => `?${statelessSuffix}`, + encodeError: err => `!${statelessSuffix}`, + } + : { + encodeRemotable: r => encodeSpecial('r', r), + encodePromise: p => encodeSpecial('?', p), + encodeError: err => encodeSpecial('!', err), + }; + const encodePassableLegacy = makeEncodePassable({ ...encoders }); + const encodePassableCompact = makeEncodePassable({ + ...encoders, + format: 'compactOrdered', + }); + const decodePassable = makeDecodePassable({ + decodeRemotable: e => decodeSpecial('r', e), + decodePromise: e => decodeSpecial('?', e), + decodeError: e => decodeSpecial('!', e), + }); -const decodePassable = encoded => { - resetCursors(); - return decodePassableInternal(encoded); + return { encodePassableLegacy, encodePassableCompact, decodePassable }; }; +const pickLegacy = kit => kit.encodePassableLegacy; +const pickCompact = kit => kit.encodePassableCompact; test('makePassableKit output shape', t => { const kit = makePassableKit(); @@ -133,7 +110,7 @@ test( (...args) => makePassableKit(...args).encodePassable, ); -const { comparator: compareFull } = makeComparatorKit(compareRemotables); +const { comparator: compareFull } = makeFullOrderComparatorKit(); const asNumber = new Float64Array(1); const asBits = new BigUint64Array(asNumber.buffer); @@ -171,6 +148,11 @@ const goldenPairs = harden([ ]); test('golden round trips', t => { + const { + encodePassableLegacy: encodePassable, + encodePassableCompact: encodePassable2, + decodePassable, + } = makeSimplePassableKit({ stateless: true }); for (const [k, e] of goldenPairs) { t.is(encodePassable(k), e, 'does k encode as expected'); t.is(encodePassable2(k), `~${e}`, 'does k small-encode as expected'); @@ -258,13 +240,14 @@ test('capability encoding', t => { }); test('compact string validity', t => { - t.notThrows(() => decodePassableInternal('~sa"z')); - t.notThrows(() => decodePassableInternal('~sa!!z')); + const { decodePassable } = makeSimplePassableKit({ stateless: true }); + t.notThrows(() => decodePassable('~sa"z')); + t.notThrows(() => decodePassable('~sa!!z')); const specialEscapes = ['!_', '!|', '_@', '__']; for (const prefix of ['!', '_']) { for (let cp = 0; cp <= 0x7f; cp += 1) { const esc = `${prefix}${String.fromCodePoint(cp)}`; - const tryDecode = () => decodePassableInternal(`~sa${esc}z`); + const tryDecode = () => decodePassable(`~sa${esc}z`); if (esc.match(/![!-@]/) || specialEscapes.includes(esc)) { t.notThrows(tryDecode, `valid string escape: ${JSON.stringify(esc)}`); } else { @@ -276,7 +259,7 @@ test('compact string validity', t => { } } t.throws( - () => decodePassableInternal(`~sa${prefix}`), + () => decodePassable(`~sa${prefix}`), { message: /invalid string escape/ }, `unterminated ${JSON.stringify(prefix)} escape`, ); @@ -285,7 +268,7 @@ test('compact string validity', t => { const ch = String.fromCodePoint(cp); const uCode = cp.toString(16).padStart(4, '0').toUpperCase(); t.throws( - () => decodePassableInternal(`~sa${ch}z`), + () => decodePassable(`~sa${ch}z`), { message: /invalid string escape/ }, `disallowed string control character: U+${uCode} ${JSON.stringify(ch)}`, ); @@ -293,6 +276,7 @@ test('compact string validity', t => { }); test('compact custom encoding validity constraints', t => { + const { encodePassableCompact } = makeSimplePassableKit(); const encodings = new Map(); const dynamicEncoder = obj => encodings.get(obj); const dynamicEncodePassable = makeEncodePassable({ @@ -332,11 +316,11 @@ test('compact custom encoding validity constraints', t => { 'custom encoding containing an invalid string escape is acceptable', ); t.notThrows( - makeTryEncode(`${sigil}${encodePassableInternal2(harden([]))}`), + makeTryEncode(`${sigil}${encodePassableCompact(harden([]))}`), 'custom encoding containing an empty array is acceptable', ); t.notThrows( - makeTryEncode(`${sigil}${encodePassableInternal2(harden(['foo', []]))}`), + makeTryEncode(`${sigil}${encodePassableCompact(harden(['foo', []]))}`), 'custom encoding containing a non-empty array is acceptable', ); @@ -353,31 +337,61 @@ test('compact custom encoding validity constraints', t => { } }); -const orderInvariants = (x, y) => { +// TODO: Make a common utility for finding the first difference between iterables. +// import { firstDiff } from '...'; +// const firstStringDiff = (a, b) => firstDiff(a, b, { getIncrement: ch => ch.length }); +// const commonStringPrefix = (a, b) => a.substring(0, firstStringDiff(a, b).index); +const commonStringPrefix = (str1, str2) => { + // Co-iterate over *code points*. + const iter1 = str1[Symbol.iterator](); + const iter2 = str2[Symbol.iterator](); + // ...but track a *code unit* index for extracting a substring at the end. + let i = 0; + for (;;) { + const { value: char1 } = iter1.next(); + const { value: char2 } = iter2.next(); + if (char1 !== char2 || char1 === undefined) { + return str1.substring(0, i); + } + // ...because some code points consist of a two-code-unit surrogate pair. + i += char1.length; + } +}; + +const orderInvariants = (x, y, statelessEncode1, statelessEncode2) => { const rankComp = compareRank(x, y); const fullComp = compareFull(x, y); if (rankComp !== 0) { Object.is(rankComp, fullComp) || - Fail`with rankComp ${rankComp}, expected matching fullComp: ${fullComp} for ${x} ${y}`; + Fail`with rankComp ${rankComp}, expected matching fullComp: ${fullComp} for ${x} vs. ${y}`; } if (fullComp === 0) { Object.is(rankComp, 0) || - Fail`with fullComp 0, expected matching rankComp: ${rankComp} for ${x} ${y}`; + Fail`with fullComp 0, expected matching rankComp: ${rankComp} for ${x} vs. ${y}`; } else { + assert(fullComp !== 0); rankComp === 0 || rankComp === fullComp || - Fail`with fullComp ${fullComp}, expected 0 or matching rankComp: ${rankComp} for ${x} ${y}`; + Fail`with fullComp ${fullComp}, expected rankComp 0 or matching: ${rankComp} for ${x} vs. ${y}`; } - const ex = encodePassable(x); - const ey = encodePassable(y); - const encComp = compareRank(ex, ey); + const ex = statelessEncode1(x); + const ey = statelessEncode1(y); if (fullComp !== 0) { - Object.is(encComp, fullComp) || - Fail`with fullComp ${fullComp}, expected matching encComp: ${encComp} for ${ex} ${ey}`; + // Comparability of encodings stops at the first incomparable special rank + // (remotable/promise/error). + const exPrefix = commonStringPrefix(ex, statelessEncode2(x)); + const eyPrefix = commonStringPrefix(ey, statelessEncode2(y)); + const encComp = compareRank(exPrefix, eyPrefix); + encComp === 0 || + encComp === fullComp || + Fail`with fullComp ${fullComp}, expected matching stateless encComp: ${encComp} for ${x} as ${ex} vs. ${y} as ${ey}`; } }; -const testRoundTrip = test.macro(async (t, encode) => { +const testRoundTrip = test.macro(async (t, pickEncode) => { + const kit = makeSimplePassableKit(); + const encode = pickEncode(kit); + const { decodePassable } = kit; await fc.assert( fc.property(arbPassable, n => { const en = encode(n); @@ -388,39 +402,60 @@ const testRoundTrip = test.macro(async (t, encode) => { }), ); }); -test('original encoding round-trips', testRoundTrip, encodePassable); -test('small encoding round-trips', testRoundTrip, encodePassable2); +test('original encoding round-trips', testRoundTrip, pickLegacy); +test('small encoding round-trips', testRoundTrip, pickCompact); -test('BigInt encoding comparison corresponds with numeric comparison', async t => { +const testBigInt = test.macro(async (t, pickEncode) => { + const kit = makeSimplePassableKit({ statelessSuffix: '' }); + const encodePassable = pickEncode(kit); await fc.assert( - fc.property(fc.bigInt(), fc.bigInt(), (a, b) => { - const ea = encodePassable(a); - const eb = encodePassable(b); - t.is(a < b, ea < eb); - t.is(a > b, ea > eb); + fc.property(fc.bigInt(), fc.bigInt(), (x, y) => { + const ex = encodePassable(x); + const ey = encodePassable(y); + t.is(x < y, ex < ey); + t.is(x > y, ex > ey); }), ); }); +test( + 'original BigInt encoding comparison corresponds with numeric comparison', + testBigInt, + pickLegacy, +); +test( + 'small BigInt encoding comparison corresponds with numeric comparison', + testBigInt, + pickCompact, +); + +const testOrderInvariants = test.macro(async (t, pickEncode) => { + const kit1 = makeSimplePassableKit({ statelessSuffix: '' }); + const statelessEncode1 = pickEncode(kit1); + const kit2 = makeSimplePassableKit({ statelessSuffix: '.' }); + const statelessEncode2 = pickEncode(kit2); + + for (const x of unsortedSample) { + for (const y of unsortedSample) { + orderInvariants(x, y, statelessEncode1, statelessEncode2); + } + } -test('Passable encoding corresponds to rankOrder', async t => { await fc.assert( - fc.property(arbPassable, arbPassable, (a, b) => { - return orderInvariants(a, b); + fc.property(arbPassable, arbPassable, (x, y) => { + return orderInvariants(x, y, statelessEncode1, statelessEncode2); }), ); - // Ensure at least one ava assertion. - t.pass(); -}); -// The following is logically like the test above, but rather than relying on -// the heuristic generation of fuzzing test cases, it always checks everything -// in `sample`. -test('Also test against all enumerated in sample', t => { - for (let i = 0; i < unsortedSample.length; i += 1) { - for (let j = i; j < unsortedSample.length; j += 1) { - orderInvariants(unsortedSample[i], unsortedSample[j]); - } - } // Ensure at least one ava assertion. t.pass(); }); +test( + 'original passable encoding corresponds to rankOrder', + testOrderInvariants, + pickLegacy, +); +test( + 'small passable encoding corresponds to rankOrder', + testOrderInvariants, + pickCompact, +); diff --git a/packages/pass-style/tools.js b/packages/pass-style/tools.js index 78fd22bf37..50fd8ca393 100644 --- a/packages/pass-style/tools.js +++ b/packages/pass-style/tools.js @@ -10,6 +10,8 @@ export { exampleBob, exampleCarol, arbString, + arbKeyLeaf, arbLeaf, + arbKey, arbPassable, } from './tools/arb-passable.js'; diff --git a/packages/pass-style/tools/arb-passable.js b/packages/pass-style/tools/arb-passable.js index 22549f5d83..5c6bcbdfbf 100644 --- a/packages/pass-style/tools/arb-passable.js +++ b/packages/pass-style/tools/arb-passable.js @@ -14,7 +14,7 @@ export const exampleCarol = Far('carol', {}); export const arbString = fc.oneof(fc.string(), fc.fullUnicodeString()); -export const arbLeaf = fc.oneof( +const keyableLeaves = [ fc.constantFrom(null, undefined, false, true), arbString, arbString.map(s => Symbol.for(s)), @@ -31,22 +31,43 @@ export const arbLeaf = fc.oneof( fc.constantFrom(-0, NaN, Infinity, -Infinity), fc.record({}), fc.constantFrom(exampleAlice, exampleBob, exampleCarol), +]; + +export const arbKeyLeaf = fc.oneof(...keyableLeaves); + +export const arbLeaf = fc.oneof( + ...keyableLeaves, arbString.map(s => Error(s)), // unresolved promise fc.constant(new Promise(() => {})), ); +const { keyDag } = fc.letrec(tie => { + return { + keyDag: fc.oneof( + { withCrossShrink: true }, + arbKeyLeaf, + fc.array(tie('keyDag')), + fc.dictionary( + arbString.filter(s => s !== 'then'), + tie('keyDag'), + ), + ), + }; +}); + const { arbDag } = fc.letrec(tie => { return { arbDag: fc.oneof( { withCrossShrink: true }, arbLeaf, - tie('arbDag').map(v => Promise.resolve(v)), fc.array(tie('arbDag')), fc.dictionary( arbString.filter(s => s !== 'then'), tie('arbDag'), ), + // A promise for a passable. + tie('arbDag').map(v => Promise.resolve(v)), // A tagged value, either of arbitrary type with arbitrary payload // or of known type with arbitrary or explicitly valid payload. // Ordered by increasing complexity. @@ -110,6 +131,11 @@ const { arbDag } = fc.letrec(tie => { }); /** - * A factory for arbitrary passables + * A factory for arbitrary keys. + */ +export const arbKey = keyDag.map(x => harden(x)); + +/** + * A factory for arbitrary passables. */ export const arbPassable = arbDag.map(x => harden(x)); diff --git a/packages/patterns/package.json b/packages/patterns/package.json index 701e5470e5..e1d806d0a8 100644 --- a/packages/patterns/package.json +++ b/packages/patterns/package.json @@ -40,7 +40,9 @@ }, "devDependencies": { "@endo/init": "workspace:^", + "@endo/pass-style": "workspace:^", "@endo/ses-ava": "workspace:^", + "@fast-check/ava": "^1.1.5", "ava": "^6.1.3", "babel-eslint": "^10.1.0", "eslint": "^8.57.0", diff --git a/packages/patterns/src/keys/copyBag.js b/packages/patterns/src/keys/copyBag.js index 09d9605ff9..7f0e93dac3 100644 --- a/packages/patterns/src/keys/copyBag.js +++ b/packages/patterns/src/keys/copyBag.js @@ -14,8 +14,8 @@ import { X } from '@endo/errors'; /** * @import {Passable} from '@endo/pass-style' - * @import {Checker} from '@endo/marshal' - * @import {CopyBag, Key, FullCompare} from '../types.js' + * @import {Checker, FullCompare} from '@endo/marshal' + * @import {CopyBag, Key} from '../types.js' */ /** diff --git a/packages/patterns/src/keys/copySet.js b/packages/patterns/src/keys/copySet.js index 3901c125bc..4c8588d93f 100644 --- a/packages/patterns/src/keys/copySet.js +++ b/packages/patterns/src/keys/copySet.js @@ -14,8 +14,8 @@ import { X } from '@endo/errors'; /** * @import {Passable} from '@endo/pass-style' - * @import {Checker} from '@endo/marshal' - * @import {CopySet, FullCompare, Key} from '../types.js' + * @import {Checker, FullCompare} from '@endo/marshal' + * @import {CopySet, Key} from '../types.js' */ /** diff --git a/packages/patterns/src/keys/keycollection-operators.js b/packages/patterns/src/keys/keycollection-operators.js index 06ed17f34d..8350aaccbe 100644 --- a/packages/patterns/src/keys/keycollection-operators.js +++ b/packages/patterns/src/keys/keycollection-operators.js @@ -9,8 +9,8 @@ import { makeIterator } from '@endo/common/make-iterator.js'; import { makeArrayIterator } from '@endo/common/make-array-iterator.js'; /** - * @import {RankCompare} from '@endo/marshal' - * @import {Key, KeyCompare, FullCompare, KeyComparison, KeyCollection} from '../types.js' + * @import {FullCompare, RankCompare} from '@endo/marshal' + * @import {Key, KeyCompare, KeyComparison, KeyCollection} from '../types.js' */ import { q, Fail } from '@endo/errors'; diff --git a/packages/patterns/src/keys/merge-bag-operators.js b/packages/patterns/src/keys/merge-bag-operators.js index 9ae1a8dd44..1cede58f6e 100644 --- a/packages/patterns/src/keys/merge-bag-operators.js +++ b/packages/patterns/src/keys/merge-bag-operators.js @@ -9,8 +9,8 @@ import { assertNoDuplicateKeys, makeBagOfEntries } from './copyBag.js'; /** * @import {Passable} from '@endo/pass-style'; - * @import {RankCompare} from '@endo/marshal' - * @import {FullCompare, Key} from '../types.js' + * @import {FullCompare, RankCompare} from '@endo/marshal' + * @import {Key} from '../types.js' */ // Based on merge-set-operators.js, but altered for the bag representation. diff --git a/packages/patterns/src/keys/merge-set-operators.js b/packages/patterns/src/keys/merge-set-operators.js index a9bdf2b65d..42be66d73f 100644 --- a/packages/patterns/src/keys/merge-set-operators.js +++ b/packages/patterns/src/keys/merge-set-operators.js @@ -9,8 +9,8 @@ import { assertNoDuplicates, makeSetOfElements } from './copySet.js'; /** * @import {Passable} from '@endo/pass-style'; - * @import {RankCompare} from '@endo/marshal' - * @import {FullCompare, KeyComparison} from '../types.js' + * @import {FullCompare, RankCompare} from '@endo/marshal' + * @import {KeyComparison} from '../types.js' */ // TODO share more code with keycollection-operators.js. diff --git a/packages/patterns/src/types.js b/packages/patterns/src/types.js index cbb4b6fa19..e84eb7fea2 100644 --- a/packages/patterns/src/types.js +++ b/packages/patterns/src/types.js @@ -5,7 +5,12 @@ export {}; // NB: as of TS 5.5 nightly, TS thinks RankCover and Checker "is declared but never read" but they are /** * @import {Checker, CopyArray, CopyRecord, CopyTagged, Passable, PassStyle, Primitive, RemotableObject} from '@endo/pass-style'; - * @import {RankCompare, RankCover} from '@endo/marshal'; + * @import {PartialCompare, PartialComparison, RankCompare, RankCover} from '@endo/marshal'; + */ + +// Re-exported types. +/** + * @typedef {import('@endo/marshal').FullCompare} FullCompare */ /** @@ -143,61 +148,17 @@ export {}; */ /** - * @typedef {RankCompare} FullCompare - * A `FullCompare` function satisfies all the invariants stated below for - * `RankCompare`'s relation with KeyCompare. - * In addition, its equality is as precise as the `KeyCompare` - * comparison defined below, in that, for all Keys `x` and `y`, - * `FullCompare(x, y) === 0` iff `KeyCompare(x, y) === 0`. - * - * For non-Key inputs, a `FullCompare` should be exactly as imprecise as - * `RankCompare`. For example, both will treat all errors as in the same - * equivalence class. Both will treat all promises as in the same - * equivalence class. Both will order tagged records the same way, which is - * admittedly weird because some (such as CopySets, CopyBags, and CopyMaps) - * will be considered Keys while others will be considered non-Keys. - */ - -/** - * @typedef {object} RankComparatorKit - * @property {RankCompare} comparator - * @property {RankCompare} antiComparator - */ - -/** - * @typedef {object} FullComparatorKit - * @property {FullCompare} comparator - * @property {FullCompare} antiComparator - */ - -/** - * @typedef {-1 | 0 | 1 | NaN} KeyComparison + * @typedef {PartialComparison} KeyComparison * The result of a `KeyCompare` function that defines a meaningful * and meaningfully precise partial order of Key values. See `KeyCompare`. */ /** - * @callback KeyCompare - * `compareKeys` implements a partial order over Keys --- it defines relative - * position between two Keys but leaves some pairs incomparable (for example, - * subsets over sets is a partial order in which {} precedes {x} and {y}, which - * are mutually incomparable but both precede {x, y}). As with the rank ordering - * produced by `compareRank`, -1, 0, and 1 respectively mean "less than", - * "equivalent to", and "greater than". NaN means "incomparable" --- the first - * key is not less, equivalent, or greater than the second. - * - * By using NaN for "incomparable", the normal equivalence for using - * the return value in a comparison is preserved. - * `compareKeys(left, right) >= 0` iff `left` is greater than or - * equivalent to `right` in the partial ordering. - * + * @typedef {PartialCompare} KeyCompare + * A function that implements a partial order over Keys. * Key order (a partial order) and rank order (a total preorder) are * co-designed to support efficient range search for Key-based queries * (@see {@link ../README.md#rank-order-and-key-order}). - * - * @param {Key} left - * @param {Key} right - * @returns {KeyComparison} */ /** diff --git a/packages/patterns/test/copySet.test.js b/packages/patterns/test/copySet.test.js index b46e79aa36..6745885288 100644 --- a/packages/patterns/test/copySet.test.js +++ b/packages/patterns/test/copySet.test.js @@ -1,12 +1,21 @@ import test from '@endo/ses-ava/prepare-endo.js'; +import { fc } from '@fast-check/ava'; import { makeTagged, getTag, passStyleOf } from '@endo/marshal'; +import { + arbKey, + exampleAlice, + exampleBob, + exampleCarol, +} from '@endo/pass-style/tools.js'; +import { Fail, q } from '@endo/errors'; import { isCopySet, assertCopySet, makeCopySet, getCopySetKeys, } from '../src/keys/checkKey.js'; +import { keyEQ } from '../src/keys/compareKeys.js'; import { setIsSuperset, setIsDisjoint, @@ -19,6 +28,8 @@ import { M, matches } from '../src/patterns/patternMatchers.js'; import '../src/types.js'; +/** @import { Key } from '../src/types.js'; */ + const assertIsCopySet = (t, s) => { t.is(passStyleOf(s), 'tagged'); t.is(getTag(s), 'copySet'); @@ -85,7 +96,7 @@ test('key uniqueness', t => { // TODO: incorporate fast-check for property-based testing that construction // reverse rank sorts keys and validation rejects any other key order. -test('operations', t => { +test('operations on golden inputs', t => { const x = makeCopySet(['b', 'a', 'c']); const y = makeCopySet(['a', 'b']); const z = makeCopySet(['c', 'b']); @@ -104,6 +115,16 @@ test('operations', t => { t.assert(setIsDisjoint(xMy, y)); t.assert(setIsSuperset(x, y)); + const twoCohorts = [ + [exampleAlice, 'z'], + [exampleBob, 'z'], + [exampleCarol, 'a'], + ]; + t.assert( + setIsSuperset(makeCopySet(twoCohorts), makeCopySet(twoCohorts.slice(-1))), + 'superset with many items in one rank cohort (issue #2588)', + ); + t.assert(matches(x, yUz)); t.assert(matches(x, M.gt(y))); t.assert(matches(x, M.gt(z))); @@ -119,6 +140,30 @@ test('operations', t => { t.deepEqual(yIz, makeTagged('copySet', ['b'])); }); +test('setIsSuperset', async t => { + await fc.assert( + fc.property( + fc.uniqueArray(arbKey, { comparator: keyEQ }), + fc.infiniteStream(fc.boolean()), + /** + * @param {Key[]} arr an array of unique Key values + * @param {Iterator} keepSeq a sequence of booleans for filtering + * arr into a subset + */ + (arr, keepSeq) => { + // Filter out the subset and assert that setIsSuperset recognizes it as + // such. + const sub = arr.filter(() => keepSeq.next().value); + setIsSuperset(makeCopySet(arr), makeCopySet(sub)) || + Fail`${q(sub)} must be a subset of ${q(arr)}`; + }, + ), + ); + + // Ensure at least one ava assertion. + t.pass(); +}); + test('matching', t => { const copySet = makeCopySet(['z', 'c', 'b', 'a']); const missingKey = makeCopySet(['z', 'c', 'b']); diff --git a/yarn.lock b/yarn.lock index 27468a3e8a..6701d0ce66 100644 --- a/yarn.lock +++ b/yarn.lock @@ -727,8 +727,10 @@ __metadata: "@endo/eventual-send": "workspace:^" "@endo/init": "workspace:^" "@endo/marshal": "workspace:^" + "@endo/pass-style": "workspace:^" "@endo/promise-kit": "workspace:^" "@endo/ses-ava": "workspace:^" + "@fast-check/ava": "npm:^1.1.5" ava: "npm:^6.1.3" babel-eslint: "npm:^10.1.0" eslint: "npm:^8.57.0"