review notes on visit() and free_memory

[mattallty]

Notes from integrating main (7c64831, PR #2) into a downstream consumer.

1. visit() — five things found while evaluating it

The API is genuinely useful and the "no unsafe, built on existing public API" approach is the right call. These are all fixable without changing that.

a. expected_values() and equity() panic inside a visitor. play() sets is_normalized_weight_cached = false (interpreter.rs:487), and visit_recursive navigates with apply_history/play before every call to the visitor. So the doc's list of methods callable from within — which includes expected_values — panics with "Normalized weights are not cached" unless the visitor calls cache_normalized_weights() itself first. Either say so in the doc, or have visit call it per node.

b. The doc suggests a combination that cannot work. It proposes pruning low-value lines with lock_current_strategy during a visit. But lock_current_strategy panics when state == Solved, and expected_values panics when it is anything else — so "read the EVs, then lock the bad actions" cannot happen in one pass. Worth either dropping the suggestion or spelling out the two-pass form.

c. A visitor that navigates corrupts the traversal. visit_recursive calls visitor(self) and then immediately reads is_terminal_node(), is_chance_node(), available_actions() and history() from wherever the visitor left the cursor. Snapshotting the history before visitor(self) and restoring it after would make the callback's behaviour irrelevant to the walk:

let here = self.history().to_vec();
visitor(self);
self.apply_history(&here);

d. visit() panics on a game with reduced storage. play() at a chance node panics with "Storage mode is not compatible" when storage_mode is Flop (or Turn past the turn). So visit() on a game saved with set_target_storage_mode dies rather than stopping at the boundary. Checking storage_mode before descending through a chance node — and simply not descending — would make the two features compose.

e. The test only asserts non-zero counts. visit_all_nodes checks num_terminal > 0, num_chance > 0, num_player > 0 and that the parts sum to the whole. That passes for a traversal that visits some nodes twice and others never. A hand-computed exact node count on a deliberately tiny tree (or a HashSet of history() vectors asserting no duplicates) would pin the property the API actually promises.

Separately, the O(sum of depths) cost is documented, which is good — but it may be worth stating outright that this makes visit() impractical on a flop tree, since that is where people will reach for it first.

2. Smaller notes

• Encode now rejects state < MemoryAllocated (was <= Uninitialized). Closing that provenance hole is right, and pairing it with free_memory is exactly the correct call. It is a behaviour change for anyone who was encoding a tree-built-but-unallocated game, though, so it deserves a line in the release notes.
• The interpreter panics where it could return Result. play, strategy, expected_values, equity, lock_current_strategy and now visit all panic on misuse. For a library driven from a UI or a long-lived server process, every navigation call is a potential process kill — we wrap ours in catch_unwind and pre-validate every index, which works but is not something each consumer should have to reinvent. try_* variants, or Result returns on the navigation methods, would be a large usability win.
• Display for Range (was ToString) is the right fix and is source-compatible via the blanket impl. Noting it only because it is the sort of change that silently alters behaviour for anyone who had specialised on ToString.

3. Larger gaps, for the roadmap

Not asks, just what a downstream tool runs into. Preflop is deliberately excluded here — that is handled elsewhere on our side.

• Aggregated / board-class reports. Solving N flops and aggregating the results by texture is a commercial staple and there is nothing in the crate for it. visit() is most of the primitive needed; a documented recipe would go a long way.
• Chip-EV terminals only. No hook to substitute a payoff function at the leaves, so postflop solving in a tournament context optimises the wrong objective. Even a trait for terminal payoff would open this up.
• Heads-up only. PostFlopGame is two-player by construction, so any genuinely three-way postflop spot is out of reach. Understood that this is a deep architectural constraint rather than a missing feature — flagging it only because it is the single biggest limit for a tool built around real hand histories.

---

Happy to open a PR for §1 (and for §3c/§3d, which are small) if that is useful.

Prepared with Claude Code while integrating the fork downstream.

[mattallty]

§1 is addressed by #6 (merged as 5e3de32). Reopening because the commit message said "closes #4", which auto-closed the whole issue — §2 and §3 are still open.

Done in #6:

• a — documented that equity/expected_values/expected_values_detail need cache_normalized_weights() inside the visitor, with the working pattern. Not cached per node on purpose: wasted work for visitors that don't read EVs, and quadratic in private hands under bunching.
• b — the lock_current_strategy suggestion is dropped, the two-pass form spelled out.
• c — fixed. Worth noting it was worse than described: a visitor that navigates to an ancestor made visit_recursive recurse without bound and overflow the stack, not just miscount. The node is now snapshotted before the callback and restored after, only when the visitor actually moved.
• d — fixed. The traversal now visits the chance nodes at the storage boundary and stops there instead of panicking with "Storage mode is not compatible".
• e — visit_all_nodes now also pins one visit per node and containment in the starting subtree; visit_exact_node_set checks the exact set of histories of a hand-enumerated nine-node tree. Regression tests added for c and d, both confirmed to fail against the previous implementation.

Also documented outright that the O(sum of depths) cost makes visit impractical on a full flop tree.

Still open:

• §2 — try_* / Result-returning navigation methods. This is the substantial one and deserves its own issue and API decision. The Encode state change and the Display for Range switch still want a release-notes line.
• §3 — the roadmap items (aggregated board-class reports, terminal payoff hook, multi-way).

Separately: the title claims main does not build on stable. That does not reproduce — cargo build, --all-targets, --no-default-features and cargo clippy --all-targets all pass on stable 1.97.1. Only cargo build --all-features fails, because it pulls in custom-alloc, which is nightly-only by design and documented in src/lib.rs and the README. Worth retitling, or filing the --all-features papercut separately.