fix(analyzer): resolve push write targets to their root binding for constness (#673) - #715
Conversation
`push with <list> and <value>` carries its target as an `Expression`, not the `list_name: String` the #671 constness check resolves, so pushing onto a constant list is silently accepted at analysis time and mutated in place at runtime. Red: `push_onto_a_constant_list_is_rejected` and `push_onto_an_indexed_constant_target_names_the_root_binding` both fail with zero reports. The three negative tests (no root binding, mutable target, action parameter / loop variable) already pass and guard against the fix widening into shapes that must stay legal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onstness (#673) `push with <list> and <value>` carries its target as an `Expression`, not the `list_name: String` the #671 constness check resolves, so pushing onto a constant list escaped analysis entirely. There is no runtime backstop either: the interpreter mutates in place through the list's `Rc<RefCell<Vec<Value>>>` and never reassigns the binding, so `Environment::assign` — the only runtime enforcement point for constness — is never reached. Add `Analyzer::write_target_root_binding`, which resolves a write-target expression to the binding the write ultimately reaches: a `Variable` resolves to itself, and index/member chains resolve to whatever they bottom out in, so `CONFIG[0]`, `CONFIG[0][1]`, and `CONFIG.entries` all resolve to `CONFIG`. A target rooted in a computed value (a call result, a literal) resolves to `None` and reports nothing. The `PushStatement` arm feeds that root name to the existing `report_constant_mutation`, so `push` now emits exactly the message `add`/`remove`/`clear` already emit. The constness test itself is unchanged — still `constant_bindings` membership, not `mutable: false` — so action and container-method parameters, loop variables, and REPL parent-scope bindings stay unaffected. Analyzer-only: the interpreter, parser, and typechecker are untouched. Aliasing (`store alias as CONST` then mutating `alias`) remains out of scope per the issue, and the docs still state that limitation. Risk class R2 (analysis errors gate execution; `wfl <file>` now exits 3 on these programs). Sweeping every gated `TestPrograms/` and `examples/` program through `--analyze` with the release binary produced zero new reports. Docs ship with the fix: the "Known gap" callout in variables-and-types.md and the matching pointer in naming-conventions.md are now false and removed; `push` is listed with the other rejected forms, the indexed-target case is documented, and the docs error-example exercises both push forms (25/25 examples validate). Dev Diary entry added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
There was a problem hiding this comment.
Pull request overview
This PR closes #673 by extending the analyzer’s constant-mutation checks to push with <list> and <value> by resolving the push target expression back to its root binding name (e.g., CONFIG[0] → CONFIG) and reusing the existing Cannot modify constant '<name>' diagnostic path. It also updates tests and documentation/examples to reflect that push is now enforced for constant lists.
Changes:
- Add analyzer support for resolving
pushwrite targets to their root binding and reporting constant mutations. - Add regression tests covering constant, indexed, no-root-binding, mutable, and parameter/loop-variable
pushtargets. - Update docs, docs examples, and dev diary to remove the previously documented “push constness gap” and demonstrate the new behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/analyzer/mod.rs |
Adds write_target_root_binding and applies constant-mutation reporting to PushStatement. |
tests/constant_mutation_analyzer_test.rs |
Adds targeted analyzer tests for push constness behavior and non-regressions. |
Docs/03-language-basics/variables-and-types.md |
Updates constants documentation to include push and adds an indexed-target example. |
Docs/06-best-practices/naming-conventions.md |
Updates constant mutation forms list to include push. |
TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl |
Extends the constants error-example to include bare and indexed push failures. |
TestPrograms/docs_examples/_meta/manifest.json |
Updates doc purpose text to reflect push and indexed push coverage. |
History/dev-diary/2026/2026-08-14-issue-673-push-write-target-constness.md |
Adds dev diary entry documenting the bug, fix, and test evidence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let Some(root) = Self::write_target_root_binding(list) { | ||
| let root = root.to_string(); | ||
| self.report_constant_mutation(&root, *line, *column); | ||
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn write_target_root_binding(expression: &Expression) -> Option<&str> { | ||
| match expression { | ||
| Expression::Variable(name, _, _) => Some(name), | ||
| Expression::IndexAccess { collection, .. } => { | ||
| Self::write_target_root_binding(collection) | ||
| } | ||
| Expression::MemberAccess { object, .. } | Expression::PropertyAccess { object, .. } => { | ||
| Self::write_target_root_binding(object) | ||
| } | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Constness enforcement for push is purely syntactic — element writes reached via a loop variable still pass
The new walk resolves CONFIG[0]/CONFIG.entries back to the root name, so a lexical write through a constant is rejected. But the same runtime mutation reached through a binding that is not the constant name still slips through, e.g.
store new constant ROLE_GROUPS as [["admin"]]
for each group in ROLE_GROUPS:
push with group and "guest" // accepted, mutates ROLE_GROUPS at run time
end for
Loop variables are only inserted into action_parameters (src/analyzer/mod.rs:1274-1275, src/analyzer/mod.rs:1352-1353), not the scope, so get_symbol_binding_key returns None in report_constant_mutation (src/analyzer/mod.rs:4786-4797) and nothing is reported. This is the same aliasing class the PR explicitly leaves out of scope, but the doc example added at Docs/03-language-basics/variables-and-types.md:319-326 now asserts "a push into an element of a constant … is rejected too" right next to "constants fix the binding, not the contents reached through it", which reads as a stronger guarantee than what is actually enforced. Worth confirming the docs wording is acceptable to the maintainer.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Statement::PushStatement { | ||
| list, | ||
| value, | ||
| line, | ||
| column, | ||
| } => { | ||
| self.analyze_expression(list); | ||
| self.analyze_expression(value); | ||
| // #673: unlike `add`/`remove`/`clear`, this statement carries its | ||
| // target as an expression rather than a bare name, so resolve the | ||
| // binding the write actually reaches before the constness check. | ||
| // A target with no root binding (a call result, a literal) has | ||
| // nothing to check and reports nothing. | ||
| if let Some(root) = Self::write_target_root_binding(list) { | ||
| let root = root.to_string(); | ||
| self.report_constant_mutation(&root, *line, *column); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Behavior change gates previously-running programs at analysis time with no CHANGELOG entry
Programs that pushed onto a constant list previously ran to completion and now exit 3 at analysis. GOVERNANCE.md §3.1 asks for a documented path plus a CHANGELOG.md entry for semantic breaks; CHANGELOG.md has no entry for this (nor for the #671 precedent, and the preceding merged PR #710 likewise did not touch it), so this appears to match existing practice rather than a deliberate omission. Flagging so a maintainer can decide whether an Unreleased → Fixed line is wanted.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb01c9b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(root) = Self::write_target_root_binding(list) { | ||
| let root = root.to_string(); | ||
| self.report_constant_mutation(&root, *line, *column); |
There was a problem hiding this comment.
For any existing program that uses push with CONST and value, this unconditional analyzer diagnostic changes the documented behavior from successful execution to exit code 3. The previous user guide explicitly documented that this form succeeded, so sweeping only repository-owned examples cannot establish downstream compatibility; retain the old behavior while introducing the required documented deprecation and migration path.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
| Risk class **R2** — analyzer diagnostics gate whether a program runs at all | ||
| (`wfl <file>` exits 3 on an analysis error), so this is public CLI/contract | ||
| behavior, and pushing onto a constant list moves from "silently allowed" to | ||
| "rejected at analysis time". |
There was a problem hiding this comment.
This section acknowledges that previously accepted WFL programs will now be rejected by the public CLI, making this a backward-compatibility change rather than R2. Reclassify it as R3 and provide the corresponding failure-path, compatibility, and release-relevant evidence before treating the change as merge-ready.
AGENTS.md reference: AGENTS.md:L138-L141
Useful? React with 👍 / 👎.
…a needless alloc (#673)
Review findings — all three addressed@copilot — fixed. The @devin-ai-integration on the docs wording — you are right, and this was the most valuable finding here. I verified your example rather than taking it on trust: So the prose I added did overclaim. This repo's docs policy is explicit that documentation describes what actually ships and must not overclaim runtime behavior, so that wording was a bug I was shipping alongside the fix. Corrected two ways:
The enforcement itself is unchanged and stays in scope: #673 asks for write-target resolution, and the aliasing class is a language-design question (what @devin-ai-integration on the CHANGELOG — agreed, added. You were also right that #671 and #710 have no entries. I have not backfilled those — that is the maintainer's call, not something to slip into this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Docs/03-language-basics/variables-and-types.md:332
- The alias example in this note references
ALLOWED_ROLES, but the code sample immediately below usesROLE_GROUPS, which can be confusing when reading the section top-to-bottom. Consider using the same constant name in both places so the alias and loop-variable caveats read as one coherent example.
> Constants fix the *binding*, not the contents reached through it. These checks
> are made by reading the code, so they catch writes spelled through the
> constant's name. A write that reaches the same list through a *different*
> name is not caught, and does change the underlying list — whether that name
> comes from an alias (`store alias as ALLOWED_ROLES`) or from a loop variable:
Summary
Closes #673.
printed
[admin, guest], exit 0 — no analyzer report, no runtime error.#671 closed this for the three bare-name mutation statements (
add ... to,remove ... from,clear), which carry their target as alist_name: Stringthe analyzer resolves directly.
pushdoes not —Statement::PushStatementcarries
list: Expression, so the constness check had nothing to look up andthe arm merely walked both sub-expressions. At runtime the interpreter mutates
in place through the
Rc<RefCell<Vec<Value>>>and never reassigns the binding,so
Environment::assign— the only runtime enforcement point for constness — isnever reached.
Caught at analysis time, exactly as #671 did it. The interpreter is
unchanged.
The change
src/analyzer/mod.rsonly — one pure helper plus thePushStatementarm:The root name feeds the existing
report_constant_mutation, so the constnesstest is still
constant_bindingsmembership and the wording is identical towhat
add/remove/clearalready report. An unresolvable name yields nobinding key, so there is no double-report alongside
analyze_expression's"not defined".
Deliberately out of scope
The issue documents a separate hole and says explicitly it must not be folded in:
aliasis a legitimately mutable binding, so no write-target analysis canreject it. Closing that means deciding what
constantmeans for referencevalues — copy-on-bind, a deep freeze, or an explicit "constants fix the binding,
not the contents". That is a language-design question. The existing docs caveat
covering it is kept verbatim.
Test evidence
Risk class: R2 — analyzer diagnostics gate whether a program runs at all
(
wfl <file>exits 3 on an analysis error), so this is public CLI behavior.Acceptance criteria → tests (in
tests/constant_mutation_analyzer_test.rs,alongside the 9 from Analyzer drops the
add ... to CONSTreport when combined with other constant mutations #671):pushonto a constant is reported →push_onto_a_constant_list_is_rejectedpush_onto_an_indexed_constant_target_names_the_root_bindingadd ... to CONSTreport when combined with other constant mutations #671 shapes)Red evidence:
0518fd9(test-only, ancestor of the fix). The two positivesfailed, the three negatives passed in both states as intended:
Green evidence:
7bb01c9— 14/14.Unit/component:
cargo test --test constant_mutation_analyzer_test→ 14passed;
diagnostics_fixtures_test3/3.Integration/contract: verified against the release binary — both repros now
exit 3 with
Cannot modify constant 'ROLES'/'CONFIG', whilepush with items and "b"on a mutable list still exits 0.End-to-end / compatibility: every gated
TestPrograms/andexamples/program swept through
--analyze— zero newCannot modify constantreports. No existing test encoded the old behavior, so nothing was weakened.
Static:
cargo fmt --all -- --checkclean;cargo clippy --all-targets --all-features -- -D warningsclean;python scripts/check_repo_hygiene.py --mode staticexit 0.Docs: required, because the docs documented the gap and this makes them
false.
Docs/03-language-basics/variables-and-types.mdloses the "Knowngap:
push with <list> and <value>is not yet checked" callout and now listspushwithadd/remove/clear, with an indexed-target example;Docs/06-best-practices/naming-conventions.mdloses its matching pointer;TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wflnowexercises bare and indexed push (10 reports, was 8) with its manifest entry
updated.
python scripts/validate_docs_examples.py→ 25/25. Dev diary added.Platforms: authored and run on Windows; CI covers Ubuntu and Windows.
Rollback/recovery: revert; no persistent state involved.
Residual risk: aliasing (above) still defeats constness for reference
values — unchanged by this PR, documented, and out of scope by the issue's own
instruction. Rebased onto
8fb784fafter Fix #698, #699, #700: replace, count loop cap, exit program #710 merged; no conflicts.🤖 Generated with Claude Code