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 1cd6a3df..7133c199 100644 --- a/Docs/03-language-basics/variables-and-types.md +++ b/Docs/03-language-basics/variables-and-types.md @@ -305,24 +305,38 @@ 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. +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"]] + +push with ROLE_GROUPS[0] and "guest" // ERROR: Cannot modify constant 'ROLE_GROUPS' +``` + +> 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: > -> Constants also 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. +> ```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/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..dd36c36d 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -2415,9 +2415,22 @@ 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) { + self.report_constant_mutation(root, *line, *column); + } } Statement::MapCreation { @@ -4742,6 +4755,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) 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(