fix(tools): decode schema options by declared type and fail loudly - #35
Conversation
…ptions main() caught every exception, printed "An error occurred" and returned, so a failed generation exited 0 with the output file untouched. A consumer that formats and stamps the output next (control4-esphome's gen-proto) then recorded provenance for a schema that was never generated. The handler is gone, so any failure exits non-zero with a traceback before the output is opened. parse_unknown_fields rejected wire types 1 and 5, so any double, float, fixed32, sfixed32, fixed64 or sfixed64 message option took that path. They now decode, using the extension's declared type for sign and float interpretation, and emit like the varint path. A non-finite float has no Lua literal (inf would load as nil), so it fails instead of emitting one. check-schema runs test/gen_schema_controls.sh: each fixed-width type round- trips into the generated Lua, and an unsupported wire type (a group option) and a non-finite double both exit non-zero with the output unchanged. Fixes DRV-144
…ture The apt protobuf-compiler on CI (3.21.x) rejects the bare `inf` identifier as a double option value, so protoc failed before the generator ran and the control saw the wrong error. `1e999` parses to infinity on both 3.21.12 and 35.1, and the control still fails when the non-finite guard is removed.
|
@svc-finitelabs please widen this PR. Derek's decision is to decode every message option by its declared type instead of failing on the types we can represent. Keep the loud-failure path for the ones we can't. Support instead of failing
Fail loudly, naming the option or field
Tests and constraints
|
Options the Lua schema can represent are now rendered instead of rejected or mis-rendered: - double/float: infinities become math.huge / -math.huge, NaN (0/0) - bool: true/false instead of 1/0 - int32/int64: two's-complement signed, so negatives stay negative - sint32/sint64: zigzag-decoded - string/bytes: a valid Lua literal. \ " CR LF and control bytes are escaped, and bytes that are not valid UTF-8 become zero-padded \ddd instead of the lossy U+FFFD replacement. Output is written as UTF-8. Generation fails loudly, naming the option or field, for message- and group-typed options, TYPE_GROUP message fields (previously emitted as ProtoSchema.WIRE.UNKNOWN, which errors on load), and 64-bit options whose magnitude exceeds 2^53, which Lua 5.1 and LuaJIT would round. Output for the existing fixtures is byte-identical. protoc 3.21 cannot express a NaN option, so the NaN control renders it through the generator's option_literal directly.
|
Widened at
One side effect: a packed repeated option now fails loudly through the same guard as message options. Non-packed repeated options still emit duplicate keys, as before. Existing fixture output is byte-identical. Each of the 16 mutations turns its own control red. Stopped here, no release. |
derek-miller
left a comment
There was a problem hiding this comment.
The fix is right and the gates pass at 1c38644 (check-schema with all 12 controls, make test, check-types, the controls under LuaJIT 2.1 and Lua 5.1 to 5.4). Output for nested, maps, test_messages_proto3 and empty is byte-identical to main. A few things before this merges.
Must fix
- A negative enum option still comes out as 2^64-1 and exits 0. Enums go on the wire as int32, so a negative value is a sign-extended 10-byte varint, but
option_literal(line 101) only applies two's complement toTYPE_INT32andTYPE_INT64, andTYPE_ENUMis not inINT64_TYPES, so the 2^53 guard misses it too.enum Sign { NEG = -1; ZERO = 0; }withoption (opt_sign) = NEG;givesopt_sign = 18446744073709551615,and loads as 1.844674407371e+19. AddTYPE_ENUMto the two's-complement branch, put a negative enum value intyped_options.proto, and drop "enumis unchanged" from the body.
Should fix
- CLAUDE.md lines 443-445 still say the generator "wraps generation in a bare
exceptthat prints the error and still exits 0". That is now the opposite of the code. Delete it, or replace it with one line: a failed generation exits non-zero and leaves OUTPUT untouched. - Option keys are emitted bare (line 386), so a packaged or keyword option name gives invalid Lua and exits 0. With
package acme;the output hasacme.opt_id = 7,insideoptions = {; an option namedendgivesend = true,. Both fail to load. This predates the PR, but it is the same exit-0 failure DRV-144 is about, on the line this PR rewrites. Emit["acme.opt_id"] = 7(aslua_table_keyalready does for qualified names) when the name is not a plain non-keyword identifier, with one round-trip control.
Nits
- A packed repeated option fails with "Option opt_packed is TYPE_INT32, which has no Lua literal", which is false for int32. Say it is a packed repeated field. The
parse_unknown_fieldsdocstring still says "Only decodes varints and strings for demonstration". - No fixture has a lone 0x80 byte, so
0xDC80 < codesurvives mutation, and under that mutant a"\200"option raises after the output file is opened and leaves it truncated. Add\200toopt_bytes. - A
-0.0double loads as +0.0 on LuaJIT and 5.1. Optional: emit-(1/math.huge). - Match the repo: load the generator in the NaN control the way
tools/check_wire_vectorsdoes (type(sys)plusexec) instead ofimportlib; give the controls script the sibling'spassedcounter andN/M controls passedfooter, and makeoutlocal; annotateINT64_TYPESandLUA_STRING_ESCAPESlike the other module tables; extend thecheck-schemacomment (Makefile line 168) to say it also runs the option and failure controls. - Fixtures:
group_option.protoand the inlinemessage_optionare the same kind of check stored two ways. Moving the checked-in ones intowrite_protoheredocs like the rest would be simpler.
Body
It describes changes against this PR's own earlier heads: "The earlier ValueError is removed" (added in d4cfe88, never on main), "still runs from the existing check-schema target" (the script is new), byte-identity "to the previous head fa67c32", and the first-round mutation paragraphs. Rewrite it against main 906d4b8, and update the client.lua line numbers to esphome main (ifdef is read at 152 and 746, id at 1989, 2065 and 2498). The 69-line downstream diff checks out, and the driver reads no option whose value changes.
…n keys A negative enum option is a sign-extended varint on the wire, so it came out as 2^64-1. Option names from a package, or that are Lua keywords, were emitted as bare keys and gave a schema that fails to load while the generator exits 0. They now go through bracket syntax. Negative zero is emitted as -(1/math.huge), because Lua 5.1 compiles a -0.0 literal to an earlier 0 constant in the same chunk. A packed repeated option now says so in its error. The controls' fixtures are all written inline, and the script reports N/M controls passed like the provenance controls. CLAUDE.md no longer says a failed generation exits 0.
|
Addressed at Must fix
Should fix
Nits
Body: rewritten against |
derek-miller
left a comment
There was a problem hiding this comment.
The review's points are addressed at 02f6890: enum options sign-extend, non-identifier option keys are bracketed, CLAUDE.md is true again, and the nits and body are fixed. All 8 checks pass.
Fixes DRV-144.
The defect
On
main,main()intools/gen_lua_proto_schemacatches every exception, printsAn error occurred, and returns. A failed generation therefore exits 0 and leaves the output file untouched.parse_unknown_fieldsraises on wire types 1 and 5, so anydouble,float,fixed32,sfixed32,fixed64orsfixed64message option takes that path. In control4-esphome,gen-protothen runsfmt-luaandproto-provenance stampon the unchanged file. The result is the old schema body stamped with the new version, and it passescheck-proto.Reproduced on
mainat906d4b8: a fixed-width and a group option both printAn error occurred, exit 0, and leave a sentinel output file as it was.mainalso emits the raw varint for every varint option and wraps string options in quotes with no escaping. So a negativeint32orenumoption comes out as 2^64 minus its magnitude, aboolcomes out as1/0, and a string containing"or a newline gives Lua that does not load, still at exit 0. Option names are emitted as bare keys. Withpackage acme;the output hasacme.opt_id = 7,insideoptions = {, and an option namedendgivesend = true,. Neither loads.Changes
main()no longer catches exceptions, so a failure exits 1 with a traceback. Every failure path runs before the output file is opened, so the output stays untouched.doublefromsfixed64,boolfromuint32, orsint32fromint32.struct, using the format for the declared type.double/float: infinities emitmath.hugeand-math.huge, and NaN emits(0/0). Negative zero emits-(1/math.huge), because Lua 5.1 compiles a-0.0literal to an earlier0constant in the same chunk and loses the sign.boolemitstrue/false.int32,int64andenumare read as two's-complement, since a negative value of any of them is a sign-extended 10-byte varint.sint32/sint64are zigzag-decoded.uint32/uint64are unchanged.\,", CR and LF are escaped, and other control bytes become zero-padded\ddd, as do bytes that are not valid UTF-8 (mainused the lossy'replace'decode). Valid UTF-8 passes through. The output file is written as UTF-8 explicitly instead of in the locale default.["acme.opt_id"] = 7and["end"] = true. Plain names are unchanged.TYPE_GROUPfield in a message, whichmainemits asProtoSchema.WIRE.UNKNOWNand which errors when the schema loadsint64,uint64,sint64,fixed64,sfixed64) whose magnitude exceeds 2^53. Exactly ±2^53 is still emitted.CLAUDE.mdno longer says a failed generation exits 0.Downstream effect
control4-esphome's next
gen-protowill change itsbooloptions from1/0totrue/false. Regenerating against ESPHome 2026.8.2 with this head gives exactly 69 changed lines, all of them these options:no_delay: 61 linesspeed_optimized: 6 lineslog: 1 lineinline_encode: 1 lineapi_options.protohas no package, so no option key changes. On control4-esphome main, the driver readsoptions.id(client.lua1989, 2065, 2498) andoptions.ifdef(152, 746). Neither changes:idis auint32, and everyifdefis plain ASCII.Tests
test/gen_schema_controls.shis new and runs from the existingcheck-schematarget, which gains one line, and the target's comment now says so. There is no new target or CI step. Every fixture is written inline to a temp directory. It has 15 controls and ends with anN/M controls passedfooter, liketest/proto_provenance_controls.sh.uint32varintint32/int64, negativesint32/sint64,uint64, a positive and a negativeenum, a string with every escape class plus\001followed by a digit and a UTF-8é, bytes\377\000\200z(a lone0x80is the lower bound of the non-UTF-8 range), and both ±2^53 boundaries. It also asserts that the generated file contains no raw control byte, because Lua accepts raw control bytes inside a literal and a value comparison alone would pass.doubleandfloat. Negative zero is checked with1/x == -math.huge, and the file must not contain a-0.0literal, because CI runscheck-schemaunder Lua 5.4, which keeps the sign.~=itself. protoc 3.21, the version CI installs from apt, cannot express a NaN option: it rejectsnan,-nanand-inf.±1e999works on 3.21 and 35.1. So this control renders NaN through the generator'soption_literaldirectly, for bothdoubleandfloat.endint64,uint64,sint64,fixed64andsfixed64at 2^53+1, negative for the signed typesoption_literaldirectly and checks the error names it as packed.Output for the existing fixtures
nested,maps,test_messages_proto3andemptyis byte-identical tomain.Revert-checks
At
02f6890, each mutation was applied alone, checked to apply exactly once, and restored, with the file's checksum confirmed after each one. Controls ran under Lua 5.5, LuaJIT 2.1 and Lua 5.1, all with protoc 3.21.12. Every mutation went red on all three, and each is killed by the control for its own arm:main()sfixed64decoded as<Qsfixed64beyond 2^53floatdecoded as<iboolemitted asstr(val)enumleft out of the two's-complement conversionsint64beyond 2^53\dddbranch disabled\dddbranch disabled0xDC80 <=changed to0xDC80 <surrogateescapereverted to'replace'is TYPE_INT32"Unsupported wire type: 3)abs(val) >changed toval >sint64andsfixed64beyond 2^53>changed to>=Local runs
These all pass:
make test,format-check,check-provenance,check-types,check-schema,typecheck,check-wire-vectorsandcheck-float-vectors. The controls also pass under LuaJIT, Lua 5.1 and Lua 5.5, with both protoc 35.1 and 3.21.12. Localmake lintfails identically onmain, because Homebrew's luacheck breaks under Lua 5.5. This PR changes no Lua files, so CI is the lint signal.No release: this rides the next lua-protobuf release, which the template re-vendors on the tag after v0.9.30.