Skip to content

focus value handling is inconsistent across API #104

Description

@luketpeterson

I had the clanker audit focus value handling (prompted by the drop_head focus value question) Turns out it's pretty inconsistent because we weren't thinking about focus values when we implemented a bunch of these methods.

The good news is that, where it has mattered, so far (graft, etc), we have a consistent policy. And imposing a consistent policy where we don't have one is unlikely to have major downstream implications to clients. (shouldn't affect MORK at all)

Here are the clanker's results (the recommendations are based on a set of desiderata specified by me, and I think I agree with them):

   API                       graft, graft_src_at, graft_map
   Current default behavior  Replaces focus value with source root value
   Recommended policy        Keep; this is correct replacement semantics. Update docs to say “subtrie,” not “below.”
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       join_map_into
   Current default behavior  Joins map root with focus value
   Recommended policy        Keep; it matches PathMap::join.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       join_into
   Current default behavior  Joins branches only; ignores both focus values
   Recommended policy        Change: join focus values too.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       join_into_take
   Current default behavior  Takes/joins branches only; leaves source focus value behind
   Recommended policy        Change: consume and join the source focus value too.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       join_k_path_into
   Current default behavior  Drops depth-k values; preserves focus value
   Recommended policy        Change as proposed: join depth-k values and replace the focus value.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       meet_k_path_into
   Current default behavior  Under default features, effectively replaces the focus value with the meet of depth-k values
   Recommended policy        This is the model join_k_path_into should match.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       meet_into, subtract_into
   Current default behavior  Include focus values only behind graft_root_vals
   Recommended policy        Make root-value behavior unconditional if this is the chosen API model.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       meet_2
   Current default behavior  Replaces destination branches but ignores both source focus values
   Recommended policy        Change: meet source focus values and replace destination focus value.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       restrict
   Current default behavior  Ignores focus values, unlike PathMap::restrict
   Recommended policy        Change: a source focus value should retain the whole destination subtrie; an absent source
                             focus value must not retain destination’s focus value.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       restricting
   Current default behavior  Root-value behavior is unspecified and undocumented
   Recommended policy        Re-specify before changing; it needs a clear map-level law.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       make_map, try_make_map, take_map
   Current default behavior  Include/move focus value only behind graft_root_vals
   Recommended policy        Make unconditional; these are the foundation for treating a focus as a PathMap.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       graft_masked_branches, graft_child_maps, remove_branches, remove_unmasked_branches
   Current default behavior  Operate on children/branches
   Recommended policy        Keep focus values untouched. Their names and contracts are explicitly branch-scoped.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       insert_prefix
   Current default behavior  Shifts branches; leaves focus value
   Recommended policy        Change alongside join_k_path_into: shift the focus value to prefix, or drop its inverse claim.
  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   API                       remove_prefix
   Current default behavior  Has no root-value contract or coverage
   Recommended policy        Specify separately; it relocates a subtrie and should have an explicit decision about the
                             focused value.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions