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

refactor: rewrite: produce simpler proof terms #3121

Merged
merged 15 commits into from
Jan 19, 2024
Merged

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Dec 28, 2023

Consider

import Std.Tactic.ShowTerm

opaque a : Nat
opaque b : Nat
axiom a_eq_b : a = b
opaque P : Nat → Prop

set_option pp.explicit true

-- Using rw
example (h : P b) : P a := by show_term rw [a_eq_b]; assumption

Before, a typical proof term for rewrite looked like this:

-- Using the proof term that rw produces
example (h : P b) : P a :=
  @Eq.mpr (P a) (P b)
  (@id (@Eq Prop (P a) (P b))
    (@Eq.ndrec Nat a (fun _a => @Eq Prop (P a) (P _a))
      (@Eq.refl Prop (P a)) b a_eq_b))
  h

which is rather round-about, applying ndrec to refl. It would be more direct to write

example (h : P b) : P a :=
  @Eq.mpr (P a) (P b)
  (@id (@Eq Prop (P a) (P b))
    (@congrArg Nat Prop a b (fun _a => (P _a)) a_eq_b))
  h

which this change does.

This makes proof terms smaller, causing mild general speed up throughout the code; if the brenchmarks don’t lie the highlights are

  • olean size -2.034 %
  • lint wall-clock -3.401 %
  • buildtactic execution s -10.462 %

H'T to @digama0 for advice and help.

NB: One might even expect the even simpler

-- Using the proof term that I would have expected
example (h : P b) : P a :=
  @Eq.ndrec Nat b (fun _a => P _a) h a a_eq_b.symm

but that would require non-local changes to the source code, so one step at a time.

this is so far a learning exercise for me, trying to understand the
Chesterton’s fence here, with the intention to comment the code better
once CI tells me why the simpler version doesn't work.
@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 28, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 28, 2023
@nomeata
Copy link
Collaborator Author

nomeata commented Dec 28, 2023

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 522810c.
There were significant changes against commit 8d04ac1:

  Benchmark   Metric       Change
  =========================================
+ stdlib      task-clock    -1.0% (-36.8 σ)

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

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

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

nomeata commented Dec 28, 2023

The test suit succeeds but mathlib fails. I hope I can distill a test case from this.

@nomeata
Copy link
Collaborator Author

nomeata commented Dec 28, 2023

Hmm, the four breaking cases were easy to fix (leanprover-community/mathlib4@af6cccb925 ):

  • Three cases have subgoals in a different order, but it’s not clear to me that the original order was better. Also unclear why this happens. Maybe an instantiateMVars that wasn’t there before?
  • In one case there was a porting note
        /- porting note: the following is indented two spaces more than it should be due to
          strange behavior of `erw` -/
        all_goals …
    
    and indeed the fix was to un-indent the all_goals, as one would expect. Did I inadvertently fix a minor bug here? But still a bit mysterious to me why this would happen with this change.

Then mathlib builds, but it’s test suite fails, e.g. at

-- Discharge side conditions from local hypotheses.
/--
info: Try this: rw [h p]
-- "no goals"
-/
#guard_msgs in
example {P : Prop} (p : P) (h : P → 1 = 2) : 2 = 1 := by
  rw?

so there is something fishy going on with side-conditions.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 28, 2023
@nomeata nomeata changed the title refactor: rewriter: Produce simpler proof terms refactor: rewrite: produce simpler proof terms Dec 28, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 28, 2023
@nomeata
Copy link
Collaborator Author

nomeata commented Dec 28, 2023

Note to myself:

Using mkCongrArg reveals that the way I construct the motive here is not always non-dependent:

./././Mathlib/CategoryTheory/Limits/IsLimit.lean:538:20: error: AppBuilder for 'congrArg', non-dependent function expected
  fun _a ↦ { pt := pt✝, π := _a }.π.app j
has type
  (_a : (Functor.cones F).obj (op pt✝)) → ((const J).obj { pt := pt✝, π := _a }.pt).obj j ⟶ F.obj j

(but it is after a bit more evaluation; maybe mkCongrArg is a bit too picky here.)

This reverts commit c5c6661.
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Dec 30, 2023
@kim-em
Copy link
Collaborator

kim-em commented Jan 8, 2024

Then mathlib builds, but it’s test suite fails, e.g. at

-- Discharge side conditions from local hypotheses.
/--
info: Try this: rw [h p]
-- "no goals"
-/
#guard_msgs in
example {P : Prop} (p : P) (h : P → 1 = 2) : 2 = 1 := by
  rw?

so there is something fishy going on with side-conditions.

@nomeata, the "something fishy" is the rw? tactic inspecting the proof term in order to reconstruct the arguments of the successful rewrite lemma: https://github.com/leanprover-community/mathlib4/blob/master/Mathlib/Tactic/Rewrites.lean#L36.

This is obviously evil, but I'm not sure of a better way to do it. In any case it should be easy to update that function to match your new scheme. Let me know if you'd like me to do something in that file.

@nomeata
Copy link
Collaborator Author

nomeata commented Jan 8, 2024

Ha, should have thought of that!

I can do it myself when I come back to this (non-pressing) experiment, now that I know where to look.

Should rw? at least warn if it by? doesn’t work (“Could rewrite with lem, but failed to extract arguments, please report a bug”) , or is that an expected error mode?

@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 Jan 8, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 8, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc January 8, 2024 13:26 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 8, 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 Jan 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 15, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels Jan 15, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc January 15, 2024 17:23 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 15, 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 Jan 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 15, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Jan 15, 2024

Here another benchmark run:
http://speed.lean-fro.org/mathlib4/compare/473da4ea-9efd-484f-a676-b52133e4ad35/to/77c1316a-877a-44d1-abf7-12b72a0e9306

  • olean size -2.18 %
  • lint wall-clock -1.144 % %
  • build tactic execution s -11.514 % %
  • build wall-clock -0.745 %
  • build instructions -1.714 %

So the gains look relatively stable.

@nomeata nomeata marked this pull request as ready for review January 15, 2024 20:55
@nomeata nomeata requested a review from leodemoura as a code owner January 15, 2024 20:55
@nomeata nomeata added the awaiting-review Waiting for someone to review the PR label Jan 15, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 15, 2024
@nomeata nomeata added this pull request to the merge queue Jan 19, 2024
Merged via the queue into master with commit 52d0f71 Jan 19, 2024
23 checks passed
nomeata added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 20, 2024
commit 05ed454
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 21:11:11 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit f984c96
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 19:02:16 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 0dcffb9
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 17:27:43 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 60c2d87
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 16:41:44 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 5b3c8cb
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:54:42 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 9cdd75f
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 15 15:48:45 2024 +0100

    Reduce diff to nightly-testing

commit d85e0f1
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:35:23 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 4239974
Merge: fee326a 2a84dcf
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:17:15 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit fee326a
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 8 13:40:55 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 84a9d48
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 8 13:16:22 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 27867dd
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 8 12:42:07 2024 +0100

    Try to fix rw?

commit 5189cc8
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 8 12:39:41 2024 +0100

    Trigger CI

commit 9dd93f9
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Sat Dec 30 11:09:38 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit 0494ec5
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 18:22:37 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit aa43b20
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 15:48:58 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit af6cccb
Author: Joachim Breitner <[email protected]>
Date:   Thu Dec 28 15:56:05 2023 +0100

    Fix some fallout (reorder goals)

commit cb55f12
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 12:32:36 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit 780f9f2
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 11:36:42 2023 +0000

    Update lean-toolchain for testing leanprover/lean4#3121

commit 8e0fec8
Merge: a21ddd8 e352bb7
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 21 09:30:49 2023 +0000

    Merge master into nightly-testing

commit a21ddd8
Author: Scott Morrison <[email protected]>
Date:   Thu Dec 21 19:37:19 2023 +1100

    merge lean-pr-testing-2964
nomeata added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 20, 2024
commit 05ed454
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 21:11:11 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit f984c96
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 19:02:16 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 0dcffb9
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 17:27:43 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 60c2d87
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 16:41:44 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 5b3c8cb
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:54:42 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 9cdd75f
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 15 15:48:45 2024 +0100

    Reduce diff to nightly-testing

commit d85e0f1
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:35:23 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 4239974
Merge: fee326a 2a84dcf
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 15 14:17:15 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit fee326a
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 8 13:40:55 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 84a9d48
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Mon Jan 8 13:16:22 2024 +0000

    Trigger CI for leanprover/lean4#3121

commit 27867dd
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 8 12:42:07 2024 +0100

    Try to fix rw?

commit 5189cc8
Author: Joachim Breitner <[email protected]>
Date:   Mon Jan 8 12:39:41 2024 +0100

    Trigger CI

commit 9dd93f9
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Sat Dec 30 11:09:38 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit 0494ec5
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 18:22:37 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit aa43b20
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 15:48:58 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit af6cccb
Author: Joachim Breitner <[email protected]>
Date:   Thu Dec 28 15:56:05 2023 +0100

    Fix some fallout (reorder goals)

commit cb55f12
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 12:32:36 2023 +0000

    Trigger CI for leanprover/lean4#3121

commit 780f9f2
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 28 11:36:42 2023 +0000

    Update lean-toolchain for testing leanprover/lean4#3121

commit 8e0fec8
Merge: a21ddd8 e352bb7
Author: leanprover-community-mathlib4-bot <[email protected]>
Date:   Thu Dec 21 09:30:49 2023 +0000

    Merge master into nightly-testing

commit a21ddd8
Author: Scott Morrison <[email protected]>
Date:   Thu Dec 21 19:37:19 2023 +1100

    merge lean-pr-testing-2964
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels Jan 20, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 25, 2024
Encouraged by the performance gains from making `rewrite` produce
smaller proof objects
(#3121) I am here looking for low-hanging fruit in `simp`.

Consider this typical example:

```
set_option pp.explicit true

theorem test
  (a : Nat)
  (b : Nat)
  (c : Nat)
  (heq : a = b)
  (h : (c.add (c.add ((c.add b).add c))).add c = c)
  : (c.add (c.add ((c.add a).add c))).add c = c
```
We get a rather nice proof term when using
```
  := by rw [heq]; assumption
```
namely
```
theorem test : ∀ (a b c : Nat),
  @eq Nat a b →
    @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c →
      @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c :=
fun a b c heq h =>
  @Eq.mpr (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c)
    (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c)
    (@congrArg Nat Prop a b (fun _a => @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c _a) c))) c) c) heq) h
```
(this is with #3121).

But with `by simp only [heq]; assumption`, it looks rather different:

```
theorem test : ∀ (a b c : Nat),
  @eq Nat a b →
    @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c →
      @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c :=
fun a b c heq h =>
  @Eq.mpr (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c)
    (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c)
    (@id
      (@eq Prop (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c)
        (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c))
      (@congrFun Nat (fun a => Prop) (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c))
        (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c))
        (@congrArg Nat (Nat → Prop) (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c)
          (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) (@eq Nat)
          (@congrFun Nat (fun a => Nat) (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))))
            (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))))
            (@congrArg Nat (Nat → Nat) (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c)))
              (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) Nat.add
              (@congrArg Nat Nat (Nat.add c (Nat.add (Nat.add c a) c)) (Nat.add c (Nat.add (Nat.add c b) c)) (Nat.add c)
                (@congrArg Nat Nat (Nat.add (Nat.add c a) c) (Nat.add (Nat.add c b) c) (Nat.add c)
                  (@congrFun Nat (fun a => Nat) (Nat.add (Nat.add c a)) (Nat.add (Nat.add c b))
                    (@congrArg Nat (Nat → Nat) (Nat.add c a) (Nat.add c b) Nat.add
                      (@congrArg Nat Nat a b (Nat.add c) heq))
                    c))))
            c))
        c))
    h
```
Since simp uses only single-step `congrArg`/`congrFun` congruence lemmas
here, the proof
term grows very large, likely quadratic in this case.

Can we do better? Every nesting of `congrArg` (and it's little brother
`congrFun`) can be
turned into a single `congrArg` call. 

In this PR I make making the smart app builders `Meta.mkCongrArg` and
`Meta.mkCongrFun` a bit
smarter and not only fuse with `Eq.refl`, but also with
`congrArg`/`congrFun`.

Now we get, in this simple example,
```
theorem test : ∀ (a b c : Nat),
  @eq Nat a b →
    @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c →
      @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c :=
fun a b c heq h =>
  @Eq.mpr (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c a) c))) c) c)
    (@eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c b) c))) c) c)
    (@congrArg Nat Prop a b (fun x => @eq Nat (Nat.add (Nat.add c (Nat.add c (Nat.add (Nat.add c x) c))) c) c) heq) h
```

Let’s see if it works and how much we gain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan 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.

4 participants