fix: emit message options in ascending field number order - #33
svc-finitelabs[bot] wants to merge 1 commit into
Conversation
protoc does not serialize extension options in a stable order: 3.21 and 22.5 emit them in .proto declaration order, 23.4 and later ascending by field number. The generator read those bytes back unsorted, so the same .proto and the same generator produced two different Lua files depending on which protoc built the descriptor. A consumer's drift check then reports a false positive against an unpinned toolchain, which is how this surfaced. Both option paths now sort by field number. That settles on the order modern protoc already produced, so the schemas committed in consumer repos are unchanged: control4-esphome's check-proto passes against this generator under both 3.21.12 and 29.3, where before it failed under 3.21.12. check-option-order builds its descriptor by hand rather than from a fixture. Under a modern protoc an unsorted generator already emits ascending, so a fixture would pass on the defect everywhere except an old-protoc runner; feeding the options descending is what makes a revert fail under every protoc. Confirmed sensitive by reverting the sort. Regenerating across protoc 3.21.12, 22.5, 23.4, 24.4, 25.1, 29.3 and 35.1 now gives one output for the ESPHome API protos and for every fixture here. Before the change the ESPHome schema had two forms, splitting between 22.5 and 23.4, which narrows the 22.5-to-24.4 bracket the ticket recorded. Options ordering was the only difference: the two forms were identical as line multisets and every differing line fell inside an options block. Fixes DRV-136
Consumer-side impact, measured end to endRun against
The bottom row and the top-right control were measured when this PR was opened. New here is the top-left cell. The top-right is the positive control and still fails at exactly the 110 diff lines recorded in the ticket, so the harness is sensitive and this PR is what flips it. Two consequences for the merge. Output-neutral for the pinned consumer
A release carrying this does not change the runtime
Worth knowing for rollout: re-vendoring the amalgamated runtime is a no-op across everything Nothing pushed, head is still |
|
Closing, per Derek. Option key order in a Lua table has no runtime meaning, and since control4-esphome moved to the provenance check nothing regenerates and diffs across toolchains, so this is won't-fix (DRV-136). The silent exit-0 you flagged under "Not addressed" is being fixed separately; no further work on this PR. |
Fixes DRV-136.
tools/gen_lua_proto_schemaemitted the keys inside eachoptionstable in whatever order protoc happened to serialize them, so the same.protoand the same generator produced two different Lua files depending on the toolchain.Mechanism
Not quite what the ticket recorded, so worth restating. The ticket described two mutually exclusive code paths selected by whether the Python runtime resolved the extensions. Measured, that is not what happens here: all 157 ESPHome messages take the unknown-fields path under every protoc tested, and the
Extensionspath never runs. The generator never registers the descriptor set into a pool, so these extensions cannot resolve.The instability is entirely inside the one live path. protoc's own serialization order for extension options changed:
optionsin the descriptor.protodeclaration orderparse_unknown_fieldswalks those bytes in wire order and the generator emitted them as-is. ForListEntitiesBinarySensorResponse, whose source declaresid,base_class,source,ifdef, protoc 3.21.12 hands over exactly that while 29.3 hands overid,source,ifdef,base_class.That also narrows the boundary: the ticket bracketed it between 22.5 and 24.4 with 23.x untested. It is between 22.5 and 23.4.
Change
Both option paths sort by field number. The
Extensionspath is dead for this input but is sorted too, so the ordering does not depend on which path runs.The order this settles on is the one modern protoc already produced, which is what the committed consumer schemas were generated from, so no consumer has to regenerate.
control4-esphome'scheck-protopasses against this generator under both 3.21.12 and 29.3. Before the change it failed under 3.21.12 with a 110 line diff.Guard
make check-option-orderis a new step in thecheckjob, alongside the existing vector checks.It builds its descriptor by hand instead of using a fixture
.proto, and that is the point. Under a modern protoc an unsorted generator already emits ascending, so a fixture based check would pass on the defect everywhere except a runner that happens to install an old protoc. Feeding the options in descending order is the one input that separates a sorted generator from an unsorted one under every protoc. It also asserts the options really did arrive descending, so it cannot pass vacuously if a future protobuf release starts sorting unknown fields on reserialize.Confirmed sensitive by reverting the sort: exit 1 with the defect, exit 0 with the fix.
Verification
Regenerated under protoc 3.21.12, 22.5, 23.4, 24.4, 25.1, 29.3 and 35.1, against the ESPHome 2026.8.2 API protos and every fixture in this repo (
nested,maps,test_messages_proto3,empty):On the ticket's open question of whether anything else is unstable, the two ESPHome forms were identical as line multisets and every differing line fell inside an
optionsblock. Structurally, options was the only site reading raw serialized bytes or an unordered collection; every other emission loop walks arepeateddescriptor field, which protoc orders by declaration.format-check,check-types,check-schema,typecheck,test,check-wire-vectors,check-float-vectorsandcheck-option-orderall pass locally.make lintfails on this machine before and after the change: luacheck 1.2.0 cannot load under Homebrew's Lua 5.5. CI pins 5.4.Not addressed
parse_unknown_fieldsraises on wire types 1 and 5, so afixed32,fixed64,floatordoublemessage option would fail to parse.mainwraps generation in a bareexceptthat prints and exits 0, so that surfaces as a silently truncated schema rather than an error. Pre-existing and not reachable from the protos in use, so left alone rather than widened into this change.