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: make elab_as_elim eagerly elaborate arguments for parameters appearing in the types of targets #4800

Merged
merged 6 commits into from
Jul 22, 2024

Conversation

kmill
Copy link
Collaborator

@kmill kmill commented Jul 20, 2024

The elab_as_elim elaborator eagerly elaborates arguments that can help with elaborating the motive, however it does not include the transitive closure of parameters appearing in types of parameters appearing in ... types of targets.

This leads to counter-intuitive behavior where arguments supplied to the eliminator may unexpectedly have postponed elaboration, causing motives to be type incorrect for under-applied eliminators such as the following:

class IsEmpty (α : Sort u) : Prop where
  protected false : α → False

@[elab_as_elim]
def isEmptyElim [IsEmpty α] {p : α → Sort _} (a : α) : p a :=
  (IsEmpty.false a).elim

example {α : Type _} [IsEmpty α] :
  id (α → False) := isEmptyElim (α := α)

The issue is that when isEmptyElim (α := α) is computing its motive, the value of the postponed argument α is still an unassignable metavariable. With this PR, this argument is now among those that are eagerly elaborated since it appears as the type of the target a.

This PR also contains some other fixes:

  • When underapplied, does unification when instantiating foralls in the expected type.
  • When overapplied, type checks the generalized-and-reverted expected type.
  • When collecting targets, collects them in the correct order.

Adds trace class trace.Elab.app.elab_as_elim.

This is a followup to #4722, which added motive type checking but exposed the eagerness issue.

…ppearing in the types of targets

The `elab_as_elim` elaborator eagerly elaborates arguments that can help with elaborating the motive, however it does not include the transitive closure of parameters that appear in types of parameters appearing in ... types of targets.

This leads to counter-intuitive behavior where arguments supplied to the eliminator may have postponed elaboration, causing motives to be type incorrect for under-applied eliminators such as the following:

```lean
class IsEmpty (α : Sort u) : Prop where
  protected false : α → False

@[elab_as_elim]
def isEmptyElim [IsEmpty α] {p : α → Sort _} (a : α) : p a :=
  (IsEmpty.false a).elim

example {α : Type _} [IsEmpty α] :
  id (α → False) := isEmptyElim (α := α)
```

The issue is that when `isEmptyElim (α := α)` is computing its motive, the value of the argument `α` is still an assignable metavariable. Now, this argument is among those that are eagerly elaborated since it appears as the type of the target `a`.

This PR also contains some other fixes:
* When underapplied, does unification when instantiating foralls in the expected type.
* When overapplied, type checks the generalized-and-reverted expected type.
* When collecting targets, collects them in the correct order.

This is a followup to leanprover#4722, which exposed the eagerness issue.
@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 Jul 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 20, 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 Jul 20, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jul 20, 2024

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 21, 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 Jul 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 22, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 22, 2024
src/Lean/Elab/App.lean Outdated Show resolved Hide resolved
@kmill kmill marked this pull request as ready for review July 22, 2024 22:37
@kmill kmill enabled auto-merge July 22, 2024 22:38
@kmill kmill added this pull request to the merge queue Jul 22, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jul 22, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jul 22, 2024
Merged via the queue into leanprover:master with commit 5938dbb Jul 22, 2024
22 checks passed
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 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.

2 participants