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.data_erase #690

Merged
merged 20 commits into from
Sep 26, 2024
Merged

feat: fill in proof of Array.data_erase #690

merged 20 commits into from
Sep 26, 2024

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

Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
@kim-em
Copy link
Collaborator

kim-em commented Mar 14, 2024

Otherwise looks good.

@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 https://github.com/Seppel3210/std4/commit/109c3ce97e182813a166c9b55418d9a599a01bb6

@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)

@kim-em
Copy link
Collaborator

kim-em commented Mar 24, 2024

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.

@kim-em
Copy link
Collaborator

kim-em commented Apr 30, 2024

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

@kim-em
Copy link
Collaborator

kim-em commented May 3, 2024

Let's summon @nomeata.

@nomeata
Copy link
Collaborator

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 Aug 5, 2024
@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 Aug 5, 2024
@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 Aug 13, 2024
Batteries/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/List/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/List/Lemmas.lean Outdated Show resolved Hide resolved
@fgdorais fgdorais 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 Aug 13, 2024
Seppel3210 and others added 6 commits August 17, 2024 18:39
Co-authored-by: François G. Dorais <fgdorais@gmail.com>
Co-authored-by: François G. Dorais <fgdorais@gmail.com>
Co-authored-by: François G. Dorais <fgdorais@gmail.com>
Co-authored-by: François G. Dorais <fgdorais@gmail.com>
Co-authored-by: François G. Dorais <fgdorais@gmail.com>
@Seppel3210 Seppel3210 changed the title feat: fill in proof of Array.erase_data feat: fill in proof of Array.data_erase Aug 17, 2024
@Seppel3210
Copy link
Contributor Author

thanks for taking a look!

@Seppel3210
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed awaiting-author Waiting for PR author to address issues labels Aug 17, 2024
@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 Aug 17, 2024
Copy link
Collaborator

@fgdorais fgdorais left a comment

Choose a reason for hiding this comment

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

Sorry for the delay following up! I just have a couple of leftover questions.

Batteries/Data/Array/Lemmas.lean Show resolved Hide resolved
Batteries/Data/Array/Lemmas.lean Outdated Show resolved Hide resolved
@fgdorais fgdorais 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 Sep 25, 2024
@fgdorais fgdorais added this pull request to the merge queue Sep 26, 2024
Merged via the queue into leanprover-community:main with commit 40d378f Sep 26, 2024
2 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants