Skip to content

Commit 337d16a

Browse files
committed
refactor(marshal,patterns): rank-cover ops default to compareRankRemotablesTied
Apply the follow-up TODO from #2883 (comment) to the remaining rank-cover operations: make `compare` optional, move it to the end, and default to `compareRankRemotablesTied`. - `unionRankCovers(compare, covers)` -> `unionRankCovers(covers, compare?)` - `intersectRankCovers(compare, covers)` -> `intersectRankCovers(covers, compare?)` - Update the only callers (`@endo/patterns`'s `patternMatchers.js`) accordingly. - Re-export `compareRankRemotablesTied` and `compareAntiRankRemotablesTied` from `@endo/marshal`'s `index.js`. Tests in `packages/marshal/test/rankOrder.test.js`: - Add coverage for the optional/default-arg behavior of `isRankSorted`, `assertRankSorted`, `sortByRank`, `getIndexCover`, `unionRankCovers`, and `intersectRankCovers`. - Resolve the two `compareRank` vs `compareRankRemotablesTied` property-test TODOs by parameterizing the `totally orders ranks` and `is transitive` properties to run for both comparators. Both are total preorders, so both properties hold for both, and we now exercise each. Refs: #2883
1 parent 45d06cd commit 337d16a

5 files changed

Lines changed: 208 additions & 89 deletions

File tree

.changeset/rank-order-remotables-tied.md

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,31 @@
11
---
22
'@endo/marshal': minor
3+
'@endo/patterns': patch
34
---
45

56
Add `compareRankRemotablesTied` and `compareAntiRankRemotablesTied`,
67
which behave like `compareRank` and `compareAntiRank` except that they
78
treat all remotables as tied for the same rank instead of short-circuiting
89
the comparison on encountering a remotable. Make the `compare` parameter
9-
optional on `isRankSorted`, `assertRankSorted`, `sortByRank`, and
10-
`getIndexCover`, defaulting to `compareRankRemotablesTied`.
10+
optional on `isRankSorted`, `assertRankSorted`, `sortByRank`,
11+
`getIndexCover`, `unionRankCovers`, and `intersectRankCovers`,
12+
defaulting to `compareRankRemotablesTied`.
1113

12-
`getIndexCover`'s parameter order changes from
13-
`(sorted, compare, rankCover)` to `(sorted, rankCover, compare?)` to
14-
move the optional parameter to the end. This is a breaking change to a
15-
helper that has no known external callers within the Endo monorepo.
14+
The following parameter orders change so the optional comparator lands
15+
at the end:
16+
17+
- `getIndexCover(sorted, compare, rankCover)`
18+
`getIndexCover(sorted, rankCover, compare?)`
19+
- `unionRankCovers(compare, covers)`
20+
`unionRankCovers(covers, compare?)`
21+
- `intersectRankCovers(compare, covers)`
22+
`intersectRankCovers(covers, compare?)`
23+
24+
These are breaking changes to helpers that have no known external
25+
callers within the Endo monorepo. Within the monorepo, the only
26+
callers of `unionRankCovers` and `intersectRankCovers` are in
27+
`@endo/patterns`'s `patternMatchers.js`, which has been updated to
28+
match the new parameter order.
1629

1730
Salvaged from https://github.com/endojs/endo/pull/2871 so that the
1831
codec-admission core of that PR (now in #3226) can land independently

packages/marshal/index.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@ export {
1919
compareByCodePoints,
2020
assertRankSorted,
2121
compareRank,
22+
compareRankRemotablesTied,
2223
isRankSorted,
2324
sortByRank,
2425
compareAntiRank,
26+
compareAntiRankRemotablesTied,
2527
makeFullOrderComparatorKit,
2628
getPassStyleCover,
2729
intersectRankCovers,

packages/marshal/src/rankOrder.js

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -482,11 +482,6 @@ const rankSearch = (
482482
return bias === 'leftMost' ? left : right - 1;
483483
};
484484

485-
// TODO https://github.com/endojs/endo/issues/2883#issuecomment-3063809592
486-
// Some (all?) rank cover operations take a compare parameter.
487-
// These should all now be optional, moved to the end, and default to
488-
// `compareRankRemotablesTied`.
489-
490485
/**
491486
* @param {Passable[]} sorted
492487
* @param {RankCover} rankCover
@@ -555,11 +550,15 @@ const maxRank = (compare, a, b) => (compare(a, b) >= 0 ? a : b);
555550
const minRank = (compare, a, b) => (compare(a, b) <= 0 ? a : b);
556551

557552
/**
558-
* @param {RankCompare} compare
559553
* @param {RankCover[]} covers
554+
* @param {RankCompare} [compare] which rank comparison function to use.
555+
* Default to `compareRankRemotablesTied`.
560556
* @returns {RankCover}
561557
*/
562-
export const unionRankCovers = (compare, covers) => {
558+
export const unionRankCovers = (
559+
covers,
560+
compare = compareRankRemotablesTied,
561+
) => {
563562
/**
564563
* @param {RankCover} a
565564
* @param {RankCover} b
@@ -574,11 +573,15 @@ export const unionRankCovers = (compare, covers) => {
574573
harden(unionRankCovers);
575574

576575
/**
577-
* @param {RankCompare} compare
578576
* @param {RankCover[]} covers
577+
* @param {RankCompare} [compare] which rank comparison function to use.
578+
* Default to `compareRankRemotablesTied`.
579579
* @returns {RankCover}
580580
*/
581-
export const intersectRankCovers = (compare, covers) => {
581+
export const intersectRankCovers = (
582+
covers,
583+
compare = compareRankRemotablesTied,
584+
) => {
582585
/**
583586
* @param {RankCover} a
584587
* @param {RankCover} b

packages/marshal/test/rankOrder.test.js

Lines changed: 173 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,15 @@ import { q } from '@endo/errors';
1111
import {
1212
FullRankCover,
1313
compareRank,
14+
compareAntiRank,
1415
isRankSorted,
1516
sortByRank,
1617
getPassStyleCover,
1718
getIndexCover,
1819
assertRankSorted,
1920
compareRankRemotablesTied,
21+
intersectRankCovers,
22+
unionRankCovers,
2023
} from '../src/rankOrder.js';
2124
import { unsortedSample, sortedSample } from '../tools/marshal-test-data.js';
2225

@@ -30,85 +33,99 @@ test('compareRank is reflexive', async t => {
3033
);
3134
});
3235

33-
// TODO https://github.com/endojs/endo/issues/2883
34-
// Should these tests use `compareRank` or `compareRankRemotablesTied`?
35-
test('compareRank totally orders ranks', async t => {
36+
test('compareRankRemotablesTied is reflexive', async t => {
3637
await fc.assert(
37-
fc.property(arbPassable, arbPassable, (a, b) => {
38-
const ab = compareRank(a, b);
39-
const ba = compareRank(b, a);
40-
if (ab === 0) {
41-
return t.is(ba, 0);
42-
}
43-
return (
44-
t.true(Math.abs(ab) > 0) &&
45-
t.true(Math.abs(ba) > 0) &&
46-
t.is(Math.sign(ba), -Math.sign(ab))
47-
);
38+
fc.property(arbPassable, x => {
39+
return t.is(compareRankRemotablesTied(x, x), 0);
4840
}),
4941
);
5042
});
5143

52-
// TODO https://github.com/endojs/endo/issues/2883
53-
// Should these tests use `compareRank` or `compareRankRemotablesTied`?
54-
test('compareRank is transitive', async t => {
55-
await fc.assert(
56-
fc.property(
57-
// operate on a set of three passables covering at least two ranks
58-
fc
59-
.uniqueArray(arbPassable, { minLength: 3, maxLength: 3 })
60-
.filter(
61-
([a, b, c]) => compareRank(a, b) !== 0 || compareRank(a, c) !== 0,
62-
),
63-
triple => {
64-
const sorted = harden(triple.sort(compareRank));
65-
assertRankSorted(sorted, compareRank);
66-
const [a, b, c] = sorted;
67-
const failures = [];
68-
69-
const testCompare = (outcome, message, failure) => {
70-
t.true(outcome, message);
71-
if (!outcome) {
72-
failures.push(failure);
73-
}
74-
};
75-
76-
testCompare(
77-
compareRank(a, b) <= 0,
78-
'a <= b',
79-
`Expected <= 0: ${q(a)} vs. ${q(b)}`,
80-
);
81-
testCompare(
82-
compareRank(a, c) <= 0,
83-
'a <= c',
84-
`Expected <= 0: ${q(a)} vs. ${q(c)}`,
85-
);
86-
testCompare(
87-
compareRank(b, c) <= 0,
88-
'b <= c',
89-
`Expected <= 0: ${q(b)} vs. ${q(c)}`,
90-
);
91-
testCompare(
92-
compareRank(c, b) >= 0,
93-
'c >= b',
94-
`Expected >= 0: ${q(c)} vs. ${q(b)}`,
95-
);
96-
testCompare(
97-
compareRank(c, a) >= 0,
98-
'c >= a',
99-
`Expected >= 0: ${q(c)} vs. ${q(a)}`,
100-
);
101-
testCompare(
102-
compareRank(b, a) >= 0,
103-
'b >= a',
104-
`Expected >= 0: ${q(b)} vs. ${q(a)}`,
44+
// Both `compareRank` and `compareRankRemotablesTied` are total preorders on
45+
// passables: anti-symmetric and transitive. They differ only in how they
46+
// handle remotables nested within compound passables: `compareRank`
47+
// short-circuits to 0 as soon as it encounters a remotable, while
48+
// `compareRankRemotablesTied` treats the remotable position as a tie and
49+
// continues to refine by the surrounding structure. Both properties hold
50+
// for both comparators, so we exercise each property against both.
51+
for (const [name, compare] of /** @type {const} */ ([
52+
['compareRank', compareRank],
53+
['compareRankRemotablesTied', compareRankRemotablesTied],
54+
])) {
55+
test(`${name} totally orders ranks`, async t => {
56+
await fc.assert(
57+
fc.property(arbPassable, arbPassable, (a, b) => {
58+
const ab = compare(a, b);
59+
const ba = compare(b, a);
60+
if (ab === 0) {
61+
return t.is(ba, 0);
62+
}
63+
return (
64+
t.true(Math.abs(ab) > 0) &&
65+
t.true(Math.abs(ba) > 0) &&
66+
t.is(Math.sign(ba), -Math.sign(ab))
10567
);
68+
}),
69+
);
70+
});
10671

107-
return t.deepEqual(failures, []);
108-
},
109-
),
110-
);
111-
});
72+
test(`${name} is transitive`, async t => {
73+
await fc.assert(
74+
fc.property(
75+
// operate on a set of three passables covering at least two ranks
76+
fc
77+
.uniqueArray(arbPassable, { minLength: 3, maxLength: 3 })
78+
.filter(([a, b, c]) => compare(a, b) !== 0 || compare(a, c) !== 0),
79+
triple => {
80+
const sorted = harden(triple.sort(compare));
81+
assertRankSorted(sorted, compare);
82+
const [a, b, c] = sorted;
83+
const failures = [];
84+
85+
const testCompare = (outcome, message, failure) => {
86+
t.true(outcome, message);
87+
if (!outcome) {
88+
failures.push(failure);
89+
}
90+
};
91+
92+
testCompare(
93+
compare(a, b) <= 0,
94+
'a <= b',
95+
`Expected <= 0: ${q(a)} vs. ${q(b)}`,
96+
);
97+
testCompare(
98+
compare(a, c) <= 0,
99+
'a <= c',
100+
`Expected <= 0: ${q(a)} vs. ${q(c)}`,
101+
);
102+
testCompare(
103+
compare(b, c) <= 0,
104+
'b <= c',
105+
`Expected <= 0: ${q(b)} vs. ${q(c)}`,
106+
);
107+
testCompare(
108+
compare(c, b) >= 0,
109+
'c >= b',
110+
`Expected >= 0: ${q(c)} vs. ${q(b)}`,
111+
);
112+
testCompare(
113+
compare(c, a) >= 0,
114+
'c >= a',
115+
`Expected >= 0: ${q(c)} vs. ${q(a)}`,
116+
);
117+
testCompare(
118+
compare(b, a) >= 0,
119+
'b >= a',
120+
`Expected >= 0: ${q(b)} vs. ${q(a)}`,
121+
);
122+
123+
return t.deepEqual(failures, []);
124+
},
125+
),
126+
);
127+
});
128+
}
112129

113130
test('compare and sort by rank', t => {
114131
assertRankSorted(sortedSample);
@@ -181,3 +198,87 @@ test.skip('range queries', t => {
181198
t.is(range[1], indexRange[1]);
182199
}
183200
});
201+
202+
// Exercise the optional `compare` defaulting to `compareRankRemotablesTied`.
203+
// If the parameter order regresses (e.g. back to (sorted, compare, rankCover)
204+
// for getIndexCover, or (compare, covers) for unionRankCovers /
205+
// intersectRankCovers), these tests fail because the trailing argument is no
206+
// longer treated as a comparator.
207+
test('isRankSorted defaults compare to compareRankRemotablesTied', t => {
208+
const sorted = harden(['a', 'b', 'c']);
209+
t.true(isRankSorted(sorted));
210+
t.true(isRankSorted(sorted, compareRankRemotablesTied));
211+
t.true(isRankSorted(sorted, compareRank));
212+
213+
const unsorted = harden(['c', 'a', 'b']);
214+
t.false(isRankSorted(unsorted));
215+
});
216+
217+
test('assertRankSorted defaults compare to compareRankRemotablesTied', t => {
218+
const sorted = harden(['a', 'b', 'c']);
219+
t.notThrows(() => assertRankSorted(sorted));
220+
t.notThrows(() => assertRankSorted(sorted, compareRankRemotablesTied));
221+
222+
const unsorted = harden(['c', 'a', 'b']);
223+
t.throws(() => assertRankSorted(unsorted), {
224+
message: /Must be rank sorted/,
225+
});
226+
});
227+
228+
test('sortByRank defaults compare to compareRankRemotablesTied', t => {
229+
const unsorted = harden(['c', 'a', 'b']);
230+
t.deepEqual(sortByRank(unsorted), ['a', 'b', 'c']);
231+
t.deepEqual(sortByRank(unsorted, compareRankRemotablesTied), ['a', 'b', 'c']);
232+
t.deepEqual(sortByRank(unsorted, compareAntiRank), ['c', 'b', 'a']);
233+
});
234+
235+
test('getIndexCover (sorted, rankCover, compare?) signature', t => {
236+
// sorted strings
237+
const sorted = harden(['a', 'b', 'c', 'd', 'e']);
238+
239+
// Default compare
240+
t.deepEqual(getIndexCover(sorted, ['b', 'd']), [1, 3]);
241+
t.deepEqual(getIndexCover(sorted, ['', '{']), [0, 4]);
242+
243+
// Explicit compare
244+
t.deepEqual(
245+
getIndexCover(sorted, ['b', 'd'], compareRankRemotablesTied),
246+
[1, 3],
247+
);
248+
t.deepEqual(getIndexCover(sorted, ['b', 'd'], compareRank), [1, 3]);
249+
});
250+
251+
test('unionRankCovers (covers, compare?) signature', t => {
252+
/** @type {[string, string][]} */
253+
const covers = harden([
254+
['b', 'd'],
255+
['c', 'e'],
256+
['a', 'b'],
257+
]);
258+
// Default compare
259+
t.deepEqual(unionRankCovers(covers), ['a', 'e']);
260+
// Explicit compare
261+
t.deepEqual(unionRankCovers(covers, compareRankRemotablesTied), ['a', 'e']);
262+
t.deepEqual(unionRankCovers(covers, compareRank), ['a', 'e']);
263+
// Empty union returns identity element ['{', '']
264+
t.deepEqual(unionRankCovers(harden([])), ['{', '']);
265+
});
266+
267+
test('intersectRankCovers (covers, compare?) signature', t => {
268+
/** @type {[string, string][]} */
269+
const covers = harden([
270+
['a', 'e'],
271+
['b', 'd'],
272+
['c', 'f'],
273+
]);
274+
// Default compare
275+
t.deepEqual(intersectRankCovers(covers), ['c', 'd']);
276+
// Explicit compare
277+
t.deepEqual(intersectRankCovers(covers, compareRankRemotablesTied), [
278+
'c',
279+
'd',
280+
]);
281+
t.deepEqual(intersectRankCovers(covers, compareRank), ['c', 'd']);
282+
// Empty intersection returns identity element ['', '{']
283+
t.deepEqual(intersectRankCovers(harden([])), ['', '{']);
284+
});

packages/patterns/src/patterns/patternMatchers.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -829,10 +829,10 @@ const makePatternKit = () => {
829829

830830
getRankCover: (patts, encodePassable) =>
831831
intersectRankCovers(
832-
compareRank,
833832
/** @type {CopyArray<Passable>} */ (patts).map(p =>
834833
getRankCover(p, encodePassable),
835834
),
835+
compareRank,
836836
),
837837
});
838838

@@ -868,10 +868,10 @@ const makePatternKit = () => {
868868

869869
getRankCover: (patts, encodePassable) =>
870870
unionRankCovers(
871-
compareRank,
872871
/** @type {CopyArray<Passable>} */ (patts).map(p =>
873872
getRankCover(p, encodePassable),
874873
),
874+
compareRank,
875875
),
876876
});
877877

0 commit comments

Comments
 (0)