Phase 5 Road B: by-value struct ABI + generated raylib binding (#208) - #307
Conversation
Phase 5 Road B needs an ABI that can express raylib's Vector2/Color/
Rectangle before a generator can emit anything useful. This adds the
argument half of that, and records the decision Road B was blocked on.
STDROT_STRUCT (ABI v3)
- New StdrotType plus a `val.blob` carrier {type_name, data, size}. A
`gang` crosses the boundary as its C-ABI byte image, which
compute_struct_layout() already produces -- a native memcpy's it
straight into the real C type.
- By value for real: the adapter copies the caller's blob, so a native
may write through its argument without reaching the caller's variable.
Matches the value-copy rule struct assignment/args/returns already
follow (Appendix B Q3), and reuses STDROT_CSTRING's scratch pattern.
- Tag-checked, not size-checked, on both sides: `gang Vector2 {chad x,y}`
and `gang Size {chad w,h}` are indistinguishable by size. A
STDROT_STRUCT descriptor without a type_name is rejected at load time.
- Accepts the same source expressions a Brainrot struct parameter does --
variable, nested member access, struct-returning call, member of a call
result -- routed around ast_expr_to_stdrot_value() so a call-shaped
argument is not evaluated twice (the #303 bug class).
- Struct *returns* are rejected outright, like STDROT_HANDLE/CSTRING: the
obvious sketch returns a pointer to a dead local, and the ownership
question is Appendix B Q6. Documented rather than half-marshalled.
ABI v3 version bump
StdrotValue grew 24 -> 32 bytes and STDROT_NONE renumbered, so
stdrot_get_api_v2 -> _v3. brainrot_module_init is renamed to
brainrot_module_init_v3 for the same reason -- StdrotAPI/StdrotEntry kept
their v2 layouts, so a stale cooked module would have loaded silently and
then been called at the wrong argument width. It now fails dlsym loudly.
Appendix B Q7 (resolved)
Generator output is derived and stays out of the repo, as AGENTS.md
already implies; a vendored, pinned raylib_api.json is a committed source
input, not generated output. raylib does not become a build dependency of
anything `make test` touches.
Tests: 436 pass (+9). New tests/nativemodules/structnative.c proves layout
agreement against _Static_assert'd C structs, per-argument copy
independence, non-mutation of the caller, interior offsets, and every
rejection path. Full valgrind sweep clean (409/409, zero leaks).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Road B. raylib has 617 functions; hand-writing that many StdrotValue
wrappers is how this project dies, so brainray/brainray_gen.py emits them
from raylib's own published API description instead.
raylib_api.json -> brainray-gen -> { raylibgen_native.c adapters
raylibgen.brainrot gang + gyatt
raylibgen_abi_check.c layout asserts }
Output: 378/617 functions, 16/35 struct types, 305 constants. Demonstrated
by examples/raylib/ohio_engine_gen.brainrot, which runs a real game loop
passing gang Vector2/Color/Rectangle BY VALUE into raylib's own
DrawCircleV(Vector2, float, Color) -- the non-handle-hack demo the phase
DoD asked for. Road A's hand-written module is untouched and still works;
this is a separate module (`#cooked <raylibgen>`).
No new ABI was needed for types or constants
A module name resolves a "<name>.brainrot" prelude BEFORE a "<name>.so",
and a prelude may itself #cooked a native module. So the generator emits
types and constants as ordinary Brainrot source and only functions go
through the C ABI. Phase 4's deferral of type/constant registration in
StdrotAPI (#207) turned out to cost nothing.
Layout correctness is testable, so it is tested three ways
generator model <-> real raylib headers generated _Static_assert TU;
compiling it IS the check
generator model <-> compute_struct_layout tests/test_brainray_gen.py runs
the interpreter, compares maxxing()
generator model <-> known raylib layouts hardcoded, so a generator bug
cannot agree with itself
make test stays raylib-free
`brainray-gen-sources` (generate) needs only Python and the pinned JSON;
only `brainray-gen` (compile) needs raylib, and neither is a prerequisite
of all/test/valgrind/install/wasm. Verified by building with a pkg-config
that denies raylib. Generated C is excluded from format-check, as lang.tab.c
already is. 31 new tests, none requiring raylib; 467 pass.
Skips are counted, reported, and --strict'd
113 struct returns (Appendix B Q6 -- the dominant coverage cost, worth more
than the other five categories combined), 102 handle-like struct params, 19
pointer/array structs, 14 cstring returns, 7 callbacks, 2 varargs. --strict
fails on any UNEXPECTED reason, so an upstream schema change is a red build
rather than a quietly smaller binding.
Two upstream/build issues found along the way
* raylib's own 6.0 tag ships a raylib_api.json that is both invalid JSON
(unescaped quotes in a description) and stale relative to its own
header. Vendored master@f3a471f instead, pinned by commit SHA; the
narrow description repair is kept in load_api() so the committed copy
stays byte-identical to upstream, and is unit-tested.
* No Makefile rule declared stdrot/stdrot_api.h as a prerequisite, so
last commit's ABI bump left a stale brainray/raylib.so that `make`
considered up to date. Added to every rule that compiles against it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clang-tidy's bugprone-branch-clone was right: `case STDROT_STRUCT:` returned the same `false` as `default:` immediately below it, so the two were a genuine branch clone. The distinction was only ever documentary -- "no coercion applies" is the whole of STDROT_STRUCT's behavior here, since nothing converts INTO a by-value aggregate and a struct argument already returns at the top of the function without reaching the switch at all. Merged the explanation into the default case's comment rather than suppressing the check. Caught by CI's static-analysis job, which runs clang-tidy as well as cppcheck -- `make tidy`, and it does run locally (unlike `make cppcheck`, which needs a newer cppcheck than this machine has). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leo-aa88
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
I built this branch, ran the suite (466 pass; the one failure is a stale
/usr/local/bin/brainrot on my box, not this patch), generated the binding,
compiled it against real raylib, ran the ABI-check TU, and drove
rl_color_to_int/rl_check_collision_recs/rl_check_collision_point_rec with
by-value gang Color/Rectangle/Vector2 end to end. It works. The ABI v3
reasoning — in particular renaming brainrot_module_init to _v3 because
StdrotAPI/StdrotEntry did not change while StdrotValue's width did —
is the correct call and the comment explaining it is the best thing in this
diff. Rejecting struct returns instead of half-marshalling them is also right.
That's the praise. Now the part that blocks.
CI is green. This is not a failing-test problem. The current tests do not
exercise these contracts.
1. BLOCKING — an array of structs is silently accepted as one struct
gang Color pal[4] passed where a Color is declared compiles clean, runs
clean, and hands the native pal[0]. Through the generated raylib binding:
#cooked <raylibgen>
skibidi main {
gang Color pal[4];
pal[0].r = 255; pal[0].g = 0; pal[0].b = 255; pal[0].a = 255;
yapping("%d", rl_color_to_int(pal)); 🚽 pal is an ARRAY
bussin 0;
}-16711681
exit=0
No diagnostic. Not at analysis time, not at the runtime boundary. Compare the
same mistake with any other type:
Error: 'tripled' argument 1: int arrays cannot be passed where a scalar/string
is expected at line 5
You already wrote the guard. It's in ast_expr_to_stdrot_value()'s new
VAR_STRUCT case, with a comment that says, verbatim, "the native would
silently receive element 0 of something the caller wrote as an array ...
leaving out->type at STDROT_NONE here makes it a loud runtime failure too,
rather than a quiet half-read." You put it on the path that a declared
STDROT_STRUCT parameter never takes. See findings 1 and 2 inline.
2. MAJOR — two marshalling paths for one job, and I can't reach the second one
ast_expr_to_stdrot_value()'s VAR_STRUCT case plus the ~35-line copy block
in execute_native_call() justify themselves as covering "a struct that
reaches an unchecked variadic/legacy tail." is_unmarshallable_expr()
statically rejects exactly that. Both routes I tried die in the analyzer.
Details inline.
3. MAJOR — "each pair is checked" is stronger than what's implemented
docs/brainray.md claims three-way layout agreement with each pair verified.
The generator↔Brainrot pair compares maxxing() — total size — and nothing
else. Interior offsets, which are the entire premise of "memcpy these bytes
into a real C struct," are never compared between the generator's model and
compute_struct_layout(). RayCollision is a concrete shape where a
size-only check is blind. Inline.
4/5. MINOR
An ownership claim in the ABI header that one of its two producers doesn't
satisfy, and a doc line naming the wrong make target. Inline.
VERDICT
The abstraction is sound and the ABI is honest about what it can't do. The
defect is that the type system has a hole at exactly the boundary this PR
introduces, and the hole exists because the invariant got implemented on the
dead path instead of the live one — which is also why no test caught it. Fix
finding 1 (one line in the analyzer, or one line in
resolve_by_value_struct_source()'s identifier arm, which closes it for
Brainrot-defined struct parameters too), resolve finding 2 one way or the
other, and soften or strengthen finding 3's claim. Then this merges.
| add_semantic_error(analyzer, SEMANTIC_ERROR_TYPE_MISMATCH, | ||
| STRING_LITERAL(error_msg), line); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
BLOCKING. This continue is why gang Color pal[4] is accepted where a Color is declared.
Every other parameter kind in this function reaches is_unmarshallable_array_arg() — the STDROT_ANY branch calls it at line ~1973, the generic scalar branch at ~2016. The new STDROT_STRUCT branch returns before either. And infer_expression_type() on an array identifier reports its element type, so actual_type == VAR_STRUCT and actual_pl == 0 both hold for pal: the check above passes, the tag check below passes (the element tag is Color), and the call is approved.
The runtime doesn't catch it either. marshal_struct_argument() delegates to resolve_by_value_struct_source(), whose NODE_IDENTIFIER arm checks desc.type != VAR_STRUCT and pointer_level > 0 but never desc.is_array, so it hands back src->value.array_data — the base of the array — and total_size bytes get copied out of element 0.
Verified against the generated binding:
#cooked <raylibgen>
skibidi main {
gang Color pal[4];
pal[0].r = 255; pal[0].g = 0; pal[0].b = 255; pal[0].a = 255;
yapping("%d", rl_color_to_int(pal));
bussin 0;
}prints -16711681, exit 0. The user wrote an array; raylib got element 0; nobody said anything.
rizz a[2] handed to tripled gets a clean "int arrays cannot be passed where a scalar/string is expected." A gang array gets silence. is_unmarshallable_array_arg() already returns true here (sym->is_array && sym->type != VAR_CHAR), so the static fix is one call before the tag check.
The better fix is in resolve_by_value_struct_source()'s identifier arm, which rejects pointer_level > 0 with a comment about not doing implicit dereferences but says nothing about arrays — it has the same hole for Brainrot-defined struct parameters (len2(pts) also silently passes pts[0]). One is_array rejection there closes both, and this branch keeps the static guard so the diagnostic arrives at compile time like it does for every other type.
| out->type = STDROT_INT; | ||
| out->val.i = var->value.ivalue; | ||
| return; | ||
| case VAR_STRUCT: |
There was a problem hiding this comment.
MAJOR. This is the second implementation of "marshal a struct argument," and I can't find a program that reaches it.
Its stated purpose — repeated at the copy block in execute_native_call() — is a struct that lands in an unchecked variadic/legacy tail. But semantic_check_native_call()'s tail loop runs is_unmarshallable_expr() on every tail argument, and that function's switch sends VAR_STRUCT to default: return true. Both shapes I tried:
Error: 'yapping' argument 2: struct has no supported native ABI representation
for yapping("%d", v) and for yapping("%d", b.pos). A declared STDROT_STRUCT parameter takes marshal_struct_argument() and continues before this ever runs; a declared non-struct parameter fails enforce_arg_type(); STDROT_ANY rejects VAR_STRUCT explicitly. That exhausts the routes I can see.
Two consequences, and the second is the one that stings:
- Duplicated machinery. Two functions that both know how to turn an expression into a
.val.blob, with different source-expression coverage (this one handles only a bare identifier;marshal_struct_argument()handles member access, array element, and call result) and different failure modes. - The
is_arrayguard you wrote here is the one the live path is missing. See my comment onsemantic_analyzer.c. The invariant landed on the unreachable path.
Also: is_unmarshallable_expr()'s own comment (semantic_analyzer.c, ~1482, outside this diff) still asserts "ast_expr_to_stdrot_value() has no VAR_STRUCT branch in its NODE_IDENTIFIER switch, so out->type survives at its initialized STDROT_NONE" — this commit added exactly that branch. Its conclusion is still right; its stated reason is now false, and in this codebase these comments are load-bearing.
Either show the call shape that reaches this (and add the test — it would be the only coverage), or delete both this case and the copy block below and let is_unmarshallable_expr() own the rule.
| and it is fatal rather than silently passing the borrowed | ||
| pointer through: continuing would hand the native the caller's | ||
| own storage under a contract that promises a copy. */ | ||
| if (arg_values[arg_count].type == STDROT_STRUCT) |
There was a problem hiding this comment.
Same finding as the VAR_STRUCT case above — this is the other half of the apparently-dead second path.
The comment argues it must live here rather than inside the typed branch "so it covers a struct that reaches an unchecked variadic/legacy tail too: such an argument has no StdrotParam to coerce against, but it is exactly as capable of letting a native mutate (or outlive) the caller's variable, and 'the tail is unchecked' is a statement about types, not about memory safety."
The reasoning is good. The premise isn't: is_unmarshallable_expr() rejects a struct in that tail before it ever gets here. If that's wrong, the shape that proves it belongs in test_brainrot.py.
| f"{sorted({n for n in names if names.count(n) > 1})}") | ||
|
|
||
|
|
||
| def test_generated_gang_layouts_match_brainrot(generated, tmp_path): |
There was a problem hiding this comment.
MAJOR. This compares maxxing() — total size — and nothing else. The module docstring above says "the three-way agreement (generator model / Brainrot runtime / real C headers) is complete; each pair is checked somewhere", and docs/brainray.md repeats it as a table. It isn't complete.
The generated ABI-check TU asserts generator↔raylib on sizeof, _Alignof and every offsetof. This test asserts generator↔Brainrot on sizeof only. Nothing anywhere compares Brainrot's field offsets to either side — and interior offsets are the entire premise. BR_READ_STRUCT checks size, so it can't catch it either.
A size-only check is genuinely blind here, not just theoretically. RayCollision:
gang RayCollision {
cap hit; offset 0
chad distance; offset 4 <- 3 bytes of padding before it
gang Vector3 point; offset 8
gang Vector3 normal; offset 20
}; 32 bytes
If compute_struct_layout() did no interior padding at all, the fields would sit at 0/1/5/17 and the struct would end at 29 — rounded up to alignment 4, that is still 32 bytes. Identical maxxing(), every interior offset wrong, this test green, and DrawRay-shaped calls quietly reading garbage.
compute_struct_layout() demonstrably does pad correctly — tests/abi/struct_layout_abi_check.c and the Mixed fixture in structnative.c prove that for the general algorithm, which is why this is a claim/coverage gap and not a live bug. But the claim is that these types are checked pairwise, and they aren't.
Either extend the probe to prove offsets (assign a distinct value per field, read back through a fixture, or fold the 16 generated shapes into the existing tests/abi/ oracle), or drop "complete"/"each pair is checked" from the docstring and from docs/brainray.md.
| | Pair | Where | | ||
| | --- | --- | | ||
| | generator's model ↔ real raylib headers | `raylibgen_abi_check.c` — `_Static_assert` on `sizeof`, `_Alignof`, and every `offsetof`. Building it *is* the check. | | ||
| | generator's model ↔ Brainrot's `compute_struct_layout()` | `tests/test_brainray_gen.py` — runs the interpreter, compares `maxxing()` per type. Needs no raylib. | |
There was a problem hiding this comment.
This row overstates what the test does — it compares maxxing(), i.e. sizeof, per type. Field offsets are compared only in the raylib-side row above. See my comment on tests/test_brainray_gen.py: RayCollision is a shape where dropping all interior padding still lands on 32 bytes, so this row's check cannot see an offset divergence.
Either the test grows offsets or this table stops implying it has them.
| size alone is NOT a type check (`gang Vector2 {chad x, y;}` | ||
| and `gang Size {chad w, h;}` are both 8 bytes and would | ||
| otherwise be silently interchangeable). */ | ||
| const char *type_name; |
There was a problem hiding this comment.
MINOR, but it's an ownership claim in an ABI header, which is the one place they have to be exact.
"Borrowed for the duration of the call (it points into the caller's StructDef, which outlives every call)" is true of exactly one of the two producers. marshal_struct_argument() deliberately sets out->val.blob.type_name = def->name.data and says why in a comment. ast_expr_to_stdrot_value()'s VAR_STRUCT case sets it to var->desc.struct_name.data — a live Variable's descriptor, not the StructDef.
Both happen to survive today (the caller's variable is in scope for the duration of its own call). But a native author reading this header is told the pointer's lifetime is anchored to the type registry, and on one path it's anchored to a stack-scoped variable's descriptor instead. If the second path is dead (see my other comment), delete it and the claim becomes true; if it isn't, either make it use def->name.data too or weaken the sentence.
| reproducible from a clean checkout and reviewable as a diff when the | ||
| pinned version moves. | ||
| - **The C adapters the generator emits are derived, and are not | ||
| committed.** They are `lang.tab.c` by another name: produced into the |
There was a problem hiding this comment.
"produced into the build tree by the brainray target only" — make brainray builds Road A's hand-written raylib.so and never runs the generator. The targets that produce these files are brainray-gen-sources and brainray-gen.
Small, but this paragraph is the resolution of an Appendix B question and will be cited later as the rule.
Addresses PR #307 review. All five findings; the reviewer was right on every one, including that CI being green was itself the problem -- the contracts had no coverage. 1. BLOCKING -- an array of structs was silently accepted as one struct `gang Color pal[4]` passed where a `Color` is declared compiled clean, ran clean, and handed the native pal[0]. No diagnostic at analysis time or at the runtime boundary, while `rizz a[2]` in the same position had always produced a clean "int arrays cannot be passed where a scalar/string is expected". infer_expression_type() reports an array identifier's ELEMENT type, so the type check, the pointer-level check and the tag check all passed. Fixed in two places, as the review suggested: * resolve_by_value_struct_source() (ast.c) now rejects an is_array identifier. That is the single choke point every by-value struct source goes through, so it closes the same hole for Brainrot-defined struct parameters (`len2(pts)` also silently passed pts[0]), struct returns, and struct copy-initializers. * semantic_check_native_call()'s STDROT_STRUCT branch now calls is_unmarshallable_array_arg(), which every other parameter kind has called all along, so a native call reports it at ANALYSIS time like every other type. The array-ELEMENT form (`arr[0]`) keeps working; tested both ways. 2. MAJOR -- two marshalling paths, the second unreachable Confirmed dead: is_unmarshallable_expr() sends VAR_STRUCT to `default: return true`, so every route to an unchecked variadic/legacy tail (`yapping("%d", v)`, a legacy STDROT_EXPORT export, a member access) is rejected before it can arrive. Deleted ast_expr_to_stdrot_value()'s VAR_STRUCT case and the copy block in execute_native_call() -- 70 lines -- leaving marshal_struct_argument() as the single producer and is_unmarshallable_expr() owning the tail rule. This is also why finding 1 existed: the is_array guard had been written, with a comment explaining exactly the bug, on the path nothing reaches. Recorded that in is_unmarshallable_expr()'s own comment, whose stale claim ("ast_expr_to_stdrot_value() has no VAR_STRUCT branch") the deletion makes true again -- now with the reason it must stay true. 3. MAJOR -- "each pair is checked" overstated what was implemented The generator<->Brainrot test compared maxxing() only. Verified the reviewer's RayCollision example: with no interior padding at all its fields sit at 0/1/5/17 and it STILL totals 32 bytes, so that test passes on a layout with every offset wrong. Strengthened rather than softened: added a prefix-probe test that compares interior padding at every field boundary. A trailing 1-byte sentinel is what makes it work -- without one the probe is masked by the same round-up-to-max-alignment that hides the original problem (my first attempt had exactly that bug). Mutation-verified: a generator with interior padding removed now fails with PfxRayCollision2: (8, 12), while the size-only test still passes. Docstring and docs/brainray.md now state precisely how strong each edge is -- the raylib-side check is exact offsetof; this one is indirect, because Brainrot cannot observe a field's address. 4. MINOR -- ownership claim in the ABI header blob.type_name is documented as anchored to the StructDef. Deleting the second producer (finding 2) makes that true of all remaining producers; the header now says so and warns any future one to match. 5. MINOR -- ROADMAP named `brainray`, which never runs the generator. Corrected to brainray-gen-sources/brainray-gen. Tests: 471 pass (+4). New coverage for the native path (rejection and the still-valid element form), a test_cases fixture for the Brainrot-defined path, and the interior-padding probe. Full valgrind sweep 410/410 clean. clang-tidy and format-check clean. Verified the reviewer's exact repro through the generated raylib binding: it now errors, and pal[0] still returns -16711681. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leo-aa88
left a comment
There was a problem hiding this comment.
APPROVE
All five findings resolved, and I verified every one independently rather than
taking the commit message for it. Building on top of these abstractions now
gets you what they promise.
Verification
Finding 1 — closed, on the surface that matters. My exact round-one repro
through the generated binding:
Error: 'rl_color_to_int' argument 1: struct arrays cannot be passed where a
by-value struct 'Color' is expected at line 8
exit=1
and rl_color_to_int(pal[0]) still returns -16711681. Also checked the
shapes you didn't list: a 2-D struct array (grid) is caught statically; a
struct-array field inside a gang, a global struct array, and an array
parameter are all syntax errors in the grammar, so there is no residual
shape that reaches the check with find_symbol blind to it. Fixing it at
resolve_by_value_struct_source() rather than only in the analyzer was the
right call — gang Vec2 a = pts; and bussin pts; now report too, and
neither goes anywhere near semantic_check_native_call().
Finding 2 — the deletion is safe, and load-bearing on something that's
actually pinned. I checked the thing that worried me: after removing the
second producer, what stops a struct from reaching a native as
STDROT_NONE? Answer, in order — is_unmarshallable_expr() rejects it
statically, test_cases/semantic_error_native_struct_variadic_tail has pinned
exactly that rejection since before this PR, and stdrot.c:2049 rejects a
STDROT_NONE tail argument at runtime as a backstop. Three layers, and the
middle one means someone can't quietly weaken the invariant the deletion rests
on. The comment you left in is_unmarshallable_expr() — "first write the
program that reaches it" — is the right artifact to leave behind.
Full marshalling sweep after the 70-line removal (variable, array element,
bare call, member-of-call, two-struct call, mutation-through-copy): all
correct, ASan/LSan clean.
Finding 3 — mutation-tested it myself, and it holds. Removed interior
alignment from _try_layout() and ran both tests:
test_generated_gang_layouts_match_brainrot PASSED <- the old, blind one
test_generated_gang_field_offsets_match_brainrot FAILED
{'PfxRayCollision2': (8, 12), 'PfxRayCollision3': (20, 24)}
So the size-only test really was blind to a layout with every offset wrong,
and the prefix probe really does catch it. The trailing sentinel is doing the
work you say it is — without it both the packed and padded prefixes round to
8 and the probe sees nothing. You strengthened the test instead of weakening
the claim, and the docstring's new hedge ("strong but indirect… not to
literally every conceivable byte-level divergence") is accurate: the probe's
resolution is bounded by each prefix's max alignment, which is exactly what
"indirect" buys you. Stating that limit rather than glossing it is the
difference between a test and a claim about a test.
Findings 4 and 5 — the header's lifetime sentence now matches reality
(both arms of marshal_struct_argument() set the tag from def->name.data /
rdef->name.data, and there is no second producer left to drift), and the
ROADMAP names the targets that actually run the generator.
Suite: 470 pass, 1 deselected (the stale /usr/local/bin/brainrot on this
machine, same as last round). make format-check clean, make tidy clean.
make brainray-gen still compiles and the ABI check passes 16/16. I did not
re-run the valgrind sweep independently — the ASan build's LSan was clean
across every struct path I exercised, which is evidence, not the same thing.
One non-blocking note
Inline on the new fixture: it pins the diagnostic but not what happens after
it. Not a merge blocker.
Two things that are not this PR's problem, but are somebody's
- User-defined calls don't check array arguments at all.
rizz a[2]; twice(a)prints1984— the array's backing pointer read as an int, the
exact union-aliasing bugis_unmarshallable_array_arg()'s own comment
describes. That helper is only wired intosemantic_check_native_call().
Worth noting that this PR leaves structs as the best-behaved case in that
hole, not the worst: they now at least report. Deserves its own issue. - Any parse failure leaks the lexer's token buffer (48 bytes via
copy_bytes,lang.l:439). Identical onmain, so pre-existing — but
AGENTS.mdsays leaks are blockers, and this one is trivially reproducible
withgang Bag { gang Vec2 pts[2]; };.
VERDICT
The interesting thing about this round is that the fix for finding 1 and the
fix for finding 2 were the same fix. The invariant had been written on a path
nothing could reach; deleting that path and moving the invariant to the choke
point every by-value struct source already goes through is the root-cause
version, not the patch version — and it closed three call sites nobody had
asked about. Finding 3 got strengthened rather than argued down, with the
mutation to prove it.
Ship it.
| gang Point pts[2]; | ||
| pts[0].x = 3.0; | ||
| pts[0].y = 4.0; | ||
| len2(pts); |
There was a problem hiding this comment.
MINOR, non-blocking. This fixture pins that the diagnostic appears. It doesn't pin anything about what happens next, and two independent things conspire to hide that:
len2(pts)'s result is discarded, so the value the failed call yields never reaches stdout.test_brainrot_examples()skips the exit-code assertion entirely when the expected output contains"Error:".
What actually happens:
before
Error: Expected a by-value struct/union value, got an array (index it, e.g. `arr[0]`) at line 13
0.0
after
exit=0
The program carries on, the call evaluates to 0.0, and the process reports success. Same for gang Vec2 a = pts; and bussin pts; — error, zeros, exit 0.
I'm not asking you to change that. It's the pre-existing convention for this whole family of runtime yyerror diagnostics (len2(*p) behaved identically on main before this PR), and the native path — the one this PR owns — correctly rejects at analysis time and exits 1. But ask the adversarial question about your own test: what wrong implementation still passes this fixture? One that prints the error and then binds garbage. Which is what it does.
Assigning the result would cost one line and turn the fixture into a statement about the contract rather than about the message:
yapping("%.1f", len2(pts));Two smaller things while you're in here:
expected_results.jsonbakes inat line 31— the closing brace, not thelen2(pts)on line 29. Also pre-existing (this family of runtime errors reports wherever the lexer's line counter finished, somainmisattributes the same way), but a fixture is where a wrong line number becomes expected behavior. Worth a comment saying it's known-wrong, so the next person doesn't read it as correct.- The fixture's own header comment says "The same rejection covers the native-call path… which additionally reports it at analysis time." True and useful — it's the one place the asymmetry is written down. Worth adding why the two paths differ in severity, since that's the part a reader will wonder about.
…ture Addresses the non-blocking note on PR #307's round-2 review. The fixture proved the diagnostic appears and nothing else. Two things conspired to hide what happened next: `len2(pts)`'s result was discarded, so the value the failed call yields never reached stdout, and test_brainrot_examples() skips the exit-code assertion whenever the expected output contains "Error:". The reviewer's adversarial question -- what wrong implementation still passes this fixture? -- has a concrete answer: one that prints the error and then binds pts[0] anyway. Now it prints the result, so the fixture states the contract. Verified by mutation: an implementation that reports the error and carries on binding element 0 prints 25.0 where the correct one prints 0.0, and the fixture fails. It would have passed before. This is the first fixture to use the harness's `Stderr:` form, which existed but nothing exercised -- needed because stdout is no longer empty, and the harness otherwise compares stdout alone. Also documents two known-wrong behaviors it necessarily bakes in, since a fixture is where wrong behavior becomes *expected* behavior and neither should be read as correct: * the reported line is main's closing brace, not the offending call -- this family of runtime yyerror() diagnostics reports wherever the lexer's line counter finished, and `len2(*p)` misattributes the same way on main; * execution continues after the error and the process exits 0. Both predate this PR. Added the explanation the reviewer asked for of why the native path is stricter (analysis-time, exit 1): a native call has a StdrotParam naming the expected tag, so the mismatch is knowable before anything runs -- and refusing to run is the only safe answer when the alternative is memcpy'ing bytes into a C struct inside someone else's library. A Brainrot-defined callee has no descriptor at the call site, so the check happens mid-evaluation, where this family's convention is to report and carry on. No C changed; 471 pass, format-check and tidy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leo-aa88
left a comment
There was a problem hiding this comment.
APPROVE
Test-only commit, and it does exactly what it claims. I mutation-tested it
rather than believing the commit message.
The fixture now catches the regression it's supposed to. Patched
resolve_by_value_struct_source() to report the error and then carry on
binding element 0 — the concrete "wrong implementation that still passes"
from the last round:
Actual:
25.0
Stderr:
Error: Expected a by-value struct/union value, got an array ... at line 63
assert '25.0\nStderr...' == '0.0\nStderr:...'
Fails, and would have passed against the old len2(pts); form. That's the
whole point of the change, verified independently.
Line attribution checks out too — the call is on 61, the closing brace on 63,
and the expectation says 63, which is what the comment documents as
known-wrong. And "first fixture to use the harness's Stderr: form" is
accurate: it's the only occurrence in expected_results.json. Nice to see
harness code that existed for nobody finally have a user.
470 pass + 1 deselected (the stale system install, unchanged), format-check
clean, and the diff genuinely touches no C.
One MINOR inline: a sentence in the new comment states the wrong mechanism for
the behavior the fixture pins. One line to fix, and worth fixing precisely
because this PR's entire review history has been about comments that describe
something the runtime doesn't do.
VERDICT
The right response to "your test pins the message, not the contract" is to
pin the contract and then prove the pin holds by breaking the implementation
on purpose. That's what happened. The three "known-wrong things this fixture
deliberately bakes in" section is the part worth keeping as a habit — a
fixture is where wrong behavior becomes expected behavior, and writing down
which parts are wrong is how the next person avoids reading them as a spec.
Ship it.
| 🚽 only the diagnostic would leave an implementation that reports the | ||
| 🚽 error and then binds garbage passing this fixture -- the failed call | ||
| 🚽 has to yield something, and what it yields is part of the contract | ||
| 🚽 (PR #307 review, round 2). The parameter is left zeroed, so this |
There was a problem hiding this comment.
MINOR. "The parameter is left zeroed" isn't what happens, and the difference matters for how someone reads this fixture.
There is no parameter. resolve_by_value_struct_source() returns false, enter_function_scope() bails, and the call is abandoned before the callee is ever entered — the 0.0 is the value of an abandoned call expression, not a struct parameter that got bound to zeroes. Instrumenting the callee shows it plainly:
chad len2(gang Point p) {
yapping("body ran; p.x=%.1f p.y=%.1f", p.x, p.y);
bussin p.x * p.x + p.y * p.y;
}Error: Expected a by-value struct/union value, got an array (index it, e.g. `arr[0]`) at line 12
0.0
exit=0
body ran never prints.
As written, a reader concludes the defined behavior of a rejected struct argument is "the callee runs with a zeroed struct." It isn't — the callee doesn't run at all, which is the stronger and more defensible guarantee, and the one this fixture is actually pinning. Say that instead:
The call is abandoned before the callee is entered — its body never runs — so the expression yields 0.0.
Everything downstream of the sentence is correct: 25.0 really is what a report-then-bind regression prints, and the fixture really does catch it (I broke resolve_by_value_struct_source() on purpose to confirm).
While you're here: known-wrong item #2 says the process exits 0, and that half is documented but not asserted — test_brainrot_examples() skips the exit-code check whenever the expected output contains "Error:". The continues half is genuinely pinned now, since an implementation that aborted at the error would leave stdout empty and fail the comparison. Only the exit code itself rides on the comment. Not worth contorting the fixture over — the harness's ExitCode: form returns early and can't be combined with an output comparison — but the comment currently reads as if both halves are locked in, and one is.
`rizz a[2]; twice(a)` printed 1984 -- the array's own heap address reinterpreted as an int -- with no diagnostic anywhere. The same mistake against a native has always been reported cleanly. enter_function_scope() (ast.c) binds a scalar parameter from arg_values[i].ivalue/.fvalue/.bvalue, and Variable's value union aliases those members with array_data, so the callee received the backing POINTER as its declared type. The value changes between builds (1984 here, 2080 on 463958a), which is what gives it away as an address. For a `cap` parameter it is undefined behavior outright, not merely a wrong number: UBSan reports "load of value 144, which is not a valid value for type '_Bool'". is_unmarshallable_array_arg() already detected this shape and was simply never wired into the user-defined call path -- it is only consulted by semantic_check_native_call(). The fix reports it at the same layer a native call does, in semantic_visit_function_call()'s existing per-argument loop, alongside the struct-pointer tag check PR #248 added there for the same "this path checks almost nothing" reason. It needs its OWN helper rather than reusing that one. is_unmarshallable_ array_arg() deliberately exempts VAR_CHAR arrays, because `yap buf[32]` IS marshallable for a native (ast_expr_to_stdrot_value() produces a STDROT_STRING, coerce_arg_to_param() converts to STDROT_CSTRING). A Brainrot-defined callee has no such conversion -- enter_function_scope() rejects VAR_STRING parameters outright and its VAR_CHAR case reads .ivalue like every other scalar -- so `yap ca[4]` to a `yap` parameter was broken exactly like the rest, and sharing the helper would have let precisely that case through. No legitimate use is affected: a parameter can never be an array (`rizz sum(rizz a[2])` is a syntax error, #194) and array-to-pointer decay is not implemented (`first(a)` for `rizz *p` already reports "Expression is not a pointer"), so no shape that used to work now fails. Verified `yap[N]` still reaches natives, and `arr[0]`/`&arr[0]` still work. Struct arrays are now caught statically too, where #307 caught them at runtime. Both layers stay: the runtime check in resolve_by_value_struct_source() still covers the by-value struct sources the analyzer's call-argument check does not see -- copy initialization and struct returns -- so struct_array_by_value_arg_fail.brainrot now pins the static rejection and a new struct_array_copy_init_fail.brainrot pins the runtime one. Neither layer subsumes the other, and both fixtures print the resulting value rather than discarding it, so a regression that reported the error and then bound the array anyway would still fail them. Tests: 473 pass (+2 fixtures, +1 rewritten). Full valgrind sweep 412/412 clean. clang-tidy and format-check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`rizz a[2]; twice(a)` printed 1984 -- the array's own heap address reinterpreted as an int -- with no diagnostic anywhere. The same mistake against a native has always been reported cleanly. enter_function_scope() (ast.c) binds a scalar parameter from arg_values[i].ivalue/.fvalue/.bvalue, and Variable's value union aliases those members with array_data, so the callee received the backing POINTER as its declared type. The value changes between builds (1984 here, 2080 on 463958a), which is what gives it away as an address. For a `cap` parameter it is undefined behavior outright, not merely a wrong number: UBSan reports "load of value 144, which is not a valid value for type '_Bool'". is_unmarshallable_array_arg() already detected this shape and was simply never wired into the user-defined call path -- it is only consulted by semantic_check_native_call(). The fix reports it at the same layer a native call does, in semantic_visit_function_call()'s existing per-argument loop, alongside the struct-pointer tag check PR #248 added there for the same "this path checks almost nothing" reason. It needs its OWN helper rather than reusing that one. is_unmarshallable_ array_arg() deliberately exempts VAR_CHAR arrays, because `yap buf[32]` IS marshallable for a native (ast_expr_to_stdrot_value() produces a STDROT_STRING, coerce_arg_to_param() converts to STDROT_CSTRING). A Brainrot-defined callee has no such conversion -- enter_function_scope() rejects VAR_STRING parameters outright and its VAR_CHAR case reads .ivalue like every other scalar -- so `yap ca[4]` to a `yap` parameter was broken exactly like the rest, and sharing the helper would have let precisely that case through. No legitimate use is affected: a parameter can never be an array (`rizz sum(rizz a[2])` is a syntax error, #194) and array-to-pointer decay is not implemented (`first(a)` for `rizz *p` already reports "Expression is not a pointer"), so no shape that used to work now fails. Verified `yap[N]` still reaches natives, and `arr[0]`/`&arr[0]` still work. Struct arrays are now caught statically too, where #307 caught them at runtime. Both layers stay: the runtime check in resolve_by_value_struct_source() still covers the by-value struct sources the analyzer's call-argument check does not see -- copy initialization and struct returns -- so struct_array_by_value_arg_fail.brainrot now pins the static rejection and a new struct_array_copy_init_fail.brainrot pins the runtime one. Neither layer subsumes the other, and both fixtures print the resulting value rather than discarding it, so a regression that reported the error and then bound the array anyway would still fail them. Tests: 473 pass (+2 fixtures, +1 rewritten). Full valgrind sweep 412/412 clean. clang-tidy and format-check clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iew) The grammar and layout work was right, but the polarity of the thing that CONSUMES the new syntax was inverted: len2(b.pts[0]) legitimate, one element -> REJECTED len2(b.pts) the whole array -> ACCEPTED, silently pts[0] and the same both ways through a native STDROT_STRUCT parameter, so array bytes went into a C library. Reproduced both before fixing. Both halves are in resolve_by_value_struct_source(), one arm apart. Half one, the missing guard. Its NODE_STRUCT_ACCESS arm checked fld->desc.type and pointer_level but not is_array. #307 added exactly that guard, with a comment describing exactly this failure, to the NODE_IDENTIFIER arm only -- because a struct-typed array field could not be declared at the time. This PR is what makes it declarable, so the arm beside it needed the same guard. #309's static check does not cover it either: array_identifier_symbol() is NODE_IDENTIFIER-only and `b.pts` is a NODE_STRUCT_ACCESS. Half two, the missing arm. `b.pts[0]` is a NODE_ARRAY_ACCESS with data.array.base set and data.array.name unset -- the field form -- so it fell past that arm's `name.data` requirement into the catch-all error. Added the base/name split, mirroring resolve_struct_access()'s equivalent branch through the same two helpers, so bounds checking and struct-aware striding are inherited rather than reimplemented. Why the fixture could not see it: it only ever read THROUGH the new syntax (`pool.es[i].x`) and never passed one of these things anywhere. So the honest answer to "what wrong implementation still passes this?" was "this one." It now passes an element (must work) and the whole array (must be rejected) -- the reviewer's two lines. Also verified by hand: returning an element, copy-initializing from one, and that the copy is independent of the array. Added a comment above resolve_by_value_struct_source() pointing at resolve_struct_access(), per the review's structural suggestion. These two dispatch on the same three node types for the same expression shapes and answer different questions, and every divergence between their lists has now been a bug -- #307, #309, and this. The note says there are two lists to update, and that a fixture which only reads a field out of a new shape will not notice the other. 476 pass. Full valgrind sweep 416/416 clean. format-check and tidy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iew) The grammar and layout work was right, but the polarity of the thing that CONSUMES the new syntax was inverted: len2(b.pts[0]) legitimate, one element -> REJECTED len2(b.pts) the whole array -> ACCEPTED, silently pts[0] and the same both ways through a native STDROT_STRUCT parameter, so array bytes went into a C library. Reproduced both before fixing. Both halves are in resolve_by_value_struct_source(), one arm apart. Half one, the missing guard. Its NODE_STRUCT_ACCESS arm checked fld->desc.type and pointer_level but not is_array. #307 added exactly that guard, with a comment describing exactly this failure, to the NODE_IDENTIFIER arm only -- because a struct-typed array field could not be declared at the time. This PR is what makes it declarable, so the arm beside it needed the same guard. #309's static check does not cover it either: array_identifier_symbol() is NODE_IDENTIFIER-only and `b.pts` is a NODE_STRUCT_ACCESS. Half two, the missing arm. `b.pts[0]` is a NODE_ARRAY_ACCESS with data.array.base set and data.array.name unset -- the field form -- so it fell past that arm's `name.data` requirement into the catch-all error. Added the base/name split, mirroring resolve_struct_access()'s equivalent branch through the same two helpers, so bounds checking and struct-aware striding are inherited rather than reimplemented. Why the fixture could not see it: it only ever read THROUGH the new syntax (`pool.es[i].x`) and never passed one of these things anywhere. So the honest answer to "what wrong implementation still passes this?" was "this one." It now passes an element (must work) and the whole array (must be rejected) -- the reviewer's two lines. Also verified by hand: returning an element, copy-initializing from one, and that the copy is independent of the array. Added a comment above resolve_by_value_struct_source() pointing at resolve_struct_access(), per the review's structural suggestion. These two dispatch on the same three node types for the same expression shapes and answer different questions, and every divergence between their lists has now been a bug -- #307, #309, and this. The note says there are two lists to update, and that a fixture which only reads a field out of a new shape will not notice the other. 476 pass. Full valgrind sweep 416/416 clean. format-check and tidy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Description
Phase 5 Road B: an ABI that can carry C aggregates, and a generator that
uses it to bind raylib from raylib's own published API description.
raylib has 617 functions. Hand-writing that many
StdrotValuewrappers is,in the issue's own words, how this project dies — so this PR builds the
pipeline instead, and raylib becomes its first client rather than a one-off
port.
378 of 617 functions, 16 of 35 struct types, 305 constants.
1.
STDROT_STRUCT— by-value aggregates across the boundary (ABI v3)A
gangnow crosses the native boundary as its C-ABI byte image, whichcompute_struct_layout()already produces. A parameter declared{STDROT_STRUCT, "Vector2", 0}receives it directly.write through its argument without touching the caller's variable — matching
the value-copy rule struct assignment/args/returns already follow (Appendix
B Q3), and reusing
STDROT_CSTRING's scratch pattern.gang Vector2 {chad x,y}andgang Size {chad w,h}are indistinguishable bysize. A descriptor with no
type_nameis rejected at load time.variable, nested member access, struct-returning call, member of a call
result — routed around
ast_expr_to_stdrot_value()so a call-shaped argumentisn't evaluated twice (the fix: a user-defined function call must run exactly once #303 bug class).
stdrot_struct("Texture2D", &tex, sizeof(tex)), returns the address of alocal that dies with the call, and
STDROT_STRING's deep-copy trick can'trescue a dead stack frame. Gated on Appendix B Q6.
2. The generator
Demonstrated by
examples/raylib/ohio_engine_gen.brainrot, which runs a realgame loop passing
gang Vector2/Color/Rectangleby value into raylib'sown
DrawCircleV(Vector2, float, Color)— the non-handle-hack demo the DoDasked for. Road A's hand-written module is untouched and still works; this is a
separate module (
#cooked <raylibgen>).No new ABI was needed for types or constants. A module name resolves a
<name>.brainrotprelude before a<name>.so, and a prelude may itself#cookeda native module — so the generator emits types and constants asordinary Brainrot source and only functions go through the C ABI. Phase 4's
deferral of type/constant registration in
StdrotAPI(#207) turned out tocost nothing.
Layout correctness is testable, so it's tested — three ways, each pair
independently:
_Static_assert/offsetofTU; compiling it is the checkcompute_struct_layout()maxxing(); needs no raylibmake teststays raylib-free.brainray-gen-sources(generate) needs onlyPython and the pinned JSON; only
brainray-gen(compile) needs raylib, andneither is a prerequisite of
all/test/valgrind/install/wasm. Verifiedby building behind a
pkg-configthat denies raylib. Generated C is excludedfrom
format-check, exactly aslang.tab.calready is.Skips are counted, reported, and
--strict-enforced — so an upstreamschema change is a red build, not a quietly smaller binding:
Image/Font/Model/Sound/Shader— resources in disguisegangsconst char *returnsnpc, Phase 9b)TraceLog,TextFormat3. Appendix B Q7, resolved
The question conflated two artifacts with opposite answers. Generator output
is derived and stays out of the repo (the
AGENTS.mdrule extends tobindings); a vendored, pinned
raylib_api.jsonis a committed source input.raylib doesn't become a build dependency of anything
make testtouches.StdrotValuegrew 24 → 32 bytes andSTDROT_NONErenumbered, sostdrot_get_api_v2→_v3.brainrot_module_initis also renamed tobrainrot_module_init_v3—StdrotAPI/StdrotEntrykept their v2 layouts,so a stale cooked module would otherwise have loaded silently and then been
called at the wrong argument width. It now fails
dlsymloudly. Anyout-of-tree cooked module must be rebuilt.
Two problems found along the way
6.0tag ships a brokenraylib_api.json— invalid JSON(unescaped quotes in a
description) and stale relative to its own header(it describes
ImageDrawRectangleLines(Image*, Rectangle, int, Color); theshipped header has
(Image*, int, int, int, int, Color)). The C compilercaught it instantly, which is the "generated C is compile-time correct"
property earning its keep. Vendored
master@f3a471fpinned by commit SHAinstead; the narrow repair is kept in
load_api()so the committed copystays byte-identical to upstream, and is unit-tested.
stdrot/stdrot_api.has a prerequisite, so theABI bump left a stale
brainray/raylib.sothatmakereported as up todate. Pre-existing bug; now a prerequisite of every rule that compiles
against it.
Related Issue
Fixes #208
Type of Change
Checklist
make format-checklocally (ormake formatto fix)Verification notes:
makeclean under-Werror -fsanitize=address,undefined.main→ +9 ABI, +31 generator). None of the newtests require raylib.
plus targeted runs over every new struct-marshalling path. Run against a
non-sanitized build —
make valgrindis currently unusable on anASan-instrumented binary (every fixture dies in
__libc_freeres), which iswhat fix(ci): separate non-sanitized binary for valgrind and fix parse-error leaks #202 addresses and is not introduced here.
make cppcheckcould not be run locally — this machine has cppcheck 2.7and the Makefile requires ≥ 2.13, so CI's
static-analysisjob is the firstreal check.
CPPCHECK_SRCSdoesn't includebrainray/, so generated codeisn't scanned either way.
Follow-up
Appendix B Q6 (ownership of native resources) is now the highest-value
item for this binding by a wide margin: it gates struct returns, which alone
account for 113 of the 239 skipped functions.
🤖 Generated with Claude Code