From 0518fd9422611b9e9516fdd50e8b683dec48659a Mon Sep 17 00:00:00 2001 From: Brad Byrd Date: Fri, 14 Aug 2026 11:43:47 -0500 Subject: [PATCH 1/3] =?UTF-8?q?test:=20reproduce=20#673=20=E2=80=94=20push?= =?UTF-8?q?=20escapes=20the=20constant=20write-target=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `push with and ` 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 --- tests/constant_mutation_analyzer_test.rs | 109 +++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/tests/constant_mutation_analyzer_test.rs b/tests/constant_mutation_analyzer_test.rs index ad9e6b87..2f82893b 100644 --- a/tests/constant_mutation_analyzer_test.rs +++ b/tests/constant_mutation_analyzer_test.rs @@ -263,6 +263,115 @@ end ); } +/// Issue #673: `push with and ` carries its target as an +/// `Expression` (`Statement::PushStatement { list: Expression, .. }`), not the +/// `list_name: String` the #671 check resolves, so `push` slipped past the +/// constant check entirely — no analyzer report, no runtime error, the +/// interpreter mutating the list in place through its `Rc>`. +#[test] +fn push_onto_a_constant_list_is_rejected() { + let reports = constant_mutation_reports( + r#" +store new constant ROLES as ["admin"] +push with ROLES and "guest" +"#, + ); + + assert_eq!( + reports.len(), + 1, + "`push with CONST and value` should be reported by the analyzer: {reports:?}" + ); + assert!( + reports[0].contains("ROLES"), + "report should name the constant: {reports:?}" + ); +} + +/// An indexed push target is still a write through the constant binding, so it +/// draws the same report — named for the *root* binding the write reaches, not +/// for the element expression. +#[test] +fn push_onto_an_indexed_constant_target_names_the_root_binding() { + let reports = constant_mutation_reports( + r#" +store new constant CONFIG as [["a"]] +push with CONFIG[0] and "b" +"#, + ); + + assert_eq!( + reports.len(), + 1, + "`push with CONST[0] and value` should be reported by the analyzer: {reports:?}" + ); + assert!( + reports[0].contains("CONFIG"), + "report should name the root binding: {reports:?}" + ); +} + +/// A push target that bottoms out in a call result or a literal has no root +/// binding at all, so there is nothing to test for constness and nothing to +/// report. Pushing onto a temporary was always legal and stays legal. +#[test] +fn push_targets_without_a_root_binding_are_accepted() { + let reports = constant_mutation_reports( + r#" +store new constant SOURCE as ["admin"] +push with (unique of SOURCE) and "guest" +push with ["literal"] and "guest" +"#, + ); + + assert!( + reports.is_empty(), + "push targets with no root binding must not be reported: {reports:?}" + ); +} + +#[test] +fn push_onto_a_mutable_list_is_accepted() { + let reports = constant_mutation_reports( + r#" +store items as ["a"] +store nested as [["a"]] +push with items and "b" +push with nested[0] and "b" +"#, + ); + + assert!( + reports.is_empty(), + "mutable push targets must not be reported as constants: {reports:?}" + ); +} + +/// Action parameters and loop variables are immutable *symbols* that are not +/// constants (see `list_parameters_and_loop_variables_are_not_constants`), and +/// pushing onto a list parameter has always been legal. Guard the #671 shapes +/// against the new write-target walk widening into them. +#[test] +fn push_onto_a_parameter_or_loop_variable_is_accepted() { + let reports = constant_mutation_reports( + r#" +define action called process_list with parameters list_param: + push with list_param and "processed" + give back list_param +end action + +for each entry in [[1] and [2]]: + push with entry and 3 +end for +"#, + ); + + assert!( + reports.is_empty(), + "parameters and loop variables must not be reported as constants: {reports:?}" + ); +} + #[test] fn mutable_targets_are_still_accepted() { let reports = constant_mutation_reports( From 7bb01c9b8f9e5ed2a36eb4b9e7cc0b368363d88d Mon Sep 17 00:00:00 2001 From: Brad Byrd Date: Fri, 14 Aug 2026 12:00:13 -0500 Subject: [PATCH 2/3] fix(analyzer): resolve push write targets to their root binding for constness (#673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `push with and ` 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>>` 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 ` 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 --- .../03-language-basics/variables-and-types.md | 21 +-- Docs/06-best-practices/naming-conventions.md | 5 +- ...4-issue-673-push-write-target-constness.md | 135 ++++++++++++++++++ .../docs_examples/_meta/manifest.json | 2 +- .../basic_syntax/constants_immutable_01.wfl | 9 +- src/analyzer/mod.rs | 43 +++++- 6 files changed, 201 insertions(+), 14 deletions(-) create mode 100644 History/dev-diary/2026/2026-08-14-issue-673-push-write-target-constness.md diff --git a/Docs/03-language-basics/variables-and-types.md b/Docs/03-language-basics/variables-and-types.md index 1cd6a3df..c311f8cc 100644 --- a/Docs/03-language-basics/variables-and-types.md +++ b/Docs/03-language-basics/variables-and-types.md @@ -305,22 +305,27 @@ multiply MAX_SIZE by 2 // ERROR: Cannot modify constant 'MAX_SIZE' divide MAX_SIZE by 2 // ERROR: Cannot modify constant 'MAX_SIZE' ``` -The same applies to a constant list — `add ... to`, `remove ... from`, and -`clear` are all rejected: +The same applies to a constant list — `add ... to`, `remove ... from`, `clear`, +and `push with ... and ...` are all rejected: ```wfl store new constant ALLOWED_ROLES as ["admin" and "editor"] add "guest" to ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' clear ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' +push with ALLOWED_ROLES and "guest" // ERROR: Cannot modify constant 'ALLOWED_ROLES' ``` -> **Known gap:** `push with and ` is **not** yet checked. Pushing -> onto a constant list currently succeeds silently, at both check time and run -> time — see [issue #673](https://github.com/WebFirstLanguage/wfl/issues/673). -> Use `add ... to` when you want the constant to be enforced. -> -> Constants also fix the *binding*, not the contents reached through it. Binding +A push into an element of a constant is still a write through the constant, so +it is rejected too — reported against the name the write reaches: + +```wfl +store new constant ROLE_GROUPS as [["admin"] and ["editor"]] + +push with ROLE_GROUPS[0] and "guest" // ERROR: Cannot modify constant 'ROLE_GROUPS' +``` + +> Constants fix the *binding*, not the contents reached through it. Binding > a constant list to a mutable name (`store alias as ALLOWED_ROLES`) and mutating > the alias changes the underlying list. diff --git a/Docs/06-best-practices/naming-conventions.md b/Docs/06-best-practices/naming-conventions.md index 7b6611a8..9b264a15 100644 --- a/Docs/06-best-practices/naming-conventions.md +++ b/Docs/06-best-practices/naming-conventions.md @@ -208,9 +208,10 @@ end ## Constants Declare a real constant with `store new constant` — WFL then rejects `change`, -`add`, `subtract`, `multiply`, `divide`, `remove`, and `clear` against it (see +`add`, `subtract`, `multiply`, `divide`, `remove`, `clear`, and `push` against +it (see [Variables and Types — Constants](../03-language-basics/variables-and-types.md#constants) -for the one form that is not yet checked) — and **use SCREAMING_SNAKE_CASE** so +for what a constant does and does not fix) — and **use SCREAMING_SNAKE_CASE** so the name itself says “do not reassign”: ```wfl diff --git a/History/dev-diary/2026/2026-08-14-issue-673-push-write-target-constness.md b/History/dev-diary/2026/2026-08-14-issue-673-push-write-target-constness.md new file mode 100644 index 00000000..51265596 --- /dev/null +++ b/History/dev-diary/2026/2026-08-14-issue-673-push-write-target-constness.md @@ -0,0 +1,135 @@ +# Dev Diary — 2026-08-14: Resolving `push` write targets to their root binding (#673) + +## Context + +#671 closed the analyzer's blind spot for the bare-name mutation statements +(`add ... to`, `remove ... from`, `clear`). Its own diary entry named the piece +it deliberately left open, and filed it as #673: + +```wfl +store new constant ROLES as ["admin"] +push with ROLES and "guest" +display ROLES // [admin, guest], exit 0 +``` + +No analyzer report, no runtime error. Nested targets escaped the same way: + +```wfl +store new constant CONFIG as [["a"]] +push with CONFIG[0] and "b" +display CONFIG // [[a, b]], exit 0 +``` + +## Root cause + +`add`/`remove`/`clear` each carry their target as a `list_name: String`, which +`report_constant_mutation` resolves directly. `push` does not: +`Statement::PushStatement` carries `list: Expression` (`src/parser/ast.rs`), +because `parse_push_statement` parses a whole primary expression for the target. +The analyzer's arm therefore had nothing to look up and merely walked both +sub-expressions. + +There is no runtime backstop either. The interpreter mutates in place through +the list's `Rc>>` and never reassigns the binding, so +`Environment::assign` — the only place constness is enforced at run time — is +never reached. Exactly the constant-*list* hole #671 described, arriving through +a statement shape its fix could not see. + +## What changed + +One analyzer-only change (`src/analyzer/mod.rs`). A new pure helper, +`write_target_root_binding`, resolves a write-target expression to the binding +the write ultimately reaches: + +- `Expression::Variable` → itself. +- `IndexAccess` / `MemberAccess` / `PropertyAccess` → recurse into the + collection or object, so `CONFIG[0]`, `CONFIG[0][1]`, and `CONFIG.entries` + all resolve to `CONFIG`. Mutating an element mutates the collection the + binding names. +- Anything else (a call result, a literal) → `None`. There is no binding whose + constness could be violated, so nothing is reported. + +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: + +``` +Cannot modify constant 'ROLES' - constants are immutable once defined +``` + +The constness test itself is unchanged — it is still `constant_bindings` +membership, not `mutable: false`, so the immutable-but-not-constant symbols +(action and container-method parameters, loop variables, REPL parent-scope +bindings) stay untouched. The missing piece was only the root-binding walk. + +Undefined names still produce exactly one diagnostic: `analyze_expression(list)` +reports `Variable '' is not defined`, and an unresolvable name has no +binding key, so `report_constant_mutation` stays silent. + +Nothing in `src/interpreter/`, `src/parser/`, or `src/typechecker/` was touched. + +## Explicitly out of scope + +The aliasing hole the issue also documents: + +```wfl +store new constant ROLES as ["admin"] +store alias as ROLES +add "guest" to alias // still mutates ROLES, exit 0 +``` + +`alias` is a legitimately mutable binding, so no write-target analysis can +reject this. What `constant` means for a reference value is a language-design +question, and the issue says plainly it must not be folded into this fix. The +constants documentation continues to state the limitation outright. + +## Compatibility + +Risk class **R2** — analyzer diagnostics gate whether a program runs at all +(`wfl ` 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". + +That is the documented meaning of `constant`, and nothing shipped relies on the +old behavior: sweeping every gated `TestPrograms/` and `examples/` program +through `--analyze` with the release binary produced **zero** new +`Cannot modify constant` reports. The only source that pushes onto a constant is +the docs error-example written to demonstrate the diagnostic. + +## Testing + +Added to `tests/constant_mutation_analyzer_test.rs` as a **test-only Red +commit** before the fix: + +| Test | Red | Green | +|---|---|---| +| `push_onto_a_constant_list_is_rejected` | 0 reports, expected 1 | pass | +| `push_onto_an_indexed_constant_target_names_the_root_binding` | 0 reports, expected 1 | pass | +| `push_targets_without_a_root_binding_are_accepted` | pass | pass | +| `push_onto_a_mutable_list_is_accepted` | pass | pass | +| `push_onto_a_parameter_or_loop_variable_is_accepted` | pass | pass | + +The last three pass in both states on purpose: they are the negative assertions +that the new walk does not over-report, and they guard the same shapes #671's +over-reporting bug hit. The mutable case covers both a bare and an indexed +target; the no-root-binding case covers both a call result and a literal. + +Also run: `cargo test --all --no-fail-fast`, `cargo clippy --all-targets +--all-features -- -D warnings`, `cargo fmt --all`, +`python scripts/validate_docs_examples.py` (25/25), and the `--analyze` sweep +described above. + +## Documentation + +`Docs/03-language-basics/variables-and-types.md` carried a **Known gap** +callout saying `push` was not checked and telling readers to "use `add ... to` +when you want the constant to be enforced". That claim is now false, so it is +gone; the constant-list section lists `push` alongside `add`/`remove`/`clear` +and adds the indexed-target case. The alias caveat below it stays — it is still +true. `Docs/06-best-practices/naming-conventions.md` had the matching "the one +form that is not yet checked" pointer and now names `push` as rejected. + +`TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl` carried a +comment explaining that `push` was "deliberately absent"; it now exercises both +the bare and indexed push forms (10 reports, up from 8), with its manifest +`doc_purpose` updated to match. diff --git a/TestPrograms/docs_examples/_meta/manifest.json b/TestPrograms/docs_examples/_meta/manifest.json index 188c0176..066e8abc 100644 --- a/TestPrograms/docs_examples/_meta/manifest.json +++ b/TestPrograms/docs_examples/_meta/manifest.json @@ -219,7 +219,7 @@ "immutability", "error-example" ], - "doc_purpose": "Every mutation form of a constant (change/add/subtract/multiply/divide, and add/remove/clear on a constant list) is rejected during semantic analysis (issue #671)" + "doc_purpose": "Every mutation form of a constant (change/add/subtract/multiply/divide, and add/remove/clear/push on a constant list, including an indexed push target) is rejected during semantic analysis (issues #671, #673)" }, "docs_examples/basic_syntax/operators_01.wfl": { "doc_section": "Docs/03-language-basics/operators-and-expressions.md", diff --git a/TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl b/TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl index 05fa5cf3..da59ad22 100644 --- a/TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl +++ b/TestPrograms/docs_examples/basic_syntax/constants_immutable_01.wfl @@ -1,8 +1,6 @@ // CI-SKIP: intentional error example — must fail semantic analysis (asserted by validate_docs_examples.py) // Constants: the mutation forms that are rejected before the program runs // Source: Docs/03-language-basics/variables-and-types.md#constants -// `push with and ` is deliberately absent — it is not yet -// checked against constants (issue #673). store new constant MAX_SIZE as 100 @@ -17,3 +15,10 @@ store new constant ALLOWED_ROLES as ["admin" and "editor"] add "guest" to ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' remove "editor" from ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' clear ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' +push with ALLOWED_ROLES and "guest" // ERROR: Cannot modify constant 'ALLOWED_ROLES' + +// A nested push target is still a write through the constant binding, so it is +// reported against the name the write reaches (issue #673). +store new constant ROLE_GROUPS as [["admin"]] + +push with ROLE_GROUPS[0] and "guest" // ERROR: Cannot modify constant 'ROLE_GROUPS' diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 1ff2adf5..86b253b1 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -2415,9 +2415,23 @@ impl Analyzer { } } - Statement::PushStatement { list, value, .. } => { + 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); + } } Statement::MapCreation { @@ -4742,6 +4756,33 @@ impl Analyzer { } } + /// Resolve a write-target expression to the name of the binding the write + /// ultimately reaches, or `None` when it reaches no binding at all. + /// + /// A bare `ROLES` resolves to itself. Index and member chains resolve to + /// whatever they bottom out in — `CONFIG[0]`, `CONFIG[0][1]`, and + /// `CONFIG.entries` all resolve to `CONFIG` — because mutating an element + /// mutates the collection the binding names. Anything rooted in a computed + /// value (a call result, a literal) resolves to `None`: there is no binding + /// whose constness could be violated, so the caller reports nothing. + /// + /// Needed by `push` (#673), whose `Statement::PushStatement` carries its + /// target as an `Expression`; the bare-name mutation statements of #671 + /// carry a `list_name: String` and go straight to + /// [`Self::report_constant_mutation`]. + 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, + } + } + fn report_constant_mutation(&mut self, name: &str, line: usize, column: usize) { let is_constant = self .get_symbol_binding_key(name) From 5b36b562062dab8759c6222b790f090d7bae7ac0 Mon Sep 17 00:00:00 2001 From: Brad Byrd Date: Fri, 14 Aug 2026 12:11:38 -0500 Subject: [PATCH 3/3] docs: state the constness limit precisely, add CHANGELOG entry, drop a needless alloc (#673) --- CHANGELOG.md | 13 +++++++++++++ .../03-language-basics/variables-and-types.md | 19 ++++++++++++++----- src/analyzer/mod.rs | 3 +-- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7242f421..ae707c56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), drop. ### Fixed +- **`push` onto a constant list is now reported** (#673). `push with ROLES and + "guest"` on a `store new constant` list previously ran and mutated the list, + because `push` carries its target as an expression rather than a bare name and + so escaped the constness check that already covered `add`/`remove`/`clear` + (#671). Targets rooted at a constant through an index or member chain + (`push with CONFIG[0] and x`) are reported the same way, against the root + binding's name. **Compatibility:** a program that pushed onto a constant now + exits 3 at analysis instead of running — it was mutating something declared + immutable, which the other three statements already refused. Targets with no + root binding (a call result, a literal), mutable bindings, parameters and loop + variables are unaffected. Writes that reach a constant's contents through a + *different* binding — an alias or a loop variable — are still not caught; + constants fix the binding, not the contents. - **Transaction control SQL sent through `query`/`execute` is now rejected** (#664). `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` previously ran on arbitrary pooled connections, so a hand-written diff --git a/Docs/03-language-basics/variables-and-types.md b/Docs/03-language-basics/variables-and-types.md index c311f8cc..7133c199 100644 --- a/Docs/03-language-basics/variables-and-types.md +++ b/Docs/03-language-basics/variables-and-types.md @@ -316,8 +316,8 @@ clear ALLOWED_ROLES // ERROR: Cannot modify constant 'ALLOWED_ROLES' push with ALLOWED_ROLES and "guest" // ERROR: Cannot modify constant 'ALLOWED_ROLES' ``` -A push into an element of a constant is still a write through the constant, so -it is rejected too — reported against the name the write reaches: +A push into an element is rejected too when the target is written *through the +constant's own name* — the report names the binding the write reaches: ```wfl store new constant ROLE_GROUPS as [["admin"] and ["editor"]] @@ -325,9 +325,18 @@ store new constant ROLE_GROUPS as [["admin"] and ["editor"]] push with ROLE_GROUPS[0] and "guest" // ERROR: Cannot modify constant 'ROLE_GROUPS' ``` -> Constants fix the *binding*, not the contents reached through it. Binding -> a constant list to a mutable name (`store alias as ALLOWED_ROLES`) and mutating -> the alias changes the underlying list. +> 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: +> +> ```wfl +> store new constant ROLE_GROUPS as [["admin"]] +> for each group in ROLE_GROUPS: +> push with group and "guest" // runs; ROLE_GROUPS is changed +> end for +> ``` **Best practice:** Use uppercase names for constants so a reader can see at a glance that a value is fixed: diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 86b253b1..dd36c36d 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -2429,8 +2429,7 @@ impl Analyzer { // 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); + self.report_constant_mutation(root, *line, *column); } }