Resolving spec inconsistency caused by join allowing value annihilation

[luketpeterson]

• Documenting that returning AlgebraicStatus::None, or AlgebraicResult::None from a join-flavored function is illegal
• Fixing Lattice impl on Option<T> so it follows the rules
• Adding debug_checks to be on the lookout for impls that break the rules
• Simplifying pathways that had a path for None results from join
• Improving LineListNode's validity checks

To be clear, the reason this is so horrible is because join assumes certain laws to optimize its implementation. Specifically it assumes:

• A U B = superset of A and of B
• A U empty = A
• empty U B = B
  etc.

If the trait is allowed to return None, then a non-empty A, join with an empty would be empty. Meaning join (and also meet by conceptual extension) becomes a generic algebra, and it's no longer possible to have optimizations that rely on one-directional-movement on the lattice.

[Adam-Vandervorst]

Yes, the strongest statement should hold. Equal to one of the arguments is stronger than being zero.

[imlvts]

These places can trigger the debug assert:

LatticeRef for Option<&V>
Lattice for Option<TrieNodeODRc>
LatticeRef for Option<&TrieNodeODRc>
set_lattice!
WriteZipper::{join_into, join_map_into, join_into_take}

repro

  //! Reproducers for `Lattice` impls in the crate that still return `AlgebraicResult::None`
  //! from a join, tripping the debug asserts added in PR #98.
  //! Every test below panics in a debug build.
  
  use std::collections::HashSet;
  use pathmap::PathMap;
  use pathmap::ring::Lattice;
  
  // ---- PathMap is itself a Lattice, and pjoin of two empty maps is still None ----
  
  /// `Option<V>::pjoin` -> debug_assert at src/ring.rs:709
  #[test]
  fn option_of_empty_pathmap() {
      let a: Option<PathMap<u64>> = Some(PathMap::new());
      let b: Option<PathMap<u64>> = Some(PathMap::new());
      let _ = a.pjoin(&b);
  }   
  
  /// Empty maps stored as values under the same key, joined through the trie
  /// -> debug_assert in `merge_guts` at src/line_list_node.rs:1334
  #[test]
  fn pathmap_of_empty_pathmaps() {
      let mut m1 = PathMap::<PathMap<u64>>::new();
      m1.set_val_at(b"a", PathMap::new());
      let mut m2 = PathMap::<PathMap<u64>>::new();
      m2.set_val_at(b"a", PathMap::new());
      let _ = m1.join(&m2);
  }   
   /// Empty maps stored as values under the same key, joined through the trie
  /// -> debug_assert in `merge_guts` at src/line_list_node.rs:1334
  #[test]
  fn pathmap_of_empty_pathmaps() {
      let mut m1 = PathMap::<PathMap<u64>>::new();
      m1.set_val_at(b"a", PathMap::new());
      let mut m2 = PathMap::<PathMap<u64>>::new();
      m2.set_val_at(b"a", PathMap::new());
      let _ = m1.join(&m2);
  }   
  
  // ---- set_lattice!-derived impls return None when the joined set is empty ----
  // (`set_lattice_integrate_into_result`: `if result_len == 0 { AlgebraicResult::None }`)
  
  /// `Option<V>::pjoin` -> debug_assert at src/ring.rs:709
  #[test]
  fn option_of_empty_hashset() {
      let a: Option<HashSet<u64>> = Some(HashSet::new());
      let b: Option<HashSet<u64>> = Some(HashSet::new());
      let _ = a.pjoin(&b);
  }   
  
  /// Empty sets stored as values under the same key -> src/line_list_node.rs:1334
  #[test]
  fn pathmap_of_empty_hashsets() {
      let mut m1 = PathMap::<HashSet<u64>>::new();
      m1.set_val_at(b"a", HashSet::new());
      let mut m2 = PathMap::<HashSet<u64>>::new();
      m2.set_val_at(b"a", HashSet::new());
      let _ = m1.join(&m2);
  }   
  
  /// In-place variant: `Lattice::join_into` default impl -> debug_assert at src/ring.rs:561
  #[test]
  fn hashset_join_into() {
      let mut a: HashSet<u64> = HashSet::new();
      let _ = a.join_into(HashSet::new());
  }

[imlvts]

It's possible to cleanly express this contract on a type level (thus eliminating the debug assert), but it would break the implementors of Lattice.
I think leaving as it is would be better.

[luketpeterson]

It's possible to cleanly express this contract on a type level (thus eliminating the debug assert)

What do you have in mind? Because I don't want to contaminate the return types, but the idea of making an invalid result unrepresentable sounds really compelling. As evidenced by the fact that a bunch of of the implementations failed to honor the spec and the debug_asserts didn't catch them as used by the tests.

but it would break the implementors of Lattice. I think leaving as it is would be better.

We don't have that many downstream clients, and v0.4.0 is going to be a big API revision. (Blind zippers, etc.)

[imlvts]

Here's the thing I'm tentatively suggesting.
The difference is, pjoin just can't return None, because it's not constructible anymore.
This makes pmeet usage a bit more annoying. (alternatively, have a second type for pmeet)

// No `None`
pub enum Ident { Myself, Counterpart, Both }
// type Ident = u64; was u64, but not really used beyond 3 values?
pub enum AlgebraicResult<V> { Identity(Ident), Element(V) }
pub enum AlgebraicStatus { Element, Identity }

pub trait Lattice {
    const IDEMPOTENT: bool = true;
    fn pjoin(&self, other: &Self) -> AlgebraicResult<Self> where Self: Sized;
    fn join_into(&mut self, other: Self) -> AlgebraicStatus where Self: Sized;
    fn pmeet(&self, other: &Self) -> Option<AlgebraicResult<Self>> where Self: Sized;
    // meet returns `None` -> annihilate
}

[luketpeterson]

Here's the thing I'm tentatively suggesting...

I see. At first I didn't like Option<AlgebraicResult<V>> because it redefines the meaning of AlgebraicResult. I.e. to me "everything cancelled" is a meaningful positive result, and not the absence of a result as is usually implied by Option::None.

But the I asked the Fable to take your position and argue for that, and it may have convinced me that the lattice math favors the semantic of Option<AlgebraicResult<V>> vs MeetResult<V> / SubtractResult<V>, which would be the other options.

I also validated that the types are the same size, regardless of whether the option wraps the type or None is one of the enum variants.

[marcin-rzeznicki]

Couldn't Lattice operations use just Option<Cow<'a, V>> - it composes better; handles Element vs Identity; lets the caller do chain computation on references without cloning; the caller can call into_owned just once if they want to store it somewhere (and sometimes it will be free)

[luketpeterson]

Couldn't Lattice operations use just Option<Cow<'a, V>>...

That's a really interesting idea. But without spending serious time prototyping it, I have a hunch it would get into hairy lifetime territory. The other downside is that is loses the information about the provenance in the case of an identity result.

Providing this kind of feature is what I was trying to do with AlgebraicStatus::unwrap_or_else, but it means you need to supply a table or a closure that can map backwards from an identity index to the V to clone from.

I think a layer of cow-returning convenience wrappers around the algebraic functions might be an worthwhile thing to provide, but doing it in the core of the algebra feels a little wrong to me.

[luketpeterson]

On Discord, @imlvts said:

I also wanted to get rid of AlgebraicStatus by having something like Discriminant<AlgebraicResult>

I added AlgebraicStatus reluctantly. But ultimately there were two reasons.

1. The type parameter isn't needed but the Discriminant wrapper insists on it due to how it sits in the type solver. By itself this is merely ugly, but it means the signature is infectious and may cause deeper problems for upstream users.
2. Having our own type for AlgebraicStatus gives us a site for trait and method implementations that would be more limited if we made it a type alias, due to orphan rules.

We could work around either of these. But on balance I felt like a dedicated AlgebraicStatus was the least ugly of the options.

[luketpeterson]

For reference, the https://github.com/Adam-Vandervorst/PathMap/tree/lattice_returns_option_experiement branch has the Option<AlgebraicResult<V>> idea fully implemented. But I think we aren't going that way.

[luketpeterson]

I laid out the API distinction I'd like to make, in the form of couple of paragraphs in the book. Check the diffs on this commit: dcff186

[luketpeterson]

I don't think we'll be merging this for a while... Converting to draft.