Static arrays with declared sizes - #75
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe compiler now supports fixed-size arrays with explicit lengths. Parsing, type checking, loop expansion, bounds validation, ABI metadata, witness handling, bindgen IR, documentation, and tests were updated for grouped source parameters and expanded stack elements. ChangesFixed-size array language and compiler
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Contract
participant Parser
participant Typechecker
participant Compiler
participant Artifact
Contract->>Parser: Declare fixed-size arrays
Parser->>Typechecker: Build array expressions and types
Typechecker->>Compiler: Validate lengths and indexes
Compiler->>Artifact: Preserve grouped ABI inputs
Compiler->>Artifact: Emit expanded stack placeholders
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Playground PreviewA live preview of this PR's playground is available at:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compiler/loops.rs (1)
172-641: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expression::ArrayLiteralis not handled in two full-expression-tree-walking passes, breaking loop unrolling and concat rewriting for array literal elements. Bothsubstitute_expressionandrewrite_expression_concatfall into their generic catch-all arms forArrayLiteral, so its child elements are never recursively processed by either pass.
src/compiler/loops.rs#L172-L641: add an explicitExpression::ArrayLiteral(elements) => Expression::ArrayLiteral(elements.iter().map(|e| substitute_expression(e, index_var, value_var, k, array_name)).collect())arm insubstitute_expression, so a local array declared inside aforloop body (e.g.,int[2] pair = [i, v];) has its loop variables correctly unrolled instead of failing compilation with"undefined binding 'i'".src/compiler/concat.rs#L145-L300: add an explicitExpression::ArrayLiteral(elements) => { let new_elements = elements.into_iter().map(|e| self.rewrite_expression_concat(e, scope).0).collect(); (Expression::ArrayLiteral(new_elements), <array type>) }arm inrewrite_expression_concat, so+between bytes-like operands inside an array literal element gets rewritten toExpression::Concatinstead of silently staying arithmetic and emittingOP_ADDinstead ofOP_CAT.As per path instructions, "Implement language features across the grammar, AST/models, parser, and compiler together; do not add expression or requirement variants without compiler emission and tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/loops.rs` around lines 172 - 641, Handle Expression::ArrayLiteral explicitly in src/compiler/loops.rs lines 172-641 within substitute_expression by recursively substituting every element and rebuilding the literal. Also update src/compiler/concat.rs lines 145-300 within rewrite_expression_concat to recursively rewrite each array-literal element, preserve the array type, and return the rebuilt Expression::ArrayLiteral. These changes must ensure loop variables and byte-like additions inside array elements are transformed by their respective passes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compiler/tapscript.rs`:
- Around line 439-441: Preserve per-element injected status when deriving and
expanding grouped witness arrays in the tapscript witness derivation flow around
WitnessElement and its leaf expansion logic. Carry injected element names or
indices through the artifact instead of applying one grouped injected value to
every expanded array entry, then assign each expanded element’s status
individually. Add a regression test covering a tapscript leaf with a mixed
signature array containing infrastructure and user signatures.
In `@src/typechecker/mod.rs`:
- Around line 512-520: The array-literal binding validation must reject
heterogeneous elements instead of inferring only from the first element. In
src/typechecker/mod.rs lines 512-520, update the Expression::ArrayLiteral
handling and in src/validator/mod.rs lines 602-666 add the corresponding
validation so every element’s resolved type is compared with the declared
element type, rejecting elements that are compatible but not equal; preserve the
existing behavior for homogeneous and empty arrays.
In `@tests/features/static_arrays.rs`:
- Around line 204-224: Add a companion test near
local_array_elements_are_assignable_at_a_literal_index that uses a larger local
array and assigns a literal non-zero index, such as weights[1] in an int[3]
array. Compile the contract, inspect spend’s assembly, and assert the assignment
value and overwrite sequence verify stack-depth handling beyond index 0.
---
Outside diff comments:
In `@src/compiler/loops.rs`:
- Around line 172-641: Handle Expression::ArrayLiteral explicitly in
src/compiler/loops.rs lines 172-641 within substitute_expression by recursively
substituting every element and rebuilding the literal. Also update
src/compiler/concat.rs lines 145-300 within rewrite_expression_concat to
recursively rewrite each array-literal element, preserve the array type, and
return the rebuilt Expression::ArrayLiteral. These changes must ensure loop
variables and byte-like additions inside array elements are transformed by their
respective passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db7686a0-26c4-41b0-b114-d867e3270957
📒 Files selected for processing (32)
README.mdarkade-bindgen/src/ir.rsarkade-bindgen/tests/ir_test.rsexamples/threshold_oracle/threshold_oracle.arkplayground/codegen.jssrc/compiler/concat.rssrc/compiler/expr.rssrc/compiler/loops.rssrc/compiler/mod.rssrc/compiler/tapscript.rssrc/lib.rssrc/models/mod.rssrc/parser/expr.rssrc/parser/grammar.pestsrc/parser/mod.rssrc/typechecker/mod.rssrc/validator/mod.rstests/e2e/contracts/static_arrays.arktests/e2e/contracts/symbolic_stack.arktests/e2e/static_arrays_test.gotests/e2e/utils_test.gotests/examples/threshold_oracle.rstests/features.rstests/features/asset_id_explicit.rstests/features/beacon.rstests/features/concat_op.rstests/features/contract_import_instantiation.rstests/features/general_comparisons.rstests/features/no_shadowing.rstests/features/opcode_functions.rstests/features/static_arrays.rstests/features/symbolic_stack.rs
💤 Files with no reviewable changes (1)
- src/lib.rs
| Expression::ArrayLiteral(elements) => ArkType::Array( | ||
| Box::new( | ||
| elements | ||
| .first() | ||
| .map(|element| infer_type(element, scope)) | ||
| .unwrap_or(ArkType::Unknown), | ||
| ), | ||
| elements.len(), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for array-literal element type validation.
rg -n -C10 'fn binding_types_compatible' src/validator/mod.rs
rg -n -C10 'fn resolved_expression_type' src/validator/mod.rsRepository: arkade-os/compiler
Length of output: 1859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)src/(typechecker|validator)/mod\.rs$|(^|/)src/models/|(^|/)src/' | head -200
echo
echo "== ark_type and array_type_parts definitions =="
rg -n -C8 'enum ArkType|struct ArkType|impl ArkType|fn array_type_parts|fn parse' src
echo
echo "== expression definitions =="
rg -n -C6 'enum Expression|ArrayLiteral|Binding' src
echo
echo "== relevant validator sections =="
sed -n '530,690p' src/validator/mod.rs
echo
echo "== relevant typechecker array literal section =="
sed -n '480,540p' src/typechecker/mod.rs
echo
echo "== tests mentioning array literals / ArrayLiteral =="
rg -n 'ArrayLiteral|\[.*Int|true.*false|int\[|Array\(' src tests | head -200 || trueRepository: arkade-os/compiler
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== typechecker outline =="
ast-grep outline src/typechecker/mod.rs --view compact || true
echo
echo "== relevant typechecker sections =="
sed -n '430,545p' src/typechecker/mod.rs
rg -n -C8 'pub enum ArkType|pub enum Expression|ArrayLiteral|fn infer_type' src/typechecker/mod.rs src/parser/mod.rs src/models/mod.rs
echo
echo "== validator relevant sections =="
sed -n '490,690p' src/validator/mod.rs
echo
echo "== model type parser/array_parts concise =="
sed -n '1,80p' src/models/mod.rs
rg -n -C6 'impl ArkType|enum ArkType|fn array_type_parts' src/models/mod.rs src/typechecker/mod.rs src/validator/mod.rs
echo
echo "== expression enum declaration =="
rg -n -C12 'pub enum Expression|ArrayLiteral' src/parser/mod.rs src/models/mod.rs src/typechecker/mod.rsRepository: arkade-os/compiler
Length of output: 25422
Reject non-homogeneous array literals during binding validation. resolve_expression_type and binding_types_compatible do not compare array literal elements to one another or to the declared element type; an untyped literal like [1, true, 3] can still bind because type inference uses the first element while validation only checks each element for compositeness. Add a validation step for Expression::ArrayLiteral that rejects any element whose resolved type is compatible with the declared element type but is not equal to it.
📍 Affects 2 files
src/typechecker/mod.rs#L512-L520(this comment)src/validator/mod.rs#L602-L666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/typechecker/mod.rs` around lines 512 - 520, The array-literal binding
validation must reject heterogeneous elements instead of inferring only from the
first element. In src/typechecker/mod.rs lines 512-520, update the
Expression::ArrayLiteral handling and in src/validator/mod.rs lines 602-666 add
the corresponding validation so every element’s resolved type is compared with
the declared element type, rejecting elements that are compatible but not equal;
preserve the existing behavior for homogeneous and empty arrays.
|
Addressed the CodeRabbit review. Three of the four findings reproduced and are fixed in 787d46e; one no longer applies. Fixed —
Both now recurse into elements, with regression tests asserting no Fixed — heterogeneous array literals. One correction to the suggested fix: it proposed rejecting elements "compatible but not equal" to the declared type. Implemented as strict equality first, that rejects Fixed — assignment at a non-zero literal index. Test added. Worth noting the concern was already covered end to end ( Not applicable — per-element
|
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
This PR adds static-size arrays to the Arkade compiler. The overall approach is sound: grammar-level enforcement of sized types, a clean array_type_parts utility centralising all parse logic, and a dual-layer defence (validator then compiler) for every new error path. The runtime-index bound check correctly uses the array's own declared length; the assert_statement_boundary guard prevents temporaries leaking between statements; the validate_output prologue check (src/validator/mod.rs:1544-1555) catches asm/ABI divergence at build time. The e2e harness exercises client-side expandInput expansion, and the mutation commentary in the PR description is unusually thorough. Below are the findings that need follow-up.
Findings
[HIGH] select_indexed_value relies on an undocumented contiguity invariant — src/compiler/mod.rs:235
self.push_integer_temporary(first_depth_without_index);
self.apply(OP_ADD, 2, 1)?;
self.apply(OP_PICK, 1, 1)The emitted (index + first_depth_without_index) OP_PICK is correct only if the symbolic slots for $array:name:0 … $array:name:{N-1} are contiguous in the stack vector, each one depth unit deeper than the previous. This holds today because for_each_expanded_param expands all elements of one array in a single inner loop before moving to the next parameter, and the subsequent .rev() in both new() function-binding and constructor-binding paths preserves group contiguity. For local arrays the enumerate().rev() push order also guarantees contiguity.
The invariant is load-bearing for correctness but nowhere documented. A future refactor that interleaves non-array items between elements (e.g., storing a scratch temporary between expansion steps, or merging two parameter lists mid-stream) would silently produce wrong OP_PICK depths with no compile-time error. Add an assertion or at minimum a prominent comment on for_each_expanded_param and the two construction sites in new() (lines ~82-106) that the invariant must hold.
[HIGH] array_type_parts accepts length 0 without assertion — src/models/mod.rs:12-15
pub fn array_type_parts(declared_type: &str) -> Option<(&str, usize)> {
let (element, length) = declared_type.strip_suffix(']')?.split_once('[')?;
Some((element, length.parse().ok()?))
}"pubkey[0]" returns Some(("pubkey", 0)). The grammar array_size = @{ ASCII_NONZERO_DIGIT ~ ASCII_DIGIT* } prevents zero-size arrays at parse time, but array_type_parts is pub and called throughout the pipeline on strings that may not originate from the grammar (e.g., bindgen build_ir with a hand-crafted artifact JSON). A zero-length result propagates silently: for_each_expanded_param emits zero placeholders, array_length returns 0, the runtime bound check becomes 0 ≤ index < 0 (always fails — safe), and for k in 0..0 silently emits no loop body. The function should assert or return None for length 0, or at minimum document the invariant and have every call site treat length 0 as an error.
[HIGH] No upper bound on array size — src/parser/grammar.pest:27
array_size = @{ ASCII_NONZERO_DIGIT ~ ASCII_DIGIT* }
pubkey[99999] parses and produces 99 999 constructor placeholders, 99 999 × (number of indexed read sites) OP_PICK depth computations, and 99 999 loop unrolls per for … in. If the compiler is callable on untrusted input there is no defence against multi-GB artifact generation or stack-blowing script emission. Consider a grammar-level cap (e.g., three digits: ASCII_NONZERO_DIGIT ~ ASCII_DIGIT{0,2}) or a validator error for length > MAX_ARRAY_SIZE. The PR description acknowledges proportional script growth but does not set a bound.
[MEDIUM] assign() gives a misleading error for negative literal indices — src/compiler/mod.rs:467-476
if let Some((array, element)) = name.strip_suffix(']').and_then(|n| n.split_once('[')) {
if element.parse::<usize>().is_err() {
return Err(format!(
"assignment to '{array}' at a runtime index is not supported; use a literal index"
));
}
}arr[-1] = v fails with "use a literal index" — the user already provided a literal. The validator's find_binding (line ~538) falls through to the first-element lookup for non-usize indices, so the out-of-bounds LHS is not rejected there; the compiler's message is the only feedback and it is wrong. "-1" should produce "literal index must be a non-negative integer" or, better, the validator should catch it the same way it catches out-of-range read indices (src/validator/mod.rs:1095-1102), which currently only applies to ArrayIndex in value position.
[MEDIUM] VTXO token expansion uses string replace on array placeholder names — src/compiler/mod.rs:381-382
for (array, elements) in &self.constructor_array_expansions {
token = token.replace(array, elements);
}array is a string like "<oracles>" and elements is "<oracles_0>,<oracles_1>,<oracles_2>". str::replace does not anchor on word boundaries, so a constructor parameter named x and another named xx would cause <xx> to match twice (once for the full name, once again for the suffix). In practice the grammar enforces unique parameter names and the expanded namespace check in check_expanded_namespace prevents overlap, so this is currently safe. Nonetheless, if the VTXO reference format ever embeds additional angle-bracketed tokens this fragile replace could corrupt a protocol-visible script. Consider switching to a structured placeholder format or anchoring on the full <name> string.
[LOW] codegen.js Number(length) is not integer-safe for large sizes — playground/codegen.js:68
return Array.from({ length: Number(length) }, (_, index) => ({...}));For length strings beyond 2^53 (unlikely in practice but allowed by the grammar), Number(length) loses integer precision. Array.from would then throw a RangeError, which surfaces as an unhandled exception rather than a clean user error. Use parseInt(length, 10) and validate Number.isSafeInteger(n) && n > 0 before proceeding, consistent with the "never silently wrong stubs" goal stated in the PR.
[LOW] infer_type for empty ArrayLiteral returns Array(Unknown, 0) — src/typechecker/mod.rs:510-520
Expression::ArrayLiteral(elements) => ArkType::Array(
Box::new(
elements.first().map(|e| infer_type(e, scope)).unwrap_or(ArkType::Unknown),
),
elements.len(),
),[] produces Array(Unknown, 0). The validator rejects a length mismatch at the compiler level (elements.len() != length) and requires a declared array type, so [] as a standalone literal is impossible in practice. But if a future parser change admits empty literals, the inferred type would silently pass the declared_type.is_some() && inferred != ArkType::Unknown guard (because Unknown short-circuits), hiding the type error. Not a current risk; noting for when empty-array syntax is ever considered.
Coverage gaps
- No test for
arr[-1] = vproducing the correct diagnostic. The out-of-range LHS assignment path is untested; adding it would pin the expected error message and prevent the confusing "use a literal index" message from surviving silently. - No test for
arr.lengthon a locally-declared array (only parameter arrays are covered inarray_length_folds_to_a_literal). Theread_binding.lengthpath hitsself.array_lengthwhich reads from the symbolic stack; a local-array test would confirm the local binding names are found there too. - The
test_ir_expands_grouped_array_fieldstest covers a leaf witness with"type": "signature[2]"— a configuration the compiler's validator rejects (arrays in tapscript witnesses). The test is useful for bindgen robustness, but a comment explaining why this otherwise-unreachable path is tested would prevent future confusion.
Minor / nit
for_each_expanded_parampasses three arguments to its closure but the middle one (binding_name) is unused inexpanded_placeholder_params—src/compiler/mod.rs:703. Consider two separate helpers or at least a_binding_namedestructure to signal intentional ignore.array_lengthis O(N) per call and called twice per runtime indexed-access site (src/compiler/mod.rs:169andsrc/compiler/mod.rs:231) and once per loop unroll (line 801). For N > ~100 in a hot path this becomes O(N²) in compile time. Caching it locally inselect_indexed_valueis trivial and eliminates the double scan.
2 participants
Arrays were fixed at three elements by a single compile-wide constant (
DEFAULT_ARRAY_LENGTH). This replaces that with sCrypt-style static arrays: the size is part of the type and every layer reads it from the declaration.What changed
T[N]is the only array form. BareT[]no longer parses;T[0]andT[01]are parse errors.DEFAULT_ARRAY_LENGTHis deleted.arr.lengthfolds to the declared size at compile time.int[3] xs = [1, 2, 3];with element assignment at a literal index. Elements bind exactly like parameter arrays, so indexing, loops and.lengthtreat them identically.constructorInputs,arkade.inputsand leafwitnesskeep one entry per source parameter, carrying the size:{ "name": "oracles", "type": "pubkey[3]" }.tapscriptfunction inputs must be scalars, so no tapleaf witness carries an array for now.for … in tx.assetGroupsis rejected. The group count is not known at compile time, so the old fixed unroll silently checked only the first three groups.Rejected with a message: bare
T[], zero or leading-zero sizes, an array input on atapscriptfunction, a literal index at or past the declared size, an initializer whose element count or type does not match, an array literal without a declared array type, andxs[i] = vat a runtime index.Notes for review
The compiler derives array lengths from the symbolic stack.
Generator::array_lengthcounts the contiguous$array:name:ibindings already tracked there, so no new state was needed for the bound check or the unroll count, and local arrays work through the same path as parameters.The ABI and the script now speak two representations. The artifact groups arrays;
asmkeeps per-element placeholders (<oracles_0>).validate_outputderives the expected constructor prologue by flatteningconstructorInputs, so a mismatch between the two fails the build. Consumers expand:arkade-bindgen(fields_from_ark_type) andplayground/codegen.js(expandFields). Without that expansion,Encoding::from_ark_type("pubkey[3]")would have fallen through toUnknownand generated[]bytefields for a pubkey — silently wrong stubs, hence the newtest_ir_expands_grouped_array_fields.Runtime-index assignment (
xs[i] = v) is deferred, not forgotten. The VM hasOP_PICK(read at a runtime depth) but no inverse, so every write is emulated —Generator::assigncostsO(depth)opcodes today for every assignment. Emulating a dynamic array write on the current opcode set costsNguarded writes. The design doc writes up anOP_PUTproposal (dual ofOP_PICK, O(1) interpreter cost, constant-2stack effect) that would make it ~11 opcodes and collapseassignto two. Note the0 <= i < Ncheck stays the compiler's job either way:OP_PUTcan only bound-check against the whole stack, while an array occupies a subrange, so a witness-supplied index could otherwise overwrite a neighbouring binding.Testing
cargo test --workspace,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --checkpass at each of the six commits../playground/build.shsucceeds.New e2e (
sh scripts/e2e.sh):tests/e2e/contracts/static_arrays.arkruns anint[4]constructor array, anint[5]function array and a localint[3]through the Arkade VM, with runtime indexing into both a parameter and a local array, and rejection cases for negative, past-the-end and local out-of-range indices. The harness builds the witness from the ABI (covenantWitness/expandInput), so it exercises the documented client-side expansion rather than assuming an order.Two things worth knowing about that e2e, from mutation-checking it:
OP_VERIFY failed. That case is pinned by the unit testruntime_index_bound_check_uses_the_arrays_own_size, which asserts the exact emitted length.Migration
Every
T[]declaration needs a size;[3]preserves current behaviour. Clients expand a grouped array entry intoname_0 … name_{N-1}in index order (witness order is reverse declaration order, element 0 closest to the top).Summary by CodeRabbit
New Features
.length, literals, assignments, and loop iteration.Documentation