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

feat: set priority in monadic class instances #5291

Closed
wants to merge 557 commits into from

Conversation

JovanGerb
Copy link
Contributor

@JovanGerb JovanGerb commented Sep 9, 2024

This PR adds the high priority to some instances with monad transformers, most notably to MonadExceptOf ε (ExceptT ε m). This means that in ExceptT α MetaM, throw will throw the α instead of the Exception.

These instances now have high priority:

  • MonadReaderOf ρ (ReaderT ρ m)
  • MonadWithReaderOf ρ (ReaderT ρ m)
  • MonadStateOf σ (StateT σ m)
  • MonadStateOf σ (EStateT ε σ m)
  • MonadExceptOf ε (ExceptT ε m)
  • MonadExceptOf ε (EStateT ε σ m)
  • MonadExceptOf ε (ExceptCpsT ε m)
  • MonadCallStackOf κ (CallStackT κ m)
  • MonadCycleOf κ (CycleT κ m)

And these instance now have low priority:

  • MonadExceptOf Unit (OptionT m)
  • [MonadExcept ε m] {α : Type v} : OrElse (m α)

The last one makes sure that the OrElse instance has lower priority than instOrElseOfAlternative (this change doesn't affect the synthesis order, because the order of the files in which the instances were declared was already 'correct').

In Prelude.lean, instead of setting the priority to high or low, it has to be set to 10000 or 100, which is the same.

The increased priority of MonadExceptOf ε (ExceptT ε m) affected a few files that had to be fixed.

Closes #4212

@JovanGerb JovanGerb requested a review from tydeu as a code owner September 9, 2024 17:59
@JovanGerb JovanGerb changed the title set high priority in monadic class instances feat: set high priority in monadic class instances Sep 9, 2024
@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 Sep 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 9, 2024
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Sep 9, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Sep 9, 2024

Mathlib CI status (docs):

  • ✅ Mathlib branch lean-pr-testing-5291 has successfully built against this PR. (2024-09-09 19:19:03) View Log
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 99070bf304931c4ff22e31d6943e72858af65800 --onto c779f3a039963fd38b03a78f635f0a7c36f24f42. (2024-11-04 16:09:35)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 906aa1be4b29c423a97d6909bc315fa236529a79 --onto a955708b6c5f25e7f9c9ae7b951f8f3d5aefe377. (2025-01-16 15:57:56)

@JovanGerb
Copy link
Contributor Author

The test that broke had the number from the name generator offset by 8.
The line local infix:65 (priority := high) "*" => mul previously used to increment the name generator by 1560, and now this became 1568. I don't understand where these 8 extra names come from, but I equally don't understand why a simple infix declaration needs to use so ridiculously many names.

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 9, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 10, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Sep 11, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 11, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6aa0c46b04e78011af49eb4f272ea07e9173e584 --onto adfd6c090ef7d6bdcc8f69c5dd6f919b1b71cab3. (2024-09-12 11:37:11)

@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Sep 13, 2024
@JovanGerb JovanGerb changed the title feat: set high priority in monadic class instances feat: set priority in monadic class instances Sep 13, 2024
@kim-em
Copy link
Collaborator

kim-em commented Sep 26, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 90ac0f7.
There were significant changes against commit 6aa0c46:

  Benchmark   Metric       Change
  ========================================
- rbmap       task-clock     2.2% (23.0 σ)
- rbmap       wall-clock     2.3% (24.8 σ)

@kim-em
Copy link
Collaborator

kim-em commented Nov 1, 2024

I'm concerned that there's an unexplained performance regression revealed by !bench. Can you determine if that is real or not, and if so try to localize the exact change that causes it?

@JovanGerb
Copy link
Contributor Author

Can you benchmark it again? There seems to be nothing in the rbmap.lean benchmarking test related to the changes in this PR.

@JovanGerb
Copy link
Contributor Author

@kim-em, can you have a look at this one again? I don't know how to verify that the slowdowns weren't real.

I've now updated the PR to the new toolchain, but I'm not sure how to get it to build the accompanying Batteries and Mathlib.

@kim-em kim-em added the changelog-library Library label Jan 21, 2025
@kim-em
Copy link
Collaborator

kim-em commented Jan 21, 2025

@JovanGerb, you'll need to rebase this PR onto origin/nightly-with-mathlib. Then you can run !bench again.

leodemoura and others added 3 commits January 21, 2025 11:39
This PR adds missing features and fixes bugs in the `Float32` support
Remove some noise from my assignments
hargoniX and others added 15 commits January 21, 2025 11:41
…er#6211)

This PR verifies the `insertMany` method on `HashMap`s for the special
case of inserting lists.

---------

Co-authored-by: jt0202 <johannes.tantow@gmail.com>
Co-authored-by: monsterkrampe <monsterkrampe@users.noreply.github.com>
Co-authored-by: Johannes Tantow <44068763+jt0202@users.noreply.github.com>
This PR improves the E-matching pattern selection heuristics in the
`grind` tactic. They now take into account type predicates and
transformers.
…dure (leanprover#6654)

This PR improves the support for partial applications in the E-matching
procedure used in `grind`.
This PR improves the diagnostic information provided in `grind` failure
states. We now include the list of issues found during the search, and
all search thresholds that have been reached. This PR also improves its
formatting.
This PR adds some tests for `grind`, working on `List` lemmas.
This PR improves the `grind` search procedure, and adds the new
configuration option: `failures`.
This PR uses `StateRefT` instead of `StateT` to equip the Lake build
monad with a build store.

As a IO reference, different threads may now contend with the build
store. However, benchmark results indicate that this does not have a
significant performance impact. On a synchronization front, the lack of
a mutex should not be a concern because the build store is a
memorization data structure and thus order is theoretically irrelevant.
This PR ensures that `grind` avoids case-splitting on terms congruent to
those that have already been case-split.
This PR fixes a bug in the `grind` term preprocessor. It was abstracting
nested proofs **before** reducible constants were unfolded.

---------

Co-authored-by: Kim Morrison <kim@tqft.net>
This PR improves the canonicalizer used in the `grind` tactic and the
diagnostics it produces. It also adds a new configuration option,
`canonHeartbeats`, to address (some of) the issues. Here is an example
illustrating the new diagnostics, where we intentionally create a
problem by using a very small number of heartbeats.

<img width="1173" alt="image"
src="https://github.com/user-attachments/assets/484005c8-dcaa-4164-8fbf-617864ed7350"
/>
This PR defines `Vector.flatMap`, changes the order of arguments in
`List.flatMap` for consistency, and aligns the lemmas for
`List`/`Array`/`Vector` `flatMap`.
This PR adds array indexing lemmas for `Vector.flatMap`. (These were not
available for `List` and `Array` due to variable lengths.)
…prover#6667)

This PR aligns `List.replicate`/`Array.mkArray`/`Vector.mkVector`
lemmas.
…eft_and_distrib`, `testBit_mul_two_pow`, `bitwise_mul_two_pow`, `shiftLeft_bitwise_distrib]` (leanprover#6630)

This PR adds theorems `Nat.[shiftLeft_or_distrib`,
shiftLeft_xor_distrib`, shiftLeft_and_distrib`, `testBit_mul_two_pow`,
`bitwise_mul_two_pow`, `shiftLeft_bitwise_distrib]`, to prove
`Nat.shiftLeft_or_distrib` by emulating the proof strategy of
`shiftRight_and_distrib`.

In particular, `Nat.shiftLeft_or_distrib` is necessary to simplify the
proofs in leanprover#6476.

---------

Co-authored-by: Alex Keizer <alex@keizer.dev>
@JovanGerb
Copy link
Contributor Author

It seems I don't understand how git works, so I've opened it as a new PR: #6725

@JovanGerb
Copy link
Contributor Author

@JovanGerb, you'll need to rebase this PR onto origin/nightly-with-mathlib. Then you can run !bench again.

I tried to !bench the new PR, but it seems that I still cannot do that myself.

@Kha
Copy link
Member

Kha commented Jan 21, 2025

Instructions at least are unchanged, most likely noise

@JovanGerb JovanGerb closed this Jan 22, 2025
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 changelog-library Library P-medium We may work on this issue if we find the time 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.

RFC: type class synthesis order when outParam has multiple possible values