cmp: apply cleanupSurroundingIdentical's prepend/append after the loop instead of in defer

[januththedev]

cleanupSurroundingIdentical: two defer closures mutate a local, but the result is unnamed

The defect

cleanupSurroundingIdentical uses defer to prepend and append groups after the loop, per its own comments:

} else {
    // No preceding group exists, so prepend a new group,
    // but do so after we finish iterating over all groups.
    defer func() {
        groups = append([]diffStats{{Name: groups[0].Name, NumIdentical: numLeadingIdentical}}, groups...)
    }()
}

The function has an unnamed result:

func cleanupSurroundingIdentical(groups []diffStats, eq func(i, j int) bool) []diffStats {

With an unnamed result, return groups assigns the result parameter before the deferred functions run. Both closures therefore reassign the local groups after its value has already been copied to the return slot, and the mutation is silently discarded.

The Go semantics, in isolation:

unnamed result, defer prepends len=1, n=99 -> [{7}]
named   result, defer prepends len=1, n=99 -> [{99} {7}]

The second defer has the same problem. FormatValue in report_reflect.go:114 uses a named result (out textNode) for the same idiom, which confirms the intended pattern was a named result that someone later removed.

Reachability — stated honestly

I could not reach either branch through the public cmp.Diff API. I instrumented both branches and ran the full suite, roughly 3M randomized string pairs (small alphabets, random edits, forced shared prefixes and suffixes), and an exhaustive sweep of shared-prefix × short-tail combinations. Zero hits.

I was also able to prove the leading branch unreachable: if x[0] == y[0], the forward diff search matches at (0,0) on its first probe and emits Identity, so group 0 is always equal; and any unequal group 0 has nx == 0 or ny == 0, which blocks the check. The trailing branch would need a sub-optimal diff leaving an unexamined misaligned equal pair at the end — possible in principle, but connect checks equality first on every probed pair.

So this is a latent defect: provably dead code that contradicts its own comments, not a demonstrated user-visible failure. If either branch ever became reachable, the consequence would be a dropped report element followed by formatDiffSlice failing to consume vx/vy fully and tripping assert(vx.Len() == 0 && vy.Len() == 0) at report_slices.go:434 — a cmp.Diff panic on valid input.

The change

Apply the prepend and append after the loop explicitly, rather than relying on defer plus an unnamed result:

var numPrepend, numAppend int
...
    numPrepend = numLeadingIdentical
...
    numAppend = numTrailingIdentical
...
if numPrepend > 0 {
    groups = append([]diffStats{{Name: groups[0].Name, NumIdentical: numPrepend}}, groups...)
}
if numAppend > 0 {
    groups = append(groups, diffStats{Name: groups[len(groups)-1].Name, NumIdentical: numAppend})
}
return groups

I chose this over switching to a named result because the two defers execute LIFO: with a single-group list where both branches fire, the append would run first and then the prepend would wrap it, so the trailing identical span would end up in the middle of the result. Applying both after the loop keeps prepend-then-append ordering correct by construction.

If you would rather just delete the two dead branches, that is equally defensible and probably the smaller change — the function's contract is then simply "never emits leading or trailing identical groups". I did not take that route because the surrounding code and comments clearly intend to handle these cases, and deleting them would silently drop the behaviour if a future diff change makes the path reachable. Happy to switch to deletion if you prefer it.

Tests

cmp/report_slices_test.go (new, package cmp, matching the existing internal-test convention) covers four cases: a leading identical span with no preceding group, a trailing one with no succeeding group, both edges of a single unequal group, and a control where a middle group correctly folds into its neighbours.

• Before the fix the three edge cases fail, e.g. cleanupSurroundingIdentical([1 removed byte], eq) = [1 removed byte], want [1 identical byte 1 removed byte]; the control already passed. After: all four pass.
• go test -count=1 ./... → all packages ok, 0 failures.
• Baseline on pristine HEAD: 710 tests passing. After: 715 — the delta is exactly this test plus its four subtests.
• gofmt -l cmp/ clean.

I checked the 41 open issues and 22 open PRs: nothing covers this. I deliberately avoided the neighbouring sliceSorter.checkSort off-by-one (#402), EquateComparable nil deref (#401), and AllowUnexported nil guard (#405), which are already open.

Note that this repository requires a CLA; the signature is not something I can do on your behalf.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.