Skip to content

[ refactor ] Infrastructure for Data.Tree.AVL.Indexed.*#2968

Open
jamesmckinna wants to merge 36 commits intoagda:masterfrom
jamesmckinna:refactor-AVL
Open

[ refactor ] Infrastructure for Data.Tree.AVL.Indexed.*#2968
jamesmckinna wants to merge 36 commits intoagda:masterfrom
jamesmckinna:refactor-AVL

Conversation

@jamesmckinna
Copy link
Copy Markdown
Collaborator

@jamesmckinna jamesmckinna commented Mar 20, 2026

This PR arises from my review of #2961 and introduces some additional typedefs and lemmas which help streamline the analysis of insert, delete and their various helper join* functions. Specifically, the analysis of base cases for such helpers when one sub-tree is a leaf relies on two new lemmas about instances of the balance relation at height 0.

Systematically uses variable declarations to aid readability; as ever, this technically might be considered breaking, because any given type signature might have subtle differences wrt their prenex implicit forms when fully elaborated, but I think we should regard such things as 'bug fixes' at best (worst?).

Intended to be minimally invasive, but in the course of doing this, I observe:

NB/TODO:

  • ADDED: propagate the use of Tree⁺/Tree⁻ through Properties (downstream/fold into Add properties that characterize Data.Tree.AVL.Indexed.delete. #2961?) DONE
  • the functions defined are not consistent wrt compare k′ k (lookup, delete, etc.) else compare k k′ (insert, insertWith) and this discrepancy should be fixed! But doing so might hold up Add properties that characterize Data.Tree.AVL.Indexed.delete. #2961 so I'd be happy to take responsibility to follow up on this downstream NOT DONE best way to tackle this would also be to fix up the types of lemma statements to use the new freshness predicate below, but that would be a breaking change (because the inequalities get reversed!)
  • doing so would tackle the inconsistency between the existing proofs in Properties and the new ones added in Add properties that characterize Data.Tree.AVL.Indexed.delete. #2961 wrt an as-yet undefined property
    keyNotAt : Key  Any P t  Set ℓ₁
    keyNotAt k p = lookupKey p ≉ k
    which in this form, or its symmetric version, are used throughout. DRY says: standardise this via a definition! (again: downstream?) DONE: added as k # p.
  • ADDED: the Properties of insertWith, for whatever original reasons, now carry a redundant Any- prefix, which seem out-of-step with the current evolution of our naming policy/policies. Easy, but tedious deprecation/renaming fix could be added, or done downstream/in parallel with this PR. DONE.

@jamesmckinna
Copy link
Copy Markdown
Collaborator Author

jamesmckinna commented Mar 25, 2026

NB @mikedelorimier re #2961 I realise (now) that these last commits deprecating names under Properties will likely generate a merge conflict which might be... fiddly to unpick. Apologies! NOW reverted, and pushed them instead to #2970

Now that #2961 has merged, I've reinstated these deprecations.

@jamesmckinna
Copy link
Copy Markdown
Collaborator Author

jamesmckinna commented Apr 2, 2026

Now that #2961 has landed, I'll fix the CHANGELOG and can go back and fix up some of the other TODOs above. NOW DONE.

@jamesmckinna
Copy link
Copy Markdown
Collaborator Author

jamesmckinna commented Apr 3, 2026

Will refactor in the light of the most recent comment on #2970

Copy link
Copy Markdown
Collaborator

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve. Hopefully we can get another review, this is a nice cleanup.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants