Implement storage arrays#469
Draft
axic wants to merge 21 commits into
Draft
Conversation
Adds an array(member, index) data type to std alongside mapping(member, index), with parallel Typedef, StorageSize, CanStore, LValueIdxAccess and RValueIdxAccess instances. Slot for arr[i] follows Solidity's dynamic array layout: keccak256(p) + i. Also exports larr/rarr helpers analogous to lidx/ridx.
Adds 129arraystorage.solc which constructs a storage(array(uint256, uint256)) at a fixed slot, writes two elements via Assign + larr, and reads them back via rarr. Registered alongside the storage mapping tests in test/Cases.hs.
Drops the index type parameter from array since the index is always a Typedef(word) value, leaving the element type as the sole parameter (matching Solidity's T[] surface form). Updates the existing storage test and adds a new test that defines a contract field `arr : array(uint256)` and accesses it via larr/rarr.
Adds test/examples/dispatch/storage.{solc,json} which exercises a
MemberRegistry contract with a storage array of addresses:
- addMember(address) appends to the array
- removeMember(address) performs swap-pop, reverting if not found
- numberOfMembers() returns the count
- getMembers() returns the full address[]
The contract maintains a separate memberCount alongside the storage
array. It also defines an ABIEncode/ABIAttribs instance for
memory(DynArray(t)) (t:Typedef(word)) so that getMembers can return a
true dynamic array via the dispatch ABI encoder.
Adds an Array typeclass to std with length/setLength methods and an instance for storage(array(t)) that reads/writes the slot itself (Solidity storage layout: length at slot p, elements at keccak256(p) + i). The dispatch test no longer carries a separate memberCount field — it calls Array.length(members) and Array.setLength(members, n) instead.
- Array typeclass gains push/pop methods (with element type as a class parameter); push grows the length, pop shrinks it (and reverts on empty). - Array bounds-check: LValueIdxAccess/RValueIdxAccess on storage(array(a)) now consult Array.length and revert via out_of_bounds when i >= length. - lidx/ridx become generic typeclass wrappers (LValueIdxAccess/ RValueIdxAccess), so arr[i] works for both mappings and arrays via the shared FieldAccess desugaring; larr/rarr are dropped. - NameResolution learns a small UFCS rule: when the receiver of a method-style call resolves to a contract field (`field.method(args)`) and no class is otherwise selected, search the class env for a uniquely-named method and rewrite to `Class.method(field, args)`. This enables `members.push(addr)`, `members.length()`, `members.pop()`. - Tests updated accordingly: dispatch/storage.solc no longer carries a separate memberCount; spec/129arraystorage and spec/130arrayfield use push and the bounds-checked accessors.
memory(ty):ABIAttribs (for any ty:ABIAttribs) + DynArray(t):ABIAttribs already derive it. Keep only the ABIEncode instance, which has no generic alternative (the commented-out memory(ty):ABIEncode based on MemoryType still fails patterson/coverage).
Switches removeMember from swap-pop to the user-specified pattern: foundIdx starts at length() as a not-found sentinel, the scan keeps going past the match (solcore has no break), then subsequent elements are shifted one slot down to close the gap and pop() drops the duplicated last item. Preserves order of remaining members.
The previous Array.push/pop bodies dispatched recursively through Array.length/Array.setLength on the same instance, which the specialiser appears to have trouble pinning down for the storage(array(t)):Array(t) MPTC instance (it produced 'no resolution found for StorageType.store : word -> array(address) -> ()'). Inlining the slot/length arithmetic with sload_/sstore_/StorageType.store directly avoids the recursive class dispatch entirely. Same inlining for the LValueIdxAccess bounds check (also drops its previously redundant a:StorageType constraint).
Restructure to avoid MPTC inference issues on length/setLength/pop, which never mention elem in their signatures. push, which actually binds the element type via val:elem, lives in its own MPTC class ArrayPush(elem). UFCS still finds each method via the class-method search, so members.push(addr), members.length(), members.pop() all keep working unchanged in the tests.
The Array/ArrayPush split moved push out of the Array class into the new ArrayPush(elem) MPTC, so the explicit Array.push(...) calls in this spec test resolve to an undefined name. Switch them to ArrayPush.push. Note: this only addresses the 'Undefined name: push' failure. The deeper StorageType.store specialization panic on array push (130arrayfield and the storage_array dispatch test) is a separate MPTC bug still open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcPQPnd1x2mB4UySv9FN5A
Instance methods were registered as specialisation resolutions with their enclosing instance's type variables left free (the `forall` lives on the instance, not the method), so sigVars did not list them. renametv only freshens variables in sigVars, so those free variables survived into the shared specialisation substitution under their original names. When such a method's body resolved a *nested* class method whose instance reused the same variable name, specCall's extSpSubst merged the nested binding into the global substitution and clobbered the outer one. Concretely, specialising ArrayPush.push (element var `t` |-> uint256) then resolving Typedef.rep on storage(array(uint256)) bound `t` |-> array(uint256), so the later `StorageType.store(_, val:t)` looked for an array(uint256) StorageType instance and panicked. Quantify each instance method's free type variables in sigVars when building its resolution, so renametv gives every specialisation fresh, non-colliding variables. Monomorphic methods are unaffected (no free variables). Fixes the storage-array push specialisation panic (129arraystorage, 130arrayfield, storage_array dispatch test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcPQPnd1x2mB4UySv9FN5A
A storage array/mapping contract field is read through the RVA field-access path, which calls CanStore.load. Both instances stubbed load with unimplemented() (reverting with 0x6e128399) before returning the slot, so every field read — members.push(x), members.length(), members[i], etc. — reverted at runtime even though dispatch and specialisation were correct. Loading a storage array/mapping field semantically yields its storage reference (the slot), which is exactly what push/pop/length/indexed access consume. Drop the unimplemented() guard so load just returns the reference. store stays unimplemented (assigning a whole array/mapping by value is unsupported and unexercised). Fixes the storage_array dispatch test reverting with 0x6e128399. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcPQPnd1x2mB4UySv9FN5A
Rewrite the storage-array tests to call class methods explicitly (ArrayPush.push(arr, x), Array.length(arr), Array.pop(arr)) instead of the UFCS receiver sugar (arr.push(x), arr.length(), arr.pop()). Bare field names still resolve to the field reference via name resolution, so these desugar identically — but the tests and std no longer depend on the UFCS method-call feature, allowing it to be split into its own PR. - storage_array.solc: push/length/pop -> ArrayPush.push / Array.length / Array.pop - 130arrayfield.solc: arr.push(...) -> ArrayPush.push(arr, ...) - 129arraystorage.solc already used explicit calls (unchanged) - indexed access arr[i] is unaffected (handled by field-access desugaring, not UFCS) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcPQPnd1x2mB4UySv9FN5A
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.