Skip to content

Commit

Permalink
fix(marshal)!: Update compareRank to terminate comparability at the f…
Browse files Browse the repository at this point in the history
…irst remotable

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'.

Fixes #2588
  • Loading branch information
gibson042 committed Oct 21, 2024
1 parent 4106320 commit 4e39e58
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 38 deletions.
38 changes: 21 additions & 17 deletions packages/marshal/src/rankOrder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -225,14 +225,18 @@ export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
};

/** @type {RankCompare} */
const antiComparator = (x, y) => comparator(y, x);
const outerComparator = (x, y) =>
/** @type {Exclude<PartialComparison, NaN>} */ (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
Expand Down
68 changes: 47 additions & 21 deletions packages/marshal/test/encodePassable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { unsortedSample } from './_marshal-test-data.js';

const statelessEncodePassableLegacy = makeEncodePassable();

const makeSimplePassableKit = ({ stateless = false } = {}) => {
const makeSimplePassableKit = ({ statelessSuffix } = {}) => {
let count = 0n;
const encodingFromVal = new Map();
const valFromEncoding = new Map();
Expand All @@ -40,17 +40,18 @@ const makeSimplePassableKit = ({ stateless = false } = {}) => {
return val;
};

const encoders = stateless
? {
encodeRemotable: r => 'r',
encodePromise: p => '?',
encodeError: err => '!',
}
: {
encodeRemotable: r => encodeSpecial('r', r),
encodePromise: p => encodeSpecial('?', p),
encodeError: err => encodeSpecial('!', err),
};
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,
Expand Down Expand Up @@ -336,7 +337,26 @@ test('compact custom encoding validity constraints', t => {
}
});

const orderInvariants = (x, y, statelessEncodePassable) => {
const commonPrefix = (str1, str2) => {
const iter1 = str1[Symbol.iterator]();
const iter2 = str2[Symbol.iterator]();
let i = 0;
for (;;) {
const { value: char1 } = iter1.next();
const { value: char2 } = iter2.next();
if (char1 === undefined) {
// str1 is a prefix of str2.
return str1;
} else if (char2 === undefined) {
// str2 is a prefix of str1.
return str2;
}
if (char1 !== char2) return str1.substring(0, i);
i += char1.length;
}
};

const orderInvariants = (x, y, statelessEncode1, statelessEncode2) => {
const rankComp = compareRank(x, y);
const fullComp = compareFull(x, y);
if (rankComp !== 0) {
Expand All @@ -352,10 +372,14 @@ const orderInvariants = (x, y, statelessEncodePassable) => {
rankComp === fullComp ||
Fail`with fullComp ${fullComp}, expected rankComp 0 or matching: ${rankComp} for ${x} vs. ${y}`;
}
const ex = statelessEncodePassable(x);
const ey = statelessEncodePassable(y);
const encComp = compareRank(ex, ey);
const ex = statelessEncode1(x);
const ey = statelessEncode1(y);
if (fullComp !== 0) {
// Comparability of encodings stops at the first incomparable special rank
// (remotable/promise/error).
const exPrefix = commonPrefix(ex, statelessEncode2(x));
const eyPrefix = commonPrefix(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}`;
Expand All @@ -380,7 +404,7 @@ test('original encoding round-trips', testRoundTrip, pickLegacy);
test('small encoding round-trips', testRoundTrip, pickCompact);

const testBigInt = test.macro(async (t, pickEncode) => {
const kit = makeSimplePassableKit({ stateless: true });
const kit = makeSimplePassableKit({ statelessSuffix: '' });
const encodePassable = pickEncode(kit);
await fc.assert(
fc.property(fc.bigInt(), fc.bigInt(), (x, y) => {
Expand All @@ -403,18 +427,20 @@ test(
);

const testOrderInvariants = test.macro(async (t, pickEncode) => {
const kit = makeSimplePassableKit({ stateless: true });
const statelessEncodePassable = pickEncode(kit);
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, statelessEncodePassable);
orderInvariants(x, y, statelessEncode1, statelessEncode2);
}
}

await fc.assert(
fc.property(arbPassable, arbPassable, (x, y) => {
return orderInvariants(x, y, statelessEncodePassable);
return orderInvariants(x, y, statelessEncode1, statelessEncode2);
}),
);

Expand Down

0 comments on commit 4e39e58

Please sign in to comment.