feat(gang): support arrays of struct/union values and pointers - #287
Conversation
Make a struct/union tag a valid array element type, closing the "Arrays / pointers to structs" item of Phase 3 (#206, 3e). Previously `gang Point pts[3];` failed to parse: the declaration grammar had no `struct_or_union name_token declarator dimensions` production, and the interpreter's array-declaration path explicitly excluded VAR_STRUCT value arrays. This wires the feature end to end: - lang.y: new declaration production for `gang Tag name[dims]`, carrying the element tag on the node's struct_name (value arrays, pointer_level 0, and pointer arrays, e.g. `gang Point *ptrs[3]`). - interpreter.c: route struct value arrays through the array-declaration visitor so each element gets its own alignment-correct blob. - ast.c: size struct value-array elements by the tag's layout (def->total_size) in set_multi_array_variable and the name-based element-address path; resolve `pts[i].field` / `ptrs[i].field` in resolve_struct_access (following one pointer level for pointer arrays); accept an array element as a by-value struct in resolve_by_value_struct_source (copy-init, by-value arg, return). - semantic_analyzer.c: infer the element tag for a `pts[i].field` object in the static analysis paths. Member access composes with multi-dimensional arrays (`grid[r][c].y`) and nested struct fields (`lines[i].a.x`). Whole-struct element assignment (`pts[i] = c;`) and array brace initializers stay unsupported, matching the existing limitations for plain struct variables; documented as such. Adds test_cases fixtures (1D, 2D, nested, by-value, pointer array, and three error cases), expected outputs, an example program, and docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI's clang-tidy (clang-analyzer-core.UndefinedBinaryOperatorResult) flagged a garbage-value read at calculate_array_offset (ast.c:647), reachable from the new struct-array member-access and by-value resolvers: the analyzer could not prove the fill loop covers every index the offset calculation reads. Zero-initialize `indices[MAX_DIMENSIONS]` in both new sites, matching the guaranteed-defined contract the offset helper expects. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Stale comment
REQUEST CHANGES
The grammar-to-interpreter path for
gang Point pts[3];is real: member access, 2D indexing, nested fields, by-value copy/arg/return, and pointer-array implicit->all run. I built this tree (-Werror -fsanitize=address,undefined; bison-Wcounterexamplessilent) and executed the new fixtures plus the example. Those happy paths matchexpected_results.json.The descriptor still says a struct value is size 0. This PR papers over that at allocation and at one indexing site, then leaves
maxxingand the shared stride helper on the old lie. That is not a missing test for a green CI to paper over. I reproducedmaxxing(pts[0]) == 0with exit 0 whilemaxxing(p) == 8andmaxxing(pts) == 24on the same program.The type-level claim is also spelling-dependent:
gang Point pts[3];works;lit gang Point PointVal; PointVal arr[3];is still a parse error from a leftover ban this feature exists to retire.1.
get_type_size_for_descriptorstill reports 0 for a struct value; sizeof and the stride helper were not updatedBLOCKING.
THE CODE SAYS (docs, README, this PR): an array element is a by-value struct wherever a plain struct variable is.
THE RUNTIME DOES:
maxxing(p)is 8,maxxing(pts)is 24,maxxing(pts[0])is 0, process exits 0.
get_type_size_for_descriptor(VAR_STRUCT, 0, …)has no tag and returns 0.set_multi_array_variableandevaluate_multi_array_accessspecial-case around that.handle_sizeof'sNODE_ARRAY_ACCESSarm (not in this diff) still early-returns the descriptor size. The genericVAR_STRUCTsizeof path viaget_struct_def_for_expressionalready knows how to size a name-based array element and is never reached.
array_element_addressexists so stride cannot drift from layout. Indexing a value-struct array through it would hit "Cannot index an array of zero-sized elements" andexit. So this PR bypasses the helper and duplicates stride math in three places. That is not an ABI. That is a size function you have to remember to ignore.Fix the size function (or stop claiming descriptor-driven layout), then make sizeof and
array_element_addressconsume it. Add a fixture that assertsmaxxing(pts[i]) == maxxing(p).CI being green would not change this. The current tests never call
maxxingon an element.2. The tag is a valid array element type except when spelled through
litMAJOR.
maybe_reject_struct_alias_arraystill parse-failsPointVal arr[3];with "Arrays of struct/union typedef aliases are not supported". That guard exists because value-struct arrays were not a thing. This PR made them a thing for thegang Tag name[dims]spelling only.The alias production already copies
struct_nameonto the samecreate_multi_array_declaration_nodeshape the new production builds. The interpreter change (if (node->is_array)) would accept it. The leftover ban is what makes the type-level claim false.Either delete the reject and test
lit gang Point P; P arr[N]; arr[i].x, or stop saying a tag is a valid array element type.VERDICT
The defect is architectural:
VarTypestill cannot represent a tagged struct value's size, so this feature is a pile of special cases around a function that returns 0. Two call sites were patched.maxxingwas not. The alias spelling of the same type is still rejected by a comment that is now a lie.Implementation of the advertised happy path is not the problem. The abstractions this PR asks the next engineer to trust are.
Do not merge until sizeof of an element is the struct's layout size and the leftover alias ban is either removed or the public contract is narrowed to match the spellings that actually work.
Local note: flex/bison were missing in this environment at start; after installing them the tree compiles. I did not run full
make test/make valgrind(pytest needstests/libstdrot.sofrom the Makefile test recipe). GitHub CI was still pending when this review was written. None of that is why this is REQUEST CHANGES. The sizeof result is exit 0 with a wrong value.Sent by Cursor Automation: Code Reviewer
…s arrays Addresses the PR #287 review (both findings). 1. maxxing(pts[i]) was 0. get_type_size_for_descriptor() has no tag and returns 0 for a struct value, and handle_sizeof()'s NODE_ARRAY_ACCESS arm early-returned that 0 instead of reaching the generic VAR_STRUCT path (which already sizes a name-based struct-array element via get_struct_def_for_expression). Let struct-value elements fall through, so maxxing(pts[i]) == maxxing(p). Rather than special-case struct stride at each indexing site, element stride now has one authority: get_array_element_stride() returns the tag's layout size for a struct/union VALUE element and the descriptor width otherwise. array_element_address() consumes it, and all element sizing/addressing — set_multi_array_variable, evaluate_multi_array_ access, and the struct-array member/by-value resolvers — funnels through it, so storage size and addressing can never disagree. 2. `lit gang Point P; P arr[3];` was still a parse error from the leftover maybe_reject_struct_alias_array ban that existed only because value- struct arrays weren't a thing. Removed it; the alias production builds the same node shape the direct `gang Tag name[dims]` production does. A braced initializer for such an array stays rejected (brace init is out of scope, same as the direct spelling), now with an accurate message. Tests: array_of_structs_sizeof (maxxing element == plain var, array == element*len), lit_struct_alias_array (alias value array works), and lit_struct_alias_array_fail repurposed to assert the brace-init rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/cursor-review |
There was a problem hiding this comment.
APPROVE
1200d28 closed both first-look blockers with the fix the previous review asked for, not another pile of special cases. I rebuilt this tree (-Werror -fsanitize=address,undefined; bison -Wcounterexamples silent) and re-ran the previously failing programs plus the new fixtures.
maxxing(pts[0]) is 8, equal to maxxing(p), and maxxing(pts) == maxxing(pts[0]) * 3. lit gang Point PointVal; PointVal arr[3]; parses and runs. Element stride now has one writer.
First-look items
sizeof / descriptor 0. Closed. get_array_element_stride() is the tag-aware size for a struct/union VALUE element (def->total_size) and the descriptor width otherwise. Allocation, array_element_address, name-based indexing, resolve_struct_access, and resolve_by_value_struct_source all consume it. handle_sizeof's NODE_ARRAY_ACCESS arm no longer early-returns get_type_size_for_descriptor(...) for a value element; it falls through to get_struct_def_for_expression(), which already knew how to size a name-based array element. Fixture array_of_structs_sizeof asserts the contract. I also checked that maxxing(pts[i = i + 1]) does not evaluate the index (i stays 0), so the fall-through does not break sizeof's never-evaluate rule.
get_type_size_for_descriptor(VAR_STRUCT, 0, …) still returns 0. That function still has no tag parameter. The point of the last review was: stop routing this feature's layout through it. They did.
Alias spelling. Closed. maybe_reject_struct_alias_array is gone. The alias dimensions production already built the same node shape; the leftover yyerror was the only thing making the type-level claim spelling-dependent. Brace-init of a value-alias array is still rejected, now with a message that matches the documented limitation rather than pretending the array itself is illegal.
array_element_address bypass. Closed. The first-look special-case in evaluate_multi_array_access that walked around the helper (because the helper would exit on stride 0) is gone. The helper now asks the stride function that knows about tags.
What I re-probed and is not a merge issue
- Union arrays:
chungus U us[2];—maxxing(u) == maxxing(us[0]) == 4,maxxing(us) == 8, copy-init from an element works. No official fixture; the code path is the sameVAR_STRUCTtag. &pts[i]and 2D copy-init/arg/maxxing(grid[1][0])work.gang Point c = ptrs[0]is rejected (pointer element is not a by-value struct).pts[i] = cisUnsupported assignment type, same diagnostic and fail-open exit 0 asp = c. Documented limitation, not a new hole.- Field arrays (
gang Holder { gang Point pts[3]; }) still do not parse.lang.y's field production says aggregate array elements are out of scope. The PR added the production todeclaration, notstruct_field. That matches the writeup.
Partial indexing (grid[0].x on a 2D array) treats the row as the first element. That is the same calculate_array_offset leniency scalar rizz a[2][2]; a[0] already has. This PR did not invent it.
Leftovers (not the merge question)
semantic_analyzer.c still contains comments claiming the grammar has no array-of-structs-by-value declaration syntax. Those comments are now false. The logic they sit on is still fine: a value-array brace-init never reaches the analyzer (direct spelling has no = {…} production; the alias spelling sets typedef_had_error and parse-fails). Update the comments when someone next touches that function.
ArrayAccessElement still has no struct_name, so sizeof special-cases a fall-through instead of calling get_array_element_stride. Both currently read def->total_size. If a later change sizes a struct by something other than total_size, sizeof is the site that will drift. That is a seam, not a present disagreement.
VERDICT
The first-look defect was architectural: layout for a tagged struct value lived in special cases around a size function that returns 0, and maxxing plus the alias spelling were left on the old lie. 1200d28 gave stride a single tagged authority and stopped claiming a tag is a valid element type only when spelled gang Tag name[dims].
If the next engineer treats get_array_element_stride, the sizeof fall-through comment, and the docs' "element is a by-value struct" sentence as true, the runtime matches. Merge.
Local note: 11 new fixtures pass under ASan/UBSan; the example program runs; full language pytest is 393 passed (17 failures are this environment missing make nativemodules / badnatives, unrelated). Valgrind is not usable on the ASan binary. GitHub lint / static-analysis / build were already green on this HEAD.
Sent by Cursor Automation: Code Reviewer


Description
Makes a
gang/chungustag a valid array element type, so you can declare and use arrays whose elements are whole structs/unions.Before this,
gang Point pts[3];failed to parse — the declaration grammar had nostruct_or_union name_token declarator dimensionsproduction, and the interpreter's array path explicitly excluded non-pointerVAR_STRUCTarrays. This wires the feature end to end:lang.y— new declaration production forgang Tag name[dims], carrying the element tag on the node'sstruct_name. Covers value arrays (gang Point pts[3],gang Point grid[2][2]) and pointer arrays (gang Point *ptrs[3]). Unknown tags are rejected at parse time.interpreter.c— route struct value arrays through the array-declaration visitor so each element gets its own alignment-correct blob (sized by the tag's layout, not a scalar guess).ast.c— size struct value-array elements bydef->total_sizeinset_multi_array_variableand the name-based element-address path; resolvepts[i].field/ptrs[i].fieldinresolve_struct_access(following one pointer level for pointer arrays, same implicit-->rule as pointer variables/fields); accept an array element as a by-value struct inresolve_by_value_struct_source(copy-init, by-value arg, and return).semantic_analyzer.c— infer the element tag for apts[i].fieldobject in the static analysis paths.Member access composes with multi-dimensional arrays (
grid[r][c].y) and nested struct fields (lines[i].a.x). An element behaves as a by-value struct anywhere a plain struct variable does:Deliberately out of scope (documented as limitations, consistent with plain struct variables today): whole-struct assignment to an existing element (
pts[i] = c;, mirrors the unsupportedp = c;) and array brace initializers (gang Point pts[2] = {{1,2},{3,4}};). Declare-and-assign per field, or copy-initialize.Related Issue
No standalone issue exists for this. It implements the "Arrays / pointers to structs" checkbox of Phase 3, item 3e (#206, now closed) — part of the roadmap tracked in #214.
Type of Change
Checklist
make format-checklocally (ormake formatto fix)Testing
array_of_structs(1D + loop),_2d,_nested,_by_value(copy-init/arg/return),_pointers(pointer array), and three error cases (_unknown_type_fail,_oob_fail,_scalar_member_fail), each with anexpected_results.jsonentry.pytest: 411 passed (404 existing + 7 new). (make test'sabi-checklink step fails on my machine due to a local linuxbrewld/glibc mismatch, unrelated to this change; ran the interpreter build + pytest directly.)run_valgrind_tests.shover all 398test_cases/*.brainrot— no memory issues.examples/array_of_structs.brainrot(+examples/README.md§10).🤖 Generated with Claude Code