diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a37cf91..5ca4c56 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -65,6 +65,9 @@ jobs: - name: Check float vectors run: make check-float-vectors + - name: Check option order + run: make check-option-order + test: needs: check runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md index 28a47c9..5248cb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -452,6 +452,20 @@ deliver them, but `parse_descriptor_set` skips any file whose package is `check-schema` fails. `test/test_messages_proto3.proto` is vendored with those fields trimmed for that reason. Tracked as FL-17. +**Message options are sorted by field number, and the sort is load-bearing.** 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 number. The generator reads +those bytes back, so without the sort the same `.proto` produces two different Lua +files depending on the toolchain, and a consumer's drift check reports a false +positive against an unpinned protoc. `make check-option-order` guards it. + +That check 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 CI runner; feeding the options in descending +order is what makes the check fail on a revert under every protoc. CI installs +Ubuntu's `protobuf-compiler`, which is 3.21.12 on noble, so `check-types` and +`check-schema` run on the declaration-order side of that split. + ## Building The build process uses `amalg` to create single-file distributions: diff --git a/Makefile b/Makefile index 84a7251..9f7531c 100644 --- a/Makefile +++ b/Makefile @@ -249,6 +249,15 @@ check-float-vectors: @echo "Checked-in float vectors match the generator." @LUA_BINARY=$(LUA_BINARY) .venv/bin/python3 tools/check_float_vectors +# Check the generator sorts message options. Needs the venv, so not in `check`. +.PHONY: check-option-order +check-option-order: + @if [ ! -f .venv/bin/python3 ]; then \ + echo "Python virtual environment not found. Run 'make setup-schema-generator' first."; \ + exit 1; \ + fi + @.venv/bin/python3 tools/check_option_order + # Format Lua code with stylua .PHONY: format format: diff --git a/tools/check_option_order b/tools/check_option_order new file mode 100755 index 0000000..8503b45 --- /dev/null +++ b/tools/check_option_order @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +""" +Verify message options are emitted in ascending field number order. + +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 reads those bytes back, so without a sort its output depends on which +protoc built the descriptor. + +A fixture `.proto` cannot check this on its own. Under a modern protoc an unsorted +generator already emits ascending, so the check would pass on the defect and only +fail where CI happens to install an old protoc. The descriptor here is therefore +built by hand with the options in descending order, which is the one input that +tells a sorted generator from an unsorted one under every protoc. + +1. Setup: the options really do arrive descending, so the comparison is not vacuous. +2. Order: the generator emits them ascending. +""" +import os +import re +import sys + +from google.protobuf.descriptor_pb2 import DescriptorProto +from google.protobuf.internal import decoder, encoder + +_GEN = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gen_lua_proto_schema") +_module = type(sys)("gen_lua_proto_schema") +exec(compile(open(_GEN).read(), _GEN, "exec"), _module.__dict__) + +# Numbers and shapes mirror ESPHome's api_options.proto, the schema that surfaced this. +EXTENSIONS = {1036: "id", 1038: "ifdef", 1041: "base_class"} + + +def varint(number: int, value: int) -> bytes: + out = bytearray() + write = encoder._VarintEncoder() + write(out.extend, number << 3 | 0, True) + write(out.extend, value, True) + return bytes(out) + + +def length_delimited(number: int, value: str) -> bytes: + out = bytearray() + write = encoder._VarintEncoder() + write(out.extend, number << 3 | 2, True) + encoded = value.encode() + write(out.extend, len(encoded), True) + out.extend(encoded) + return bytes(out) + + +def wire_order(raw: bytes) -> list[int]: + """Field numbers in the order they appear in `raw`.""" + pos = 0 + numbers = [] + while pos < len(raw): + tag, pos = decoder._DecodeVarint(raw, pos) + numbers.append(tag >> 3) + if tag & 0x7 == 0: + _, pos = decoder._DecodeVarint(raw, pos) + else: + length, pos = decoder._DecodeVarint(raw, pos) + pos += length + return numbers + + +def main() -> int: + failures = [] + + message = DescriptorProto() + message.name = "OptionOrder" + message.options.MergeFromString( + length_delimited(1041, "BaseProtoMessage") + varint(1038, 1) + varint(1036, 3) + ) + + arrived = wire_order(message.options.SerializeToString()) + expected_input = sorted(EXTENSIONS, reverse=True) + if arrived != expected_input: + failures.append( + "options did not arrive descending: %s, wanted %s. The generator would be " + "handed sorted input and the order assertion below would pass on any " + "generator." % (arrived, expected_input) + ) + + lua = _module.parse_message(message, message_option_extensions=EXTENSIONS) + body = lua.split("options = {", 1)[1].split("}", 1)[0] + emitted = re.findall(r"^\s*(\w+) = ", body, re.MULTILINE) + wanted = [EXTENSIONS[number] for number in sorted(EXTENSIONS)] + if emitted != wanted: + failures.append("emitted %s, wanted %s" % (emitted, wanted)) + + print("Option order: %d options fed in descending order, emitted as %s" + % (len(arrived), emitted)) + for detail in failures: + print(" FAIL: " + detail) + if failures: + print("Option order: FAILED") + return 1 + print("Option order: the generator sorts by field number") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/gen_lua_proto_schema b/tools/gen_lua_proto_schema index 456c93d..8fe4827 100755 --- a/tools/gen_lua_proto_schema +++ b/tools/gen_lua_proto_schema @@ -295,8 +295,11 @@ def parse_message( # Some messages extend MessageOptions in a way that does not get parsed # into the Extensions field but is available in the raw data. if len(message.options.Extensions) == 0 and message_option_extensions: - for num, typ, val in parse_unknown_fields( - message.options.SerializeToString() + # protoc does not serialize these in a stable order: 3.21 and 22.5 emit + # them in .proto declaration order, 23.4 and later ascending by number. + for num, typ, val in sorted( + parse_unknown_fields(message.options.SerializeToString()), + key=lambda unknown: unknown[0], ): if num not in message_option_extensions: continue @@ -308,7 +311,7 @@ def parse_message( val = f"\"{val.decode('utf-8', 'replace')}\"" lua += (" " * (indent + 2)) + f"{name} = {val},\n" else: - for field in message.options.Extensions: + for field in sorted(message.options.Extensions, key=lambda ext: ext.number): if not new_line_added: lua += "\n" new_line_added = True