Clarify direct value construction and deep snapshots - #158
Conversation
📝 WalkthroughWalkthroughThe specification clarifies recursive deep copying, direct construction of fresh values, safe overwrite ordering, and validation rules for concurrent boxed-value snapshots. ChangesMemory and snapshot rules
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@spec/concurrency.md`:
- Line 134: Update spec/concurrency.md lines 134-134 to state that rejected
snapshot attempts must reclaim all partial-copy blocks from the destination
scope before retrying, preventing retained allocations across retries. Add the
same reclamation rule to stories/memory.md lines 225-225; both documentation
sites require direct updates.
In `@spec/memory.md`:
- Line 641: Add exactly one trailing newline at the end of spec/memory.md, after
the final “lifetimes.md” reference, without changing the documented content.
🪄 Autofix
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: Repository: zane-lang/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba010d06-2051-4d49-90da-07b3aaae33ff
📒 Files selected for processing (3)
spec/concurrency.mdspec/memory.mdstories/memory.md
|
review from ai: PR #158 review summary PR #158 (Clarify direct value construction and deep snapshots) makes targeted documentation-only updates across four files to align the language specification with the deep-copy semantics enabled by PR #155. The changes are internally consistent and correctly thread new terminology through the spec. spec/memory.md • §2.3 (Value construction): Now distinguishes two binding paths explicitly. Fresh non-place expressions construct directly into their destination, recursively through nested fresh results. Place expressions are copied whole. Overwrite ordering is formalized: the RHS observes the pre-overwrite occupant, any overlap is materialized before the old occupant is destroyed, and the destruction of the old occupant's dynamic blocks happens recursively before the replacement is installed. Recursive cases like Nat.succ avoid redundant copy costs by building directly in the final destination.. spec/glossary.md • §3.2 (value-downstream enforcement): Reworded so the "copying" rationale names the two binding paths from §2.3 (place expression is copied, fresh non-place constructs in place). Preserves the recursion exception via boxed members.. spec/concurrency.md • §4.4 (Reads take a coherent snapshot): Two notable additions and one substantial rewrite.. stories/memory.md • Updated "What a copy is for, and the ban that survived it" to emphasize that deep copy is the standard definition of copying for values with out-of-line storage, and to reinforce that reference types (List, String) remain banned from value types because they are designed to be moved, not copied. The "Nat.succ" example continues to work as a fresh-construction case, consistent with the new §2.3.. Cross-cutting observations • The terminology thread (non-place expression, place expression, destination-driven construction, overlap-safe, untrusted snapshot bytes) is used consistently across §2.3, §3.2, glossary §3.2/§3.40, and concurrency §4.4.. Overall, the changes are accurate, scoped, and consistent. Recommend approval, optionally with the §4.4 depth-bound nit. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@spec/concurrency.md`:
- Around line 133-134: Update the bounded-walk and retry wording in
spec/concurrency.md lines 133-134 so each snapshot retry starts a new walk with
the same per-attempt depth limit. Mirror this per-attempt depth-limit
requirement in stories/memory.md line 225; both sites require direct
documentation updates.
🪄 Autofix
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: Repository: zane-lang/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dccc7074-4869-41cf-994b-aba8ce7860e2
📒 Files selected for processing (4)
spec/concurrency.mdspec/glossary.mdspec/memory.mdstories/memory.md
2299fe8
into
claude/player-pointer-aliasing-frj4ae
Review asked directly why a recursive variant must box, given that every value bottoms out at a leaf. The spec only asserted that an inline self-reference "would have infinite size", which does not answer it. adt.md §4 now separates the two things being conflated. A recursive type's *values* are finite; layout is fixed per type and settled before any value exists, so the question is what one stride must be for every value at once. `size(Nat) = tag + size(Nat)` has no finite solution, and no stride can be taken from the deepest value because there is no deepest value — one `Nat` holds three nodes and the next three million, and uniform stride requires both to be the same size. The leaf terminates a value, not the type's size equation. Also reconciles two passages that #158 left describing the pre-#158 copy rule, now that a fresh non-place result constructs directly in its destination: - adt.md §4 said a recursive value type walks and reallocates on *every* store into fresh storage. That is now true only for binding an existing place; `Nat.succ(Nat.succ(...))` builds each node once where it will live. - memory.md §6 said a value parameter is "copied when bound into a fresh slot", which reads as though passing were the copy. The copy happens when the parameter, an existing place, is itself bound onward. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017AYiJXjzCPEDfYRW1ZtxH7
1 participant
Summary
This follow-up fixes the three findings from the ChatGPT review on #155:
matcharms; an existing place is copied.Why
Without direct destination construction, nested fresh values such as
Nat.succ(Nat.succ(...))can be read as repeatedly copying completed prefixes, making ordinary construction quadratic. The same rule applies orthogonally to product values such asVector2(Int(3), Int(4)).Without explicit overlap ordering, forms such as
x = xorx = x.childcould destroy the source before it is copied.The snapshot text also claimed same-stack contention as a general rule even though ordinary deep copies allocate in the destination binding's scope. Finally, a concurrent reader must validate a transient sum tag or other layout-directing metadata before using it to interpret payload bytes.
Files
spec/memory.mdspec/concurrency.mdspec/glossary.mdstories/memory.mdValidation
f6f7ca50bfc7f4bea3fef7270725123f24198b92.spec/memory.mdends with a single trailing newline.Summary by CodeRabbit