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: out of scope let-variable can appear in fun binder type of metav… #5948

Closed
wants to merge 23 commits into from

Conversation

JovanGerb
Copy link
Contributor

@JovanGerb JovanGerb commented Nov 5, 2024

This PR fixes a bug in checkAsssignment.

When unification tries to solve ?m a₁ ... aₙ =?= v by assigning ?m := fun a₁ ... aₙ => v, a number of things needs to be checked.

  1. occurs check, that is, check that ?m doesn't appear in the assignment
  2. check that the free variables in the assignment are present in the local context of ?m. If a let-varaible isn't in the local context, then this can be resolved by instantiating it with its definition.
  3. check that the metavariables in the assignment have local contexts that are at most as large as the local context of ?m. If a metavariable has a local context with extra free variables, this can be resolved by assigning it to a new fresh metavariable with a restricted local context.

Previously, checkAssignment does checks 1-3 on v, and does check 1 on the types of a₁ ... aₙ and on the type of v.

However, as reported in #5939, checks 2-3 also need to be executed on the types of a₁ ... aₙ, because these types appear as lambda binder types in ?m := fun a₁ ... aₙ => v.

This PR fixes checkAssignment: besides returning an updated v, it now also returns an updated LocalContext. This LocalContext is then used in mkLambdaFVars in order to obtain the correct lambda binder types.

In order to also fix #5387, I modified the function Lean.Meta.CheckAssignment.checkMVar, so that it can use the checked types of a₁ ... aₙ, which makes it so that it can avoid removing some of a₁ ... aₙ from the context of the metavariable that is being reduced.

This PR also removes the function mkLambdaFVarsWithLetDeps. This function was introduced as a fix for a bug that is independently fixed by this fix. Additionally, mkLambdaFVarsWithLetDeps gives surprising behaviour in that it creates let-binders for let-variables that would normally just be instantiated (see check 2). Additionally, mkLambdaFVarsWithLetDeps has a faulty implementation, because it assumes the free variables appear in the same order as they appear in the local context, which may not be true (see this comment).

I added the tests to the file that contains the original test for mkLambdaFVarsWithLetDeps

Closes #5939
Closes #5387

@JovanGerb JovanGerb marked this pull request as draft November 5, 2024 02:10
@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 Nov 5, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 5, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 5, 2024
@leanprover-community-bot leanprover-community-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Nov 5, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 6, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 9, 2024
@leanprover-community-bot leanprover-community-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 Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
@leanprover-community-bot leanprover-community-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
@leanprover-community-bot leanprover-community-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 Nov 10, 2024
@JovanGerb JovanGerb marked this pull request as ready for review November 10, 2024 14:26
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 10, 2024
@Kha
Copy link
Member

Kha commented Nov 15, 2024

We cannot accept such an invasive change to a core component, please consider contributing to less critical parts first!

@Kha Kha closed this Nov 15, 2024
@JovanGerb
Copy link
Contributor Author

Ok, I hope this at least helped with understanding what the problem is and how it can be fixed.

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
3 participants