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: instantiate mvars of indices before instantiating fvars #4717

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

arthur-adjedj
Copy link
Contributor

@arthur-adjedj arthur-adjedj commented Jul 10, 2024

When elaborating the headers of mutual indexed inductive types, mvars have to be synthesized and instantiated before replacing the fvars present there. Otherwise, some fvars present in uninstantiated mvars may be missed and lead to an error later.
Closes #3242 (again)

@arthur-adjedj
Copy link
Contributor Author

WIP

@github-actions github-actions bot added WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN labels Jul 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 10, 2024
@leodemoura leodemoura force-pushed the master branch 2 times, most recently from 32d60c5 to 696f70b Compare July 20, 2024 02:58
@Kha
Copy link
Member

Kha commented Aug 1, 2024

@arthur-adjedj Why is this a draft?

@arthur-adjedj
Copy link
Contributor Author

arthur-adjedj commented Aug 1, 2024

I haven't had time to make thorough tests to see if this really solves the issue for good. Knowing the issue popped up once after being already closed, I'd like to make sure this doesn't happen again. I haven't dived into how inductive headers are unified yet, and don't know if some expr mvars aren't expected to be unified later on here. If the expected behavior is that all expr mvars in the headers have already been solved at this point, then this certainly is the right solution, and I'd be happy to get this merged.
I'm not available to investigate this right now, but plan on looking into this in the coming days.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 8, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 8, 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 release-ci Enable all CI checks for a PR, like is done for releases labels Aug 9, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

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

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 9, 2024
@arthur-adjedj
Copy link
Contributor Author

Despite multiple rebases, the CI on mathlib/batteries seems to keep on failing, and I'm not sure why. Looking at the errors, I don't think the failures are due to the changes made in that PR. I'd like to see at least one CI run work "correctly" on this to ensure no breakage was made, but otherwise, I'm confident this PR makes the right fixes, and will be marking it as ready for review.

@arthur-adjedj
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added awaiting-review Waiting for someone to review the PR and removed WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. labels Aug 12, 2024
@nomeata
Copy link
Contributor

nomeata commented Aug 12, 2024

I see likely unwanted differences in

git diff nightly-testing-2024-08-09..lean-pr-testing-4717

With

git checkout -p nightly-testing-2024-08-09 --

I’m removing all changes but the one to the toolchain, let’s give it a shot.

What also often works if our scripts failed to update the pr testing branch through rebases (not uncommon) is to delete that branch in mathlib4 and re-trigger the CI here to recreate it freshly.

nomeata added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 12, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Aug 12, 2024
@arthur-adjedj arthur-adjedj marked this pull request as ready for review August 13, 2024 09:50
@nomeata
Copy link
Contributor

nomeata commented Aug 13, 2024

Glad it's green now. Don’t forget to update the PR description, as it will become the commit message.

@arthur-adjedj
Copy link
Contributor Author

Thanks for the notice ! That's now fixed.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 13, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 13, 2024
@nomeata
Copy link
Contributor

nomeata commented Aug 16, 2024

Looks good to me, I don’t think I understand the intricacies any better than you. Would you mind leaving a comment in the code with whatever insights you have so that the next person looking at the code won’t have to re-discover it?

@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Aug 16, 2024
@nomeata nomeata enabled auto-merge August 16, 2024 14:45
@nomeata nomeata added this pull request to the merge queue Aug 16, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 16, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 16, 2024
Merged via the queue into leanprover:master with commit eb15c08 Aug 16, 2024
22 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 P-medium We may work on this issue if we find the time release-ci Enable all CI checks for a PR, like is done for releases 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.

Unabstracted variables in indices of inductive types in mutual block
5 participants