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: fill in proof of Array.erase_data #690

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Seppel3210
Copy link
Contributor

I also added some lemmas in List.Lemmas that were necessary.
I'm not sure about the naming of some of the lemmas I added, so I would appreciate feedback!

@Seppel3210
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Mar 8, 2024
I also added some lemmas in List.Lemmas that were necessary
@Seppel3210
Copy link
Contributor Author

Oops 🤦 Linter errors should be fixed now

@semorrison
Copy link
Collaborator

Otherwise looks good.

Comment on lines +796 to +797
erase_none {xs : List α} (i : Nat) (h : List.indexOf? a xs = none) :
xs.drop i = (xs.drop i).erase a := by
Copy link
Contributor

@timotree3 timotree3 Mar 14, 2024

Choose a reason for hiding this comment

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

Can erase_none be factored into these three more generally useful lemmas?

theorem erase_eq_of_nmem : x ∉ xs -> xs.erase x = xs := sorry
theorem nmem_drop_of_nmem : x ∉ xs -> x ∉ xs.drop i := sorry
theorem indexOf?_eq_none_iff : List.indexOf? x xs = none ↔ x ∉ xs := sorry

@timotree3
Copy link
Contributor

I thought it was a bit ugly that this PR had to unfold morally private definitions from core like eraseIdxAux so I made a PR which would allow us to simply unfold feraseIdx instead: leanprover/lean4#3676. If that PR gets merged it will break the proofs here.

@Seppel3210
Copy link
Contributor Author

as I said on the other PR @timotree3: In the case that your PR gets merged (and std4's lean-toolchain gets bumped) I have a commit lying around that fixes the proof Seppel3210@109c3ce

@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 Mar 15, 2024
@Seppel3210
Copy link
Contributor Author

I think it makes sense to wait for the lean4 version to get bumped since the lean4 PR was merged. Or should I PR it to the bump branch instead? (@semorrison you probably know this)

@semorrison
Copy link
Collaborator

Yes, if you'd like to PR to bump/v4.8.0 that's fine. It's also fine to wait: hopefully the release will be out next Tuesday.

@semorrison
Copy link
Collaborator

Sorry, the release was much delayed, but now should be out in the next few days again!

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label May 2, 2024
@Seppel3210
Copy link
Contributor Author

hmm, the linter fail seems to be an issue with derive_functional_induction. The this it's referring to is the ignored argument in case1

@Seppel3210
Copy link
Contributor Author

Not really sure who to ping for this issue 😅 @semorrison

@semorrison
Copy link
Collaborator

Let's summon @nomeata.

@nomeata
Copy link
Contributor

nomeata commented May 6, 2024

I think the problem isn’t with feraseIdx.induct per se, but rather with rw [eraseIdx], because

diff --git a/Std/Data/Array/Lemmas.lean b/Std/Data/Array/Lemmas.lean
index 31d9a385..a56c8c5e 100644
--- a/Std/Data/Array/Lemmas.lean
+++ b/Std/Data/Array/Lemmas.lean
@@ -843,16 +843,24 @@ theorem eraseIdx_swap_data {l : Array α} (i : Nat) (lt : i + 1 < size l) :
       simp [swap_def, List.set_succ, getElem_eq_data_get]
     simp [this, ih]

+theorem feraseIdx_eq (a : Array α) (i : Fin a.size) :
+  a.feraseIdx i =
+    if h : ↑i + 1 < a.size then
+      let a' := a.swap ⟨↑i + 1, h⟩ i;
+      let i' : Fin a'.size := ⟨i.val + 1, by simp [a', h]⟩
+      a'.feraseIdx i'
+    else a.pop := by rw [feraseIdx]
+
 theorem feraseIdx_data {l : Array α} {i : Fin l.size} :
     (l.feraseIdx i).data = l.data.eraseIdx i := by
   induction l, i using feraseIdx.induct with
   | @case1 a i lt a' i' _ ih =>
-    rw [feraseIdx]
+    rw [feraseIdx_eq]
     simp [lt, ih, a', eraseIdx_swap_data i lt]
   | case2 a i lt =>
     have : i + 1 ≥ a.size := Nat.ge_of_not_lt lt
     have last : i + 1 = a.size := Nat.le_antisymm i.is_lt this
-    simp [feraseIdx, lt, List.dropLast_eq_eraseIdx last]
+    simp [feraseIdx_eq, lt, List.dropLast_eq_eraseIdx last]

 @[simp] theorem erase_data [BEq α] {l : Array α} {a : α} : (l.erase a).data = l.data.erase a := by
   let ⟨xs⟩ := l

moves the error there.

The problem is that eraseIdx.eq_1 has these unnecessary have on the right-hand side.

The proper fix that I’d advocate for is to move everything that’s only needed for the termination argument into decerasing_by, this way you get the proper equational lemma. (leanprover/lean4#4074)

If you don't want to wait with this PR until that lands, just mark the proof as nolint until this is fixed:

import Std.Tactic.Lint.Misc
…
@[nolint unusedHavesSuffices] -- until https://github.com/leanprover/lean4/pull/4074 lands

nomeata added a commit to leanprover/lean4 that referenced this pull request May 6, 2024
otherwise it remains in the equational theorem and may cause the
“unused have linter” to trigger. By moving the proof into
`decreasing_by`, the equational theorems are unencumbered by termination
arguments.

see also leanprover-community/batteries#690 (comment)
github-merge-queue bot pushed a commit to leanprover/lean4 that referenced this pull request May 6, 2024
otherwise it remains in the equational theorem and may cause the
“unused have linter” to trigger. By moving the proof into
`decreasing_by`, the equational theorems are unencumbered by termination
arguments.

see also
leanprover-community/batteries#690 (comment)
@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 May 7, 2024
@semorrison semorrison 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 May 13, 2024
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.

None yet

5 participants