[Krzmbrzl]

This is supposed to be an alternative (longer-term perhaps even replacement) for ExprPtr. It can be used to store expressions but contrary to ExprPtr, the new ExprContainer has value semantics. That is, copying the container actually copies the underlying expression. This makes things much easier to reason about and as a side-effect this fixes the const issue of ExprPtr which allows you to do

void func(const ExprPtr &ptr) {
  auto copy = ptr; // only copies _pointer_
  expand(copy);
}

which ends up modifying the original expression ptr was is pointing to. Hence, with ExprPtr we don't have any way to avoid accidental modification.

---

Note: This PR is based on top of #589

[evaleev]

@Krzmbrzl would it make sense to do copy-on-mutable-access to save on copying in trivial cases?

[Krzmbrzl]

would it make sense to do copy-on-mutable-access to save on copying in trivial cases?

I don't have experience implementing something like this but it seems like that would significantly complicate the implementation, no? Also, I would really like to not give up on the implicit conversion to (const) Expr &(&) which is what almost all operations act on. Having a copy-on-write seems like it would require introducing something like ExprContainer::to_expr() and Expr::to_mutable_expr() where the former returns const Expr & and the latter returns a copy (which would then still have to be stored in its own container again) 🤔

EDIT: Or are you strictly speaking about the copies performed during construction of ExprContainer instances themselves?

[Krzmbrzl]

@evaleev Here's my proposal for how to proceed with this change:

1. Decide on the final name for ExprContainer - see below
2. Merge this PR as-is (or with ExprContainer renamed to whatever we decide)
3. Perform migrations in a series of separate PRs
   1. Deprecate copy and copy-assignment ctor's of ExprPtr and fix all warnings this causes
   2. Deprecate "forwarding" interface of ExprPtr. With that, I mean all member functions that allow to do ptr.func(…) instead of having to do ptr->func(…), e.g. such as member is<…>() and fix all warnings this causes
   3. Get rid of all cases in which we use a null-ExprPtr. That is, deprecate the default-ctor, deprecate conversion to bool that checks whether the ptr is null or not, deprecate comparison overloads for nullptr, etc. In other words, make all code work under the assumption that a provided ExprPtr is always non-null. In certain cases such as canonicalize(…) this will require a change in the return type that should either return optional<Constant> or Constant and we return 1 in case no sign change is necessary. This change is all about getting rid of expression objects that don't actually represent any expression.
   4. Make ExprPtr implicitly convertible to Expr & and change function interfaces that currently take a const ExprPtr & reference and instead make them take a const Expr &. Due to the pointer semantic of ExprPtr, taking ExprPtr instead of Expr & does not signify any kind of ownership transfer and hence the conversion should be preserving existing semantics. Any issues caused in function bodys due to the need of an ExprPtr are most likely signs of a forgotten clone to prevent changing the input.

Up to this point, all changes should be backwards-compatible except for changing things like the return type of the internal canonicalize functions, which I don't expect to really appear in downstream user code.

Finally, changing how Sum and Product store expressions will require changing the interface by which we iterate over expressions to iterate over either ExprContainer & or Expr & where the former would be in-line with current semantics of being able to overwrite e.g. a summand with a different expression while iterating over the sum. The latter would forbid that. I don't think there is a way to make this change backwards-compatible. Hence, I would propose using this change as a reason to switch SeQuant to major version 3. In the same change, we could also completely get rid of the ExprPtr class.

An alternative strategy for the final step would be to remove the ExprPtr class and rename ExprContainer to ExprPtr. This would also represent a breaking change unless we mirror all deprecated APIs of the latter in the former (which I don't think is a good idea) and even then I dislike using the name ExprPtr for something that is very intentionally designed to not behave like a pointer.

In a bump to SeQuant v3, we could also perform renaming of e.g. Expr -> ExprBase, if we feel like it.

---

About the name

In the meeting ExprValue instead of ExprContainer was suggested. After thinking a bit more about it, I'm not sure I am quite convinced. ExprValue doesn't seem quite as descriptive to me. After all, the whole point is that you only need ExprContainer to store an expression. Most functions work on references anyway and these should just take a reference of Expr and not of ExprContainer in most cases. That would also prevent the issue we currently have that you have to convert an expression to an ExprPtr just for the sake of being able to call a function. ExprValue seems like it would semantically more encourage writing functions taking a reference to it rather than a Expr.

If ExprContainer (13 chars) is too long for your taste, I would propose one of ExprHolder (10), ExprCrate (9) or ExprStore (9). My favorite being ExprHolder (by quite a margin). In fact, I think I might like ExprHolder even more than ExprContainer 🤔

Copy vs Clone

The new ExprContainer doesn't have a clone() function. Instead, it provides a copy() function. The reason why I chose to switch to a different API is that clone as a public API function is not needed once we use ExprContainer. Due to its value-semantics, it will implicitly clone the represented expression whenever it is copied (or whenever a non-rvalue expression is assigned to it). Hence, I believe that all code that currently uses clone() should just use this regular copy-semantic instead.

Then why create a copy() function instead? Because the copy ctor of ExprContainer is explicit and always writing ExprContainer(other) to copy is cumbersome. This is only required for copies though. The move ctor is implicit. Hence, the copy() function is merely intended as syntactic sugar for the explicit copy ctor whereas clone has an important semantic meaning (distinct from just copying the object). Therefore, I figured that these functions should have different names.

[Krzmbrzl]

@evaleev how do you want me to proceed with this? (see writeup above)

[evaleev]

Thanks for the write-up and the ping. I built the branch in a worktree (Debug, SEQUANT_ASSERT_BEHAVIOR=THROW) and checked each point below with a compile or runtime probe; CI is green only because the tests never exercise these paths. Verdict: needs changes before merge, and the branch has to be rebased first (540 commits behind, 7 conflicting files: constant.cpp, expr.cpp, five under export/; master now has 15 clone() const override sites that must become unique_copy, and #686 rewrites Power::flatten).

Blocking

1. Container-with-container arithmetic is broken for all four operators. a + b, a - b, a ^ b with two ExprContainers are ambiguous between the (ExprContainer, Expr) and (Expr, ExprContainer) overloads. a * b compiles because expr_container.hpp#L78 declares a (ExprContainer, ExprContainer) overload, but nothing defines it, so it fails at link time; line 79 re-declares the (Expr, Expr) form instead of the missing mixed one.
2. Sum(container) and Product(container) silently produce an empty sum/product. ExprContainer has begin()/end() over subexpressions, so it satisfies the range constraint of the range constructors (sum.hpp#L60, product.hpp#L53). A container holding a Variable yields a Sum of size 0 with no diagnostic; same for ex<Sum>(cont).
3. Sum::filter no longer compiles when instantiated. The transform lambda at sum.hpp#L119 returns *e by deduced value, i.e. the abstract Expr. No in-tree callers, so it is invisible until someone uses it.

Should fix

• The "move" constructor deep-copies. ExprContainer(Expr&&) (expr_container.cpp#L31) calls the const unique_copy(); the source is left intact (probe: a Sum with 3 summands still has 3 after being "moved" into a container). Every *this = Sum(...) inside the in-place operators therefore clones the freshly built tree a second time. The explicit-copy/implicit-move split in the design rests on a move that does not exist; a real move needs a virtual rvalue hook.
• Moved-from containers are undetectable. After a move or take_expr(), expr_ is null. operator*, operator-> and the copy ctor have no assert, so copying a moved-from container segfaults even under THROW; only begin()/end() assert. ExprContainer(const ExprPtr&) with a null pointer dereferences before the assert at line 25 can fire.
• Root-level visit semantics changed silently. A const ExprPtr& visitor now receives expr.clone() for the root (expr.hpp#L422); before it received the real object via exprptr_from_this, so mutations through the pointer and identity checks applied. Probe: a visitor relabelling the root Variable now has no effect. The in-tree const ExprPtr& visitors (wick.impl.hpp, utility/indices.hpp, biorthogonalization.cpp) only read, so nothing breaks today, but downstream code can.
• The deprecated hash_value(shared_ptr hasher) overload deep-clones the whole expression on every call (expr.hpp#L270) and hands the hasher a different object, so identity-keyed hashers change answers.
• Explicit copy ctor makes std::copyable<ExprContainer> false. std::vector<ExprContainer>{a} does not compile, nor will any std::ranges/range-v3 view or algorithm requiring copyable elements. This matters directly for the plan to store containers in Sum::summands_.
• Duplicated arithmetic semantics. The four in-place ExprContainer operators re-implement the ExprPtr operator logic in expr_ptr.cpp; the two have already diverged (container clones factors, pointer aliases them). Per AGENTS.md this should be one implementation.
• No scalar arithmetic. cont + 2 does not compile while ptr + 2 does, since expr_operators.hpp was narrowed to exactly ExprPtr.
• Test gaps: container-with-container operators, ^=, adjoint(ExprContainer), iteration, take_expr, Power::flatten(ExprContainer&), Power::unique_copy with conjugation, Sum::filter, visiting an expression not managed by shared_ptr (the removed throw).

Minor

• ExprContainerList (expr_container.hpp#L67) is unused, and with an explicit copy ctor elements cannot be copied out of it.
• Power(lvalue_container, exp) does not compile (by-value parameter + explicit copy). Fine if intended, but should be documented.
• Typos: flatt_impl (power.cpp#L41), test sections "from conrete", "Variabkle", "from base".
• Several commits fix earlier commits in the same PR ("Move begin/end impl to cpp file (and fix it)", "Fix deprecation warning in test_expr", "Include supporting header"); please squash those into the commits they fix.
• After rebase, doc/user/guide/expressions.rst needs the new type and the changed visit contract.

On the proposal

• Merge as-is: not possible; the three blocking items plus the rebase come first.
• 3.1 deprecate ExprPtr copy: not backwards-compatible in practice. ExprPtrList is an initializer_list, every summands() range copies pointers, and MPQC builds with warnings as errors. It also lands before the storage change that would make it natural, so call sites get churned twice.
• 3.3 remove null ExprPtr: the accumulator idiom (ExprPtr r; for (...) r += x;) is everywhere, and ExprContainer has no empty state and no default ctor. The proposal needs an answer for that before deprecating null, beyond the canonicalize return types.
• 3.4 interfaces take const Expr&: sound, but functions that replace the root (simplify, expand, Power::flatten) must take ExprContainer&, which forces callers holding ExprPtr to copy.
• Final step (containers inside Sum/Product, v3): the copyable issue above bites here. There is also no performance evidence yet; subtree sharing and memoized hashes are lost under value semantics, so I would like a benchmarks/ run before committing to it.
• Name: "Container" is a poor fit because the class is itself an iterable range over subexpressions, so readers will ask "container of what". ExprHolder avoids that; I am fine with it.
• Copy vs copy(): reasonable, but the design should state outright that the type is not std::copyable and what that implies for ranges.

The core idea is right and the unique_copy refactor is clean. Fix the blocking items, make the rvalue path actually move, rebase, and then the migration sequence is worth settling, with 3.1 and 3.3 reconsidered as above.