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

Superfluous parentheses can inhibit structural recursion #2810

Closed
1 task done
kmill opened this issue Nov 3, 2023 · 4 comments · Fixed by #2818
Closed
1 task done

Superfluous parentheses can inhibit structural recursion #2810

kmill opened this issue Nov 3, 2023 · 4 comments · Fixed by #2818
Assignees
Labels
bug Something isn't working

Comments

@kmill
Copy link
Collaborator

kmill commented Nov 3, 2023

Prerequisites

  • Put an X between the brackets on this line if you have done all of the following:
    • Check that your issue is not already filed.
    • Reduce the issue to a minimal, self-contained, reproducible test case. Avoid dependencies to mathlib4 or std4.

Description

When additional parentheses are inserted around a function, this can prevent structural recursion from succeeding.

Simple example:

def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n

With the parentheses, "structural recursion cannot be used." Replacing (f) n with f n makes it succeed. Looking at the terms (f) n and f n, the difference is whether n is included within the recApp metadata expression.

This behavior indirectly appeared in a Zulip question when trying to make use of structural recursion within a theorem. This is a minified example:

theorem thm (N : Nat) :
    N * 0 = 0 :=
  match N with
  | Nat.zero => rfl
  | Nat.succ n => by
    rewrite [Nat.succ_mul, Nat.add_zero]
    rw [thm]

Replacing rw [thm] with rw [thm n] makes structural recursion succeed -- the terms are identical except for whether n is included within a recApp metadata expression. Replacing rw [thm] by simp only [thm] also makes it succeed -- in this case simp does not create an appRec metadata expression.

Context

Zulip discussion

Versions

leanprover/lean4:v4.3.0-rc1

Impact

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

@kmill kmill added the bug Something isn't working label Nov 3, 2023
@nomeata
Copy link
Contributor

nomeata commented Nov 3, 2023

I ran into this issues a few weeks ago as well, while helping some user.

Mostly a guess at this point, so jumping in here just for learning, but probably at this line:

e.withApp fun f args => do

the withApp may need to be a variant looks through the recApp metadata?

@digama0
Copy link
Collaborator

digama0 commented Nov 3, 2023

I think that would work to recognize this application, but this is in the context of something that is transforming the expression, so you shouldn't throw away any mdata except the _recApp node; see the .mdata case of the same function. I think this will need another recursive function doing the equivalent of withApp.

@nomeata
Copy link
Contributor

nomeata commented Nov 3, 2023

Yes, I’d expect a annoyingly special withAppIgnoringRecApp.

@nomeata
Copy link
Contributor

nomeata commented Nov 3, 2023

Let me give this a shot. I am supposed to work on that code soon anyways, so it’s a good learning experience.

@nomeata nomeata self-assigned this Nov 3, 2023
nomeata added a commit that referenced this issue Nov 3, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practical,
such code seems to be produced when using `rw` or `simp` in recursive
theorems.

This creates variants of the `Expr.app`-decomposing functions that look
through that annotation and uses it in a few neuralgic points in the
structural recursion code.

Test case is included. Fixes #2810.
nomeata added a commit that referenced this issue Nov 3, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practical,
such code seems to be produced when using `rw` or `simp` in recursive
theorems.

This creates variants of the `Expr.app`-decomposing functions that look
through that annotation and uses it in a few neuralgic points in the
structural recursion code.

Test case is included. Fixes #2810.
nomeata added a commit that referenced this issue Nov 3, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practical,
such code seems to be produced when using `rw` or `simp` in recursive
theorems.

We can fix the issue by storing the `.mdata` not around the full
application expression, but rather around the inner `.const`. This way,
`Expr.withApp` and similar traversals work without without issue.

While I’m at it: So far only `WellFounded` was actually making use of
this. Now (unlike before)
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => f (n + 1)
```
will show the error with squiggly lines under `f (n + 1)`.

Test case is included. Fixes #2810.
nomeata added a commit that referenced this issue Nov 4, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practically
relevant, such code is to be produced when using `rw` or `simp` in
recursive theorems (see included test case).

We can fix this by preprocessing the definitions and floating the
`.mdata` marker out of applications.

For structural recursion, there already exists a `preprocess` function;
this now also floats out `.mdata` markers.

For well-founded recursion, this introduces an analogous `preprocess`
function.

Fixes #2810.

One test case output changes: With the `.mdata` out of the way, we get a
different error message. Seems fine.

Alternative approaches are:

* Leaving the `.mdata` marker where it is, and looking around it.
  Tried in #2813, but not nice (many many places where `withApp` etc.
  need to be adjusted).
* Moving the `.mdata` _inside_ the application, so that `withApp` still
  works. Tried in #2814. Also not nice, the invariant that the `.mdata`
  is around the `.const` is tedious to maintain.
kim-em pushed a commit that referenced this issue Nov 6, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practically
relevant, such code is to be produced when using `rw` or `simp` in
recursive theorems (see included test case).

We can fix this by preprocessing the definitions and floating the
`.mdata` marker out of applications.

For structural recursion, there already exists a `preprocess` function;
this now also floats out `.mdata` markers.

For well-founded recursion, this introduces an analogous `preprocess`
function.

Fixes #2810.

One test case output changes: With the `.mdata` out of the way, we get a
different error message. Seems fine.

Alternative approaches are:

* Leaving the `.mdata` marker where it is, and looking around it.
  Tried in #2813, but not nice (many many places where `withApp` etc.
  need to be adjusted).
* Moving the `.mdata` _inside_ the application, so that `withApp` still
  works. Tried in #2814. Also not nice, the invariant that the `.mdata`
  is around the `.const` is tedious to maintain.
nomeata added a commit that referenced this issue Nov 6, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practically
relevant, such code is to be produced when using `rw` or `simp` in
recursive theorems (see included test case).

We can fix this by preprocessing the definitions and floating the
`.mdata` marker out of applications.

For structural recursion, there already exists a `preprocess` function;
this now also floats out `.mdata` markers.

For well-founded recursion, this introduces an analogous `preprocess`
function.

Fixes #2810.

One test case output changes: With the `.mdata` out of the way, we get a
different error message. Seems fine.

Alternative approaches are:

* Leaving the `.mdata` marker where it is, and looking around it.
  Tried in #2813, but not nice (many many places where `withApp` etc.
  need to be adjusted).
* Moving the `.mdata` _inside_ the application, so that `withApp` still
  works. Tried in #2814. Also not nice, the invariant that the `.mdata`
  is around the `.const` is tedious to maintain.
github-merge-queue bot pushed a commit that referenced this issue Nov 22, 2023
This didn't work before
```
def f (n : Nat) : Nat :=
  match n with
  | 0 => 0
  | n + 1 => (f) n
```
because the `RecApp` metadata marker gets in the way. More practically
relevant, such code is to be produced when using `rw` or `simp` in
recursive theorems (see included test case).

We can fix this by preprocessing the definitions and floating the
`.mdata` marker out of applications.

For structural recursion, there already exists a `preprocess` function;
this now also floats out `.mdata` markers.

For well-founded recursion, this introduces an analogous `preprocess`
function.

Fixes #2810.

One test case output changes: With the `.mdata` out of the way, we get a
different error message. Seems fine.

Alternative approaches are:

* Leaving the `.mdata` marker where it is, and looking around it.
  Tried in #2813, but not nice (many many places where `withApp` etc.
  need to be adjusted).
* Moving the `.mdata` _inside_ the application, so that `withApp` still
  works. Tried in #2814. Also not nice, the invariant that the `.mdata`
  is around the `.const` is tedious to maintain.
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
3 participants