fix(slang): type a pushed reference element as its own place - #637
fix(slang): type a pushed reference element as its own place#637hedgar2017 wants to merge 2 commits into
Conversation
| .expect("an array push in place position yields the new element's slot"); | ||
| (Place::from(slot), slot.r#type().element_type(0)) | ||
| let slang_type = node.get_type().expect("the binder types every expression"); | ||
| let element_type = if slang_type.is_reference_type() { |
There was a problem hiding this comment.
I'm not sure about having 2 sources of truths here.
@abinavpp @ggiraldez do you think we should unify it somehow?
There was a problem hiding this comment.
There was a problem hiding this comment.
Re the two sources: agree - derive it from the push result's type alone (ptr -> pointee, else the type itself is the element); needs only an is_pointer FFI predicate.
Re sol.byte: keep it - it's !sol.string's element type and carries packed-byte place semantics bytes1 doesn't (mstore8 stores, masked loads, never-reverting cleanup); value-level casts between the two are no-ops.
There was a problem hiding this comment.
Pull request overview
Fixes Slang→Sol dialect lowering for array.push when the element type is reference-typed in Storage/CallData, aligning with the dialect rule that such references are their own place (not !sol.ptr<...>). This prevents crashes in the Sol pass pipeline and corrects previously incorrect stores for struct-valued pushes.
Changes:
- Derive the pushed slot type via
SourceUnitScope::pointer(throughscope.pointer_type) so reference-typed elements in storage yield their own place type instead of a fabricated pointer. - Switch
push(value)lowering fromstore(converted(..))toPlace::assign(..)so reference-typed assignments correctly lower tosol.copywhen appropriate. - Add lit coverage for struct-member assignment, struct-value push (copy), and nested array pushes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
solx-slang/src/contract/function/expression/call/mod.rs |
Fixes .push slot typing for reference elements and uses Place::assign to correctly handle copy-vs-store semantics (incl. bytes-like string literal folding). |
solx-mlir/tests/lit/array_string_ops.sol |
Adds regression tests validating correct sol.push result types and expected sol.gep / sol.copy / nested push lowering. |
Coverage Summary
|
The array push lowering built every new slot as `!sol.ptr<element, Storage>`, violating the `Sol_GepOp` rule that a reference-typed element in storage is its own place: a struct element's member write crashed the Sol pass pipeline, a nested array's inner push crashed it too, and `push(value)` of a struct wrote the memory pointer into the slot as one word. The slot type now comes from `SourceUnitScope::pointer`, the rule's owner; `push(value)` assigns through `Place::assign`, taking the copy branch for aggregates; and the place path reads the element back off the slot, the pointee where the slot points at one.
642013e to
2dd78a7
Compare
Fixes the array push lowering fabricating
!sol.ptr<element, Storage>slots for reference-typed elements, against theSol_GepOprule that a reference in storage is its own place.s.push().a = 1no longer crashes the Sol pass pipeline: the slot type comes fromSourceUnitScope::pointer, the rule's owner, so a struct element's slot is the struct itself and the membergeplowers.nested.push().push(2)no longer crashes: the inner push sees the array place instead of a pointer to it.s.push(Y(1, 2))was silently mis-lowered — a single-word store of the memory pointer into a two-slot struct; it now assigns throughPlace::assignand takes thesol.copybranch.push() = v) reads the element back off the slot alone — the pointee where the slot points at one, the slot itself otherwise — invertingSourceUnitScope::pointerrather than consulting the binder a second time.Type::is_pointeris thesol::PointerTypepredicate this needs.Three new dual-oracle cases join
array_string_ops.sol(pushStructMember,pushStructValue,pushNested). Tester: 8792 → 8825 passing against the same base, zero regressed files. Pre-existing and untouched: push in value position (uint y = arr.push();,push_no_args_1d/2d).