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

isDefEq unfolds recursive definition #3988

Closed
nomeata opened this issue Apr 24, 2024 · 3 comments · Fixed by #4061
Closed

isDefEq unfolds recursive definition #3988

nomeata opened this issue Apr 24, 2024 · 3 comments · Fixed by #4061
Labels
bug Something isn't working

Comments

@nomeata
Copy link
Contributor

nomeata commented Apr 24, 2024

This is something I have seen today during office hours, and is plausible the reason for significant slow downs.

Consider

def foo : Nat → Nat
  | 0 => 0
  | (n +1) => foo n + 1
termination_by n => n

example (h : n = m) (h2 : foo n > 42) : foo (m + 1) > 42 :=
  set_option trace.Meta.isDefEq true in
  (h ▸ h2)

The trace shows something like

[Meta.isDefEq] ❌ foo (n + 1) > 42 =?= foo n > 42 ▼
  [] ❌ foo (n + 1) =?= foo n ▼
    [] ❌ n + 1 =?= n ▶
    [] ❌ WellFounded.fix foo.proof_1
          (fun a a_1 =>
            (match (motive :=
                (x : Nat) → ((y : Nat) → (invImage (fun a => a) instWellFoundedRelation).1 y x → Nat) → Nat) a with
              | 0 => fun x => 0
              | Nat.succ n => fun x => x n ⋯ + 1)
              a_1)
          (n +
            1) =?= WellFounded.fix foo.proof_1
          (fun a a_1 =>
            (match (motive :=
                (x : Nat) → ((y : Nat) → (invImage (fun a => a) instWellFoundedRelation).1 y x → Nat) → Nat) a with
              | 0 => fun x => 0
              | Nat.succ n => fun x => x n ⋯ + 1)
              a_1)
          n ▶
  [] ❌ 42 < foo (n + 1) =?= 42 < foo n ▶

and I wonder if this means that is using isDefEq at the wrong reducibility setting.

Wrapping this in by with_reducible exact avoids going near WellFounded.fix, as expected.

Maybe unrelated, but it seems it tries the same equality twice, ordered differently:

[Meta.isDefEq] ❌ foo (n + 1) > 42 =?= foo n > 42 ▶

[Meta.isDefEq] ❌ foo n > 42 =?= foo (n + 1) > 42 ▶

Versions

4.7.0

Impact

Add 👍 to issues you consider important. If others are impacted by this issue, please ask them to add 👍 to it.

@nomeata nomeata added the bug Something isn't working label Apr 24, 2024
@nomeata nomeata changed the title Subst syntax unfolds recursive definition Subst syntax (▶) unfolds recursive definition Apr 24, 2024
@leodemoura leodemoura changed the title Subst syntax (▶) unfolds recursive definition isDefEq unfolds recursive definition Apr 24, 2024
@leodemoura
Copy link
Member

This is an issue with isDefEq. I have revised the title. Yes, it is worth reconsidering whether we should unfold declarations defined using well-founded recursion in isDefEq. The current default behavior is to unfold them. This seldom produces desirable results, although some users rely on these definitions being unfolded. One option is to introduce a flag to control whether declarations defined by well-founded recursion can be unfolded by isDefEq. This flag would be set to false by default, but users could set it to true when needed. We could also provide better diagnostics (e.g., indicating functions that were not unfolded).

Remark: Declarations defined by structural recursion are unfolded all the time in practice by isDefEq, and many users rely on this.

We have been postponing the introduction of the flag because we will have a module system that will allow us to control much more precisely what can be unfolded and where it can occur.

@nomeata
Copy link
Contributor Author

nomeata commented Apr 24, 2024

That is one way to ask the question - should wf-recursive functions be unfolded with default transparency (usually not).

But what about the alternative question we could ask: should a “rewriting-like” tactic like use default setting or should it use reducible setting? Especially as it seems to perform failing isDedEq checks even when the tactic is overall successful (now shown in this example).

For conv we recently changed the setting of the terminal try rfl to reducible. I have a gut feeling that here, too, we don't want to spend too much time on a backtracked DefEq check, independent of whether there is a wf-recursive or other function involved.

@leodemoura
Copy link
Member

But what about the alternative question we could ask: should a “rewriting-like” tactic like ▶ use default setting or should it use reducible setting?

Users rely on default setting in multiple places.

nomeata added a commit that referenced this issue Apr 26, 2024
we keep running into examples where working with well-founded recursion
is slow becuase defeq checks (which are all over the place, including
failing ones that are back-tracked) unfold well-founded definitions.

The definition of a function defined by well-founded recursion should be
an implementation detail that should only be peeked inside by the
equation generator and the functional induction generator.

We now mark the mutual recursive function as irreducible, and use
`withAtLeastTransparency .all` when producing the equations.

This fixes #3988
nomeata added a commit that referenced this issue May 3, 2024
we keep running into examples where working with well-founded recursion
is slow becuase defeq checks (which are all over the place, including
failing ones that are back-tracked) unfold well-founded definitions.

The definition of a function defined by well-founded recursion should be
an implementation detail that should only be peeked inside by the
equation generator and the functional induction generator.

We now mark the mutual recursive function as irreducible, and use
`withAtLeastTransparency .all` when producing the equations.

This fixes #3988
github-merge-queue bot pushed a commit that referenced this issue May 10, 2024
we keep running into examples where working with well-founded recursion
is slow because defeq checks (which are all over the place, including
failing ones that are back-tracked) unfold well-founded definitions.

The definition of a function defined by well-founded recursion should be
an implementation detail that should only be peeked inside by the
equation generator and the functional induction generator.

We now mark the mutual recursive function as irreducible (if the user
did not
set a flag explicitly), and use `withAtLeastTransparency .all` when
producing
the equations.

Proofs can be fixed by using rewriting, or – a bit blunt, but nice for
adjusting
existing proofs – using `unseal` (a.k.a. `attribute [local
semireducible]`).

Mathlib performance does not change a whole lot:

http://speed.lean-fro.org/mathlib4/compare/08b82265-75db-4a28-b12b-08751b9ad04a/to/16f46d5e-28b1-41c4-a107-a6f6594841f8
Build instructions -0.126 %, four modules with significant instructions
decrease.

To reduce impact, these definitions were changed:

* `Nat.mod`, to make `1 % n` reduce definitionally, so that `1` as a
`Fin 2` literal
works nicely. Theorems with larger `Fin` literals tend to need a `unseal
Nat.modCore`
   #4098
* `List.ofFn` rewritten to be structurally recursive and not go via
`Array.ofFn`:
   leanprover-community/batteries#784

Alternative designs explored were

 * Making `WellFounded.fix` irreducible. 
 
One benefit is that recursive functions with equal definitions (possibly
after
instantiating fixed parameters) are defeq; this is used in mathlib to
relate

[`OrdinalApprox.gfpApprox`](https://leanprover-community.github.io/mathlib4_docs/Mathlib/SetTheory/Ordinal/FixedPointApproximants.html#OrdinalApprox.gfpApprox)
with `.lfpApprox`.
   
   But the downside is that one cannot use `unseal` in a
targeted way, being explicit in which recursive function needs to be
reducible here.

And in cases where Lean does unwanted unfolding, we’d still unfold the
recursive
definition once to expose `WellFounded.fix`, leading to large terms for
often no good
   reason.

* Defining `WellFounded.fix` to unroll defintionally once before hitting
a irreducible
`WellFounded.fixF`. This was explored in #4002. It shares most of the
ups and downs
with the previous variant, with the additional neat benefit that
function calls that
do not lead to recursive cases (e.g. a `[]` base case) reduce nicely.
This means that
   the majority of existing `rfl` proofs continue to work.

Issue #4051, which demonstrates how badly things can go if wf recursive
functions can be
unrolled, showed that making the recursive function irreducible there
leads to noticeably
faster elaboration than making `WellFounded.fix` irreducible; this is
good evidence that
the present PR is the way to go. 

This fixes #3988

---------

Co-authored-by: Leonardo de Moura <leomoura@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants