Treat an unchanged map/list component as wire-compatible in diff#119
Merged
Conversation
wireCompatibleScalars recursed into map and list types requiring both components to be wire-compatible, but had no equality short-circuit. An unchanged component that is not a member of a wire-compatible scalar group (e.g. a string map key) returned false, dragging the whole verdict to incompatible. So a genuinely wire-safe map value widening such as map<string,int32> -> map<string,int64> was reported as an Error in Wire mode, failing CI on a safe change. Short-circuit identical components to wire-compatible.
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.
Problem
wireCompatibleScalarsrecurses intomap<K,V>(andrepeated) requiring both components to be wire-compatible:but it had no equality short-circuit. For a component that is unchanged yet is not a member of any
wireCompatibleScalarGroups(e.g. astringmap key), the recursive call fell through to the group-membership check, which returnsfalse— so an unchanged sibling dragged the whole verdict to incompatible.Concrete case
map<string, int32> counts = 1;→map<string, int64> counts = 1;is a genuinely wire-compatible value widening (map entries are sub-messages; the value field stays varint). ButwireCompatibleScalars(String, String)returnedfalse, so the change was classifiedErrorin Wire mode — failingdiff --fail-on erroron a safe change. The mirrormap<int32, string>→map<int64, string>(key widening) failed identically.Fix
Short-circuit identical components:
case (a, b) if a == b => true. An unchanged component is trivially wire-compatible with itself, letting the map/list recursion succeed when only a genuinely wire-compatible component changed.Test
ProtoDiffSpec: both map key- and value-widening cases assertSeverity.Infoin Wire mode. Red before the fix (Error), green after. FullProtoDiffSpec(79) green.