Follow-ups to #593: iterator interop, checked Expr::at(), hash invalidation

[evaleev]

Follow-up to #593, which turned Expr into a proper range. Three defects that CI could not catch, plus regression tests for each, plus one performance restoration.

Each commit stands alone, so the perf commit (27f02258) can be dropped without touching the correctness fixes.

1. const/non-const iterators do not interoperate at all

ExprIteratorImpl's heterogeneous operator- / operator== / operator<=> read other.ptr_ of the other specialization, which is private, with no friend declaration anywhere. Every one of those overloads is a hard error the moment it is instantiated:

error: 'ptr_' is a private member of 'sequant::detail::ExprIteratorImpl<true>'

Nothing in tree currently mixes the two types, which is why master builds. But expr.begin() != expr.cend() does not compile, and neither does pairing sequant::cbegin(ExprPtr const&) (expr_algorithms.hpp:65) with the non-const sequant::end(ExprPtr&) (expr_algorithms.hpp:78). There was also no ExprIterator → ConstExprIterator conversion, so the const/non-const interop these overloads exist to provide was entirely non-functional.

Fixed by befriending all specializations, collapsing each <is_const>/<!is_const> overload pair into one member template, and adding a converting constructor (mutable → const only; it is a constructor template so it is never treated as a copy constructor). Also dropped operator-(difference_type, ExprIteratorImpl) — n - it is not a valid random-access-iterator expression and it silently computed it - n. The valid n + it counterpart stays.

2. Expr::at() lost its bounds check

Dropping ranges::view_interface also dropped its at(), which threw when the index was out of range. The replacement forwards to operator[], whose only guard is SEQUANT_ASSERT — a no-op unless SEQUANT_ASSERT_ENABLED is #defined. In a build configured with SEQUANT_ASSERT_BEHAVIOR=IGNORE, sum.at(p) (optimize/sum.cpp:118) degrades from a thrown exception to an out-of-bounds read returning a garbage ExprPtr&. The parameter also changed from a signed difference type to std::size_t, so at(-1) went from throwing to wrapping to SIZE_MAX.

back() had the same problem from the other end: at(size() - 1) on an empty Expr — every atom — computes at(SIZE_MAX).

at() now always checks and throws sequant::Exception (project convention; nothing in tree catches std::out_of_range from here). The throwing path is out of line so it does not bloat callers. operator[] keeps its assert-only check, now documented as unchecked.

3. Product::end_subexpr() no longer invalidates the memoized hash

Product::end_cursor() used to call reset_hash_value(); the end_subexpr() that replaced it does not. So *(--product.end()) = new_factor mutates a factor while leaving the memoized hash in place — which trips the *hash_value_ == compute_hash() assert in Product::memoizing_hash(), and with asserts disabled leaves a stale hash that makes static_equal() short-circuit to false for products that are in fact equal.

Sum::begin_subexpr() gained the reset the old Sum::begin_cursor() never had, which is the right call. Sum::end_subexpr() gets it too, so both ends of both containers agree.

4. Hot accessors moved back into the header (separate commit, 27f02258)

begin/end/cbegin/cend/size/empty/operator[]/at/front/back are one-line forwarders on the hottest paths in the library. Expr::is_atom() alone is called from visit_impl(), is_scalar(), is_cnumber(), ExprRange::next_atom() and the Wick/canonicalization code; out-of-line definitions turn each into a non-inlinable cross-TU call on top of the virtual dispatch they already pay for. empty() now compares begin/end rather than computing size() == 0, saving a pair of virtual calls.

This is a restoration, not a new decision — before the switch away from ranges::view_interface these were all header-inline (as CRTP base templates), and is_atom() was ranges::empty(*this).

Measured rather than asserted. Release/-O3, SEQUANT_ASSERT_BEHAVIOR=IGNORE, Apple clang 17, against the identical tree with these bodies moved back to expr.cpp — 3 interleaved rounds of 3 repetitions, comparing per-benchmark medians over sequant_benchmarks (canonicalize, simplify, rapid_simplify, spintrace, tensor_block, random_tensor_network):

benchmarks faster inline	53 of 53
median delta	−2.29%
mean delta	−2.26%
best / worst	−4.31% (rapid_simplify/1) / −0.09%

No code-size cost — the trivial bodies inline away rather than bloat:

artifact	inline	out-of-line
libSeQuant-symb.a	7 437 952 B	7 441 224 B
sequant_benchmarks	4 985 824 B	4 986 816 B

Mechanism, for the record: out-of-line, libSeQuant-symb.a alone carries 34 undefined cross-TU references to these accessors and emits 16 definitions for them; inline it has zero of either.

_{cc_full_derivation was excluded from the benchmark set: it segfaults, but it does so on unmodified master (3168a655) too, so it is unrelated to this branch — reported separately.}

Verification

Each new test fails without its fix:

test	without the fix
mixed const/non-const iteration	does not compile ('ptr_' is a private member, no viable conversion)
checked element access	REQUIRE_THROWS_AS(sum->at(2), Exception) fails with SEQUANT_ASSERT_BEHAVIOR=IGNORE
hash invalidation on mutable iteration	fails for both Product and Sum

Full unit suite run locally in two configurations:

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

CI is green on all 16 checks — GCC 14 and clang, Debug and Release, sanitizers, Valgrind.

[Krzmbrzl]

I don't think it is good practice to move implementations into header files unless crucial for performance.

Enabling LTO seems like the better way to address potential performance bottlenecks from these sorts of things 🤔

[Copilot]

Pull request overview

This PR follows up on #593’s shift of Expr to a proper range by fixing const/non-const iterator interoperability, restoring checked element access semantics for Expr::at()/front()/back(), and ensuring memoized hashes are invalidated when handing out mutable iterators (including from end()), with regression tests for each.

Changes:

• Fix ExprIteratorImpl const/non-const interop (conversion + heterogeneous comparisons/differences) and remove invalid n - it operator.
• Restore always-checked Expr::at() (and front()/back()) that throws sequant::Exception on out-of-range access, independent of SEQUANT_ASSERT.
• Invalidate memoized hash when returning mutable iterators from Sum::end_subexpr() and Product::end_subexpr(), plus add unit coverage.

Reviewed changes

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

Show a summary per file

File	Description
tests/unit/test_expr.cpp	Adds regression tests for mixed const/non-const iteration, checked access throwing, and hash invalidation on mutable iteration.
SeQuant/core/expressions/sum.hpp	Resets memoized hash when handing out mutable iterators from end_subexpr().
SeQuant/core/expressions/product.hpp	Resets memoized hash when handing out mutable iterators from end_subexpr().
SeQuant/core/expressions/expr.hpp	Moves hot accessors inline; implements checked at() throwing Exception; documents unchecked operator[].
SeQuant/core/expressions/expr.cpp	Removes out-of-line accessor bodies; adds out-of-line throwing helper to avoid inlining cold paths.
SeQuant/core/expressions/expr_iterator.hpp	Fixes iterator const/non-const interop and removes invalid n - it overload; adds concept/static-assert checks.

---

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

[evaleev]

Enabling LTO seems like the better way to address potential performance bottlenecks from these sorts of things 🤔

LTO is tricky, especially in packaging scenarios: both SQ and consumer would need to be built with LTO flags, cross-package LTO is reportedly fragile.

[evaleev]

@Krzmbrzl LTO needs a separate PR/deeper thinking. The changes here mostly revert the inlinability to what it was before #593 ... LTO seems to be the way to go, but I don't know enough best-practices to deal with that right now

[Krzmbrzl]

I can create a LTO PR. However, I would consider it cleaner to move the implementations back into the CPP file.

Alternatively, I could make moving them to the CPP file part of my expr impl cleanup PR, if you prefer. I guess from the benchmark results here, we should run some benchmarks before merging that PR anyway 👀

[evaleev]

Yes, LTO PR would be good.

Alternatively, I could make moving them to the CPP file part of my expr impl cleanup PR, if you prefer. I guess from the benchmark results here, we should run some benchmarks before meeting that PR anyway 👀

This sounds good.