ExprContainer with value semantics

[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.