tubuild: stop cutting declarations in half, and name the block when a definition is out of reach

[andrewboudreau]

Tools-only, off origin/main. No src/, no config/, no generated state — so nothing here can move a byte of the ROM. (The validator restores all of tools/ from base, which is exactly why this cannot ride along with a promotion PR.)

The bug

tubuild.split_legacy_source decided where a local declaration ended by counting braces on its first line only:

depth = lines[start_i].count("{") - lines[start_i].count("}")

For an Allman-braced record that is 0, so the loop never runs and the block is cut to the bare words struct Obj. Two things then go wrong at once, and #2071's MCarlo TU happened to show both:

• the body falls through to the function split and lands in function_text as a headless { ... }; at file scope — the TU carries an incomplete type and a stray block;
• the same truncated text is what _merge_field quotes, so the TUBUILD CONFLICT comment asks a reviewer to compare a full struct against two words.

82 legacy sources tree-wide were handed a bare { as their function body this way — 53 opened by struct, 31 by typedef, 5 by class.

Fixing the walk exposed two more shapes:

2. Elaborated return types. struct dActor_c *dCapEnemy_c::RespawnIfHasCap() opens on a decl keyword, but as an elaborated type specifier on the return type — a spelling the flat-C sources use constantly. It was being filed as a shadow declaration, taking the function's own signature with it. A record head never carries a ( before its brace; a parameter list always does, so that is the test (applied to the text before the opening brace, so struct S { void (*fn)(void); }; still reads as a record).

3. Definitions inside a wrapper block. When the definition sits inside namespace X { ... } or extern "C" { ... }, every line of it is consumed as a declaration and nothing is left to be the body. tubuild has nowhere to put a block-scoped member, so refusing is correct (plan sec 7.3: an unfitting shape is "assemble this one by hand"). But the old message —

scanned to end of file without finding a function body

— reads as "your file has no function in it" and sends a reader hunting for one. It now names the block and the line that opened it:

the definition is inside the namespace Memory block opened on line 5, so it was
consumed as a declaration -- tubuild cannot place a block-scoped member in a TU

Plus a fail-loud backstop for any residual mis-split: a function body never starts on a bare brace, so reaching one is refused rather than emitted.

Measured, base vs this branch, over all 11150 legacy sources

	base	this
headless { handed back as the function body	82	0
files that refuse	693	683
…of those, refusals that name the offending block	0	683
files that split cleanly	10457	10467
newly refusing	—	7

The 7 newly-refusing files (_ZN6Memory8Allocate*, _ZN2GX15SetBankForSubBGEt, func_ov006_021063a0) are exactly the wrapper-block shape: base produced a headless body for them, which is the outcome this PR exists to stop. None of the seven newly-refusing files is enrolled in a TU, so no manifest entry changes shape. An earlier revision of this sentence said "none of the affected files", which overstated it: seven of the headless-repaired files are enrolled, across four TUs — ov002/Enemy (3), ov062/Koopa+KoopaSmall (2), ov004/unit020b0a38 (1) and ov062/KoopaTheQuick (1). Nothing is damaged, and the correction strengthens the case rather than weakening it: all four are text-verified, none of their promoted_source destinations exists on main, so none of that text is in the build — and none carries a headless block today only because a human repaired them by hand, which is precisely what this change makes unnecessary. (Measured by running the pre-#2072 split_legacy_source over the 1246 legacy sources the manifest enrolls.)

Tests

Five new cases, built from the real failing inputs rather than synthetic ones, per review feedback that tools/ failure paths here are under-tested:

• src/func_ov006_020f8224.c — the MCarlo struct Obj, asserted at both ends of the bug: the declaration keeps its body, and the conflict comment carries a real alternate body instead of two words;
• src/_ZN11dCapEnemy_c15RespawnIfHasCapEv.cpp and src/func_02041b60.c — elaborated return type, method and free function;
• src/_ZN6Memory8AllocateEj.cpp and src/func_ov006_021063a0.cpp — the two wrapper-block refusals, asserting the message names the block;
• the shapes the fix must not disturb: a bare forward declaration, a declaration wrapped across lines, and a record whose member is a function pointer.

python -m pytest tools/test_tubuild.py tools/test_tubuild_owned_relocs.py \
                 tools/test_check_src_tu_compiles.py tools/test_tu_manifest.py \
                 tools/test_srcpath.py -q
145 passed, 2 deselected

The 2 deselected (test_verify_reproduces_pilot_1s_7_of_7_and_clean_objisolate, test_compile_report_matches_the_pilots_object_inventory) fail identically on unmodified origin/main — verified by stashing this diff and re-running. Pre-existing, not touched here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QhhAeJwXBnfPp7B5DNjCwh

[tangos-validator]

✅ PR validation — Passed

noverify: no source/build-data changes in this PR

_{Each changed src/*.c|*.cpp is compiled and its relocated bytes compared to the binary data on a private build box. Passing requires every changed file to reproduce the ROM byte-for-byte with correct relocation targets — this catches WRONG-DEST relocations and non-reproducing near-misses that ledger-scoped linkcheck skips.}

[andrewboudreau]

Reproduced every number independently — approving and merging. One claim in the body is narrower than you wrote it, and the truth is stronger.

Reviewed at f15b25e89c9d. I re-ran your whole measurement from a clean git archive origin/main tools src include config scratch tree, loading both tubuild.py versions side by side and splitting all 11,132 legacy sources under each.

              headless body   refuses   refusals naming the block   clean
base                     89       693                          0   10350
this PR                   0       683                        683   10449

The zero is the zero. My headless count is 89 where you said 82 — I flag a file when the first non-blank line of function_text is a bare {, } or };, which is probably a slightly wider net than yours. It does not matter: both counts go to 0, and no file is headless under this branch. Refusals 693 → 683 with 7 new reconciles exactly against your table (693 − 17 + 7 = 683).

premerge_check against origin/main @ ccb65b8b3c38, merge tree 6bcf2cc34877:

converted-ratchet / dead-references / duplicate-sources / header-offsets /
langmode-ratchet / layout-check / src-tu-refs / source-coverage    8/8  pass -> pass
RESULT: nothing goes green -> red.

---

The one correction — and it argues for the PR, not against it

None of the affected files is enrolled in a TU, so no manifest entry changes shape.

That is true of the 7 newly-refusing files. I checked each against every config/tu_manifest.d/** legacy_source row and all seven come back with zero manifest hits, so that half stands.

It is not true of the 89 headless ones. Seven of those are enrolled today:

src/_ZN12dEnemyBase_c22SpawnMegaCharParticlesER8dActor_cPc.cpp -> ov002/Enemy
src/func_ov002_020ae87c.c                                      -> ov002/Enemy
src/func_ov002_020ae954.c                                      -> ov002/Enemy
src/_ZN5Koopa16CleanupResourcesEv.cpp                          -> ov062/Koopa+KoopaSmall
src/func_ov062_02118334.c                                      -> ov062/Koopa+KoopaSmall
src/func_ov062_0211a9c4.c                                      -> ov062/KoopaTheQuick
src/FreeGfxSlotsById.c                                         -> ov004/unit020b0a38

So I went and checked the landed content, because a headless block sitting in a landed TU would be a real defect and not a hypothetical one. Depth-tracking the four shadow sources on origin/main for a bare { at brace-depth 0:

src_tu/actors/Enemy.cpp                 23 file-scope braces, ALL have a head above
src_tu/actors/Koopa+KoopaSmall.cpp       9 file-scope braces, ALL have a head above
src_tu/actors/KoopaTheQuick.cpp          6 file-scope braces, ALL have a head above
src_tu/actors/unit_ov004_020b0a38.cpp    2 file-scope braces, ALL have a head above

Every one is an Allman-styled function or record — void dEnemyBase_c::SpawnCoin(), struct Unk954A, dScMgBase_c::~dScMgBase_c(). Nothing landed is damaged. All four are status: text-verified and none of the four promoted_source destinations exists on main, so none of this is in the build either. Your conclusion holds; only the premise was overstated.

Why that matters more than the wording

Those four TUs are clean because a human repaired them. The generator on main does not produce them. Run base against their own enrolled inputs today:

src/func_ov002_020ae954.c
  BASE  decls=[('typedef', 'struct', 1 line)]   body starts: '{'
  PR    decls=[('typedef', 'UnkA', 5), ('typedef', 'UnkB', 5)]
        body starts: 'void func_ov002_020ae954(UnkA *a, UnkB *b)'

src/FreeGfxSlotsById.c
  BASE  decls=[('struct', 'E', 1 line)]         body starts: '{'
  PR    decls=[('struct', 'E', 6 lines)]        body starts: 'void FreeGfxSlotsById(int arg)'

On base the whole function signature is gone, and the shadow declaration is a one-line stub named literally struct. So this is not a cosmetic fix to a conflict comment — it is a regeneration-safety fix for four already-landed TUs. Anyone re-running tubuild over ov002/Enemy on main today silently gets back a file worse than the one in the tree. That belongs in the PR body far more than the row it currently occupies.

---

What else I verified

The 17 files that stop refusing lose nothing

This was my main worry — a refusal becoming a wrong split is worse than the refusal, and the body lists these only as a table cell. For each of the 17 I reassembled every output part (function_text, shadow_decls, includes, externs, pragmas, macros, notes, cpp) and diffed the set of non-blank lines against the source:

all 17 files: LOSTLINES=0

Spot-checking two by eye, they are exactly right:

// src/func_02043f4c.c
[struct Inner] 'struct Inner { char pad[0xc]; unsigned short id; };'
[struct Node ] 'struct Node { char pad[4]; struct Node* next; struct Inner* inner; };'
body: struct Node* func_02043f4c(struct Node** pp, int key, struct Node* alt) {

And the reason they refused before is the interesting part: struct Node* func_02043f4c(...) opens on struct, so base ate the entire function as a shadow declaration and then reported "scanned to end of file without finding a function body." That is 17 sources tubuild could not touch at all, now splittable. Another thing the body undersells.

The elaborated-return-type heuristic has no false positive in this tree

"(" in stripped.split("{", 1)[0] misfires on a record head that carries a paren before its brace — a macro-decorated head like struct ALIGN(4) Foo {. I grepped all of src/ on origin/main for that shape and got exactly one hit, and it is prose inside a comment:

src/_ZN12dScMgAmida_c6RenderEv.cpp:4:   struct dispatch (`Base::m_90()`) for slot 36, ...

That file is in neither of my two difference sets, so its split is unchanged base vs branch. The heuristic is empirically safe on the tree as it stands. Worth knowing it is a shape-based guess, not a parse — if a #defined alignment attribute ever lands on a record head, this is where it bites.

The fail-loud backstop catches the residual case I went looking for

consume_block's walk-forward stops at the first line containing ;, which is tested on the opening line too. So a head with a semicolon in a trailing comment still truncates:

struct Obj // count; here
{
    int a;
};

I built that input to see whether it slips through silently. It does not:

-> ERROR line 2 is a bare '{' where a function signature was expected --
   a declaration above it was split mid-body

The bare-brace refusal catches it and names the line. No instance of that shape exists in the tree today, and when one appears it fails loudly instead of quietly. That is the right trade, and it is why I am not asking for anything here.

Tests

51 pass / 3 fail in my scratch tree. All three fail identically against unmodified base tubuild.py —

test_list_finds_polelift_and_its_module_neighbours
test_list_has_no_duplicate_ids_anywhere_in_the_tree
test_promote_dry_run_refuses_a_tu_that_is_not_link_verified_but_still_explains

— because they shell out against the real working tree and my scratch archive has no src_tu/. Environmental, not yours; it is the same tracked-tree coupling I have on my list separately. Every new and adjacent case passes: 11/11.

Building the cases from the real failing inputs rather than synthetic ones is exactly what I asked for, and asserting struct Obj at both ends — the declaration keeps its body, and the TUBUILD CONFLICT comment quotes a real alternate — is the version that would have caught this bug in the first place.

---

Verdict

Approved — merging. Tools-only, no src/, no config/, no generated state; 8/8 static gates pass→pass on the real merge tree; every measured claim reproduced from an independent scratch tree; the one overstated sentence understates the actual benefit.

Please fix the body forward when convenient — "none of the affected files is enrolled" should read "none of the seven newly-refusing files is enrolled; seven of the repaired-headless files are, in four text-verified shadow TUs, none of which carries a headless block today because they were repaired by hand — which is precisely what this makes unnecessary."