docs(adr): draft ADR-0007 — set is always an effect - #71
Merged
Merged
Conversation
Decides the design note's §4.4 open question: - `set :x to <expr>` is classified effectful (mutation), unconditionally — independent of which mutable engine it targets and of whether the enclosing transaction ever commits. Any form calling `set` is impure. - Resolves the transaction-rollback wrinkle the design note raised directly: purity is a static, evaluation-time property (per ADR-0006), not a runtime-outcome one. A set that later rolls back was still an attempted mutation at evaluation time — treating rollback as purity-restoring would make purity depend on runtime state, which ADR-0006 already rules out, and would be unsound for any future type checker (rollback can depend on state set doesn't see). - Restates (does not re-decide) ADR-0006's let-into-mutable-engine rule for a complete let/set picture: let is pure only for the lexical engine; set never has a pure case, because set on a lexical binding is already an error (AGENTS.md). Status: Proposed (acceptance = editing the Status line, same convention as ADR-0006). Also updates design-note.md §4.4/§7 and execution-queue.md's epic 008/009 criteria to link ADR-0007 in place of the interim "planned" placeholder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SbjL7643FUSoVuwCGtkJv
Reviewer's GuideThis PR drafts ADR-0007, establishing that File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
Up to standards ✅🟢 Issues
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="specs/decisions/ADR-0007-set-effect-classification.md" line_range="61-71" />
<code_context>
+- Per ADR-0006, purity is a property of **evaluation**, determined
</code_context>
<issue_to_address>
**issue (broader_impact):** ADR-0007 defines `set` as an evaluation-time mutation attempt but does not reconcile that behavior with ADR-0006 and `LANGUAGE.md`, which say effectful forms produce `^action ^t` values and effects execute only at explicit effect boundaries. Implementers cannot determine whether `set` mutates immediately during ordinary evaluation, returns an action to be run later, or is a special evaluation effect outside the `^action` model.
**Triggers:** When a future type checker or runtime implements the `^action` marker model.
**Suggested fix:** Explicitly state that `set` is an immediate evaluation-time mutation that is effectful without returning `^action`, and explain why it is intentionally exempt from the action-boundary rule; alternatively specify that `set` returns an action and revise the surrounding purity examples accordingly.
</issue_to_address>
### Comment 2
<location path="specs/decisions/ADR-0007-set-effect-classification.md" line_range="55" />
<code_context>
+ form whose evaluation calls `set` is impure — there is no condition under
+ which a `set`-calling form is nonetheless pure.
+- This is unconditional on **which** mutable engine is targeted
+ (`graph` / `dynamic` / `reactive` / `sync`) and on **whether `<expr>`
+ itself is pure** — evaluating `<expr>` may be pure, but the `set` that
+ consumes its result is not.
+
+### 2. Transaction rollback does not change the classification
+
</code_context>
<issue_to_address>
**nitpick:** The ADR names `sync` as a mutable engine, but the canonical binding table in `LANGUAGE.md` names that engine `synchronised` and only uses `sync` in the `sync%token` binding prefix. Epic 009 implementers following the ADR can therefore treat `sync` as the engine identifier and diverge from the established taxonomy.
**Triggers:** When epic 008/009 defines the engine mapping and binding-kind vocabulary.
**Suggested fix:** Use the established engine name `synchronised` (or explicitly declare `sync` as its canonical abbreviation and update the binding table).
```suggestion
(`graph` / `dynamic` / `reactive` / `synchronised`) and on **whether `<expr>`
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: specs/decisions/ADR-0007-set-effect-classification.md:71
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Five review findings from sourcery-ai + cubic-dev-ai, all valid: - `set` vs the ^action model was never reconciled: ADR-0006 models IO effects as build-then-run ^action values, but set mutates immediately during evaluation. Added an explicit exemption: set is a language primitive gated by the transaction contract, not a composable IO action -- there's no useful "build the intent to set, run it later" split the way cp "a" "b" usefully represents a copy intent. set was already classified this way pre-^action; this ADR keeps it there. - `set :x` used the lexical sigil (:name) as a set example, which this same ADR says set must reject. Replaced with LANGUAGE.md's own confirmed example, `set $a to <expr>` (dynamic engine). - Engine name `sync` doesn't match the canonical `synchronized` (AGENTS.md's non-negotiable rule); fixed throughout, with a note that LANGUAGE.md's own Transaction contract section abbreviates it as `sync` and epic 008 settles the canonical name. - `let :x be run :action` would have been misclassified pure: the ADR said lexical `let` is pure by engine alone, but purity needs `<expr>` itself to be pure too -- the lexical engine only removes the *extra* effect a let could add (the commit), it doesn't launder whatever <expr> does. Qualified §3 accordingly. - The AGENTS.md quote was misattributed partly to LANGUAGE.md and used the wrong engine list; fixed to cite AGENTS.md verbatim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012SbjL7643FUSoVuwCGtkJv
|
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.
Drafts ADR-0007, deciding the design note's §4.4 open question:
setis always an effect —set :x to <expr>is classifiedeffectful (mutation), unconditionally, regardless of which mutable
engine it targets. Any form calling
setis impure.evaluation-time property (per ADR-0006), not a runtime-outcome one. A
setthat later rolls back was still an attempted mutation when itevaluated — outcome-dependent purity would be unsound for any future
type checker (rollback can depend on state
setnever sees).letstays pure only for the lexical engine — restates (doesn'tre-decide) ADR-0006's existing rule, for a complete
let/setpicture.The asymmetry is intentional:
seton a lexical binding is already anerror (
AGENTS.md), sosetnever has a pure case to begin with.Status: Proposed — acceptance = editing the Status line, same
convention as ADR-0006.
Also updates the design note §4.4/§7 and
execution-queue.md's epic008/009 criteria to link ADR-0007 in place of the interim "planned"
placeholder.
Part of the epic 007 unblock path (
ADR-0006 merged → ADR-0007 (this) → ADR-0008 + IK1 grammar ADR → 008 → 007).Summary by Sourcery
Draft ADR-0007 to establish that every
setis an effect regardless of its target or transaction outcome, unblocking the dependent binding-model and binding-kind work.New Features:
setas unconditionally effectful and restating the lexical-engine-only purity rule forlet.Enhancements:
setor make mutation pure by outcome.Documentation:
Summary by cubic
Drafts ADR-0007, resolving the design note's §4.4 open question:
setis always an effect (mutation), regardless of target engine, expression purity, or transaction outcome, so any form callingsetis impure. Status is Proposed — acceptance means editing the Status line, same convention as ADR-0006. Also updates the design note and execution queue to link ADR-0007, and incorporates the first review round's fixes.What this decides
setpure; purity is a static, evaluation-time property per ADR-0006, and outcome-dependent purity would be unsound for a future type checker.setis deliberately exempt from the^action ^tpattern — it mutates immediately during evaluation, gated by the transaction contract, not built-then-run.letis pure only for the lexical engine, and only when<expr>is itself pure;sethas no pure case sinceseton a lexical binding is an error.setinside a rolled-back transaction is still impure; authors needing that pattern use an^actiona caller runs.Written for commit a4f4c63. Summary will update on new commits.