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: solveByElim would add symm hypotheses to local context and make impossible-to-elaborate terms #3962

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

kmill
Copy link
Collaborator

@kmill kmill commented Apr 20, 2024

Rather than adding symm hypotheses to the local context, it now adds them to the list of hypotheses derived from the local context.

This is not ideal for performance reasons, but it at least closes #3922.

In the future, solveByElim could maintain its own cache of facts that it updates whenever it does intro.

@kmill kmill requested a review from leodemoura as a code owner April 20, 2024 17:30
@kmill
Copy link
Collaborator Author

kmill commented Apr 20, 2024

@semorrison I took a stab at this issue. It's likely a performance regression, but it's a big usability improvement, so maybe it's ok in the meantime? Feel free to take over from here.

(Big warning: I did very little testing of this PR!)

@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 Apr 20, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

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

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase aeacb7b69e71c26f77a5fe42455465918dc0ee88 --onto 291bb84c972dae470e8f5602729e9d5a5e9433a2. (2024-04-20 17:48:05)
  • 💥 Mathlib branch lean-pr-testing-3962 build failed against this PR. (2024-04-20 18:22:22) View Log
  • 💥 Mathlib branch lean-pr-testing-3962 build failed against this PR. (2024-04-20 18:43:12) View Log
  • 💥 Mathlib branch lean-pr-testing-3962 build failed against this PR. (2024-04-22 01:39:01) View Log
  • ✅ Mathlib branch lean-pr-testing-3962 has successfully built against this PR. (2024-04-22 05:22:20) View Log

…impossible-to-elaborate terms

Rather than adding symm hypotheses to the local context, it now adds them to the list of hypotheses derived from the local context.

This is not idea for performance reasons, but it at least closes leanprover#3922.

In the future, solveByElim could maintain its own cache of facts that it updates whenever it does intro.
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 20, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Apr 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 20, 2024
@kim-em kim-em added the full-ci label Apr 22, 2024
@kim-em
Copy link
Collaborator

kim-em commented Apr 22, 2024

Adding full-ci so I can look at Mathlib.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 22, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 22, 2024
kim-em added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 22, 2024
@kim-em
Copy link
Collaborator

kim-em commented Apr 22, 2024

Mathlib is easy enough to fix (pushed to lean-pr-testing-3962). Agree we need to fix this despite the possible performance problems.

@kim-em kim-em added this pull request to the merge queue Apr 22, 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 Apr 22, 2024
Merged via the queue into leanprover:master with commit 41d310a Apr 22, 2024
47 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 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.

Interaction with symm attribute makes apply? nearly unusable
3 participants