feat(persistent): Redesign CommitStateSpace, bound Commit lifetime

[lmondada]

This PR updates the relrc dependency to v0.5.0 and completely redesigns the CommitStateSpace struct. The result is much cleaner:

• PersistentHugr is an immutable data type following the design of the types of the im crate, i.e. it is a mutable Rust type that is cheap to clone and mutate (immutability -- if desired -- is achieved by cloning before mutating).
• Commit is a "transaction", i.e. one atomic mutation that was applied to a PersistentHugr. It has an ID, obtainable through commit.id() and a lifetime parameter that ensures it does not stay in scope beyond the lifetime of the PersistentHugr
• Each PersistentHugr belongs to a unique CommitStateSpace. A CommitStateSpace is a registry of all commits of the PersistentHugrs within that state space. It keeps the map between commits and IDs.

This is a breaking change, but does not trigger a breaking HUGR release because it is hidden behind an unstable feature.

[codecov]

Codecov Report

❌ Patch coverage is 88.73057% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.00%. Comparing base (d4242b6) to head (bb38518).

Files with missing lines	Patch %	Lines
hugr-persistent/src/persistent_hugr.rs	85.50%	27 Missing and 3 partials ⚠️
hugr-persistent/src/commit.rs	90.21%	16 Missing and 2 partials ⚠️
hugr-persistent/src/state_space.rs	74.07%	12 Missing and 2 partials ⚠️
hugr-persistent/src/commit/boundary.rs	94.64%	5 Missing and 1 partial ⚠️
hugr-persistent/src/walker.rs	86.04%	3 Missing and 3 partials ⚠️
hugr-persistent/src/trait_impls.rs	86.48%	4 Missing and 1 partial ⚠️
hugr-persistent/src/tests.rs	86.95%	3 Missing ⚠️
hugr-persistent/src/persistent_hugr/serial.rs	75.00%	2 Missing ⚠️
hugr-persistent/src/subgraph.rs	90.00%	2 Missing ⚠️
hugr-persistent/src/state_space/serial.rs	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2534      +/-   ##
==========================================
+ Coverage   82.99%   83.00%   +0.01%     
==========================================
  Files         254      255       +1     
  Lines       48040    48086      +46     
  Branches    43550    43596      +46     
==========================================
+ Hits        39873    39916      +43     
+ Misses       6098     6096       -2     
- Partials     2069     2074       +5     

Flag	Coverage Δ
python	91.60% <ø> (ø)
rust	82.12% <88.73%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zrho]

Could you explain what the purpose of the lifetime parameter is in more detail? It looks to me as if the Commit seems to reference the CommitStateSpace and is keeping it alive, so it can't occur that a Commit lives but its CommitStateSpace does not. What is the scenario you want to exclude? Is it simply one where you have Commits that aren't necessary anymore, or is it a safety issue of Commits referencing freed data?

[lmondada]

Of course, sorry that was not clear.

I've expanded the docs of Commit with the following section:

/// # Lifetime of commits
///
/// A commit remains valid as long as the [`CommitStateSpace`] containing it is
/// alive. Note that it is also sufficient that a [`PersistentHugr`] containing
/// the commit is alive, given that the [`CommitStateSpace`] is guaranteed to
/// be alive as long as any of its contained [`PersistentHugr`]s. In other
/// words, the lifetime dependency is:
/// ```ignore
/// PersistentHugr -> CommitStateSpace -> Commit
/// ```
/// where `->` can be read as "is outlived by" (or "maintains a strong reference
/// to"). Note that the dependencies are NOT valid in the other direction: a
/// [`Commit`] only maintains a weak reference to its [`CommitStateSpace`].
///
/// When a [`CommitStateSpace`] goes out of scope, all its commit become
/// invalid. The implementation uses lifetimes to ensure at compile time that
/// the commit is valid throughout its lifetime. All constructors of [`Commit`]
/// thus expect a reference to the state space that the commit should be added
/// to, which fixes the lifetime of the commit.
///
/// Methods that directly modify the lifetime are marked as `unsafe`. It is up
/// to the user to ensure that the commit is valid throughout its updated
/// lifetime.

I hope that clarifies your question!

[acl-cqc]

Does the Rc really make sense here? That means you can cheaply clone a CommitStateSpace, but the clones will then share the state hidden inside the RefCell. I would not expect cloning a CommitStateSpace to be something you want to do often....???

(One can imagine a "fork" operation but TBH I'm not really sure why you would want to do that either ;-) unless Ids are scarce or something)

[lmondada]

Good comment. You are right that cloning could mean two different things here: i) cloning handles to a same state space or ii) creating a new state space that as you say "forks" the state space.

The semantics of cloning here are the former. We need to have multiple handles to the same state space, as every PersistentHugr contains an Rc to the state space it belongs to.

I've considered deleting the Clone implementation (as we often don't want to clone handles ourselves), but there are cases where this is useful, e.g. in testing where we first create PersistentHugr, and then want to create new PersistentHugrs in the same state space as the first.

All in all, I think keeping this as is is the simplest solution. I've added a sentence in the docs specifying the behaviour of cloning.

[acl-cqc]

"(Cache of)" perhaps in that it could be computed (by following the parent pointer from any commit, result would be the same regardless of which commit you started from)

[lmondada]

Indeed. Have added "cache of" as well as written explicitly the invariant that all paths from a commit in self through ancestors will lead to this root.

[acl-cqc]

So this is the bit that might be expensive to clone. I'm not terribly clear from the docs of HistoryGraph as to whether it's cheap, or whether it's actually a complete+owned petgraph constructed from (a cache of) the parent pointers of some set of commits. It might be worth nothing that a persistent (im::) list/set of "leaf commits" would store the same information (perhaps in less accessible form) and really would be cheap to clone...

[acl-cqc]

Ok, it's not that cheap to clone, so a persistent list (inside HistoryGraph) would be better - but storing the complete list of commits (as it does, rather than just leaves) might be best because then clones would only ever append to the list rather than removing nodes that are no longer leaves.

[lmondada]

Yes agreed; I will create an issue for this, to be picked up when this becomes a bottleneck.

[lmondada]

CQCL/tket2#1141