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: getElem!_cons_zero #214

Closed
wants to merge 5 commits into from
Closed

Conversation

mo271
Copy link
Contributor

@mo271 mo271 commented Aug 8, 2023

No description provided.

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.

Nice! Lean core has List.cons_getElem_zero and List.cons_getElem_succ, so following the same convention makes sense here. Also, you might as well add all the companion theorems at the same time:

@[simp] theorem cons_getElem!_zero [Inhabited α] {as : List α} : (a :: as)[0]! = a := by
  unfold getElem!; simp [Nat.zero_lt_succ]

@[simp] theorem cons_getElem!_succ [Inhabited α] {as : List α} : (a :: as)[i+1]! = as[i]! := by
  unfold getElem!; split
  next h => rw [dif_pos (Nat.lt_of_succ_lt_succ h)]; rfl
  next h => rw [dif_neg (mt Nat.succ_lt_succ h)]

@[simp] theorem cons_getElem?_zero {as : List α} : (a :: as)[0]? = a := by
  unfold getElem?; simp [Nat.zero_lt_succ]

@[simp] theorem cons_getElem?_succ {as : List α} : (a :: as)[i+1]? = as[i]? := by
  unfold getElem?; split
  next h => rw [dif_pos (Nat.lt_of_succ_lt_succ h)]; rfl
  next h => rw [dif_neg (mt Nat.succ_lt_succ h)]

Co-authored-by: François G. Dorais <fgdorais@gmail.com>
@mo271
Copy link
Contributor Author

mo271 commented Aug 9, 2023

Great, I added the lemmas you suggested.
Just above we have get_cons_zero. Should this be changed (perhaps in a separate pull request) to cons_get_zero for more consistent naming?

@mo271
Copy link
Contributor Author

mo271 commented Aug 9, 2023

Without removing the simps I got a lint error:
https://github.com/leanprover/std4/actions/runs/5806195289/job/15738605134?pr=214

@fgdorais
Copy link
Collaborator

fgdorais commented Aug 9, 2023

I think get_cons_zero is fine. I think getElem is reversed because it's a postfix operator.

The simp lint error is interesting: it appears that the simp normal form for lists is to change getElem and getElem? to List.get and List.get?, respectively. This works fine since there is a large API for List.get and List.get? that was inherited from Lean 3. Unfortunately, the List.get! API is not nearly as well developed, so that translation doesn't work so well for getElem!.

@mo271
Copy link
Contributor Author

mo271 commented Aug 10, 2023

Regarding the simp lint error:
Is there anything we should do to address this in this pull request or is it good to go?

If I understand correctly, we should add lemma like getElem!_eq_get! and then the simp linter will also fire for cons_getElem!_zero and cons_getElem!_succ, so then we also remove the simp attribute there.

@fgdorais
Copy link
Collaborator

I think it's fine as-is. getElem!_eq_get! wouldn't be useful since there aren't enough lemmas about get!. In the future, once there are enough getElem lemmas, the simp normal form might reverse toward getElem form. Time will tell.

Std/Data/List/Lemmas.lean Outdated Show resolved Hide resolved
@fgdorais fgdorais added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Aug 11, 2023
@digama0
Copy link
Member

digama0 commented Aug 26, 2023

I don't like these lemmas, because getElem? and getElem! on list are not simp-normal, the .get / .get? function is preferred. For panicking functions the simp normal form would be something like .getD default. (The reason for this is because even though getElem? has a cleaner syntax, it doesn't have very good defeqs compared to get?.) We should not encourage having two ways to say the same thing as this squares the number of required lemmas (since every preferred spelling has to interact with every other preferred spelling).

@digama0 digama0 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 26, 2023
@mo271
Copy link
Contributor Author

mo271 commented Aug 26, 2023

ok, let's close this pull request then, I suppose...

@mo271 mo271 closed this Aug 26, 2023
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.

None yet

3 participants