feat(proto): eleven vanilla enums stop being typed out by hand - #1049
Merged
Conversation
A Java enum's constant order is a wire table. `FriendlyByteBuf.writeEnum` sends `ordinal()` and `ByteBufCodecs.idMapper` sends a field each constant declares, so the declaration order in the jar is as much a definition with a source of truth as a registry is -- with the extra hazard that an ordinal carries no name on the wire, so a transcription one constant out is a wrong value rather than a failed lookup. Eleven of them were typed out here: the boss bar pair, the four scoreboard and team enums, `Direction`, `EquipmentSlot`, `HeightmapKind`, `ChatTypeParameter` and `ObjectiveRenderType`. `EquipmentSlot` twice, once per module, with different accessor names on the two copies. They were hand-written for one reason, and it is structural: a hand-written ordinal enum in this crate is almost exactly the set whose enclosing packet the extractor refused. Where a layout is recovered, `build.rs` already emits the enums it mentions from `protocol.json` -- twenty of them exist today. These eleven sit inside `set_player_team`, `set_objective`, `set_equipment`, `commands`, `level_chunk_with_light`, or the boss bar packet that until tonight extracted as carrying no bytes at all. `JAVA_ENUMS` reads them out of the decompiled class directly rather than waiting for those layouts. The two numberings are declared per class rather than guessed, because the difference is invisible in the result. `EquipmentSlot` sends `ordinal()` from `ClientboundSetEquipmentPacket` and `s.id` from its own `STREAM_CODEC`, and those disagree: `OFFHAND` is 1 by ordinal and 5 by id. Because the constants are read out of decompiled Java by a regex, and no test written from that reading can catch a misreading of it, `nix/java/VanillaEncoder.java` now emits the number the *server* produces for all eighty constants -- through the class's own `StreamCodec` for the six that publish one -- and `tests/java_enum.rs` walks `ALL` and compares every one, both directions, so an invented constant and a dropped constant both fail.
Benchmark Results for generalComparing to 04e85fb |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Java enum's constant order is a wire table.
FriendlyByteBuf.writeEnumsendsordinal();ByteBufCodecs.idMappersends a field each constant declares. Either way the declaration order in the jar is as much a definition with a source of truth as a registry is, and hand-transcribing one is the same defect — with the extra hazard that an ordinal carries no name on the wire, so a transcription one constant out is a wrong value rather than a failed lookup.Eleven of them were typed out in this crate.
EquipmentSlottwice, once per module, with different accessor names on the two copies.Why exactly these eleven, which is the interesting part
A hand-written ordinal enum here is almost exactly the set whose enclosing packet the extractor refused.
Where a packet's layout is recovered in full,
build.rs::lower_enumalready turns the enums it mentions into closed#[repr]Rust enums fromprotocol.json— twenty of them exist inOUT_DIRright now, and nobody typed any of them.protocol.jsoncarries 42 extracted enum definitions with full constant lists; the other 22 never reach Rust because they sit inside a refused packet or data component.AttributeModifier$Operationis the clean proof: it is extracted, with its three constants, and it is also hand-written initem/payload.rs, purely becausedataComponent/minecraft:attribute_modifiersis a refusal.So these eleven are the ones no recovered layout reaches — carried by
set_player_team,set_objective,set_equipment,commands,level_chunk_with_light, or by the boss bar packet that until #1038 was extracted as carrying no bytes at all.JAVA_ENUMSinnix/extract-protocol.pyreads them out of the decompiled class directly rather than waiting for those layouts to be recovered.BossBarColorworld.BossEvent$BossBarColorBossBarOverlayworld.BossEvent$BossBarOverlayChatTypeParameternetwork.chat.ChatTypeDecoration$ParameterDirectioncore.DirectionDisplaySlotworld.scores.DisplaySlotidEquipmentSlotworld.entity.EquipmentSlotHeightmapKindworld.level.levelgen.Heightmap$TypesidObjectiveRenderTypeworld.scores.criteria.ObjectiveCriteria$RenderTypeTeamCollisionRuleworld.scores.Team$CollisionRuleidTeamColorworld.scores.TeamColoridTeamVisibilityworld.scores.Team$Visibilityid80 constants. Same conventions as #1021:
#[repr(uN)]with the discriminant being the number,id()anascast,name()off a static table,from_namea binary search over a table the generator sorted, closed with noUnknown, and aconstassertion per file proving the discriminant folds.The two numberings are declared, not guessed
EquipmentSlotsendsordinal()fromClientboundSetEquipmentPacket(output.writeByte(slotType.ordinal())) ands.idfrom its ownSTREAM_CODEC(idMapper(BY_ID, s -> s.id)), and the two disagree:OFFHANDis 1 by ordinal and 5 by id. Which numbering a class uses is the second element of itsJAVA_ENUMSentry, resolved against the source and named in the generated module's own docs. Nothing infers it, because a value read with the wrong table is a wrong value rather than a failed lookup, and the two tables are the same shape.The extractor refuses rather than approximates: a class that has moved, an id field that has gone, a constant the id table does not mention, or a set that is not dense
0..n-1all fail the extraction with the class named.Checked against the server, not against the parse
These constants are read out of decompiled Java by a regex. No test written from that same reading could catch a misreading of it, so the numbers are taken from the other side:
nix/java/VanillaEncoder.javanow emitsjava_enum.<Type>.<CONSTANT>for all 80, and for the six classes that publish aStreamCodecit gets the number by running Mojang's encoder and reading theVarIntback out (asserting the encoding is exactly oneVarIntand no more). For the five with no codec it readsDisplaySlot.id()orordinal()off the class.EquipmentSlotdeliberately uses the ordinal and not itsSTREAM_CODEC, with the reason at the call site.tests/java_enum.rsthen walksALLfor every type and compares both directions: every variant against the server's number for the name it claims, and the count ofjava_enum.<Type>.fixtures against the number of variants. The second half is what catches a dropped constant, which the first half cannot see.Guards broken, and what they said
Each mutation was checked to have applied (
assert t.count(old) == 1, then grep for the replacement in the file) and to parse, before being run.G — a discriminant one out (
TeamColor::DarkBlue = 1→= 2). Caught, but not by the test I was aiming at:Reporting it as it happened: this mutation never reached the test, so it proves the compiler and not the fixture check. Hence H.
H — two entries swapped in the generated name table, which compiles cleanly and is what a sorting bug in the generator would actually look like. Both tests fire:
I — the extractor drops a constant, patched into
scan_java_enumsso the whole pipeline runs on a realistic misparse (BossBarColorlosesWHITE). Regenerated, rebuilt, and the count half fires:All three restored;
nix run .#sync-minecraft-protoreproduces the committed tables andminecraft-proto-generatedpasses.Call sites that changed, and one naming decision
DisplaySlot::to_id()→id()(three tests and one line inevents/smash/src/adapter.rs).HeightmapKind::from_idreturnedResult, the generated one returnsOption, soworld/chunk.rsdoes theok_oritself with the reason for not clamping kept at the site. The twoEquipmentSlotcopies collapse into one, sofrom_raw/from_ordinalandto_raw/ordinalbecomefrom_id/id, plus one privateslot_byteinpackets/play/entity.rsbecause that packet packs the number into a byte with a continuation bit and the narrowing should happen once.EquipmentSlot::MainHandbecomesMainhand, andOffHandbecomesOffhand. Mojang's constants areMAINHANDandOFFHAND, one word each;MainHandneeds a dictionary the generator does not have, and hand-maintaining an exception list is the thing this PR removes. Two call sites incrates/hyperion/src/simulation/inventory.rs.Decisions on the hand-written enums this PR does not generate
Listed because a hand-written type nobody has decided about is the thing to avoid, not the existence of hand-written types.
ChatVisiblity,ParticleStatus— generatable, blocked. Neither class is in the decompiled tree:nix/minecraft-data.nixselects classes referenced one hop fromnet/minecraft/network, and both are reached only vianet.minecraft.server.level.ClientInformation, which is itself one hop out. Filed as ENG-10940. The extractor sees the gap today — both degrade to a barevarintnotedenum ordinal, with the class name dropped.StringArgumentKind—com.mojang.brigadier.arguments.StringArgumentType$StringType. Brigadier is a separate artifact and the decompile coversnet/minecraftonly. Staying hand-written; the reason is now at the type.NamedColor,Decoration—ChatFormatting, but travelling as JSON/NBT names rather than numbers, andDecorationis a reordered subset. Their truth lives in DFUMapCodecs, which this extractor cannot read at all. Staying hand-written; see the note on the text module in the enumeration.EntityDataSerializer— not a Java enum.EntityDataSerializersis a static registry of fields, numbered byregisterSerializercall order, so it needs a different reader. Not attempted.ComponentType— ENG-10872, three enums over one registry, blocked on a hand-transcribedshape()whose variant order is load-bearing.HumanoidArm— the pure duplicate.build.rsalready emits it intoOUT_DIR/types.rs; the copy inpackets/configuration.rsexists because that module was written in parallel with the generator and landed first. Deliberately left for its own change, becausepackets/mod.rsdocuments eleven packets defined twice for exactly the same reason andHumanoidArmis one thread of that knot rather than a separate cleanup. Filed as ENG-10941 with the eleven named.Checks
cargo test --workspace,cargo clippy --workspace --all-targets --all-features -- -D warnings(CI's exact command) andcargo fmt --all --checkall green.nix build .#checks.aarch64-darwin.{minecraft-proto-coverage,minecraft-proto-json,minecraft-proto-generated,minecraft-encoder-fixtures,tool-paths}all green.minecraft-literalsis red onmainand not from this branch:events/smash/tests/visuals.rsgainedminecraft:entity.player.hurt_on_firein #1035, one over the baseline of 322. Untouched here, reported rather than fixed.