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 : derive DecidableEq for mutual inductives #2591

Merged
merged 4 commits into from
Oct 3, 2023

Conversation

arthur-adjedj
Copy link
Contributor

@arthur-adjedj arthur-adjedj commented Sep 26, 2023

This partly implements #2329 by making the derive handler manage mutual inductive types. The general framework for derive handlers is currently not well-suited to manage nested inductives without resorting to making the derived instances partial, and managing them would require big changes to the framework, and thus, to other handlers as well. Such work/design decisions should be discussed in a subsequent RFC.

@github-actions
Copy link
Contributor

Thanks for your contribution! Please make sure to follow our Commit Convention.

@arthur-adjedj
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added awaiting-review Waiting for someone to review the PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN labels Sep 26, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 26, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Sep 26, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Sep 26, 2023

@semorrison
Copy link
Collaborator

I was slightly surprised to find that writing deriving DecidableEq on both inductives in a mutual block didn't error. That's great, but could you explain to me why? :-)

@semorrison
Copy link
Collaborator

Would you mind adding a sentence about this to RELEASES.md in the v4.3.0 section (you will need to rebase on master for this section to appear)? We're trying to get better about recording the lovely new features arriving in Lean!

@semorrison semorrison added the missing release notes Please add a short bullet point to RELEASES.md label Sep 26, 2023
@semorrison semorrison self-assigned this Sep 26, 2023
@arthur-adjedj
Copy link
Contributor Author

arthur-adjedj commented Sep 27, 2023

I was slightly surprised to find that writing deriving DecidableEq on both inductives in a mutual block didn't error. That's great, but could you explain to me why? :-)

I was surprised by this behaviour too ! As it turns out, other handlers share the same behaviour, so I figured I shouldn't change it here. Another thing that surprised me is that deriving on one inductive doesn't lead to the other one(s) also getting the instances generated. Furthermore, you can't use deriving _ on a whole mutual block, despite handlers being handled arrays of names instead of just a single one. Because of this, you need to derive on each and every inductive, which leads to code duplication of the definitions behind the instances (!). I believe such fixes/changes could (should ?) come through reworking the whole framework as well, instead of it being on a case-by-case basis.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 27, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 27, 2023
@semorrison semorrison removed the missing release notes Please add a short bullet point to RELEASES.md label Oct 3, 2023
@semorrison semorrison enabled auto-merge (squash) October 3, 2023 01:10
@semorrison semorrison merged commit 6b93f05 into leanprover:master Oct 3, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR 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.

None yet

3 participants