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: allow users to disable simpCtorEq simproc #5167

Merged
merged 14 commits into from
Aug 26, 2024
Merged

feat: allow users to disable simpCtorEq simproc #5167

merged 14 commits into from
Aug 26, 2024

Conversation

leodemoura
Copy link
Member

@leodemoura leodemoura commented Aug 26, 2024

simp only will not apply this simproc anymore. Users must now write simp only [reduceCtorEq]. See RFC #5046 for motivation.
This PR also renames simproc to reduceCtorEq.

close #5046

@semorrison A few simp only ... tactics will probably break in Mathlib. Fix: include reduceCtorEq.

@leodemoura leodemoura enabled auto-merge August 26, 2024 02:27
@leodemoura leodemoura disabled auto-merge August 26, 2024 02:28
@leodemoura leodemoura enabled auto-merge August 26, 2024 02:29
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 26, 2024 02:46 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 Aug 26, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 26, 2024
@leodemoura leodemoura added this pull request to the merge queue Aug 26, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 26, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Aug 26, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 26, 2024

Mathlib CI status (docs):

  • 💥 Mathlib branch lean-pr-testing-5167 build failed against this PR. (2024-08-26 02:53:15) View Log
  • 💥 Mathlib branch lean-pr-testing-5167 build failed against this PR. (2024-08-26 05:00:52) View Log
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase b54a9ec9b986f4833278caf750ab06c6352fdf7c --onto 9305049f1ef7309842806c2a994a2020bb32a71f. (2024-08-26 13:54:37)

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 26, 2024
@leodemoura
Copy link
Member Author

TODO: we must update-stage0 after merging this PR, and then remove staging workarounds.

@leodemoura leodemoura requested a review from tydeu as a code owner August 26, 2024 04:40
@leodemoura
Copy link
Member Author

Managed to build stage2 locally.

@leodemoura
Copy link
Member Author

@semorrison I had to add a few workarounds to build stage2. I will cleanup after merging the PR, and updating stage0.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 26, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 26, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 26, 2024 04:57 Inactive
@nomeata
Copy link
Collaborator

nomeata commented Aug 26, 2024

About the naming of the simproc: judging from the code it only works for equalities between different constructors, but doesn't apply injectivity? The name could make users believe that it would.

Also do we have a consistent naming scheme for simprocs? reduceIte doesn't start with simp.

I think it's worth thinking about a good name for a moment here, as due to simp? this will be exposed to unsuspecting users, and if it looks too scarily internal (as simpCtorEq maybe does), it's not great for the early user experience.

Maybe to stick to existing jargon, why not noConfusion or noConstrConfusion? Or less jargony constrDontOverlap?

@nomeata
Copy link
Collaborator

nomeata commented Aug 26, 2024

And while we are at it: I would find it useful if, when reading code (either as a learner or as a reviewer) I could quickly grasp which simp arguments are plain theorems, and which are simprocs.

So a consistent naming convention for simprocs may be useful. I’d say it needn’t have simp, as it’s always about simp.

I was thinking about using a Proc suffix, e.g. noConfusionProc, iteProc, diteProc

Most (all?) of the simprocs in core are called reduceFoo, so just sticking to that (reduceNoConfusion) is also an feasible option.

@leodemoura
Copy link
Member Author

Also do we have a consistent naming scheme for simprocs? reduceIte doesn't start with simp.

Note that the simproc was renamed to reduceCtorEq.

@leodemoura
Copy link
Member Author

I was thinking about using a Proc suffix, e.g. noConfusionProc, iteProc, diteProc

I am not opposing this suggestion, but it should be a different PR if users see a benefit on this change. Remark: it will break many proofs in Mathlib.

@leodemoura
Copy link
Member Author

About the naming of the simproc: judging from the code it only works for equalities between different constructors, but doesn't apply injectivity?

We use the injectivity theorems for that. We can add a docstring to make it clear the simproc only handles the different constructor case.

@leodemoura leodemoura added this pull request to the merge queue Aug 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 26, 2024
The simproc is renamed to `reduceCtorEq`.

closes #5046
We added this option to ensure `simproc`s do not invoke `whnf`.
`simp` is already reducing the terms.
@leodemoura leodemoura enabled auto-merge August 26, 2024 13:39
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 26, 2024 13:50 Inactive
@leodemoura leodemoura added this pull request to the merge queue Aug 26, 2024
Merged via the queue into master with commit 45475d6 Aug 26, 2024
14 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2024
PR #5167 implemented RFC #5046, but it required several workarounds due
to staging issues. This PR cleans up these workarounds.
kim-em added a commit to leanprover-community/batteries that referenced this pull request Aug 27, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
#5167 removed `reduceCtorEq` from the default simproc set. `norm_cast`
relies on it, so we add it back in there.
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Oct 9, 2024
Prior to leanprover/lean4#5167, this test (would have) had a less helpful error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan 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.

RFC: remove simpCtorEq from the default simprocs
3 participants