Fix spurious reserved-reformat and enum-alias-removal diffs#115
Merged
Conversation
- diffReserved compared reserved entries structurally, so rewriting a range as the equivalent list of numbers (reserved 2 to 4 vs reserved 2, 3, 4) reported a false ReservedRemoved. Compare the covered number set (as merged intervals, without expanding, so 'to max' stays cheap) plus reserved names, and emit nothing when both are unchanged. - Removing an enum alias (a duplicate name whose number is still assigned to another value) was flagged EnumValueRemoved with numberReserved=false, i.e. wire-breaking, even though the number is still live. Treat the removal as wire-safe when the number remains present among the new values.
A reserved-name change in the same edit as a numeric range reformat (reserved 2 to 4 -> reserved 2, 3, 4; reserved "foo") set sameNames to false, so the whole reserved set fell back to a structural diff and still reported the range reformat as a spurious ReservedRemoved. Diff the covered numbers and the reserved names separately so a change to one never resurfaces reformat noise from the other.
The fallback still diffed raw Reserved entries when numeric coverage changed, so extending a range while reformatting (reserved 2 to 4 -> reserved 2, 3, 4, 5) reported ReservedRemoved(2 to 4) even though 2-4 stayed reserved. Compute the genuinely removed/added coverage as interval-set subtraction and report those intervals, so only numbers that actually left or entered the reserved set are reported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two false-positive classifications in the diff engine, from a fresh bug-hunt pass.
1. Reformatting a reserved range is reported as a removal
diffReservedcompared reserved entries structurally (oldRes.toSet -- newRes.toSet). Soreserved 2 to 4;andreserved 2, 3, 4;— identical coverage — had an empty intersection and produced a spuriousReservedRemoved(Range(2,4))plus threeReservedAdded, surfacing a false "reservation removed" warning.Now it compares the covered number set (as canonical merged intervals — no expansion, so
reserved N to maxstays cheap) plus the reserved names, and emits nothing when both are unchanged. Genuine changes still fall back to the structural diff, so their reporting is unchanged.2. Removing an enum alias is flagged wire-breaking
For an
allow_aliasenum, removing a duplicate value (e.g.C = 1whileB = 1stays) producedEnumValueRemoved(..., numberReserved = false), which maps toErrorin Wire mode — even though number1is still assigned toB, so the wire is unaffected. ThenumberReservedflag was computed only from the new reserved list, ignoring numbers still in use.The removal is now treated as wire-safe when the number remains present among the new enum values (matching buf's number-level rule). The flag drives severity to
Infoin Wire mode; its description now reads "(number retained)" to cover both the reserved and still-in-use cases.Tests
Two tests (red before the fix, green after), plus the full
ProtoDiffSpecand the goldenIntegrationSpec(24) staying green:reserved 2 to 4;→reserved 2, 3, 4;now yields no changes;allow_aliasduplicate whose number is still used yields a singleEnumValueRemovedthat isInfoin Wire mode.