THRIFT-5806: Count zero set fields on a nil union rather than dereferencing it - #3822
Open
slachiewicz wants to merge 1 commit into
Open
slachiewicz wants to merge 1 commit into
slachiewicz wants to merge 1 commit into
Conversation
slachiewicz
force-pushed
the
THRIFT-5806
branch
3 times, most recently
from
September 8, 2026 13:43
f3515fd to
c860a97
Compare
slachiewicz
force-pushed
the
THRIFT-5806
branch
from
September 11, 2026 07:01
c860a97 to
3eeb97b
Compare
Member
Code reviewFound 1 issue:
Lines 60 to 74 in 3eeb97b 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
slachiewicz
force-pushed
the
THRIFT-5806
branch
2 times, most recently
from
September 13, 2026 16:02
7900773 to
8517fd8
Compare
Member
Author
|
Rebased on master and added the This comment was created with AI assistance. |
…encing it Client: go CountSetFields read the union's fields straight off the receiver, so a struct holding a nil union field panicked with a nil pointer dereference the moment it was serialized. Return 0 for a nil receiver, which is the honest answer and lets the union's own arity check report which union could not be written. The panic becomes: *Descendant error writing struct: *Descendant write union: exactly one field must be set (0 set) That is a diagnosis rather than a crash, but it is still an error: the field is not omitted. Omitting it is what the IDL specification asks for and what Python, Java, Node.js and C# do, and it is deliberately not part of this change, because the guard that would do it also covers every default-requiredness struct and exception field and alters the bytes on the wire for cases that never crashed. That is THRIFT-6204, with the measurements. So do not widen this guard to the writer without reading THRIFT-6204 first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
slachiewicz
force-pushed
the
THRIFT-5806
branch
from
September 14, 2026 20:23
8517fd8 to
ff72aee
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-5806
Client: go
CountSetFieldsread the union's fields straight off the receiver, so a struct holding a nil union field panicked the moment it was serialized:Returning 0 for a nil receiver lets the union's own arity check speak instead:
Scope
This PR is only the nil guard: no generated write path changes, no wire bytes change.
Omitting an unset default-requiredness pointer field from the output, the way Python, Java, Node.js and C# omit it, is a wire change rather than a panic fix and is THRIFT-6204, #3827, which is stacked on this PR. The measurements of what that guard reaches are in that PR.
Tests
StructWithUnsetUnioninUnionDefaultValueTest.thriftplusTestNilUnionandTestStructWithUnsetUnion.Verified: with the master generator
TestNilUnionfails withpanic: runtime error: invalid memory address or nil pointer dereference; with this changego test ./tests/ -count=1inlib/go/testpasses.This change was created with AI assistance (Claude Opus 5).