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: encode let_fun using a letFun function #2973

Merged
merged 6 commits into from Dec 18, 2023
Merged

Conversation

kmill
Copy link
Collaborator

@kmill kmill commented Nov 28, 2023

Switches from encoding let_fun using an annotated (fun x : t => b) v expression to a function application letFun v (fun x : t => b).

@kmill
Copy link
Collaborator Author

kmill commented Nov 28, 2023

@leodemoura I carried out your suggestion to re-encode let_fun as a function application.

I'm not sure if you were expecting it to be for dependent or non-dependent lets. I investigated making it be only for non-dependent lets, but I found that the first example in tests/lean/letFun.lean was dependent until metavariables were finally solved for when synthesizing default instances.

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

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 28, 2023
@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 Nov 28, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 29, 2023
@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 Nov 29, 2023
@leodemoura
Copy link
Member

I'm not sure if you were expecting it to be for dependent or non-dependent lets.

It is for non-dependent lets.

I investigated making it be only for non-dependent lets, but I found that the first example in tests/lean/letFun.lean

I can encode all let_funs in this test file using the auxiliary letFun function. Could you please clarify the issue you are having?

def letFun {α : Sort u} {β : α → Sort v} (v : α) (f : (x : α) → β x) : β v := f v

#check
  letFun (fun x => x * 2) fun f =>
  letFun 1 fun x =>
  letFun (x + 1) fun y =>
  f (y + x)

example (a b : Nat) (h1 : a = 0) (h2 : b = 0) : (letFun (a + 1) fun x => x + x) > b := by
  simp (config := { beta := false }) [h1]
  trace_state
  simp (config := { decide := true }) [h2]

@kmill
Copy link
Collaborator Author

kmill commented Dec 1, 2023

@leodemoura I've used the wrong terminology -- using SimpLetCase for clarification, I mean to say I wasn't sure if you were expecting the nondepDepVar or nondep encoding. I got the nondepDepVar encoding of letFun working in this PR, and it does exactly what you wrote in your code block.

@leodemoura
Copy link
Member

leodemoura commented Dec 6, 2023

I got the nondepDepVar encoding of letFun working in this PR, and it does exactly what you wrote in your code block.

@kmill
If I understood you correctly, you are saying that you can only handle cases where β is a constant function fun _ => β0, correct?

What is the obstacle for getting it working in the more general case?

If I understood you correctly, you currently cannot elaborate an example that would produce:

  letFun (α := Nat) (β := fun n => { as : List Bool // as.length ≤ n } ) 5 fun n =>
    ⟨[], Nat.zero_le n⟩

As far as I remember, the old let_fun elaborator does not support this kind of example. Example:

#check id (α := { as : List Bool // as.length ≤ 5 }) $ 
  let_fun n := 5
  ⟨[], Nat.zero_le n⟩ -- Error here

Even using lambda directly, we need to include a helper type ascription.

#check id (α := { as : List Bool // as.length ≤ 5 }) $ 
  (fun n => (⟨[], Nat.zero_le n⟩ : { as : List Bool // as.length ≤ n })) 5

We usually use kabstract in this kind of situation. Not sure whether it is worth the trouble if your current implementation works for every example where the old one works.
Moreover, if we will never support the more general case, should we simplify letFun?

def letFun {α : Sort u} {β : Sort v} (v : α) (f : α → β) : β := f v

@kmill
Copy link
Collaborator Author

kmill commented Dec 6, 2023

@leodemoura I don't have any obstruction here, and I can handle non-constant type families. I just wanted to be sure that you weren't expecting this simplified letFun encoding that you had proposed in your original message since I instead used this "nonDepDepVar" encoding, which I switched to at the first sign of trouble. The latter was more obvious how to implement. In particular mkLetFun, to produce the simple encoding, would need to force β to not depend on the fvar x, even in the presence of metavariables.

Examples like this one definitely work with this PR's implementation:

#check let_fun n := 5
  (⟨[], Nat.zero_le n⟩ : { as : List Bool // as.length ≤ n })

I haven't evaluated whether these sorts of examples appear in the wild, but I would not be surprised if they do.

You are correct that it does not generalize the value in the expected type, so your examples with { as : List Bool // as.length ≤ 5 } do not work. I personally believe generalization would be a surprising feature for let_fun, and if that is at some point a requested feature, that it should be either an option (like let_fun (generalizing := true) n := 5; ...) or be a new binding syntax.

@leodemoura
Copy link
Member

@kmill Let's move forward and merge.

@kmill
Copy link
Collaborator Author

kmill commented Dec 12, 2023

@leodemoura Sounds good. Let me know if you need me to do anything. (Maybe I should rebase this onto the newest nightly and regenerate the stage0?)

@semorrison
Copy link
Collaborator

(Maybe I should rebase this onto the newest nightly and regenerate the stage0?)

Yes please.

@kmill
Copy link
Collaborator Author

kmill commented Dec 13, 2023

@semorrison Done.

By the way, this will take a small change to std. I don't have push access to that project, so didn't update the testing branch. Instead, I made my own branch and modified the mathlib testing branch to use that. It was an easy fix: kmill/std4@7ca1c2c

This is an abbreviation for `(fun x => b) v`, so the value of `x` is not accessible to `b`,
unlike with `let x := v; b`.
-/
def letFun {α : Sort u} {β : α → Sort v} (v : α) (f : (x : α) → β x) : β v := f v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This encoding means that even with config.zeta false, we reduce let_funs when at default reducibility, correct? Is that intentional or should it be [irreducible]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that is something I did not think enough about. I made it be irreducible and added more tests. (I also extended the optimization for ignoring unnecessary lets in isDefEqQuick to also consider let_fun.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leodemoura I'll wait for your final confirmation on this point before merging

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We don't want to reduce let_funs when config.zeta = false and using the default reducibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We don't want to reduce let_funs when config.zeta = false and using the default reducibility.

src/Init/Prelude.lean Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Ullrich <sebasti@nullri.ch>
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the builds-mathlib CI has verified that Mathlib builds against this PR label Dec 14, 2023
@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 14, 2023
@Kha Kha added this pull request to the merge queue Dec 18, 2023
Merged via the queue into leanprover:master with commit a2226a4 Dec 18, 2023
13 checks passed
@semorrison
Copy link
Collaborator

@Kha, @kmill, I don't think we should have merged this while it had a breaks-mathlib label!

It has indeed broken both Std and Mathlib's nightly-testing branches, and without understanding this PR in detail I don't immediately know how to fix them.

(The discussion on zulip might be a better place for follow-up than here.)

@kmill
Copy link
Collaborator Author

kmill commented Dec 20, 2023

@semorrison Did you see my message earlier, #2973 (comment) ?

I'll follow up on zulip too.

mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 21, 2023
This is a PR to Mathlib's `bump/v4.5.0` branch, containing adaptations required for leanprover/lean4#2973, which landed in `leanprover/lean4:nightly-2023-12-19` and will become part of `v4.5.0-rc1` soon.



Co-authored-by: Kyle Miller <kmill31415@gmail.com>
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Dec 22, 2023
…h. (#9188)

This PR:
* bumps to lean-toolchain to `v4.5.0-rc1`
* bumps the Std and Aesop dependencies to their versions using `v4.5.0-rc1`
* merge the already reviewed changes from the `bump/v4.5.0` branch
  * adaptations for leanprover/lean4#2923 in #9011
  * adaptations for leanprover/lean4#2973 in #9161
  * adaptations for leanprover/lean4#2964 in #9176

Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
nomeata added a commit that referenced this pull request Jan 22, 2024
both Structural and WF recursion have a preprocess module (since #2818
at least). Back then I found that betareducing in WF, like it happens
for structural, bytes because it would drop `let_fun` annotations, which
are of course worth preserving.

Now that #2973 is landed let's see if we can do it now.

I hope that this fixes an issue where code like
```
theorem size_rev {α} (arr : Array α) (i : Nat) : (rev arr i).size = arr.size := by
  unfold rev
  split
  · rw [size_rev]
    rw [Array.size_swap]
  · rfl
termination_by arr.size - i
decreasing_by simp_wf; omega
```
fails but works with `size_rev _ _`.

It fails due to some lambda-redexes that maybe go away.

(Maybe we can also/additinally/alternatively track down where that redex
is created and use `.beta` instead of `.app`.)
github-merge-queue bot pushed a commit that referenced this pull request Jan 24, 2024
…cations (#3204)

Recursive predefinitions contains “rec app” markers as mdata in the
predefinitions,
but sometimes these get in the way of termination checking, when you
have
```
  [mdata (fun x => f)] arg
```

Therefore, the `preprocess` pass floats them out of applications
(originally
only for structural recursion, since #2818 also for well-founded
recursion).

But the code was incomplete: Because `Meta.transform` calls `post` on `f
x y` only
once (and not also on `f x`) one has to float out of nested applications
as well.

A consequence of this can be that in a recursive proof, `rw [foo]` does
not work
although `rw [foo _ _]` does.

Also adding the testcase where @david-christiansen and I stumbled over
this


(Maybe the two preprocess modules can be combined, now that #2973 is
landed, will try that
in a follow-up).
@leodemoura leodemoura mentioned this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants