THRIFT-5807: Format out-of-range enums as EnumName(val) instead of <UNSET> - #3823
Merged
Merged
Conversation
slachiewicz
force-pushed
the
THRIFT-5807
branch
4 times, most recently
from
September 11, 2026 07:01
8136e2a to
aed6b47
Compare
Member
Code reviewFound 1 issue:
Lines 60 to 74 in aed6b47 One suggestion, below the bar for the list above but verified:
Lines 141 to 143 in aed6b47 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
slachiewicz
force-pushed
the
THRIFT-5807
branch
from
September 11, 2026 13:19
aed6b47 to
7b65445
Compare
Member
Author
|
Rewrote the branch after the review above: the commit carries the attribution trailer, and lib/go/README.md gains a "A note about undefined enum values" section covering the This comment was created with AI assistance. |
…NSET> Client: go When an enum field held an out-of-range integer value, String() returned "<UNSET>", misleading users and loggers into believing the field was omitted even though the raw integer was preserved and correctly written to the wire. Unknown enum values are now formatted as EnumName(%d), matching idiomatic Go stringer conventions, and a new IsDefined() method is generated to cleanly check whether a value is one of the IDL-defined constants (also updating go_validator_generator). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
slachiewicz
force-pushed
the
THRIFT-5807
branch
from
September 11, 2026 13:20
7b65445 to
f857670
Compare
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
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.
JIRA: THRIFT-5807
Client: go
An enum holding a value the IDL does not define printed
<UNSET>, which reads as "no value was sent" when in fact the integer arrived intact and was written back out unchanged. Rust printsEnum0(10), and C++, Python and Node.js print the integer. Go now printsColor(999), the shapestringerproduces.A generated
IsDefined() boolcomes with it, so callers have something to ask instead of comparingString()against a sentinel.go_validator_generatorwas doing exactly that comparison forvt.defined_onlyand now callsIsDefined().lib/go/README.mdgains a "A note about undefined enum values" section covering theString(),MarshalTextand JSON change and the newIsDefined(), in line with the notes for THRIFT-2063, THRIFT-6175 and THRIFT-6195.What else moves with String()
MarshalTextreturns[]byte(p.String()), so this changes text and JSON encoding for out-of-range values:Neither round-trips:
UnmarshalTextgoes throughFromString, which rejects both withnot a valid Color string, so no encoding that used to survive a round trip stops surviving one. The new form at least says which value failed.No import risk: the generated
FromStringalready referencesfmtfor every enum, sofmt.Sprintfneeds no new import in any file.Blast radius
Regenerating every IDL under
test/,lib/go/test/andtutorial/with both compilers, 73 of 650 generated files change, and the change is additive apart from one line per enum. Across the whole corpus the only removed lines are:return "<UNSET>"if (p.Enum0).String() == "<UNSET>" {if (*p.Enum1).String() == "<UNSET>" {Everything else added is the new
IsDefined()method, 153 of them, one per enum in the corpus. No existing generated logic is restructured.What can break
Code comparing
String()to the literal"<UNSET>"to test for an undefined value. The two call sites of that pattern in this repository are the validator lines above;IsDefined()is the replacement, and it says what the caller meant.Verified:
Color(999).String()returnsColor(999)andFoo_One.String()returnsOne;json.Marshaloutput as quoted above, read from a run against both compilers.This change was created with AI assistance.