fix: correct non-transitive keywordComparator in validation.js

[Suyog241005]

Problem

In lib/keywords/validation.js, the keywordComparator is used to sort compiled keywords so that unevaluatedProperties and unevaluatedItems are evaluated last.

The previous comparator was defined as:

const keywordComparator = (_a, b) => lastKeywords.has(b[0]) ? -1 : 1;

This comparator violates the Array.prototype.sort() contract:

1. It never returns 0 (which signifies equal sorting priority).
2. It is non-transitive and contradictory (e.g., comparing two normal keywords A and B results in A > B and B > A simultaneously).

Because of this, the sorting behavior becomes non-deterministic and engine-dependent. Depending on the JS engine (V8, SpiderMonkey, JSC) or array size, unevaluatedProperties/unevaluatedItems could fail to sort to the end, causing unexpected validation bugs.

Solution

Refactored the comparator to correctly compare both inputs and respect the sort contract:

const keywordComparator = (a, b) => {
  const isA = lastKeywords.has(a[0]);
  const isB = lastKeywords.has(b[0]);
  return isA === isB ? 0 : isA ? 1 : -1;
};

This ensures that:

• Two keywords of the same priority return 0 (stable sorting).
• A keyword that belongs at the end returns 1 when compared to a normal one.
• A keyword that belongs at the end returns -1 when compared from the other side.

[Suyog241005]

Hi @jdesrosiers ,

I noticed a bug in the keyword sorting logic where the custom comparator for unevaluatedProperties and unevaluatedItems violates the standard sorting contract (it never returns 0 and has contradictory results when comparing items of equal priority).

This can cause unstable or incorrect keyword ordering depending on the JavaScript engine. I've submitted a quick PR to fix it using a standard transitive comparator. All linting and tests pass successfully. Let me know what you think!

[jdesrosiers]

There's isn't a bug here or even a potential bug. This comparator will always sort the desired keywords at the end. There's a possibility that other keywords end up in a different order depending on the sort algorithm, but that doesn't matter in this context. As long as the unevaluated keywords end up at the end, it's fine.

I was trying to make this sort as efficient as possible by making the comparator do as little work as possible, but after some thought and research, I've concluded that it's plausible that the sort algorithm has to do more work given this comparator. I'm not 100% convinced, but considering the effort it would take to be sure and that any difference is negligible at these scales, we might as well do it correctly.

[jdesrosiers]

I'm not a big fan of the nested ternary expression. How about this instead.

Suggested change

	const keywordComparator = (a, b) => {
	const isA = lastKeywords.has(a[0]);
	const isB = lastKeywords.has(b[0]);
	return isA === isB ? 0 : isA ? 1 : -1;
	};
	const keywordComparator = (a, b) => lastKeywords.has(a[0]) - lastKeywords.has(b[0]);

[Suyog241005]

Agreed, that's much cleaner! Applied your suggestion. The boolean-to-number coercion makes it concise and easy to follow once you know the trick.

[Suyog241005]

There's isn't a bug here or even a potential bug. This comparator will always sort the desired keywords at the end. There's a possibility that other keywords end up in a different order depending on the sort algorithm, but that doesn't matter in this context. As long as the unevaluated keywords end up at the end, it's fine.

I was trying to make this sort as efficient as possible by making the comparator do as little work as possible, but after some thought and research, I've concluded that it's plausible that the sort algorithm has to do more work given this comparator. I'm not 100% convinced, but considering the effort it would take to be sure and that any difference is negligible at these scales, we might as well do it correctly.

That's a fair correction, I appreciate you clarifying the intent behind the original implementation. I may have overstated it as a "bug" when it was more of a correctness concern with the sort contract. Thanks for taking the time to think it through and for being open to the change regardless!