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

fix: Funind: no do unfold auxillary defs #6921

Closed
wants to merge 1 commit into from
Closed

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Feb 3, 2025

This PR changes internals in the funindn construction.

Previously we would unfold defs Tree.revrev._unary.proof_3 when transforming the function body. This is somewhat expensive (lots of proofs copied) and it may expose recursive calls within proofs that we probably do not care about, and that can look different than what the user wrote somewhere (simpMatch, looking at you here).

So we try to leave them in place now.

Also previously we did not include the “old” IH in the local context, so that creating a MVar would not pick it up. But this prevented us from inferring types, which I really need here.

So now let's keep them in the local context, but wrap the creation of the metavariables in withErasedFVars.

This PR changes internals in the funidn construction.

Previously we would unfold defs `Tree.revrev._unary.proof_3` when
transforming the function body. This is somewhat expensive (lots of
proofs copied) and it may expose recursive calls within proofs that we
probably do not care about, and that can look different than what the
user wrote somewhere (`simpMatch`, looking at you here).

So we try to leave them in place now.

Also previously we did not include the “old” IH in the local context, so
that creating a MVar would not pick it up. But this prevented us from
inferring types, which I really need here.

So now let's keep them in the local context, but wrap the creation of
the metavariables in `withErasedFVars`.
@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 Feb 3, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Feb 3, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Feb 3, 2025
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Feb 3, 2025
@nomeata
Copy link
Collaborator Author

nomeata commented Feb 3, 2025

Hmm, may not actually work, at least for structural recursion, due to #5903. Too bad.

@nomeata nomeata closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants