Clean up expression implementations

[Krzmbrzl]

Splits the expression class implementations out of the headers and tightens the Expr interface. Mostly mechanical, but a few changes are behavioural — those are called out below.

Motivation

expr.cpp had grown into a catch-all holding the out-of-line bodies of Constant, Variable, Product, CProduct, NCProduct, Sum, HashingAccumulator and ExprPtr, while the rest of each class lived inline in its header. Editing any one expression type meant recompiling everything that includes expr.hpp, and the split between "inline in the header" and "out-of-line in expr.cpp" followed no rule.

At the same time, several Expr virtuals had a base implementation that threw at runtime when a derived class forgot to override them. That turns a class-authoring mistake into a runtime failure in whatever code path first happens to call it.

What changed

One implementation file per expression type. New constant.cpp, variable.cpp, power.cpp, product.cpp, sum.cpp and expr_ptr.cpp; expr.cpp shrinks to just Expr itself. Member functions move out of the headers unless they are templates or genuinely want to be inline. Header includes are pruned to what each header actually needs, with the rest moved to the corresponding .cpp.

A missing override is now a compile error. clone(), adjoint(), type_id() and static_equal() are pure virtual. type_id() and static_equal() previously had a #if __GNUG__ { abort(); } workaround in place of = 0; that is gone. NormalOperatorSequence gains the clone() it was missing.

In-place arithmetic leaves the Expr interface. Expr::operator*=, ^=, += and -= were virtual with throwing defaults, and only Constant, Product, Power and Sum ever overrode them. They are now non-virtual members of those four classes, returning the derived type. The eight call sites that relied on virtual dispatch now name the type they already knew they had, e.g.

-*prefactor *= *factor;
+prefactor.as<Product>() *= *factor;

Each such site was already inside an is<T>() guard or an equivalent invariant.

Memoized hashes are reset on mutation. Variable::conjugate() and Constant::operator*= / += / -= mutated hashed state without calling reset_hash_value(), so a later hash_value() would trip the memoized-vs-recomputed assertion in Debug and silently return a stale hash in Release. Reachable through Sum::append, which folds constant summands in place. Power::conjugate() and Variable::set_label() already did this.

NormalOperator<S>::labels() explicit specializations are now declared after the class template and before any use that would trigger implicit instantiation — an ill-formed-NDR fix.

Behavioural changes

Everything above is behaviour-preserving except:

• clone() no longer slices CProduct/NCProduct. Product::clone() returns ex<Product>(deep_copy()) and neither subclass overrode it, so cloning either one silently produced a plain Product. Since sequant::adjoint(const ExprPtr&) is clone() then adjoint(), this was observable: adjoint() of a CProduct reversed its factors (which CProduct::adjoint() deliberately does not do), and adjoint() of an NCProduct yielded a Product whose is_commutative() is a recursive pairwise check rather than an unconditional false — letting canonicalization reorder factors that must not be reordered. Reachable from Sum::adjoint() over NCProduct summands. Both overrides are added, with regression tests.
• CProduct(Product&&) and NCProduct(Product&&) now move rather than copy; they read Product(other) where other is a named rvalue reference, so they were silently copying.
• Expr::to_latex()'s exception message changed as part of dropping the not_implemented() helper.

Not addressed here

Expr::to_latex() remains a non-pure virtual with a throwing default, unlike the four that became pure. If that asymmetry is deliberate it deserves a @note; if not, it wants the same treatment. Left alone rather than guessed at.

Verification

CI is a unity build, which can mask a missing include in a .cpp, so the include changes were additionally checked against a non-unity Debug build in which every TU compiles standalone. Full unit suite passes (6694 assertions, 62 test cases).

[evaleev]

Review

WARNING this Claude-generated, reviewed by me; some accompanying fixes are in #590

Reviewed the full diff (18 files) against master, plus surrounding context in expr.hpp, expr_ptr.hpp, product.hpp, sum.hpp, op.hpp, wick.impl.hpp and tensor_network/v1.cpp. Line references are as of ab3531c.

The risky mechanical rewrites all check out:

• prefactor.as<Product>() *= *factor (wick.impl.hpp:834) — prefactor is ex<CProduct>(...), and CProduct does not override type_id(), so is<Product>() holds and Product::operator*= is reached exactly as before.
• canon_byproduct.as<Constant>() *= *bp (tensor_network/v1.cpp:461) — canon_byproduct is initialized ex<Constant>(1) at line 62 and never rebound.
• The as<Sum>() +=/-= and as<Product>() *= rewrites in expr_ptr.cpp are all inside is<Sum>()/is<Product>() guards, and neither CProduct nor NCProduct ever overrode those operators, so the devirtualization is behavior-preserving.
• operator==(const ExprPtr&, const ExprPtr&) keeps its hidden-friend declaration at expr_ptr.hpp:87, so moving the definition into expr_ptr.cpp does not silently fall back to shared_ptr pointer comparison. This was the main thing I wanted to rule out.
• Every Expr subclass implements the now-pure clone()/adjoint()/type_id()/static_equal().
• The NormalOperator::labels() explicit-specialization declarations are correctly placed after the class template and before any use that would trigger implicit instantiation — a genuine ill-formed-NDR fix.

Findings below. The NormalOperatorSequence::static_equal self-comparison I hit while reviewing this is pre-existing rather than yours, so it is split out into #590 against master; worth noting here only because adding clone() at op.hpp:1071 is what first lets those objects into Sum/Product and hence into simplify/canonicalization.

---

1. Variable::conjugate() does not reset the memoized hash — variable.cpp:50

void Variable::conjugate() { conjugated_ = !conjugated_; }

memoizing_hash() (line 22-25) folds conjugated_ into the hash and asserts memoized-vs-recomputed consistency:

auto v = ex<Variable>(L"x");
v->hash_value();   // memoizes
v->adjoint();      // -> conjugate(), flips conjugated_, no reset
v->hash_value();   // SEQUANT_ASSERT fires in Debug; stale hash in Release

Variable::set_label (line 45-47) and the sibling Power::conjugate() (power.cpp:44) both reset, so this is inconsistent within the same layer. Moved code rather than new breakage, but this PR is the natural place to fix it.

2. Constant::operator*=, +=, -= do not reset the memoized hash — constant.cpp:24, :33, :42

All three mutate value_ and return, while Constant::memoizing_hash() (line 55) asserts the memoized hash still matches value_. Constant::adjoint() (line 19) does reset, so the file is self-inconsistent.

Reachable through Sum::append, which folds constants in place:

Sum s;
s.append(ex<Constant>(1));
s.hash_value();          // Sum::memoizing_hash -> hash::range over dereferenced
                         // summands, memoizing the Constant summand's own hash
s.append(ex<Constant>(2));  // sum.hpp:135 -> summands_[i].as<Constant>() += *summand
                            // mutates the resident Constant 1 -> 3 in place
s.hash_value();          // assert fires in Debug; stale hash silently used in Release

Note Sum::append resets the Sum's hash, not the folded-into Constant's.

3. Expr::not_implemented() is declared but no longer defined — expr.hpp:485

Exception not_implemented(const char *fn) const;

The definition was deleted from expr.cpp along with the throwing default operators, and git grep finds no other definition in the tree. It links today only because nothing calls it. Any future use — in-class, or from a derived class in another TU, since the name is still visible in the inherited context — becomes an undefined reference at link time rather than a compile error. Suggest deleting the declaration.

4. clone() and adjoint() docs still describe the removed throwing default — expr.hpp:82, :244

/// @note - must be overridden in the derived class.
///       - the default implementation throws an exception
virtual ExprPtr clone() const = 0;
...
/// @note base implementation throws, must be reimplemented in the derived class
virtual void adjoint() = 0;

Both are = 0 now. Since the point of the PR is that a missing override is a compile error rather than a runtime throw, these comments currently assert the opposite of the new contract.

5. Product::operator*= is still virtual with nothing to override — product.hpp:364

The Expr base declaration was removed in this PR and nothing derives-and-overrides it. Sum::operator+=/-=, Constant::operator*=/+=/-= and Power::operator*= all correctly dropped virtual in the same change, so this reads as an oversight; it leaves a vestigial vtable slot that suggests base-level dispatch which no longer exists.

[Copilot]

Pull request overview

This PR refactors SeQuant’s expression subsystem by making key Expr APIs mandatory at compile time and moving multiple expression implementations out of headers and the monolithic expr.cpp into dedicated translation units. This aligns expression behavior with stricter interfaces and reduces header/compile coupling, while updating call sites and tests accordingly.

Changes:

• Make Expr::clone(), Expr::adjoint(), Expr::type_id(), and Expr::static_equal() pure-virtual, removing the prior “throwing default” behavior.
• Split implementations for Constant, Variable, Power, Product, Sum, and ExprPtr operators into new .cpp files; update build sources and affected call sites.
• Adjust tests and selected algorithms/canonicalization code to use concrete expression operations now that generic virtual arithmetic hooks were removed.

Reviewed changes

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

Show a summary per file

File	Description
tests/unit/test_expr.cpp	Updates unit tests and dummy Expr implementations to satisfy new pure-virtual interface.
SeQuant/core/wick.impl.hpp	Updates prefactor multiplication to use concrete expression types.
SeQuant/core/tensor_network/v1.cpp	Updates byproduct accumulation to use concrete Constant operations.
SeQuant/core/op.hpp	Adds explicit NormalOperator<...>::labels() specializations and NormalOperatorSequence cloning.
SeQuant/core/expressions/expr.hpp	Makes several Expr methods pure-virtual (interface hardening).
SeQuant/core/expressions/expr.cpp	Removes moved implementations; keeps base to_latex() throwing implementation.
SeQuant/core/expressions/expr_ptr.hpp	Declares non-template ExprPtr binary operators (now out-of-line).
SeQuant/core/expressions/expr_ptr.cpp	Defines ExprPtr operators and ExprPtr helper methods formerly in other TUs/headers.
SeQuant/core/expressions/expr_operators.hpp	Retains only templated mixed ExprPtr/scalar and ExprPtr/label operators.
SeQuant/core/expressions/constant.hpp / constant.cpp	Moves Constant implementation out-of-line.
SeQuant/core/expressions/variable.hpp / variable.cpp	Moves Variable implementation out-of-line.
SeQuant/core/expressions/power.hpp / power.cpp	Moves Power implementation out-of-line.
SeQuant/core/expressions/product.hpp / product.cpp	Moves Product/CProduct/NCProduct implementation out-of-line.
SeQuant/core/expressions/sum.hpp / sum.cpp	Moves Sum and HashingAccumulator implementation out-of-line.
CMakeLists.txt	Adds new .cpp files to the build.

Suppressed comments (1)

SeQuant/core/expressions/sum.cpp:217

• std::swap(*this, *new_sum) will move-assign the Expr base (including the std::enable_shared_from_this subobject). Since shared_from_this() is called immediately afterwards for logging, this can produce a bad_weak_ptr throw or an incorrect self pointer when canonicalization logging is enabled. Swap only the Sum data members (and the memoized hash) instead of swapping the full object.

    auto new_sum =
        (pass == npasses - 1) ? acc.make_canonicalized_sum() : acc.make_sum();
    using std::swap;
    swap(*this, *new_sum);

    if (Logger::instance().canonicalize)
      std::wcout << "Sum::canonicalize_impl (pass=" << pass
                 << "): after reducing summands = "
                 << to_latex_align(shared_from_this()) << std::endl;

---

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

[evaleev]

Correct, and already fixed in 53e3585 — the comment was copy-pasted from CProduct::adjoint(). It now reads "factors must be reversed since they do not commute". Same commit also moved this function back to range-v3 views:: for consistency with Product::adjoint() and CProduct::adjoint() in the same file, which it had diverged from.

[evaleev]

The include-hygiene point is right and is fixed in b2a5204, but the stated consequence is not: this compiles today. constant.cpp includes constant.hpp, which includes complex.hpp, which includes hash.hpp. Verified with clang++ -H on a non-unity build:

. SeQuant/core/expressions/constant.hpp
.. SeQuant/core/complex.hpp
... SeQuant/core/hash.hpp

It is still worth including directly — power.cpp and variable.cpp already do — so the header is now explicit.

[evaleev]

Fixed in b2a5204, for both sum.cpp and sum.hpp. Note sum.hpp needs it in its own right, not just as a conduit: line 198 declares container::unordered_set<ExprPtr, sequant::hash::_<ExprPtr>, proportional_to>, so the header is used in a class member declaration. tensor.hpp is the precedent — it includes hash.hpp directly for the same reason.

As with the sibling comments, this was not a build break: sum.hpp includes constant.hpp -> complex.hpp -> hash.hpp.

[evaleev]

Fixed in b2a5204. Same as the sibling comments: not a build break — product.hpp includes constant.hpp, which reaches hash.hpp via complex.hpp — but the direct include belongs there and is now present.

[evaleev]

Review

WARNING this is Claude-generated, reviewed by me. Unlike my last pass, this one comes with fixes: I pushed four commits to this branch (92c8eeb2..25ada930) rather than leaving a list of nits. Revert anything you disagree with.

Reviewed the full diff (20 files, +1466/−1285) against merge-base 3168a655, plus surrounding context in expr.hpp, expr_ptr.hpp, op.hpp, wick.impl.hpp and tensor_network/v1.cpp. Line references are as of 92c8eeb2.

My 2026-08-13 review is fully addressed

All five items landed in the 2026-08-20 force-push — Variable::conjugate() and the three Constant in-place operators now reset the memoized hash, Expr::not_implemented() is gone, the stale clone()/adjoint() @notes are gone, and Product::operator*= is non-virtual. Thanks.

One bookkeeping note: the resolved product.hpp thread with @ajay-mk concluded to keep virtual on Product::operator*=, and the code now does the opposite. Removing it is right — neither CProduct nor NCProduct overrides operator*=, and both keep Product's type_id(), so prefactor.as<Product>() *= *factor still reaches the same body — but the thread and the code disagree, so it's worth a line here lest it get "fixed" back.

Verification of the mechanical rewrites

All eight *expr op= *that → expr.as<T>() op= *that sites, and each is type-safe:

• expr_ptr.cpp:52,68,84 — inside is<Sum>() / is<Product>() guards.
• sum.cpp:28,54,79 — guarded by summand->is<Constant>() plus the constant_summand_idx_ invariant.
• v1.cpp:461 — canon_byproduct is ex<Constant>(1) at v1.cpp:62, same function (canonicalize, lines 59–470), never rebound; re-asserted at v1.cpp:467.
• wick.impl.hpp:834 — as above.

Sum::canonicalize_impl's this->swap(*new_sum) → using std::swap; swap(*this, *new_sum) is equivalent: there is no free sequant::swap, the deleted member was never reachable from that unqualified call anyway (the block-scope using std::swap shadows class scope), and std::swap performs exactly the move-ctor/two-move-assign sequence the member did.

Every Expr subclass in the tree implements the four now-pure virtuals — Tensor, Operator<S>, NormalOperator<S>, NormalOperatorSequence<S> (clone() added here at op.hpp:1071), mbpt::Operator<void,S>, and the three test fixtures.

Fixed: CProduct/NCProduct were sliced by clone()

This is the one with correctness weight, and it predates the PR — but it is squarely the thing this PR is about, so I fixed it here (1ad9644).

Product::clone() returns ex<Product>(this->deep_copy()) and neither subclass overrode it. Making Expr::clone() pure virtual does not catch this: the base override exists, so the compiler is satisfied and the derived type is silently dropped. Since sequant::adjoint(const ExprPtr&) is clone() then adjoint(), the slice was observable:

• adjoint() of a CProduct reversed its factors, which CProduct::adjoint() deliberately does not do;
• adjoint() of an NCProduct yielded a Product, whose is_commutative() is a recursive pairwise commutes_with check instead of an unconditional false — so canonicalization may reorder factors that must not be reordered.

Reachable from Sum::adjoint() over NCProduct summands, among others. Both clone() overrides are added, with regression tests in test_expr.cpp's clone section. I confirmed both tests fail on 92c8eeb2 and pass after:

test_expr.cpp:518: FAILED:
  REQUIRE( e_adj.as<Product>().factors()[0]->as<Adjointable>().v == -1 )
with expansion: 2 == -1          # CProduct's factors got reversed

test_expr.cpp:531: FAILED:
  REQUIRE_FALSE( e_clone.as<Product>().is_commutative() )
with expansion: !true            # NCProduct's clone reports itself commutative

Fixed: three smaller things

• NCProduct::adjoint() silently switched range library (53e3585). It moved to std::views::reverse | std::views::transform while Product::adjoint() and CProduct::adjoint() in the same file kept range-v3, and the result is still handed to ranges::begin/ranges::end. Behavior-equivalent, but an unexplained switch inside a commit whose message is "Cleanly separate (N)CProduct impl". Reverted to views::. Also fixed the comment above it, which was copy-pasted from CProduct and claims no reversal is needed — NCProduct::adjoint() does reverse, as it must.
• product.cpp pulled <range/v3/all.hpp> (da3050f). The code it absorbed from expr.cpp was served by nine targeted headers; all.hpp is a well-known compile-time sink and this TU already includes tensor_network.hpp. Restored the targeted list. Also added <SeQuant/core/algorithm.hpp> explicitly — bubble_sort only resolved transitively through tensor_canonicalizer.hpp. Worth knowing that CI builds this with CMAKE_UNITY_BUILD=ON, so a missing include here can be masked by whatever TU it gets concatenated with; I verified product.cpp compiles standalone.
• Leftovers (25ada93): stray ; after the out-of-line Product::type_id() and Sum::type_id() definitions; the dead using std::swap; in Product::adjoint(), which no longer has a swap() call beside it now that Sum::swap() is gone; and Product::is_commutative()'s @note this is memoizing, which the implementation contradicts.

Not changed — two things for you

• CProduct(Product&&) / NCProduct(Product&&) now move instead of copy (product.cpp:337,381): Product(other) became Product(std::move(other)). That is a genuine fix — the old code copied from an rvalue parameter — and it is safe, since the only in-tree caller (operator^ in expr_ptr.cpp) binds an lvalue and still selects the copy ctor. But it is a semantic change buried in a commit that reads as pure motion; worth its own line in the commit message. I left the code alone.
• Expr::to_latex() is still a throwing non-pure default while clone()/adjoint()/type_id()/static_equal() went pure. If that is deliberate — classes with no sensible LaTeX form — fine, but the asymmetry isn't documented, and I didn't want to invent a rationale for it. Your call whether to make it pure or to add a @note.

Verification

Configured a non-unity Debug build (clang++, -Werror -Wall -Wpedantic -Wextra) so every TU compiles standalone. unit_tests-sequant: all 6694 assertions in 62 test cases pass. clang-format-17 --dry-run --Werror clean on all four touched files.

Nothing else blocks merge from my side.