fix: emit fields and map entries in a stable order

[derek-miller]

Follow-up to #27, item 6 of the bot's review there.

pb.encode iterated messageSchema.fields and map entries with pairs(), so one message could encode to different bytes from one process to the next. Every interpreter in the matrix is exposed: PUC Lua has seeded string hashing per state since 5.2, and LuaJIT randomises it per process. Hashing the whole 37-vector encoding dump across repeated runs gave five distinct encodings at 0c5f840 (Lua 5.5 and LuaJIT both varied) and one at this head. Fields now go out ascending by field number and map entries ascending by key; keys within one map share a type, and Int64 {high, low} pairs order by high word then low. No cache: sorting a message's field numbers per encode is negligible next to the string concatenation already there, and a cache keyed on the schema table would go stale if a caller mutated it.

Guarded. Neither direction of the wire-vector harness can see order — both re-parse into messages — so a revert of sorted_keys shipped green. The suite now walks each vector's encoding and asserts top-level field numbers are non-decreasing and map keys ascending per field. The int32, int64 and bool maps in the corpus carry extra keys so the guard has an adjacent pair for every comparator branch (number, string, boolean, Int64 table). Mutation-tested one loop at a time, five runs per interpreter on 5.1, 5.4 and LuaJIT 2.1: restoring pairs() in the field loop fails every run, and restoring it in the map loop fails every run. Both pass at this head.

Effect. Wire vectors byte-identical to the reference implementation: 21 of 37 on every interpreter, up from an interpreter-dependent 8 (LuaJIT), 14 (5.1) or 20–21 (5.5). Of the 16 that still differ, 4 are packing (this encoder never packs), 10 are assembled goldens the encoder re-emits canonically, and 2 are maps whose keys the reference's deterministic serialization orders differently — int32 1 before -3, and int64 likewise. Probed directly with SerializeToString(deterministic=True):

  map_int32_int32      inserted=[1, -3, 2, -1]                 deterministic emission=[2, 1, -1, -3]
  map_sint32_sint32    inserted=[1, -7, 3, -1, 2]              deterministic emission=[3, 2, 1, -1, -7]
  map_int64_int64      inserted=[1, -3, 2, -1]                 deterministic emission=[2, 1, -1, -3]
  map_uint32_uint32    inserted=[3, 4294967295, 2]             deterministic emission=[4294967295, 3, 2]
  map_string_string    inserted=['b', 'a', 'ab', '']           deterministic emission=['ab', 'a', 'b', '']
  map_bool_bool        inserted=[True, False]                  deterministic emission=[True, False]

That order is upb's own, not a wire-format property, so this PR does not try to reproduce it; the semantic encode comparison stays, and CLAUDE.md's description of what it normalises is unchanged.

Verified. make check clean; 252/252 selftest and 111/111 wire-vector assertions on 5.1, 5.4 and LuaJIT 2.1; make check-wire-vectors agrees in both directions; bool-keyed and mixed table/number Int64-keyed maps encode without a comparator error.

[svc-finitelabs]

Reviewed 6cf0325 against 0c5f840. Two files, +29/-4. This is item 6 of my #27 review, so I went looking for ways the fix could be wrong rather than ways it could be right. The code is correct, and the guarantee is stronger than the body claims. Two things are worth fixing before it lands, both about what the repo will still know after the merge.

The comparator holds, and the determinism claim is understated

ascending is a valid strict weak ordering, not just on the happy path. I pulled it and sorted_keys verbatim out of init.lua:603-623 and checked irreflexivity, asymmetry and transitivity over every pair and triple of seven adversarial key sets, plus 50 repeat sorts of each, under Lua 5.1, Lua 5.5 and LuaJIT 2.1:

  ok  field numbers              -> 1 2 15 401 536870911
  ok  int32 keys                 -> -3 -1 0 1 2
  ok  string keys                ->  a ab b
  ok  bool keys                  -> false true
  ok  int64 tables               -> {0,0} {0,1} {1,0} {4294967295,4294967295}
  ok  MIXED number+table         -> -5 1 {0,2} {7,0}
  ok  MIXED bool+number+string   -> false true 3 x

No invalid order function for sorting anywhere. The type-name fallback at :605 is doing more than avoiding an error on the mixed sets: it keeps them totally ordered, so a map spelling some Int64 keys as numbers and others as {high, low} still emits one fixed order rather than an arbitrary one.

On determinism I deliberately used a different instrument from yours. Instead of repeating one vector across processes, I hashed the whole 37-vector encoding dump, four runs on each of three interpreters:

base 0c5f840   lua 5.5   f14c175c  f14c175c  f14c175c  5ae1fca8
               lua 5.1   376e7e8c  376e7e8c  376e7e8c  376e7e8c
               luajit    582d6e7a  c7d56ad1  c7d56ad1  582d6e7a
head 6cf0325   all three ee914652  (x12)

Five distinct encodings of the same corpus before, one after. Two corrections to the body fall out of that:

• It was never only LuaJIT. Lua 5.5 also produced two distinct hashes across four runs at the base. PUC Lua has seeded string hashing per state since 5.2, so attributing this to LuaJIT understates it: every interpreter in the matrix could emit different bytes from one process to the next.
• 8 of 37 is a LuaJIT reading, not a baseline. I measure the base at 8 under LuaJIT, 14 under Lua 5.1, and 20 or 21 under Lua 5.5 depending on the run. 22 of 37 reproduces exactly on all three. The durable claim is that the count stopped depending on the interpreter, which is a better result than the before/after pair suggests. Worth writing as "8 under LuaJIT", or nobody later reads 8 as a stable property of the old code.

Your 15-vector breakdown partitions exactly: 4 packing (packed repeated, repeated scalars, proto3 default, repeated single element, negative sfixed32, repeated), 10 assembled goldens, and maps with 32-bit keys.

1. Nothing in the repo can detect a regression of this

I reverted the two loops at init.lua:642 and :661 to their pairs() form, left the rest of the branch alone, and ran everything:

• ./run_tests.sh 5/5, both math modes, Lua 5.5
• LUA_BINARY=luajit ./run_tests.sh 5/5, five consecutive runs
• make check-wire-vectors 37 goldens read back, 37 encodings compared as messages, "the reference implementation agrees with both directions"

All green with the fix removed. That is structural rather than a gap in the corpus. check_wire_vectors:73-77 does parsed.ParseFromString(...) and then parsed != cases[name], a comparison of parsed message objects, which cannot see wire order by construction. wire_vectors_test.lua:189-196 re-decodes the encoder's own output and is order-independent for the same reason. So the single property this PR adds is the one property both directions of the harness exist to normalise away.

CLAUDE.md:372-374 says "There is no list of expected failures: a defect the vectors find is fixed in the change that adds the vector." The converse is worth holding too. As it stands, a future refactor that drops sorted_keys silently reverts this PR and ships green on all six matrix entries.

The invariant is cheap to assert head-on, and unlike an encode-it-twice test it does not depend on hash seeds. Walking the encoder's output and asserting the top-level tag sequence is non-decreasing came to about 40 lines of Python for me; in Lua it is less, since decode already has the tag reader. I validated that shape as an instrument before trusting it. Against the reverted tree it fails loudly and specifically:

FIELD ORDER NOT ASCENDING: singular scalars: [2, 11, 1, 14, 10, 7, 5, 13, 9, 12, 8, 6, 4, 3, 15]
FIELD ORDER NOT ASCENDING: unusual field names: [401, 411, 417, 408, 404, 403]

and against 6cf0325 it passes on all three interpreters. Map keys are the same idea one level down, with one trap worth passing on: my first pass compared raw key bytes and flagged maps with 32-bit keys as out of order, because -3 sign-extends to a ten-byte varint that sorts above 1 bytewise. That was my instrument being wrong, not the encoder. Sorting is by the Lua key, and [-3, 1] is right.

2. The CLAUDE.md hunk at 367-370 turns a true sentence false

The hunk drops "field order and map order" from the list of what the semantic direction normalises away, leaving "compares messages, which normalises packing away."

That sentence describes what check-wire-vectors does, and check-wire-vectors is untouched by this PR. parsed != cases[name] still normalises field order and map order away exactly as before, and finding 1 is the proof: it passes on a tree with the ordering fix removed. As edited it reads as though the harness now holds the encoder to an order, which is the opposite of what it does, and it is the sentence a future reader would rely on when deciding whether ordering is covered.

I would revert that hunk. The new guarantee is already stated correctly at 247-249, in the encode section, which is where it belongs: it is a property of the encoder, not of the comparison.

State

8/8 checks green on 6cf0325. make check fails on my box at lint, but identically at 0c5f840 (luacheck 1.2.0 under Lua 5.5, attempt to assign to const variable 'field_name'), so that is my environment and not this branch. CI's Check job is the one that counts and it is green.

Leaving this as a comment rather than a verdict. The encoder change itself I am satisfied with and would approve as written; I am holding the approve for the doc hunk, and I think the ordering guard belongs in this PR rather than downstream of it, since the harness cannot notice its absence later.

[derek-miller]

Both taken. The wire-vector suite now walks each encoding and asserts field numbers non-decreasing and map keys ascending per field; restoring pairs() in either loop fails it on 5.1, 5.4 and LuaJIT, and it passes at this head. The CLAUDE.md sentence about check-wire-vectors is back to "packing, field order and map order", since that comparison is unchanged. The body now says the instability was every interpreter, not LuaJIT, and gives the base count per interpreter. Please re-review.

[svc-finitelabs]

Round 2 on a401347, scoped to 6cf0325..a401347 (two files, +50/-1). Base is unchanged at 0c5f840, so this is a plain follow-up commit, not a rebase. 8/8 checks green.

Both items from round 1 are addressed, and the field-order half of the new guard is solid. I re-derived that independently rather than taking the body's word: restoring pairs() at init.lua:661 and leaving the map loop alone fails the suite 5 runs out of 5 on Lua 5.5.0, Lua 5.1.5 and LuaJIT 2.1. The CLAUDE.md hunk is semantically back to the base wording.

The map-order half does not hold, and it is the half that the body's evidence section is mostly about.

The map-order assertion is two comparisons, and one interpreter never sees either

I instrumented check_order to count what it actually compares. Across all 37 vectors it walks 28 map entries and performs exactly 2 key comparisons:

CMP field=map_int32_int32    type=number  prev=-3       key=1
CMP field=map_string_string  type=string  prev="alpha"  key="beta"

That is the whole thing. Every other map field in every vector carries exactly one entry, and a single-entry map has no adjacent pair, so previous ~= nil is false and the assertion is vacuous for it. Per-vector entry counts:

maps with 32-bit keys   map_int32_int32=2   everything else =1
maps with 64-bit keys   all five            =1
maps with string keys   map_string_string=2 everything else =1
the six edge-case map vectors                =1 each

Mutating the two loops separately, rather than together, is what makes this visible:

mutation            lua 5.5        lua 5.1        luajit 2.1
field loop :661     5/5 caught     5/5 caught     5/5 caught
map loop   :642     14/20 caught   0/20 caught    5/5 caught

So "restoring pairs() in either loop fails it on 5.1, 5.4 and LuaJIT" is true of the field loop and not of the map loop. On 5.1 a map-order regression is a guaranteed pass, 20 runs out of 20. On PUC 5.2 and up it is a coin flip: the failure there always comes from map_string_string, which is the one place seeded string hashing can reorder a two-element table. LuaJIT catches it because it additionally reorders the int32 map. Lua 5.1 does not seed string hashes, so those two maps come out in the same order every process, and that order happens to be ascending.

Nothing else in the repo covers the gap. With both loops on pairs(), protobuf_test.lua is 252/252 and math_fallback_test.lua passes 48013 comparisons.

What this leaves unexercised

key_less is a verbatim copy of ascending, so the guard asserts the encoder is sorted by its own comparator, which is the right shape for catching a pairs() regression. But only two of the comparator's branches are ever reached, and they are the two simple ones. Never reached at all:

• the ta == "table" branch, the Int64 {high, low} high-word-then-low-word comparison. All five 64-bit-key maps carry one entry. This is the branch the body's deterministic-emission table is about.
• the ta == "boolean" branch. map_bool_bool carries one entry, True.
• the ta ~= tb mixed-type branch, which is the one the body cites for mixed table and number Int64 keys.

Cheap fix

The vectors are generated, so this is a few lines in tools/gen_wire_vectors around :391 and :404 plus a regen. Giving two of the maps a third key, in non-ascending insertion order, and both keys to the bool map covers every branch and removes the dependence on the hash seed:

message.map_int64_int64[-3] = 6      # table branch, negative high word
message.map_int64_int64[2] = 7
message.map_bool_bool[False] = True  # boolean branch
message.map_int32_int32[2] = 9       # 3 entries, so 5.1 can no longer pass by luck

Both directions re-parse into messages, so extra entries cannot introduce spurious byte mismatches, and maps with 32-bit keys stays in the 15 for the same upb-ordering reason the body already gives.

I would rather see that land with this PR than after it, because the guard exists specifically to answer round 1's finding, and as written it answers it for field order only.

Two smaller things

The body's Guarded. paragraph and the comment both say the mutation fails on every interpreter. That wants narrowing to the field loop before this becomes the permanent record.

The restored CLAUDE.md sentence is now one long line where the base wrapped it at the file's usual width. Cosmetic, but it shows up in the diff.

Verdict and what I did not do

Leaving this as a comment rather than a blocking review: the encoder change itself is correct, and I proved the comparator is a valid strict weak ordering in round 1 over every pair and triple of seven adversarial key sets. The gap is in the guard's strength and in one claim about it. Push the generator change and I will approve.

Not re-run this round: the reference-side probe of upb's deterministic order, and the make check lint failure, which I measured at the base commit last round and is pre-existing. My matrix was 5.5.0, 5.1.5 and LuaJIT 2.1; I do not have 5.2, 5.3 or 5.4 locally, though CI covers them.

[derek-miller]

Generator change pushed as d65ee5a: map_int32_int32 has three keys, map_int64_int64 three including a negative high word, map_bool_bool both. The guard now makes 2 number, 2 Int64-table, 1 boolean and 1 string adjacent comparisons. Reverting the map loop alone to pairs() fails 5 of 5 runs on 5.1, 5.4 and LuaJIT; the field loop likewise. Body narrowed accordingly, and the CLAUDE.md line is re-wrapped to the base's width. Byte-identical vectors are 21 of 37 now, since the three-key int64 map joins the int32 one in differing from upb's key order. Please re-review.

[svc-finitelabs]

Round 3 on d65ee5a. The generator change is exactly what round 2 asked for, and the gap it was aimed at is closed. Re-derived the numbers rather than re-running your commands, since confirming a finding with the author's own method is an echo.

The map-order assertion is now live on every interpreter. Round 2's problem was that reverting the map loop alone to pairs() was caught 14/20 on 5.5, 5/5 on LuaJIT and 0/20 on 5.1, so the guard was a guaranteed pass on one matrix entry. Re-ran the same one-loop-at-a-time mutation, 5 runs per interpreter:

                      lua 5.5     lua 5.1     luajit 2.1
  baseline            0/5         0/5         0/5        (green)
  map loop  :642      5/5         5/5         5/5
  field loop :661     5/5         5/5         5/5

On 5.1 the catch is structural, not lucky: every run reports map_int32_int32: key -3 emitted after 2. With two keys, 5.1 put 1 and -3 in the hash part and they happened to come out ascending. With [2]=9 added, 1 and 2 occupy the array part and iterate first, so -3 is forced last and the comparison always inverts. The third key is doing the work, not the extra entropy.

Comparator coverage checks out. Instrumented key_less to census what it actually compares over all 37 vectors: 2 number, 2 Int64 table, 1 boolean, 1 string, 6 total, identical on 5.5, 5.1 and LuaJIT. The two Int64 comparisons split across both sub-branches, one deciding on the high word and one falling through to the low word, so the {high, low} path in your emission table is now genuinely exercised. The ta ~= tb branch stays unreached, which is correct: map keys are homogeneous per schema, as the body says.

21 of 37 verified, and the determinism claim is directly visible. Measured 21/37 byte-identical on 5.5, 5.1 and LuaJIT, and the 16 that differ are the same 16 names on all three. That is the stronger statement than the count alone. The partition holds exactly: 4 packing, 10 assembled goldens, 2 maps, no leftovers.

Local make check is red at lint, identically at base 0c5f840 (luacheck 1.2.0 under Lua 5.5), so it is my toolchain and not this branch. CI Check is green, 8/8 green overall.

One note, not a blocker. The table branch of ascending (init.lua:603) and its copy key_less compare a[1] < b[1], and the high word is unsigned, so a negative 64-bit key sorts after every non-negative one. The two new fixtures hold the same logical key set and come out in different orders:

  map_int32_int32  emits  -3, 1, 2     (signed)
  map_int64_int64  emits  1, 2, -3     (unsigned high word)

Nothing is wrong here. Map entry order is not a wire-format property, the PR only claims stability, and the guard asserts self-consistency against the same comparator. The reason to mention it is that the new vectors now pin this ordering in a test. If someone later makes the table branch sign-extend so 64-bit keys sort like the 32-bit ones, the order guard fails and the message reads like a regression rather than the deliberate choice it would be. A line at that branch saying the high word compares unsigned, so negative 64-bit keys sort last, and that is arbitrary but stable, would save that reader the round trip. Worth folding into a later commit, not worth another push here.

Approving.

What I did not re-run: make check-wire-vectors against the reference, and the 5.2, 5.3 and 5.4 matrix entries. CI covers all four.