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: WF.Fix: deduplicate subsumed goals before running tactic #3024

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Dec 5, 2023

before code like

def dup (a : Nat) (b : Nat := a) := a + b

def rec : Nat → Nat
 | 0 => 1
 | n+1 => dup (dup (dup (rec n)))
decreasing_by decreasing_tactic

would run the decreasing_tactic 8 tims, because the recursive call
rec n gets duplicate due to the default paramter. Similar effects can
be observed due to dependent types or tactics like cases.

This is wasteful, and is confusing to the user when they use
decreasing_by interactively. Therfore, we now go through the proof
obligations (MVars) and if solving one would imply solving another one,
we assign the mvars to each other accordingly.

This PR is a sibling of #3004.

before code like

    def dup (a : Nat) (b : Nat := a) := a + b

    def rec : Nat → Nat
     | 0 => 1
     | n+1 => dup (dup (dup (rec n)))
    decreasing_by decreasing_tactic

would run the `decreasing_tactic` 8 tims, because the recursive call
`rec n` gets duplicate due to the default paramter. Similar effects can
be observed due to dependent types or tactics like `cases`.

This is wasteful, and is confusing to the user when they use
`decreasing_by` interactively. Therfore, we now go through the proof
obligations (MVars) and if solving one would imply solving another one,
we assign the mvars to each other accordingly.

This PR is a sibling of #3004.
@nomeata
Copy link
Contributor Author

nomeata commented Dec 5, 2023

I’ll run this against mathlib once 9290b49 is tagged as nightly-with-mathlib.

@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 Dec 5, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Dec 5, 2023

  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-12-05' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-12-05 09:34:51)
  • ✅ Mathlib branch lean-pr-testing-3024 has successfully built against this PR. (2023-12-05 13:51:32) View Log
  • ✅ Mathlib branch lean-pr-testing-3024 has successfully built against this PR. (2023-12-05 15:18:23) View Log
  • ✅ Mathlib branch lean-pr-testing-3024 has successfully built against this PR. (2023-12-06 10:05:23) View Log
  • ✅ Mathlib branch lean-pr-testing-3024 has successfully built against this PR. (2023-12-06 16:29:01) View Log
  • ✅ Mathlib branch lean-pr-testing-3024 has successfully built against this PR. (2023-12-06 18:15:57) View Log

@nomeata nomeata changed the title fix: WF.Fix: Deduplicate subsumed goals before running tactic fix: WF.Fix: deduplicate subsumed goals before running tactic Dec 5, 2023
@nomeata nomeata closed this Dec 5, 2023
@nomeata nomeata reopened this Dec 5, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 5, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Dec 5, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 5, 2023
@nomeata
Copy link
Contributor Author

nomeata commented Dec 5, 2023

Got mathlib perf results, nothing to see. http://speed.lean-fro.org/mathlib4/compare/6d60f603-6faa-4727-9a04-42abb15e1477/to/535315ad-b4ab-4d6a-b3fd-67e8b2ff12c7

There is an “open Mathlib — wall-clock 7.8%” but I think it’s a fluke: It doesn't show up when comparing this branch with the latest master, and it it seems unlikely that there is a change in testing-nightly that improved open Mathlib by exactly the amount that this regressed then. Also “open Mathlib – instructions” goes down.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2023
@nomeata nomeata added the will-merge-soon …unless someone speaks up label Dec 6, 2023
@nomeata nomeata marked this pull request as ready for review December 6, 2023 17:36
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 6, 2023
@nomeata nomeata added this pull request to the merge queue Dec 7, 2023
Merged via the queue into master with commit ec8811a Dec 7, 2023
13 checks passed
@nomeata nomeata deleted the joachim/wf-fix-dedup branch December 7, 2023 08:53
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 will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants