Skip to content

Conversation

@nomeata
Copy link
Collaborator

@nomeata nomeata commented Oct 23, 2025

This PR topologically sorts abstracted vars in
Meta.Closure.mkValueTypeClosure if MVars are being abstracted.
Fixes #10705

This PR topologically sorts abstracted vars in
`Meta.Closure.mkValueTypeClosure` if MVars are being abstracted.
Fixes #10705
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 23, 2025

!bench

@leanprover-radar
Copy link

leanprover-radar commented Oct 23, 2025

Benchmark results for 2cf8794 against efbbb0b are in! @nomeata

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 2cf8794.
There were significant changes against commit efbbb0b:

  Benchmark                          Metric         Change
  ==================================================================
+ workspaceSymbols with new ranges   branches        -1.1% (-43.5 σ)
+ workspaceSymbols with new ranges   instructions    -1.5% (-46.7 σ)

@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 Oct 23, 2025
@leanprover-bot
Copy link
Collaborator

Reference manual CI status:

  • ❗ Reference manual CI can not be attempted yet, as the nightly-testing-2025-10-21 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-manual, reference manual CI should run now. You can force reference manual CI using the force-manual-ci label. (2025-10-23 10:33:35)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Oct 23, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4-nightly-testing that referenced this pull request Oct 23, 2025
@eric-wieser
Copy link
Contributor

It's not immediately clear to me why a sort is needed here; presumably we could store the variables in the order we visit them or similar (as revert does)? Or is the point that we do actually want a different (topologically-compatilble) order that is somehow more efficient for reducing term sizes?

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 23, 2025

It's not immediately clear to me why a sort is needed here; presumably we could store the variables in the order we visit them

This might be true if we visited the types of fvars before adding them, but the existing code puts them on a separate to-process-queue:

      let newFVarId ← mkFreshFVarId
      pushToProcess ⟨fvarId, newFVarId⟩
      return mkFVar newFVarId

This was introduced in 555a3f0 and I since don't fully understand the implications I’d rather leave it like this.

Also, adding the infrastructure to sort these would allow me to make progress on #10723 and then maybe remove the hacks where termination checking has to unfold abstracted proofs.

@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Oct 23, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Oct 23, 2025

Mathlib CI status (docs):

@nomeata nomeata marked this pull request as ready for review October 23, 2025 12:45
@nomeata nomeata added the changelog-language Language features and metaprograms label Oct 23, 2025
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 23, 2025

presumably we could store the variables in the order we visit them

Also, isn’t this what you tried in #10702? I thought you said it didn't work.

I gave it another shot at #10932 but it didn’t work out of the box, so parking it there for now.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Oct 23, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4-nightly-testing that referenced this pull request Oct 23, 2025
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 24, 2025

Maybe we’ll find a more elegant fix later, but for now this fixes the bug and seems to work.

@nomeata nomeata added this pull request to the merge queue Oct 24, 2025
Merged via the queue into master with commit a6d50a6 Oct 24, 2025
21 of 22 checks passed
wkrozowski pushed a commit to wkrozowski/lean4 that referenced this pull request Oct 27, 2025
This PR topologically sorts abstracted vars in
`Meta.Closure.mkValueTypeClosure` if MVars are being abstracted.
Fixes leanprover#10705

---------

Co-authored-by: Eric Wieser <[email protected]>
kim-em pushed a commit that referenced this pull request Oct 27, 2025
This PR topologically sorts abstracted vars in
`Meta.Closure.mkValueTypeClosure` if MVars are being abstracted.
Fixes #10705

---------

Co-authored-by: Eric Wieser <[email protected]>
github-actions bot pushed a commit that referenced this pull request Oct 29, 2025
This PR topologically sorts abstracted vars in
`Meta.Closure.mkValueTypeClosure` if MVars are being abstracted.
Fixes #10705

---------

Co-authored-by: Eric Wieser <[email protected]>
(cherry picked from commit a6d50a6)
kim-em pushed a commit that referenced this pull request Nov 3, 2025
This PR topologically sorts abstracted vars in
`Meta.Closure.mkValueTypeClosure` if MVars are being abstracted.
Fixes #10705

---------

Co-authored-by: Eric Wieser <[email protected]>
(cherry picked from commit a6d50a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport releases/v4.25.0 builds-mathlib CI has verified that Mathlib builds against this PR changelog-language Language features and metaprograms 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.

Closure.mkValueTypeClosure fails to eliminate fvars which depend on mvars, breaking mkAuxTheorem

7 participants