docs: record what protobuf support is missing in CLAUDE.md - #16
Conversation
'All standard protobuf types' overstated the implementation in ways that produce silently wrong decodes rather than errors. - Add an explicit not-implemented section. Packed repeated fields are unsupported in both directions, and proto3 packs scalar repeated fields by default, so a message from protoc decodes to one raw byte-string instead of the values. oneof, map, defaults and required are absent, unknown fields are dropped on re-encode, and groups are unsupported: DataType has no GROUP and WireType has no SGROUP/EGROUP. - The schema generator only walks top-level messages and registers bare names while fields reference <package>.<Message>, so any .proto with a package or a nested message yields a schema whose subschema lookup misses. empty.proto avoids this by being empty. - 64-bit pairs carry a metatable; the documented bare literal is classified as a list and fails encoding. Decode is asymmetric: 64-bit types return Int64 tables, 32-bit types return numbers. - decode returns two values, and every failure raises rather than returning nil, err. - Document the codegen workflow: types.lua is tracked, check-types fails on stale output, and because check-types needs .venv both a fresh clone and make clean leave make check failing until setup-schema-generator runs. Only the first PROTO file reaches protoc, a bare OUTPUT filename raises, and the generator exits 0 after printing an error. - CI builds the luajit-2.1 matrix entry as luajit-openresty because rolling HEAD miscompiled arshift of INT_MIN in zigzag encoding, keeping the job name so the required check matches. The matrix does not test what its label says. - make all is not the gate, and make check never rewrites files.
derek-miller
left a comment
There was a problem hiding this comment.
Approving. The "What is not implemented" section is the most valuable thing in this file, so I verified it against the source rather than trusting it — a false gap claim would be as damaging as a missing one, because it sends people writing workarounds they do not need.
Every claim holds:
- Packed repeated fields: no occurrence of
packedanywhere insrc/. Unsupported in both directions, as stated. oneof,required, field defaults: zero hits each insrc/protobuf/.map: the only matches are LuaCATS prose ("Maps enum names to their definitions"), not amapimplementation.- Groups:
WireTypeis exactlyVARINT 0, FIXED64 1, LENGTH_DELIMITED 2, FIXED32 5— no SGROUP/EGROUP — andGROUPappears nowhere intypes.lua.error("Unknown wire type: " ...)is atinit.lua:559. DataTypeis 17 entries, so "17 of the 18 standard types" is literally right rather than approximately right.- The generator never emits
nested_type— zero hits intools/gen_lua_proto_schema. decodereturns two values:return result, pos.- No
nil, errpath: noreturn nil,ininit.lua, so "every failure raises, callers mustpcall" is accurate.
The packed-fields entry is the one that earns the whole section. "proto3 packs scalar repeated fields by default, so a message produced by protoc decodes to a single raw byte-string in the list rather than the values" — that is a silent wrong answer, not an error, and nothing about the API hints at it. Calling it out as the most likely source of one is the right emphasis.
The generator limitation is the second. Registering messages under their bare name while fields reference <package>.<Message> means any .proto with a package line produces a schema whose subschema lookup misses — and "empty.proto avoids this only by being empty" explains why the existing test suite never caught it. That sentence is doing a lot of work.
The decode asymmetry — 64-bit types returning Int64 tables while 32-bit types return plain numbers, plus decode_varint truncating past 53 bits — is exactly the kind of thing that produces a bug three call sites away from the cause.
One nice detail: noting that int64_to_hex, equals and is_zero accept plain pairs while only the encode path rejects them, with the actual error text ("Field '...' is not repeated but received a list"). That error is deeply unhelpful on its own — a bare {hi, lo} being classified as a list is not a leap anyone makes unaided — so putting the string in the doc is what makes it searchable.
make all vs make check and the gen-schema OUTPUT path correction are both consistent with what I found in lua-bitn's Makefile structure.
|
Approved with nothing outstanding — please merge. I verified the "What is not implemented" list against the source rather than trusting it (no |
Part of AGENT-44 (audit the CLAUDE.md files that exist). One PR per repo.
"All standard protobuf types" was doing a lot of work
The file said the DataType table covers "All standard protobuf types (DOUBLE,
FLOAT, INT32, INT64, STRING, MESSAGE, etc.)". It covers 17 of 18 —
GROUP(10)is absent, and
WireTypehas no SGROUP (3) or EGROUP (4). Both raise"Unknown wire type".That is the small one. Auditing around it turned up a set of unimplemented
features that produce silently wrong decodes rather than errors, none of which
the file mentioned. Added as an explicit "What is not implemented" section,
because this is the thing to read before assuming a
.protowill round-trip:tag per element; decode has no packed branch and the generator never reads
options.packed. proto3 packs scalar repeated fields by default, so amessage produced by
protocdecodes to a single raw byte-string in the listinstead of the values. No error is raised. This is the most likely source of a
wrong answer in the repo.
oneof,map, field defaults andrequiredare absent. Absent fieldsdecode to
nilwith no default applied; nothing enforcesrequired.drops them.
nested_typeis neveremitted, and messages register under their bare name while fields reference
<package>.<Message>. So any.protowith apackagedeclaration or a nestedmessage yields a schema whose subschema lookup misses.
empty.protoavoids thisonly by being empty.
I have documented these as limitations. Several are arguably code bugs rather
than doc bugs — packed decoding especially, since it is standard proto3 output.
Happy to open them as separate tickets; I did not want to expand a docs PR into
an implementation one without asking.
64-bit representation
Same defect as lua-bitn's file, and present since
5e49f59:bit64attaches a private metatable andis_int64tests for it, so a bare pairis not an Int64.
pb.encodeclassifies it as a list and fails with"Field '...' is not repeated but received a list."Easy to miss becauseint64_to_hex,equalsandis_zerodo accept plain pairs — only the encodepath rejects them. The file's next line already said to use
bit64.new, so itcontradicted itself.
Also added the asymmetry that actually catches people: 64-bit types decode to
Int64 tables while INT32/UINT32/ENUM/BOOL/FIXED32 decode to plain numbers,
and
decode_varintsilently truncates past 53 bits.The codegen workflow was undocumented, and it has two sharp edges
setup-schema-generator/gen-schema/gen-types/check-typesform asequence no agent can guess from the source, and
check-typesis part ofcheckin this repo (unlike its three siblings). Consequences now written down:
make checkfails on a fresh clone untilmake setup-schema-generatorhasrun, because
check-typeshard-requires.venv/bin/python3.make cleanremoves.venv/, somake clean && make checkbreaks the sameway.
types.luais tracked, not build-generated — onlybuild/and.venv/aregitignored — and
check-typesfails on stale output, so editingempty.protowithout committing the regenerated file breaks CI.
Two documented examples were also wrong: "Supports URLs and multiple files" —
only the first
PROTOreachesprotoc, the rest are fetched and ignored unlessimported by it; and
OUTPUT=schema.luaraisesFileNotFoundError, because thegenerator calls
os.makedirs(os.path.dirname(args.output))and a bare filenamegives it
"". Fixed the example to carry a directory. Worth knowing separately:the generator catches all exceptions, prints them, and still exits 0, so check
the output file rather than the status.
CI does not test what its label says
The
luajit-2.1matrix entry is built asluajit-openresty. The workflowcomment explains why — rolling LuaJIT HEAD miscompiled the signed
arithmetic-shift edge cases in zigzag encoding (
arshiftofINT_MIN) — anddeliberately keeps the job name
Lua luajit-2.1so the required status checkstill matches. That is a good workaround and exactly the kind of upstream-bug fact
a doc should carry, so it is now in the CI section instead of only in a YAML
comment.
Also: local
make test-matrixomits LuaJIT 2.0, which CI covers, and needsluaenvplus theluaenv-luarocksplugin.Smaller
decodereturns two values (message, pos). Every failure raises; thereis no
nil, errpath anywhere in encode or decode, so callers mustpcall.pb.version()is public and load-bearing for the build smoke test and wasunlisted.
make checknever rewrites files, andmake allis not the gate.Verification
make typecheckwith the CI-pinned lua-language-server 3.19.0: clean.GROUP/SGROUP/EGROUP entries and the absence of anypackedhandling were confirmed by reading
types.luaand greppingsrc/andtools/.make test,make build,make check-types(needs the venv).No Lua source changed.