Skip to content

fix: correct off-by-one in SortSlices equal-run check - #402

Open
nileshpatil6 wants to merge 1 commit into
google:masterfrom
nileshpatil6:fix/sortslices-equal-run-off-by-one
Open

fix: correct off-by-one in SortSlices equal-run check#402
nileshpatil6 wants to merge 1 commit into
google:masterfrom
nileshpatil6:fix/sortslices-equal-run-off-by-one

Conversation

@nileshpatil6

Copy link
Copy Markdown

The bug

sliceSorter.checkSort tracks the start of a run of mutually "equal" elements:

for i := 1; i < v.Len(); i++ {
	if ss.less(v, i-1, i) {
		// Check that first and last elements in v[start:i] are equal.
		if start >= 0 && (ss.less(v, start, i-1) || ss.less(v, i-1, start)) {
			panic(...)
		}
		start = -1
	} else if start == -1 {
		start = i
	}
}

When !less(v[i-1], v[i]) the run begins at i-1, not i, so the first element of every equal run is excluded from the check.

The loop's own comment gives it away: it says the check covers v[start:i], the whole run. With start = i, a two-element run produces v.Slice(i, i), an empty slice, in the panic message.

Why it matters

checkSort exists to panic with incomparable values detected when the supplied less function is not transitive over the data, because the sorted order and therefore cmp.Equal's answer is otherwise arbitrary. With the off-by-one, a violation whose offending element is first in a run goes undetected, and cmp.Equal quietly returns an order-dependent result instead of telling the caller their option is broken.

The everyday case is a float slice containing NaN with the obvious less function, which is exactly the misuse this package already has a wantPanic test for:

x := []float64{0, math.NaN(), 1, 2}
cmpopts.SortSlices(func(x, y float64) bool { return x < y })

Sorted, the run is [0 NaN 1] and less(0, 1) is true, so it must panic. It does not, because start points at NaN rather than at 0, and NaN compares "equal" to 1 in both directions.

When the check does fire, the message also prints the wrong window. On the existing test input it reports [2 2 NaN 3 3 3 3] where the run is [2 2 2 NaN 3 3 3 3].

mapSorter.checkSort is unaffected: it requires a total order and checks every adjacent pair, so it has no run-start concept.

The change

start = i - 1, plus one case in the existing TestOptions table where the incomparable element begins the run.

Widening the window by one cannot cause a false panic: for a valid strict weak ordering every element of a run is mutually equal, so the added comparison is against an element already known to be equivalent.

Testing

Without the fix the new case fails:

--- FAIL: TestOptions/SortSlices#06
    util_test.go:1169: expected Equal panic
        reason: panics because SortSlices used with non-transitive less function,
        where the incomparable element is the first of a run of equal elements

With it, ./cmp/... passes in full. gofmt -l cmp/ is clean, and go vet output is byte-identical to the unmodified tree.

One user-visible note: this changes the panic message on the existing wantPanic case from [2 2 NaN 3 3 3 3] to [2 2 2 NaN 3 3 3 3]. No test asserts that string.

sliceSorter.checkSort records the start of a run of mutually equal
elements as i, but when !less(v[i-1], v[i]) the run begins at i-1. The
loop's own comment says the check covers v[start:i], the whole run, and
with start = i a two element run yields the empty slice v[i:i] in the
panic message.

Two consequences. The guard misses violations whose offending element is
first in a run, so a non-transitive less function silently produces an
order dependent result from cmp.Equal instead of the documented
"incomparable values detected" panic. And when it does fire, the message
prints a window missing its leading element.

Adds a case where the incomparable element starts the run, which does
not panic before this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant