THRIFT-6204: Omit an unset default-requiredness field from the Go writer - #3827
Draft
slachiewicz wants to merge 2 commits into
Draft
THRIFT-6204: Omit an unset default-requiredness field from the Go writer#3827slachiewicz wants to merge 2 commits into
slachiewicz wants to merge 2 commits into
Conversation
…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.
slachiewicz
force-pushed
the
THRIFT-6204
branch
from
September 11, 2026 07:00
e5b21c6 to
ca293f3
Compare
Client: go Default requiredness means "write if set" (doc/specs/idl.md), and Python, Java, Node.js and C# all omit such a field when it holds no value. The Go writer wrote it unconditionally, so a nil pointer field either panicked on the way out or, when its struct had no members, went onto the wire as an empty struct. Guard the write with the generated IsSet helper, as optional fields already are. This changes bytes on the wire, and not only for the case that panicked. A nil field whose struct has no members is written today and is omitted after this: before: 0c 00 01 00 08 00 02 00 00 00 07 00 after: 08 00 02 00 00 00 07 00 A peer that declares that field required accepts the first and rejects the second with "Required field E is not set". is_pointer_field() is true for every struct, union and exception field whatever its requiredness, and for every cpp.ref field, so the guard is wider than unions alone. Regenerating every IDL under test/, lib/go/test/ and tutorial/ changes 75 of 650 generated files and guards 227 write sites, 59 of the changed files holding service Args and Result structs.
slachiewicz
force-pushed
the
THRIFT-6204
branch
from
September 11, 2026 07:21
ca293f3 to
bc01dca
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-6204
Client: go
Stacked on #3822; the first commit is that PR. Review the second one.
Default requiredness means "write if set" (
doc/specs/idl.md), and Python, Java, Node.js and C# all omit such a field when it holds no value. The Go writer wrote it unconditionally, so a nil pointer field either panicked on the way out — that half is #3822 — or, when its struct had no members, went onto the wire as an empty struct. Guarding the write with the generatedIsSethelper, as optional fields already are, is a two-line change.This is a wire change, and that is the whole question. Opening it as a draft because that is a maintainer's call, not mine.
What moves
A nil field whose struct has no members is written today and omitted after:
A peer that declares that field
requiredaccepts the first and rejects the second:Nothing panicked in that case, so this is working code changing behaviour, not a crash being fixed.
Blast radius
is_pointer_field()is true for every struct, union and exception field whatever its requiredness, and for everycpp.reffield, so the guard reaches wider than unions. Regenerating every IDL undertest/,lib/go/test/andtutorial/with and without it:Args/ResultstructsThe last row is the one to weigh: this reaches RPC argument and result encoding, not only user-declared structs.
How I would land it
Three options, and I have no stake in which:
go:generator option defaulting off, so nobody's wire changes without asking.Say which and I will rework it.
Verified:
go test ./tests/ -count=1inlib/go/testpasses after regeneratinggopaththrough theMakefile.amrecipe.go build ./gopath/src/...reports only the two pre-existingincludestest.Numberzerrors that #3817 fixes.CI
The two red cross-test jobs are inherited from master —
d6782a87turned the Python peer-certificate check on and 178ip-sslcases fail identically on every open PR, including a two-line.asf.yamlchange. See #3818.This change was created with AI assistance.