fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

gibson042 · 2024-10-21T04:07:58Z

Description

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

Security Considerations

None known.

Scaling Considerations

None known.

Documentation Considerations

This includes implicit fixup in types.js, but there may be some Markdown that needs updating as well.

Testing Considerations

This expands property-based testing, but only narrowly. Expansion is expected in the future.

Compatibility Considerations

This affects storage of compound Keys in e.g. CopySets/CopyBags/CopyMaps, but any ordering written by the old compareRank should be accepted by the new one (which now reclassifies as ties what its predecessor considered to be distinct ranks). As such, we should be fine, but I welcome challenges to this assertion.

Upgrade Considerations

This is breaking in the sense of changing compareRank output, but as far as I know, nothing depends upon that except in combination with compareKeys (which is fixed by this PR). I am not aware of any concerns that should hinder upgrade to a version of endo including this PR.

But I am wondering what if anything to put in NEWS.md file(s).

kriskowal

Flushing this review before I get to the end, to keep the conversation lively.

kriskowal · 2024-10-21T20:08:37Z

packages/marshal/src/rankOrder.js

-export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
-  /** @type {RankCompare} */
+export const makeComparatorKit = (compareRemotables = (_x, _y) => NaN) => {
+  /** @type {PartialCompare} */


Confirm or deny:

Previously:

values from dispirit ranks are compared by the total order of ranks

all values within atom ranks, e.g., numbers and strings, are mutually comparable

all values within composite ranks, e.g., arrays, are mutually comparable provided

that their respective entries are all comparable, or

if they contain any respective pairs that are incomparable, those arrays have non-equivalent prefixes

and are otherwise incomparable

within the rank of promises, all values are incomparable

within the rank of remotables, all values are equivalent

comparing incomparable values produces 0

After:

within the rank of remotables, all values are incomparable

comparing incomparable values produces NaN

That seems correct, although I would rephrase a bit:

Previously:

…

within the rank of remotables, all values are equivalent in the default comparison (used by compareRank)

~~comparing incomparable values produces 0~~ within the respective ranks of errors and promises, all values are equivalent (and encountering an unacceptable pass style would result in a thrown exception)

After:

within the rank of remotables, all values are incomparable

comparing incomparable values produces NaN in the inner comparator and short-circuits the outer comparator result to 0

So, both before and after this change, comparing any pair of promises would by default return NaN?

Consequently, both before and after this change, the range of comparison was -1, 0, 1, and NaN?

Consequently, both before and after this change, rank compare is and was a partial order?

I’m far enough along in my read to see with my own eyes that, both before and after this change:

all exported comparators produce values in the range -1, 0, +1 for the default comparator for remotables.

all promises are equivalent, not incoparable

erights · 2024-10-21T22:28:12Z

Just skimming, but saw the q at end. rankCompare is a full order. It cannot answer NaN. (Unless this PR is making a more radical change than I thought.

For all promises p,q, compareRank(p,q) === 0

kriskowal

I’m convinced about the correctness and practical forward-compatibility of the fix.

I note that the fix occurs almost entirely outside the diff as an emergent behavior of having compareRemoatables return NaN and coercing NaN back down to 0 at the border. The relevant behavior appears to be stopping early and propagating NaN on:

endo/packages/marshal/src/rankOrder.js

Lines 199 to 206 in bf8b1f8

    
           case 'copyArray': { 
        
             // Lexicographic 
        
             const len = Math.min(left.length, right.length); 
        
             for (let i = 0; i < len; i += 1) { 
        
               const result = comparator(left[i], right[i]); 
        
               if (result !== 0) { 
        
                 return result; 
        
               }

I am not qualified to read the property based tests yet.

kriskowal · 2024-10-21T23:10:52Z

packages/marshal/test/encodePassable.test.js

+  let i = 0;
+  for (;;) {
+    const { value: char1 } = iter1.next();
+    const { value: char2 } = iter2.next();
+    if (char1 !== char2 || char1 === undefined) {
+      return str1.substring(0, i);
+    }
+    i += char1.length;
+  }


I’m sure you’re cognizant of the discrepancy here between iterating code points and indexing code units, but that might not be obvious under maintenance of these tests. Anything that compensates for that would be great, albeit accumulating common code points by concatenation or documenting a limitation to correctness under strings where code unit and code point are the same, but if documenting, consider using the dumber algorithm that co-iterates code units to be consistent with the substring call.

kriskowal · 2024-10-21T23:12:33Z

packages/marshal/src/rankOrder.js

+  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);


✅ the undefined fix

kriskowal · 2024-10-21T23:14:28Z

packages/marshal/src/rankOrder.js

+  const outerComparator = (x, y) =>
+    /** @type {Exclude<PartialComparison, NaN>} */ (comparator(x, y) || 0);


✅ narrowing the range in case of a compareRemotables that returns NaN, as it now will by default.

This does break any existing program that expects to distinguish NaN and provided a compareRemotables to produce that effect, but as you state you’re convinced nobody does that in practice, I’m fine with this. It may be worth noting in NEWS.md if only to build trust.

It might be worth explicitly reminding reviewers that NaN || 0 === 0 because it’s not entirely obvious which JavaScript operators implicitly coerce down from the float to integer domain.

kriskowal · 2024-10-21T23:15:08Z

packages/marshal/src/rankOrder.js

-export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
-  /** @type {RankCompare} */
+export const makeComparatorKit = (compareRemotables = (_x, _y) => NaN) => {
+  /** @type {PartialCompare} */


I’m far enough along in my read to see with my own eyes that, both before and after this change:

all exported comparators produce values in the range -1, 0, +1 for the default comparator for remotables.

all promises are equivalent, not incoparable

kriskowal · 2024-10-21T23:23:16Z

packages/patterns/test/copySet.test.js

+  await fc.assert(
+    fc.property(
+      fc.uniqueArray(arbKey, { comparator: keyEQ }),
+      fc.infiniteStream(fc.boolean()),
+      (arr, keepSeq) => {
+        const sub = arr.filter(() => keepSeq.next().value);
+        setIsSuperset(makeCopySet(arr), makeCopySet(sub)) ||
+          Fail`${q(sub)} of ${q(arr)}`;
+      },
+    ),
+  );


I’m not a qualified reviewer for property tests yet.

I read this as: For an arbitrary array of distinct keys arr and subsequence sub of arr, setIsSuperset(makeCopySet(arr), makeCopySet(sub)) holds, where a subsequence is formed by filtering using elements of an arbitrary infinite stream of booleans.

I thought it would be illustrative to show the output when it fails, before the fix is applied. But even when I check out 14fca7b that has the test and not the fix, this test seems to pass. I suppose the failing case is pretty obscure and the generator doesn't get that far?

When I crank numRuns up to 4000, I get a failure that might be showing another problem? Got error: Error: ["[undefined]"] of ["[Symbol()]",null,"[undefined]"] with seed: -1238147821

oic... it's finding cases of the undefined problem.

Ref #2588

gibson042 · 2024-10-22T01:22:48Z

Just skimming, but saw the q at end. rankCompare is a full order. It cannot answer NaN. (Unless this PR is making a more radical change than I thought.

@erights Right, and that remains true... the default compareRemotables (i.e., the one used by compareRank) now returns NaN to avoid misclassification of e.g. [r1, 'x'] vs. [r2, 'y'] as being in different ranks (without disrupting such correct classification of e.g. [0, 'x'] vs. [0, 'y']), but that NaN is replaced with 0 on the way out—compareRank and indeed any function returned from makeComparatorKit continues to implement a total preorder in which all valid inputs are comparable but ties are possible.

Do you have further comments or reason(s) to delay merging now that @kriskowal has approved?

dckc · 2024-10-22T02:07:25Z

packages/patterns/src/types.js

@@ -143,61 +143,17 @@ export {};
 */

 /**
- * @typedef {RankCompare} FullCompare


Are static types considered part of the API? This FullCompare type is exported, right? So is removing it from this package a breaking change?

Hmm, that's a good question. I've updated to reëxport FullCompare to preserve any remaining imports, on the assumption that dropping it would be considered breaking.

erights · 2024-10-22T02:48:06Z

Do you have further comments or reason(s) to delay merging now that @kriskowal has approved?

I have not found time to review this in enough depth to approve it. But I saw no red flags and am satisfied from the conversations above that all the issues I thought to worry about have been extremely well thought through and reviewed. So please consider your existing approval adequate and do not wait for me. In this case, I'm happy to complete my review post merge.

Really looks great. A clever simple solution that I would not have thought of. Thanks much!!

Ref #2588

Fixes #2594

…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

gibson042 requested review from kriskowal and erights October 21, 2024 04:07

gibson042 force-pushed the gibson-2588-fullorder-narrows-rankorder branch 2 times, most recently from e502d10 to 4e39e58 Compare October 21, 2024 04:16

kriskowal reviewed Oct 21, 2024

View reviewed changes

kriskowal approved these changes Oct 21, 2024

View reviewed changes

test(marshal): Refactor encodePassable tests and add failing inputs

234872c

Ref #2588

gibson042 force-pushed the gibson-2588-fullorder-narrows-rankorder branch from 6094bcb to 85ff875 Compare October 22, 2024 01:12

dckc reviewed Oct 22, 2024

View reviewed changes

gibson042 added 4 commits October 22, 2024 01:41

test(patterns): Increase coverage for setIsSuperset

74c0108

Ref #2588

fix(marshal): Reverse-sort undefined values into the right place

ad085ad

Fixes #2594

refactor: Move some types up from patterns to marshal

7996b77

gibson042 force-pushed the gibson-2588-fullorder-narrows-rankorder branch from 85ff875 to 95c427a Compare October 22, 2024 06:08

docs(marshal): Update NEWS.md

39e7187

gibson042 enabled auto-merge (squash) October 22, 2024 06:38

gibson042 merged commit 1dea84d into master Oct 22, 2024
15 checks passed

gibson042 deleted the gibson-2588-fullorder-narrows-rankorder branch October 22, 2024 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

gibson042 commented Oct 21, 2024 •

edited

Loading

kriskowal left a comment

kriskowal Oct 21, 2024

gibson042 Oct 21, 2024 •

edited

Loading

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

erights commented Oct 21, 2024 •

edited by gibson042

Loading

kriskowal left a comment

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

kriskowal Oct 21, 2024

dckc Oct 22, 2024

dckc Oct 22, 2024 •

edited

Loading

dckc Oct 22, 2024

gibson042 commented Oct 22, 2024

dckc Oct 22, 2024

gibson042 Oct 22, 2024

erights commented Oct 22, 2024

	case 'copyArray': {
	// Lexicographic
	const len = Math.min(left.length, right.length);
	for (let i = 0; i < len; i += 1) {
	const result = comparator(left[i], right[i]);
	if (result !== 0) {
	return result;
	}

		const outerComparator = (x, y) =>
		/** @type {Exclude<PartialComparison, NaN>} */ (comparator(x, y) \|\| 0);

fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

Conversation

gibson042 commented Oct 21, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights commented Oct 21, 2024 • edited by gibson042 Loading

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights commented Oct 22, 2024

gibson042 commented Oct 21, 2024 •

edited

Loading

gibson042 Oct 21, 2024 •

edited

Loading

erights commented Oct 21, 2024 •

edited by gibson042

Loading

dckc Oct 22, 2024 •

edited

Loading