Container events are half-wired: handlers can never be registered or run

[logbie]

Summary

While auditing Docs/04-advanced-features/containers-oop.md against the implementation (branch claude/containers-md-wiring-cleanup-8k19ys, which wired up interface contracts), I found that the container event system is only half-wired. Events parse and trigger executes, but the handler side is unreachable end to end — so event/trigger are effectively no-ops that only validate the event exists. Events are not covered by the containers doc, so this was left out of that change as a follow-up.

What works today

• event on_click inside a container body parses (parse_event_definition_full), including needs parameters, and is stored in ContainerDefinitionValue.events.
• trigger on_click inside a container action parses and executes: method calls inject the definition's events into the method environment, and the EventTrigger arm runs all registered handlers.
• TestPrograms/containers_comprehensive.wfl exercises exactly this much — its triggers succeed because running zero handlers is a no-op, so the test never proves a handler fires.

What is broken

1. The on <source> <event> handler statement cannot be written. parse_event_handler (src/parser/stmt/containers.rs) calls parse_expression() for the event source, which consumes both the source and the event name as one natural-language expression, then fails with Expected identifier for event name, found Eol:

   create container Button:
       property label: Text
       event on_click
       action click:
           display "clicking"
           trigger on_click
       end
   end
   
   create new Button as btn:
       label is "Submit"
   end
   
   on btn on_click
       display "handler ran"
   
   btn.click()
   

   error[ERROR]: Expected identifier for event name, found Eol
   

2. Even if it parsed, the handler body is always dropped. parse_event_handler is a stub that returns handler_body: Vec::new() — the statements under the handler are never attached to it.

3. Statement-level trigger never parses arguments (parse_event_trigger stub returns arguments: Vec::new()), so an event declared with needs parameters can never receive values; runtime binds parameters to Null.

4. Handler registration and triggering don't share state. The runtime EventHandler arm clones the event, appends the handler, and defines the result as a plain environment binding under the event name (src/interpreter/mod.rs, Statement::EventHandler). But trigger inside a method looks up events freshly injected from ContainerDefinitionValue.events — whose handlers list is always empty (Rc<ContainerEventValue> is immutable, handlers: Vec::new() at definition). So even with 1–3 fixed, a registered handler would never be seen by a trigger inside a method.

Expected

Either events work end to end — define, register a handler with a body, trigger with arguments, handler runs with bound parameters — or the surface is removed/documented as unimplemented. Half-parsed syntax that silently does nothing is the worst of both (a program can trigger all day and look successful).

Suggested direction

• Decide the handler grammar (e.g. on <instance> <event>: ... end on) and parse the body properly.
• Store handlers somewhere shared and mutable (e.g. RefCell on the event value in the container definition, or per-instance), so registration and triggering agree.
• Parse trigger arguments and bind event parameters.
• Add red-first tests that assert a handler actually runs (current tests only prove triggers don't crash), including the needs-parameter path.
• Update docs (containers doc or a dedicated events page) only once behavior is real, per the docs-honesty policy.

Code pointers

• src/parser/stmt/containers.rs — parse_event_handler, parse_event_trigger, parse_event_definition stubs ("For now, just create a simple ...").
• src/interpreter/mod.rs — Statement::EventTrigger, Statement::EventHandler arms; event injection in the MethodCall path.
• src/interpreter/value.rs — ContainerEventValue / EventHandler.

[logbie]

Confirmed against current main (4178714a, v26.8.2) — all four claims reproduce in the source. Verdict: confirmed bug, and the most serious kind: a language surface that parses and silently does nothing.

Verification:

• src/parser/stmt/containers.rs:522 — parse_event_handler is the stub; line 555 is literally handler_body: Vec::new(), so claim 2 holds exactly as written.
• src/parser/stmt/containers.rs:487 — parse_event_trigger is the matching stub for claim 3.
• src/parser/stmt/containers.rs:452 — parse_event_definition is a third stub, distinct from the working parse_event_definition_full at line 792. That split is the source of the confusion: the container-body path is real, the statement path is not.
• src/interpreter/value.rs:172 — ContainerEventValue with EventHandler at line 181, held as Rc<ContainerEventValue> (line 37) inside ContainerDefinitionValue.events: HashMap<String, ContainerEventValue> (line 109). Immutable behind an Rc, so claim 4 is structural, not incidental — registration cannot mutate what trigger reads.

Two things worth adding to the plan:

The shared-state fix drives the grammar decision, not the other way round. Claim 4 is the load-bearing one; 1–3 are cheap once storage is settled. The Rc<ContainerEventValue> needs to become Rc<RefCell<…>> (or handlers move to per-instance storage). Per-instance is the more defensible semantic — on btn on_click reads as registering against that button, not against every Button ever created — but it is also the bigger change, since trigger inside a method currently resolves events from the definition. Worth settling explicitly before any parser work, or 1–3 get written twice.

Backward compatibility is not a constraint here, which is unusual and helpful. TestPrograms/containers_comprehensive.wfl only proves trigger doesn't crash with zero handlers, as you note. Since no program can currently register a handler, no existing program can depend on handler behaviour — so this can be fixed without the usual "never break an existing TestPrograms/ program" tension. The one real compat risk is the reverse: if on <instance> <event> ever becomes valid, make sure it can't be mistaken for existing on-prefixed grammar during parse.

On the "remove or fix" fork: given event and trigger already parse inside container bodies and are exercised by a shipped test program, removal would be the breaking option and fixing is the cheaper one. But an interim guard is worth considering — making statement-level on … a clear "event handlers are not yet implemented" parse error beats the current Expected identifier for event name, found Eol, which reads like a syntax mistake by the user rather than a missing feature.

Red-first per testing.md: the failing test is a TestPrograms/ program that triggers an event and asserts the handler's side effect, plus a needs-parameter variant asserting bound values rather than Null. Both fail today. R2 (parser + interpreter, no concurrency).

Suggested labels: bug, parser, interpreter, containers.

Posted by the WFL repo warden (automated triage pass).

[logbie]

Follow-up to my assessment above, and the reason I'm posting again rather than staying quiet: an unmerged branch already implements this, and it has never been opened as a PR. It has been sitting untouched for six days and is not visible from this issue, so it is on its way to being lost.

Branch: claude/github-issue-685-aw89at — two commits, last touched 2026-08-14T07:51Z, no PR in any state (open, closed, or merged).

199952b test: cover container events end to end (red for #685)
b9f51ad feat(containers): wire container events end to end

Red-first, in that order, which is what testing.md §15 asks for.

It merges cleanly onto current main (36de4fa7). I test-merged it: no conflicts, exit 0. It is 14 commits behind but nothing it touches has moved underneath it.

It addresses claim 4 — the load-bearing one — the way this issue's assessment argued it should. Reading src/interpreter/value.rs:

• ContainerInstanceValue gains its own events: HashMap<String, Rc<ContainerEventValue>>, so registration is per-instance, not per-definition. That is the "more defensible semantic" fork, taken.
• ContainerEventValue.handlers becomes Rc<RefCell<Vec<EventHandler>>>, so registration and trigger — which reach the event by different paths — share one list. That is exactly the Rc<ContainerEventValue> immutability problem, resolved.
• A fresh_instance() constructor gives each instance an independent handler list, and deep_clone_container_instance carries handlers registered so far while keeping later registrations local to each copy. The clone semantics were thought about, not left to fall out.

Claims 1–3 are addressed too. The handler_body: Vec::new() stub at src/parser/stmt/containers.rs:555 is gone on the branch — it parses a real body (handler_body.push(self.parse_statement()?)).

Test surface: tests/container_events_test.rs (635 lines, new) plus seven new TestPrograms/docs_examples/containers/events_*.wfl programs covering handling, inheritance, multiple handlers, parameters, per-instance isolation, scope, and standalone use. Roughly 2,000 insertions across 25 files, touching analyzer, fixer, interpreter, parser, typechecker.

What I have not verified: I read the diff; I did not build or run the test suite (no Rust toolchain in this pass's environment). So this is "the work exists, merges, and is shaped correctly" — not "it passes." Opening it as a PR is what would settle that, and it costs nothing: CI would run all 20 checks against it.

Suggested next step: open claude/github-issue-685-aw89at as a PR against main and let CI judge it. If it goes green, this issue closes with a real per-instance implementation and seven new backward-compat-safe test programs. If it goes red, the failures are the next piece of work — still better than the branch quietly aging out.

Posted by the WFL repo warden (automated triage pass).