Jens-G commented on PR #3730: URL: https://github.com/apache/thrift/pull/3730#issuecomment-5417002385
### Code review Found 9 issues: 1. `$(MKDIR)` is not a variable this build defines — `configure.ac:94` uses `AC_PROG_MKDIR_P`, which substitutes `$(MKDIR_P)`. It expands to nothing, so the recipe runs as `-p gen-zig`, `gen-zig/` is never created and the following `thrift -out gen-zig` fails. These two lines are the only bare `$(MKDIR)` in the repo; `tutorial/zig/Makefile.am` uses plain `mkdir -p` and is unaffected. This is why `lib-zig` is red, and because `cross-test` lists `lib-zig` in `needs:` it is *skipped* rather than failed — so the Zig cross-language tests have not run against this branch. Same fix in `test/zig/Makefile.am#L22-L24`. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/tests/Makefile.am#L26-L28 2. Compact protocol cannot write `bool` inside a container: `tTypeToByte` maps `.Bool` to an error alongside `.Stop`/`.Void`, so any `list<bool>`, `set<bool>` or bool-keyed/valued map fails with `NoTypeValueForType`. The read path already accepts it (`byteToTType` maps `1, 2 => .Bool`) and `binary_protocol.zig` maps `.Bool => 2`, so this is compact-only. Java uses `ttypeToCompactType[TType.BOOL] = Types.BOOLEAN_TRUE` and Go `BOOL: COMPACT_BOOLEAN_TRUE`. `ThriftTest.thrift` has no bool containers and the conformance suite only uses `.I32` elements, so the cross-tests would not surface it. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/protocol/compact_protocol.zig#L53-L57 3. `readVarint32` narrows with a checked `@intCast`, which aborts instead of returning a protocol error. `readVarint` bounds the byte count but not the magnitude, so five bytes decode up to 2^35-1 — `80 80 80 80 08` gives 2^31, one past `i32` max — and this runs before `checkReadLength`/`checkContainerSize`. It also breaks interop: Java's `writeVarint32` uses `n >>>= 7`, so a negative seqid from any other binding encodes into [2^31, 2^32) and aborts a Zig server on parse. `writeVarint32` at `L133-L135` has the mirror problem on the write side. Relatedly, `shift` is a `u6` incremented before the `shift >= maxBits` guard, so for `i64` it reaches 63, passes the guard, then overflows on the next `+= 7` — the guard is unreachable for `i64`. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/protocol/compact_protocol.zig#L136-L140 https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/protocol/compact_protocol.zig#L166-L177 4. The `TTransport` vtable's `destroy` shim does not dispatch to the concrete type. `deinit` just above it does (`ptr_info.pointer.child.deinit(self)`), as do `open`/`close`/`isOpen`/`reader`/`writer`/`flush`, but `destroy` calls `a.destroy(self)` directly. That makes `TFramedTransport.destroy()`'s cascade (`self.underlying.destroy(a)`) unreachable, so the wrapped transport is never freed — contradicting the comment at `framed_transport.zig#L41` that this transport "owns the underlying transport interface, i.e. manages it's lifecycle". `TSimpleServer` destroys through the interface per accepted connection. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/transport/interface.zig#L43-L52 5. Reserved-word escaping is applied to declarations but not to references, so the two disagree. `zig_struct_nameb`/`zig_field_name` route through `zig_safe_name`, but `to_zig_type` returns `zig_namespace(ttype) + ttype->get_name()` unescaped for enums and structs — a struct named `error` is declared `error_` and referenced as `error`. `service_call_client_function_name`/`service_call_handler_function_name` (`L3360-L3366`) and the enum name and members in `generate_enum` (`L807`, `L816`) are likewise unescaped. `ZIG_RESERVED_WORDS` includes `error`, `test`, `union`, `return`, `continue`. There is no keyword fixture in the PR, so nothing exercises it — same shape as THRIFT-6114/6115/6116 in the Python generator. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/compiler/cpp/src/thrift/generate/t_zig_generator.cc#L3187-L3192 6. Wire-declared container counts and string lengths are never checked against `TConfiguration.maxMessageSize` — `checkContainerSize` and `checkReadLength` reject negatives only and take no configuration. `TList.readFromProtocol` then reserves capacity straight from that count, whereas `map.zig` and `set.zig` fill incrementally, so `list.zig` is the outlier. The framed transport bounds frame bytes, not the count declared inside them. Since this binding ships finite defaults (`maxMessageSize` 100 MiB), `doc/thrift-threat-model.md` treats the gap as in scope rather than property-disclaimed; Go's equivalent is `checkContainerSizeForProtocol`. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/protocol/utils.zig#L48-L54 https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/collections/list.zig#L119-L121 7. `TLoggingProtocolFactory.getProtocol` dupes `"logging"` and passes it to `TLoggingProtocol.init`, which dupes it again; `deinit` frees only the second copy, so the first leaks on every call. `test/zig/src/test_server.zig` installs this factory as the server protocol factory, so it leaks per connection. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/protocol/logging_protocol.zig#L322-L327 8. Five new files are missing the ASF license header that the rest of the new `.zig` files carry: `lib/zig/src/collections/utils.zig`, `lib/zig/src/internal/mod.zig`, `lib/zig/src/internal/protocol.zig`, `lib/zig/src/internal/struct_utilities.zig` and `lib/zig/src/lib/types.zig`. `lib/zig/src/collections/list.zig` in the same directory has one, and there is no RAT check in CI that would catch the omission. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/collections/utils.zig#L1-L5 9. Dead scaffolding, and none of it compiles. `ClientResult` is marked `// DEPRECATED` and is never instantiated — the generator hand-rolls its own `...ClientResult` type per method in `render_client_result_union` — yet both it and `ServiceCallResult` are exported unqualified from `root.zig#L72-L73`, and `README.md:238` points users at the `*ClientResult` name. Because Zig only analyses referenced functions, this code is never type-checked, and all of it is broken: `ServiceCallResult.get()` (`result.zig#L61-L68`) returns `res.result_value` where `res` is already the value and no such field exists; `TApplicationException.eql()` (`application_exception.zig#L74-L81`) declares `u64` but returns `bool` and falls off the end on the equal path; `Map.format()` (`map.zig#L140-L142`) prints the key twice instead of `key: value`. For a new binding this seems worth deleting rather than shipping. https://github.com/apache/thrift/blob/36eb758bfc2ce646361470eaac043453074bfde1/lib/zig/src/lib/result.zig#L160-L164 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
