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: GuessLex: deduplicate recursive calls #3004

Merged
merged 15 commits into from
Dec 7, 2023
Merged

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Dec 1, 2023

The elaborator is prone to duplicate terms, including recursive calls,
even if the user only wrote a single one. This duplication is wasteful
if we run the tactics on duplicated calls, and confusing in the output
of GuessLex. So prune the list of recursive calls, and remove those
where another call exists that has the same goal and context that is no
more specific.

The elaborator is prone to duplicate terms, including recursive calls,
even if the user only wrote a single one. This duplication is wasteful
if we run the tactics on duplicated calls, and confusing in the output
of GuessLex. So prune the list of recursive calls, and remove those
where another call exists that has the same goal and context that is no
more specific.
@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 1, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 1, 2023
@nomeata
Copy link
Contributor Author

nomeata commented Dec 1, 2023

!bench

@nomeata
Copy link
Contributor Author

nomeata commented Dec 1, 2023

The effect of this will be more pronounced with #2960. Maybe I’ll finish that first, to have a more pleasant diff here :-)

I also plan to make an analogous changes to WF.Fix, but that will be part of #2921, or at least a step towards that.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Dec 1, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

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

  • ✅ Mathlib branch lean-pr-testing-3004 has successfully built against this PR. (2023-12-01 12:39:37) View Log
  • ✅ Mathlib branch lean-pr-testing-3004 has successfully built against this PR. (2023-12-01 13:17:26) View Log
  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-12-01' 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-01 18:10:18)
  • ❗ Mathlib CI will not be attempted unless you rebase your PR onto the 'nightly' branch. (2023-12-05 10:13:00)
  • ✅ Mathlib branch lean-pr-testing-3004 has successfully built against this PR. (2023-12-05 10:28:49) View Log

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 1, 2023
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit e554409.
There were no significant changes against commit 9933121.

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.
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 5, 2023
@nomeata nomeata marked this pull request as ready for review December 6, 2023 17:35
@nomeata nomeata added this pull request to the merge queue Dec 7, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 7, 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.
@nomeata nomeata removed this pull request from the merge queue due to a manual request Dec 7, 2023
@nomeata nomeata added this pull request to the merge queue Dec 7, 2023
Merged via the queue into master with commit f2a92f3 Dec 7, 2023
7 checks passed
@nomeata nomeata deleted the joachim/guesslex-dedup branch December 24, 2023 10:18
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.

3 participants