chore: fix rebase suggestion for Mathlib CI #3701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
toolchain-available
|
Mathlib CI status (docs):
|
nomeata
approved these changes
Mar 17, 2024
nomeata
reviewed
Mar 17, 2024
nomeata
reviewed
Mar 17, 2024
joelriou
pushed a commit
to leanprover-community/mathlib4
that referenced
this pull request
Mar 17, 2024
The FunLike hierarchy is very big and gets scanned through each time we need a coercion (via the `CoeFun` instance). It looks like unbundled inheritance suits Lean 4 better here. The only class that still extends `FunLike` is `EquivLike`, since that has a custom `coe_injective'` field that is easier to implement. All other classes should take `FunLike` or `EquivLike` as a parameter. [Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/.60example.20.28p.20.3A.20P.29.20.3A.20Q.20.3A.3D.20p.60.20takes.200.2E25s.20to.20fail!) ## Important changes Previously, morphism classes would be `Type`-valued and extend `FunLike`: ```lean /-- `MyHomClass F A B` states that `F` is a type of `MyClass.op`-preserving morphisms. You should extend this class when you extend `MyHom`. -/ class MyHomClass (F : Type*) (A B : outParam <| Type*) [MyClass A] [MyClass B] extends FunLike F A B := (map_op : ∀ (f : F) (x y : A), f (MyClass.op x y) = MyClass.op (f x) (f y)) ``` After this PR, they should be `Prop`-valued and take `FunLike` as a parameter: ```lean /-- `MyHomClass F A B` states that `F` is a type of `MyClass.op`-preserving morphisms. You should extend this class when you extend `MyHom`. -/ class MyHomClass (F : Type*) (A B : outParam <| Type*) [MyClass A] [MyClass B] [FunLike F A B] : Prop := (map_op : ∀ (f : F) (x y : A), f (MyClass.op x y) = MyClass.op (f x) (f y)) ``` (Note that `A B` stay marked as `outParam` even though they are not purely required to be so due to the `FunLike` parameter already filling them in. This is required to see through type synonyms, which is important in the category theory library. Also, I think keeping them as `outParam` is slightly faster.) Similarly, `MyEquivClass` should take `EquivLike` as a parameter. As a result, every mention of `[MyHomClass F A B]` should become `[FunLike F A B] [MyHomClass F A B]`. ## Remaining issues ### Slower (failing) search While overall this gives some great speedups, there are some cases that are noticeably slower. In particular, a *failing* application of a lemma such as `map_mul` is more expensive. This is due to suboptimal processing of arguments. For example: ```lean variable [FunLike F M N] [Mul M] [Mul N] (f : F) (x : M) (y : M) theorem map_mul [MulHomClass F M N] : f (x * y) = f x * f y example [AddHomClass F A B] : f (x * y) = f x * f y := map_mul f _ _ ``` Before this PR, applying `map_mul f` gives the goals `[Mul ?M] [Mul ?N] [MulHomClass F ?M ?N]`. Since `M` and `N` are `out_param`s, `[MulHomClass F ?M ?N]` is synthesized first, supplies values for `?M` and `?N` and then the `Mul M` and `Mul N` instances can be found. After this PR, the goals become `[FunLike F ?M ?N] [Mul ?M] [Mul ?N] [MulHomClass F ?M ?N]`. Now `[FunLike F ?M ?N]` is synthesized first, supplies values for `?M` and `?N` and then the `Mul M` and `Mul N` instances can be found, before trying `MulHomClass F M N` which *fails*. Since the `Mul` hierarchy is very big, this can be slow to fail, especially when there is no such `Mul` instance. A long-term but harder to achieve solution would be to specify the order in which instance goals get solved. For example, we'd like to change the arguments to `map_mul` to look like `[FunLike F M N] [Mul M] [Mul N] [highPriority <| MulHomClass F M N]` because `MulHomClass` fails or succeeds much faster than the others. As a consequence, the `simpNF` linter is much slower since by design it tries and fails to apply many `map_` lemmas. The same issue occurs a few times in existing calls to `simp [map_mul]`, where `map_mul` is tried "too soon" and fails. Thanks to the speedup of leanprover/lean4#2478 the impact is very limited, only in files that already were close to the timeout. ### `simp` not firing sometimes This affects `map_smulₛₗ` and related definitions. For `simp` lemmas Lean apparently uses a slightly different mechanism to find instances, so that `rw` can find every argument to `map_smulₛₗ` successfully but `simp` can't: leanprover/lean4#3701. ### Missing instances due to unification failing Especially in the category theory library, we might sometimes have a type `A` which is also accessible as a synonym `(Bundled A hA).1`. Instance synthesis doesn't always work if we have `f : A →* B` but `x * y : (Bundled A hA).1` or vice versa. This seems to be mostly fixed by keeping `A B` as `outParam`s in `MulHomClass F A B`. (Presumably because Lean will do a definitional check `A =?= (Bundled A hA).1` instead of using the syntax in the discrimination tree.) ## Workaround for issues The timeouts can be worked around for now by specifying which `map_mul` we mean, either as `map_mul f` for some explicit `f`, or as e.g. `MonoidHomClass.map_mul`. `map_smulₛₗ` not firing as `simp` lemma can be worked around by going back to the pre-FunLike situation and making `LinearMap.map_smulₛₗ` a `simp` lemma instead of the generic `map_smulₛₗ`. Writing `simp [map_smulₛₗ _]` also works. Co-authored-by: Matthew Ballard <matt@mrb.email> Co-authored-by: Scott Morrison <scott.morrison@gmail.com> Co-authored-by: Scott Morrison <scott@tqft.net> Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Joachim Breitner <mail@joachim-breitner.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were suggesting rebasing onto the most recently nightly in the branches history, but that is incorrect and we should always suggest rebasing on
origin/nightly-with-mathlib.