Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: unbundle array size constraint from hash map bucket array #748

Open
wants to merge 370 commits into
base: nightly-testing
Choose a base branch
from

Conversation

TwoFX
Copy link
Member

@TwoFX TwoFX commented Apr 17, 2024

This PR makes sure that the positivity checker is happy with the following construction:

inductive ContainsItselfInHashMapValues where
  | base
  | recursive (l : HashMap.Imp Bool ContainsItselfInHashMapValues)

Users actually want to do this, see this article (search for "recursive datatype") and the code. The users had to build their own primitive map (see here), but arguably the standard HashMap.Imp should just support this. Users will have to pass the well-formedness constraints around by hand, but that is much better than nothing.

To make this possible, HashMap.Imp.Buckets had to lose the constraint that the bucket array has positive size. This is now part of Buckets.WF. Without knowing that there is at least one bucket, none of the operations are able to access the bucket array, so they are all wrapped in an if 0 < buckets.size. It's not pretty, but we should be able to live with the performance hit. I fixed all of the well-formedness lemmas, though I struggled with expand_size and ended up rewriting that one into a style that I hope is easier to follow for someone without Mario-level Lean skills :-)

This PR targets nightly-testing for now because it uses the new functional induction principles from Lean 4.8.

@TwoFX TwoFX added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Apr 17, 2024
@digama0
Copy link
Member

digama0 commented Apr 17, 2024

I'm concerned about the precedent that this sets. The rules on nested inductives are very strict and also abstraction-breaking, so having such a promise limits our ability to hide implementation details and evolve the library in the future. I think it would be better to support nested inductives of this kind through an external package (or future improvement to the inductive command) which has more semantic requirements on nested inductive occurrences.

@@ -102,8 +110,10 @@ already in the array, which is appropriate when reinserting elements into the ar
-/
@[inline] def reinsertAux [Hashable α]
(data : Buckets α β) (a : α) (b : β) : Buckets α β :=
let ⟨i, h⟩ := mkIdx data.2 (hash a |>.toUSize)
data.update i (.cons a b data.1[i]) h
if hd : 0 < data.1.size then
Copy link
Member

@digama0 digama0 Apr 17, 2024

Choose a reason for hiding this comment

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

This should be a parameter to the function. There is no need to be doing this check here. You can also avoid having an additional parameter in the IR by smuggling the property in a subtype, as in (data : {b : Buckets α β // 0 < b.size}).

@TwoFX TwoFX added awaiting-author Waiting for PR author to address issues and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Apr 18, 2024
@digama0
Copy link
Member

digama0 commented Apr 19, 2024

Here's a counterproposal which includes elements of my other suggestion and also leads to a much smaller diff, while still meeting the goal of having HashMap.Imp be nested-inductive-safe:

  • Change the definition of HashMap.Imp to contain buckets : Array (AssocList α β) instead of buckets : Imp.Buckets α β.
  • Buckets retains its original definition as {b : Array (AssocList α β) // 0 < b.size}, and all the operations use this type
  • HashMap.WF gets a size : 0 < buckets.size field as in this PR
  • Operations on HashMap like HashMap.insert recombine the buckets from the Imp with the property from WF on the fly

This way, there is no need for if statements in the main functions, and most of the hard proofs are unaffected.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Apr 23, 2024
@kim-em kim-em deleted the branch nightly-testing May 3, 2024 02:49
@kim-em kim-em closed this May 3, 2024
@kim-em
Copy link
Collaborator

kim-em commented May 3, 2024

Apologies, I needed to reset nightly-testing, and incorrectly deleted it instead. @TwoFX, you may need to reconsistute this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author Waiting for PR author to address issues merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants