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

chore: extend GetElem with getElem! and getElem? #3694

Merged
merged 9 commits into from
Mar 28, 2024
Merged

Conversation

joehendrix
Copy link
Contributor

@joehendrix joehendrix commented Mar 15, 2024

This makes changes to the GetElem class so that it does not lead to unnecessary overhead in container like RBMap.

The changes are to:

  1. Make getElem? and getElem! part of the GetElem class so they can be overridden in instances.
  2. Introduce a LawfulGetElem class that contains correctness theorems for getElem? and getElem! using the original definitions.
  3. Reorganize definitions (e.g, by moving GetElem out of Init.Prelude) so that the GetElem changes are feasible.
  4. Provide LawfulGetElem instances to complement all existing GetElem instances in Lean core.

To reduce the size of the PR, this doesn't do the work of providing new GetElem instances for RBMap, HashMap etc. That will be done in a separate PR (#3688) that depends on this.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 15, 2024 17:50 Inactive
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Mar 15, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Mar 15, 2024

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 4e3a8468c351531235ba5cffd5566b7283b7a8ca --onto 317adf42e92a4dbe07295bc1f0429b61bb8079fe. (2024-03-15 17:51:16)
  • 💥 Mathlib branch lean-pr-testing-3694 build failed against this PR. (2024-03-15 21:08:58) View Log
  • 💥 Mathlib branch lean-pr-testing-3694 build failed against this PR. (2024-03-17 21:50:35) View Log
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2d18eff544093882d45b7e1e97a0876247324f2b --onto 8e96d7ba1d7a36c7160112ef12307e4f361e8c3b. (2024-03-17 22:41:24)

@joehendrix joehendrix force-pushed the getElem_extension branch 2 times, most recently from a7e1a76 to 21f2e42 Compare March 15, 2024 20:13
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 15, 2024 21:02 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 15, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan full-ci labels Mar 15, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 15, 2024 21:11 Inactive
@joehendrix joehendrix force-pushed the getElem_extension branch 2 times, most recently from 71904a5 to 9f675a5 Compare March 16, 2024 00:23
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 16, 2024 00:46 Inactive
@joehendrix joehendrix removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan full-ci labels Mar 17, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan full-ci labels Mar 17, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 17, 2024 22:07 Inactive
@joehendrix joehendrix removed the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Mar 18, 2024
@joehendrix
Copy link
Contributor Author

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 5a14080.
There were no significant changes against commit 2d18eff.


There are other variations on this syntax:
* `arr[i]`: proves the proof side goal by `get_elem_tactic`
* `arr[i]!`: panics if the side goal is false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `arr[i]!`: panics if the side goal is false
* `arr[i]!`: panics if the side goal is false using `getElem!` in this class

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think actually it just expands to getElem!, which is intended to panic when the value isn't there, right? That is, the side goal is not checked anymore? In that case,

Suggested change
* `arr[i]!`: panics if the side goal is false
* `arr[i]!`: invokes `GetElem.getElem!`, which panics if the index is not found

There are other variations on this syntax:
* `arr[i]`: proves the proof side goal by `get_elem_tactic`
* `arr[i]!`: panics if the side goal is false
* `arr[i]?`: returns `none` if the side goal is false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `arr[i]?`: returns `none` if the side goal is false
* `arr[i]?`: expands to `GetElem.getElem?`, which returns `none` if the index is not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've tried to clarify

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc March 28, 2024 01:32 Inactive
@joehendrix joehendrix added this pull request to the merge queue Mar 28, 2024
Merged via the queue into master with commit 0963f34 Mar 28, 2024
11 checks passed
@joehendrix joehendrix deleted the getElem_extension branch April 4, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants