Product: reset the memoized hash in the non-const factors()

[evaleev]

Product::factors() non-const hands out a mutable reference to factors_ without invalidating the memoized hash:

Product::factors_type &Product::factors() { return factors_; }

Mutating a factor through it leaves the stale hash in place. With asserts enabled that trips

SEQUANT_ASSERT(*hash_value_ == compute_hash());

in Product::memoizing_hash(); with SEQUANT_ASSERT_BEHAVIOR=IGNORE it silently returns the stale value, which makes static_equal() short-circuit to false for Products that are in fact equal.

This is the same defect #594 fixes for begin_subexpr()/end_subexpr(). factors() is the remaining hole, and it is the wider one — the other two hand out iterators, this hands out the whole container.

No caller in tree mutates through the reference today, so the bug is latent rather than live. Split out of the #594 review rather than folded into it, since it is independent of that PR's three fixes.

Sum needs no equivalent change: it exposes summands() as const only.

The !empty() guard is deliberately absent

begin_subexpr()/end_subexpr() guard their reset with !factors_.empty(), which is safe there — an empty range yields nothing to dereference. It is not safe here, because a caller can grow an empty factors_ through this reference:

prod->as<Product>().factors().push_back(ex<Variable>(L"x"));  // no factor to
                                                              // dereference,
                                                              // hash still stale

so the reset is unconditional. The second test below pins this down and fails if the guard is added.

Verification

Written test-first; both tests were watched failing before the fix went in:

test	without the fix
hash invalidation on mutable factors() access	8269482685011228508 != 8269482685011228508 — stale hash returned verbatim
hash invalidation on growing an empty Product via factors()	0 != 0 — with the !empty() guard added, the empty Product's hash survives a push_back

Full unit suite, both assertion configurations:

• Debug, SEQUANT_ASSERT_BEHAVIOR=THROW: 6795 assertions in 62 test cases, all passed
• Release, SEQUANT_ASSERT_BEHAVIOR=IGNORE: 310956 assertions in 63 test cases, all passed

Note on internal callers

Product::adjoint() (and the CProduct/NCProduct overrides) read through the non-const factors(), as does the SEQUANT_ASSERT in add_identical(), so those now take a spurious reset. Both are harmless — adjoint() assigns a freshly built Product over *this anyway, and add_identical() only touches scalar_, which Product's hash deliberately excludes. Routing those reads through the const overload would avoid the resets; left out of this PR as a separate cleanup.

[Copilot]

Pull request overview

This PR addresses a latent correctness issue in Product where the memoized hash could become stale after callers mutate the factors_ container via the non-const Product::factors() accessor, which can lead to incorrect static_equal() results when assertions are disabled.

Changes:

• Invalidate (reset_hash_value()) the memoized hash whenever the non-const Product::factors() accessor is called.
• Add regression tests covering mutation through factors() and growing an initially-empty Product via factors().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
SeQuant/core/expressions/product.cpp	Reset memoized hash on non-const factors() access to prevent stale-hash behavior after mutation.
tests/unit/test_expr.cpp	Add regression tests to ensure hash invalidation occurs for mutable factors() access, including when growing from empty.

Suppressed comments (1)

tests/unit/test_expr.cpp:979

• This comment attributes the !empty()-guarded hash invalidation pattern to both begin_subexpr()/end_subexpr(), but only begin_subexpr() currently has the guard and performs reset_hash_value(). Consider narrowing the reference to the actual code path so the rationale stays accurate.

    // an empty Product can still be *grown* through the mutable factors()
    // reference, so the invalidation must not be guarded by
    // `!factors_.empty()` the way begin_subexpr()/end_subexpr() are
    auto prod = ex<Product>(ExprPtrList{});

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[evaleev]

Stale base. #594 merged at 13:08 UTC, this review ran at 13:11, and the branch was still on ea830c11c (pre-#594) — where only begin_subexpr() reset the hash.

On current master both do, each under the !factors_.empty() guard:

ExprIterator Product::end_subexpr() {
  // N.B. handing out a mutable iterator into factors_ invalidates the
  // memoized hash, regardless of which end of the range it points at
  // (`*(--end())` mutates just as `*begin()` does)
  if (!factors_.empty()) {
    reset_hash_value();
  }
  ...

Branch rebased onto master, so the comment is accurate as written. No wording change.

[evaleev]

Same stale base as the product.cpp comment (and as the suppressed one on line 979): the review ran against ea830c11c, three minutes before #594 landed. Since #594, end_subexpr() resets the hash too, under the same !factors_.empty() guard as begin_subexpr(), so both test comments — and the !empty() rationale in the second one — describe the code accurately.

Branch rebased onto master. Re-verified on the new base: reverting only the factors() fix makes both new sections fail, and the full suite passes with it in place (Debug/THROW: 6824 assertions in 62 cases; Release/IGNORE: 310985 assertions in 63 cases).