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: kernel exception from fvars left from ?m a b instantiation #4410

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Jun 9, 2024

Closes #4375

The following example raises error: (kernel) declaration has free variables '_example':

example: Nat → Nat :=
  let a : Nat := Nat.zero
  fun (_ : Nat) =>
    let b : Nat := Nat.zero
    (fun (_ : a = b) => 0) (Eq.refl a)

During elaboration of 0, elabNumLit creates a synthetic mvar ?_uniq.16 which gets abstracted by elabFun to ?_uniq.16 := ?_uniq.50 _uniq.6 _uniq.12. The isDefEq to instOfNatNat 0 results in:

?_uniq.50 :=
  fun (x._@.4375._hyg.13 : Nat) =>
    let b : Nat := Nat.zero
    fun (x._@.4375._hyg.23 : Eq.{1} Nat _uniq.4 b) =>
      instOfNatNat 0

This has a free variable _uniq.4 which was a.

When the application of ?_uniq.50 to #[#2, #0] is instantiated, the let b : Nat := Nat.zero blocks the beta-reduction and _uniq.4 remains in the expression.

fix: add (useZeta := true) here:

instantiateExprMVars (f.betaRev args.reverse)

@llllvvuu llllvvuu force-pushed the fix/recursive_let_deps branch 2 times, most recently from aaaee8c to dcfd93b Compare June 9, 2024 08:40
@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 Jun 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 9, 2024
@llllvvuu llllvvuu changed the title fix: mkLambdaFVarsWithLetDeps handles lets before lambdas fix: in mkLambdaFVarsWithLetDeps, handle lets before lambdas Jun 9, 2024
@nomeata
Copy link
Contributor

nomeata commented Jun 9, 2024

Hello @llllvvuu and thanks for your first contribution to lean!

The src/Lean/Meta/ExprDefEq.lean file is very central to lean, and typically not the first module to touch when contributing to lean, and for the long-term maintainability of the code it is preferrable if such central modules are written by not too many people. Therefore it is possible and likely that the maintainers of this component will chose to implement their own fix, instead of using this one. Nevertheless your contribution is useful as it helps to understand the issue and points to one possible way to address it.

Good issue reports, including good minimized test cases, and contributions to less central components (error messages, leaf tactics, etc.) are a good way to build expertise, reputation and trust, before high-risk areas of the code.

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Jun 9, 2024

No worries, it's not a big deal to me whether this particular patch + test case is what gets merged. I'm glad if it's of some use.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 9, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Jun 9, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

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

Mathlib CI status (docs):

@llllvvuu llllvvuu changed the title fix: in mkLambdaFVarsWithLetDeps, handle lets before lambdas fix: kernel exception due to fvars left after instantiating?m a b c; add (useZeta := true) Jun 10, 2024
@llllvvuu llllvvuu changed the title fix: kernel exception due to fvars left after instantiating?m a b c; add (useZeta := true) fix: kernel exception due to fvars left after instantiating ?m a b c; add (useZeta := true) Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 10, 2024
@llllvvuu llllvvuu changed the title fix: kernel exception due to fvars left after instantiating ?m a b c; add (useZeta := true) fix: kernel exception from fvars left from ?m a b instantiation Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 10, 2024
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Jun 10, 2024

I just changed the fix/description, new one is just changing a flag. It still doesn't achieve not touching central modules, but I hope it sheds some more light on the issue (test case further minimized as well).

The following example raises `error: (kernel) declaration has free
variables '_example'`:
```lean
example: True → Nat :=
  let a : Nat := Nat.zero
  fun (_ : True) =>
    let b : Nat := Nat.zero
    (fun (_ : a = b) => 0) (Eq.refl a)
```

During elaboration of `0`, `elabNumLit` creates a synthetic mvar
`?_uniq.16` which gets abstracted by `elabFun` to `?_uniq.16 :=
?_uniq.50 _uniq.6 _uniq.12`. The `isDefEq` to `instOfNatNat 0` results
in:
```
?_uniq.50 :=
  fun (x._@.4375._hyg.13 : True) =>
    let b : Nat := Nat.zero
    fun (x._@.4375._hyg.23 : Eq.{1} Nat _uniq.4 b) =>
      instOfNatNat 0
```

This has a free variable `_uniq.4` which was `a`.

When the application of `?_uniq.50` to `#[leanprover#2, #0]` is instantiated, the
`let b : Nat := Nat.zero` blocks the beta-reduction and `_uniq.4`
remains in the expression.

fix: add `(useZeta := true)` here:
https://github.com/leanprover/lean4/blob/ea46bf2839ad1c98d3a0c3e5caad8a81f812934c/src/Lean/MetavarContext.lean#L567
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 10, 2024
@leodemoura
Copy link
Member

I just changed the fix/description, new one is just changing a flag. It still doesn't achieve not touching central modules, but I hope it sheds some more light on the issue (test case further minimized as well).

Looks good. We will merge it. Thanks!

@leodemoura leodemoura added this pull request to the merge queue Jun 10, 2024
Merged via the queue into leanprover:master with commit 6a7bed9 Jun 10, 2024
12 checks passed
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jun 12, 2024
This linter was silently not doing anything until leanprover/lean4#4410 was fixed, and now it is working so a backlog of warnings needed to be addressed. Some were addressed here: #13680.

The warnings in this PRs are false positives (leanprover-community/batteries#428?), but a workaround is put in place.



Co-authored-by: L Lllvvuu <git@llllvvuu.dev>
AntoineChambert-Loir pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jun 20, 2024
This linter was silently not doing anything until leanprover/lean4#4410 was fixed, and now it is working so a backlog of warnings needed to be addressed. Some were addressed here: #13680.

The warnings in this PRs are false positives (leanprover-community/batteries#428?), but a workaround is put in place.



Co-authored-by: L Lllvvuu <git@llllvvuu.dev>
@kmill kmill mentioned this pull request Jul 25, 2024
3 tasks
This pull request was closed.
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.

Bug with nested ifs and lets
4 participants