[adamv-symbolica]

No description provided.

[imlvts]

This introduces inconsistency when working on dangling paths:

repro code

//! Restricting by a trie that shares nodes with the restricted one must give the same answer as
//! restricting by an equal trie that shares nothing.
//!
//! `restrict` keeps a path only when the restrictor has a value at it or above it, so a dangling path
//! (structure with no value at or below it) that has no valued ancestor is dropped -- and is, when the
//! two operands are separate allocations.  The `x <| x = x` shortcut keeps it when they are the same
//! node, so the result depends on sharing rather than on contents.  Values are never affected.

use pathmap::PathMap;
use pathmap::zipper::*;

/// Every path in the trie, valued or not, in order: what `restrict` decides about, dangling paths included
fn paths(m: &PathMap<u64>) -> Vec<Vec<u8>> {
    let mut z = m.read_zipper();
    let mut out = Vec::new();
    while z.to_next_step() {
        out.push(z.path().to_vec());
    }
    out
}

/// An equal trie sharing no nodes with `m`, rebuilt path by path
fn unshared(m: &PathMap<u64>) -> PathMap<u64> {
    let mut out = PathMap::new();
    let mut z = m.read_zipper();
    let mut wz = out.write_zipper();
    if let Some(v) = z.val() {
        wz.set_val(*v);
    }
    while z.to_next_step() {
        wz.reset();
        wz.descend_to(z.path());
        match z.val() {
            Some(v) => { wz.set_val(*v); }
            None => { wz.create_path(); }
        }
    }
    drop(wz);
    out
}

fn dangling(paths: &[&[u8]]) -> PathMap<u64> {
    let mut m = PathMap::new();
    for p in paths {
        m.write_zipper_at_path(p).create_path();
    }
    m
}

/// The smallest case: a trie holding one dangling path, restricted by a clone of itself
#[test]
fn restrict_by_a_clone_drops_dangling_paths() {
    let a = dangling(&[&[0]]);
    let by_clone = a.restrict(&a.clone());
    let by_copy = a.restrict(&unshared(&a));
    assert_eq!(paths(&by_copy), Vec::<Vec<u8>>::new());
    assert_eq!(paths(&by_clone), paths(&by_copy), "restrict by a clone kept a dangling path");
}

/// The same through `WriteZipper::restrict`, restricting a map by a zipper over a clone of it
#[test]
fn write_zipper_restrict_by_a_clone_drops_dangling_paths() {
    let a = dangling(&[&[0]]);
    let b = a.clone();

    let mut by_clone = a.clone();
    by_clone.write_zipper().restrict(&b.read_zipper());
    let copy = unshared(&a);
    let mut by_copy = a.clone();
    by_copy.write_zipper().restrict(&copy.read_zipper());

    assert_eq!(paths(&by_copy), Vec::<Vec<u8>>::new());
    assert_eq!(paths(&by_clone), paths(&by_copy), "WriteZipper::restrict by a clone kept a dangling path");
}

/// Values survive either way; only the dangling sibling with no valued ancestor differs
#[test]
fn restrict_by_a_clone_keeps_values_and_drops_unvalued_branches() {
    let mut a = dangling(&[&[0, 0, 0]]);
    a.set_val_at([1, 2], 7);

    let by_clone = a.restrict(&a.clone());
    let by_copy = a.restrict(&unshared(&a));
    assert_eq!(by_clone.iter().map(|(k, v)| (k, *v)).collect::<Vec<_>>(), vec![(vec![1, 2], 7)]);
    assert_eq!(paths(&by_copy), vec![vec![1], vec![1, 2]]);
    assert_eq!(paths(&by_clone), paths(&by_copy), "restrict by a clone kept the dangling branch [0,0,0]");
}

/// The shortcut below the root: the two roots differ, but the subtrie at `p` is one node in both,
/// so the byte node's child restrict meets it as `prestrict_dyn(x, x)`
#[test]
fn restrict_with_a_shared_subtrie_drops_its_dangling_paths() {
    let mut a = PathMap::<u64>::new();
    for (i, key) in [&b"a"[..], b"b", b"c"].iter().enumerate() {
        a.set_val_at(key, i as u64);
    }
    // At `p`: a value further down, and a dangling branch beside it with no value above it
    a.write_zipper_at_path(b"pxx").create_path();
    a.set_val_at(b"pyy", 9);

    let mut b = a.clone();
    b.set_val_at(b"d", 3); // a new root node; the child at `p` is still the one in `a`

    let by_shared = a.restrict(&b);
    let by_copy = a.restrict(&unshared(&b));
    assert!(!paths(&by_copy).contains(&b"pxx".to_vec()));
    assert_eq!(paths(&by_shared), paths(&by_copy), "restrict kept the shared subtrie's dangling branch");
}

[luketpeterson]

The sharing validator is a tool designed to systematically identify sharing that should exist but doesn't. #143

[luketpeterson]

the shortcut if self == other { return } can't be done if you expect dangling paths to be dropped

This is actually semantically changing the behavior of restrict from what's documented. Which is conceptually fine - we invented the op so we're free to change it.

I'm not going to fight you here because restrict doesn't deserve a first-class place in the alg-ops; I see it as a special policy on the meet/subtract category of value-subtrie behaviors. Although to mirror this behavior we'd need it to be a subtrie-subtrie behavior.

[adamv-symbolica]

Perhaps this is something we need to put in an AGENTS.md