-
Notifications
You must be signed in to change notification settings - Fork 437
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
RFC: Run decreasing_by tactic on all goals, not each goal #2921
Comments
An implementation seems to be rather simple: In I have a POC at https://github.com/nomeata/lean-derec-lab/blob/master/Derec/SingleGoal.lean It would be a breaking change requiring existing uses of |
Like the idea, but what about mutual recursion? The actual order may not be clear to users. Recall that we compute SCCs. |
Hmm, yes, the order may be surprising. We could sort the MVars by the syntax I'll experiment with multiple SCCs and see. |
Hmm, indeed, I didn’t have it on the radar that the This already causes corner case issues like: If some recursions in the block should be done structurally, and others require a
Not a problem per se, just an example of the complications to keep in mind here. |
Together with Leo I we had an idea for a nice way that solves
We allow the user to specify the tactic/proof keyed by the function it should be used in; like we already do with What would be a good syntax? One idea is to use
It’s a bit verbose and silly if there is only one recursive function, which is a common case. But at least it’s consistent with |
By the way, this relates to a syntax nit that I have been meaning to bring up: Why does mutual
def foo : Nat → Nat
| 0 => 0
| .succ n => bar n + bar (n - 1)
decreasing_by
· -- first recursive call in foo
· -- second recursive call in foo
def bar : Nat → Nat
| n => if n = 0 then 0 else foo (n - 1)
decreasing_by
· -- first recursive call in bar
end They would still be processed together in the same way as is done currently, this is just a syntactic change. I can see some reasons for keeping all the termination_by clauses together, if for example it makes it easier to review the functions for the mutual block and see that they interleave appropriately. Perhaps both could be supported and you could mix and match this style with the current everything-at-the-end style if this is a stylistic preference. |
That’s a good idea as well. Makes it also much more natural where to place thigns in case of
I tend to lean towards less options, as having to decide between options itself is a UX cost, so preferrably only if there are strong reasons to have both. Maybe Do we have precedence for “tactic-valued options”? |
Ah, one possible reason to have a common |
I rewrote the proposal to include some of the ideas from this discussion, in particular
|
Is there a concrete syntax proposal in the case of |
I didn't spell it out yet, but my hope is that both |
I suppose it would have to be indentation sensitive at least. Indentation would determine whether a clause belongs to the last |
Ah, I see. I sometimes forget that Lean is less indentation sensitive than it looks like :-) Do you think that’s feasible and reasonable? |
Yes to both I think. At least it's where I, probably, would have tried to put them as a user. |
I think it would also be okay to have the |
I just had a discussion with Leo about this, and he’s generally supportive of this idea. I thus plan to work on this eventually. |
With set_option showInferredTerminationBy true this prints a message like Inferred termination argument: termination_by ackermann n m => (sizeOf n, sizeOf m) it tries hard to use names that * match the names that the user used, if present * have no daggers (so that it can be copied) * do not shadow each other * do not shadow anything from the environment (just to be nice) it does so by appending sufficient `'` to the name. Some of the emitted `sizeOf` calls unnecessary, but they are needed sometimes with dependent parameters. A follow-up PR will not emit them for non-dependent arguments, so that in most cases the output is pretty. Somewhen down the road we also want a code action, maybe triggered by `termination_by?`. This should come after #2921, as that simplifies that feature (no need to merge termination arguments from different cliques for example.)
With set_option showInferredTerminationBy true this prints a message like Inferred termination argument: termination_by ackermann n m => (sizeOf n, sizeOf m) it tries hard to use names that * match the names that the user used, if present * have no daggers (so that it can be copied) * do not shadow each other * do not shadow anything from the environment (just to be nice) it does so by appending sufficient `'` to the name. Some of the emitted `sizeOf` calls unnecessary, but they are needed sometimes with dependent parameters. A follow-up PR will not emit them for non-dependent arguments, so that in most cases the output is pretty. Somewhen down the road we also want a code action, maybe triggered by `termination_by?`. This should come after #2921, as that simplifies that feature (no need to merge termination arguments from different cliques for example.)
This is pure refactoring: Instead of solving each subgoal as we encounter it while traversing the syntax tree, we leave the `MVar` there, at the end collect them all using `getMVarsNoDelayed`, and then solve them. This is a refactoring preparing for two upcoming changes: * removing unexpected duplicate goals that can arise from term duplication * running interactive tactics on all, not each goal (#2921) In order to not regress with error locations, we have to associated the `TermElabM`’s syntax refernce with the `MVar` somehow. I do this using the existing `mkRecAppWithSyntax` expression annotation, on the `MVar`’s type. Alternatives would be stack another `StateT` on the traversal and accumulate `Array (MVarId, Syntax)` explicitly, but that did not seem to be more appealing.
This is pure refactoring: Instead of solving each subgoal as we encounter it while traversing the syntax tree, we leave the `MVar` there, at the end collect them all using `getMVarsNoDelayed`, and then solve them. This is a refactoring preparing for two upcoming changes: * removing unexpected duplicate goals that can arise from term duplication * running interactive tactics on all, not each goal (#2921) In order to not regress with error locations, we have to associated the `TermElabM`’s syntax refernce with the `MVar` somehow. I do this using the existing `mkRecAppWithSyntax` expression annotation, on the `MVar`’s type. Alternatives would be stack another `StateT` on the traversal and accumulate `Array (MVarId, Syntax)` explicitly, but that did not seem to be more appealing.
With set_option showInferredTerminationBy true this prints a message like Inferred termination argument: termination_by ackermann n m => (sizeOf n, sizeOf m) it tries hard to use names that * match the names that the user used, if present * have no daggers (so that it can be copied) * do not shadow each other * do not shadow anything from the environment (just to be nice) it does so by appending sufficient `'` to the name. Some of the emitted `sizeOf` calls are unnecessary, but they are needed sometimes with dependent parameters. A follow-up PR will not emit them for non-dependent arguments, so that in most cases the output is pretty. Somewhen down the road we also want a code action, maybe triggered by `termination_by?`. This should come after #2921, as that simplifies that feature (no need to merge termination arguments from different cliques for example.)
until around 7fe6881 the way to define well-founded recursions was to specify a `WellFoundedRelation` on the argument explicitly. This was rather low-level, for example one had to predict the packing of multiple arguments into `PProd`s, the packing of mutual functions into `PSum`s, and the cliques that were calculated. Then the current `termination_by` syntax was introduced, where you sepecify the termination argument at a higher level (one clause per functions, unpacked arguments), and the `WellFoundedRelation` is found using type class resolution. The old syntax was kept around as `termination_by'`. But at the time of writing, this is not used anywhere in the lean, std, mathlib or the theroem-proving-in-lean repositories. Also, should be possible to express anything that the old syntax supported also with the new one, possibly requiring a helper type with a suitable instance, or a very generic wrapper like ``` inductive WithWFRel {a : Type _} {rel : a → a → Prop} (h : WellFounded rel) where | mk : a -> WithWFRel h instance : WellFoundedRelation (WithWFRel h) := invImage (fun x => match x with | .mk x => x) ⟨_, h⟩ ``` Since the old syntax is unused, has an unhelpful name and relies on internals, this removes the support. Now is a good time before the refactoring that's planned in #2921. The test suite was updated without particular surprises. The parametric `terminationHint` parser is gone, which means we can match on syntax more easily now, in `expandDecreasingBy?`.
until around 7fe6881 the way to define well-founded recursions was to specify a `WellFoundedRelation` on the argument explicitly. This was rather low-level, for example one had to predict the packing of multiple arguments into `PProd`s, the packing of mutual functions into `PSum`s, and the cliques that were calculated. Then the current `termination_by` syntax was introduced, where you specify the termination argument at a higher level (one clause per functions, unpacked arguments), and the `WellFoundedRelation` is found using type class resolution. The old syntax was kept around as `termination_by'`. This is not used anywhere in the lean, std, mathlib or the theorem-proving-in-lean repositories, and three occurrences I found in the wild can do without In particular, it should be possible to express anything that the old syntax supported also with the new one, possibly requiring a helper type with a suitable instance, or the following generic wrapper that now lives in std ``` def wrap {α : Sort u} {r : α → α → Prop} (h : WellFounded r) (x : α) : {x : α // Acc r x} ``` Since the old syntax is unused, has an unhelpful name and relies on internals, this removes the support. Now is a good time before the refactoring that's planned in #2921. The test suite was updated without particular surprises. The parametric `terminationHint` parser is gone, which means we can match on syntax more easily now, in `expandDecreasingBy?`.
I made good progress here in #3040, and would like to have some feedback regarding the parsing of I am currently experimenting with The corner case that doesn't work if there is no indentation, like in
Not sure how relevant that is in practice; if the rule otherwise works well and simple, maybe this is ok? Or is there a better solution for this problem? |
My suggestion would be to have the def Ex9.foo (n : Nat) : Nat := foo (dec1 n) + bar n
decreasing_by apply dec1_lt -- this applies to foo
where
bar (m : Nat) : Nat := m
decreasing_by apply dec1_lt -- this applies to bar For backward compatibility (and for convenience when using the same tactic in multiple places) we may wish to also have (You should probably think about how migration is going to go here, because this will break every |
I thought about putting it before the As for migration: the current plan is to move fast and break things, knowing that this is an approach we can only do for so much longer, but can for now. It's basically impossible to support both syntaxes at the same time, because they unfortunately overlap and it's hard to know whether What would make the migration easier without too much overhead is, I believe, if the keyword
|
It's not though? That would be nice, but that's not the current situation.
I think it would be okay to have a simpler backup syntax like I would really like us to start thinking about how to do these things sooner rather than later, because the need for it is not ever going to go away and at least for this change it's worth putting in the effort to automate this to some degree just for the mathlib bump. |
Ah, I got confused by the namespace of the
and
But that’s worth a try! Avoiding the ambiguities with |
Do you propose that we keep the old (renamed) syntax, so that we don’t have an unhelpful parse error, but rather a helpful elaboration error, but still an error? Or do you want the old syntax to also to still work, which means the internal data strutures and logic have to support both variant at the same time? Mathlib has 121 occurrences of Yes, it’s more convenient if you can migrate parsing (or even fully working) code, but I’m not surprised it’s worth the detour at this point. |
I'd prefer if it still works. It should be relatively straightforward to normalize both syntaxes into one data structure and process them with the new code. My understanding is that this kind of syntactic normalization is a major reason for the
I'm also concerned about more than just std + mathlib here: Big CS projects doing formal verification tend to involve complex inductive types and termination measures, so I anticipate a fair amount of breakage in code we don't have much visibility into. |
It's certainly doable (although it'd be nicer if the syntaxes wouldn't overlap, and one would not have to rename to |
For what it's worth, we're working on a fairly large CS verification project in Lean, and we'd be okay with adopting the new syntax: https://github.com/cedar-policy/cedar-spec/tree/main/cedar-lean We do indeed have complex inductive types, but so far, Lean hasn't needed much help proving termination. We have just 5 occurrences of If it helps, this is the entry point to our most complex proof so far: https://github.com/cedar-policy/cedar-spec/blob/main/cedar-lean/Cedar/Thm/Validation/Typechecker.lean |
While working on this, I noticed that there is another bit of |
Problem: Interactive termination proofs
Consider the (of course non-terminating) definition:
The tactic passed to
decreasing_by
is currently executed on each recursive call separately, with one goal present. This makes it harder to write these termination tactics individually, for example when different recursive calls need widely different approaches, because whatever you write has to work with all goals at the same time.As a user I would have expected to be able to focus individual calls with
\.
, like sobut here the second
\.
says “no goals to be solved”.Solution: One subgoal for each recursive goal
To solve this, the proof following
decreasing_by
should have the proof obligations for each recursive call as a separate subgoal.This has a few knock-on effects:
Problem: Mutual recursion and SCCs
If we have a
mutual
block (or localwhere
,letrec
), then Lean will look at all involved definitions, build strongly connected components (SCCs), and derecursify each SCC individually.It seems technically hard to get all subgoals from these independent definitions into a single
TacticM
state. Furthremore, the user experience would be bad if the order of subgoals would change, and sorting the subgoals afterwards is tricky.Solution: Per-function
decreasing_by
.A good solution to this problem seems to be to attach the
decreasing_by
to each function definition, e.g.This naturally associates the proofs with the subgoals of that function. It is arguably also more intuitive in cases of
letrec
orwhere
.Problem: What happens to
terminating_by
?Now that
decreasing_by
is attached to each function, it would be strange to not do the same forterminating_by
.Solution: Per-function
terminating_by
So let’s also move it to per-function. This has the additional effect that we can simplify the syntax and not require the function name as part of the termination hint.
So instead of
we would write
Problem: Changing the default tactic
At the moment,
decreasing_by
is used for two similar, but distinct use-cases:decreasing_tactic
.For example, if there is no
termination_by
clause, and Lean wants to guess the right measure. It’d be silly to apply the interactive proof when Lean is trying out various measures. But if the user has configured a different automatic tactic, then guessing the measure is sensible.Also, for interactive proofs, red squiggly lines should be placed in the interactive proof, while for automatic proofs, the recursive call that cannot be proven should be highlighted.
Solution: Command to change default
decrasing_tactic
tacticThis calls for different command/syntax for the two use cases. The proposal is to use
decreasing_by
for interactive proofs. This meansterminating_by
clause, as we cannot guess the order. (Maybe allow to omit if there is exactly one changing parameter).For temporarily using a different automatic tactic, introduce a command
that effectively works like
termination_by
now:default_decreasing_by
is used.Typical use might be
or
which should feel familiar to the users (similar to
set_option
).This seems to be a serious papercut: Users should have a nice environment to write these proofs interactively. So ideally, the following should hold
decreasing_by
proofs interactively, there should be recursive calls for each subgoal.Open questions
This is a breaking change: Some
terminating_by
need to be moved, somedecreasing_by
may becomedefault_decreasing_by … in
. Is this worth it? Should we try to find a backward compat way of achieving the goals (at the expense of a possibly worse UX)?Is there a good way to name each recursive call, either automatically or manually, so that
case <name> =>
can be used? Such names will also come in handy when deriving an induction principle from a recursive definition.We could say that whenever the recursive call is let-bound, we use the name as case name:
Too ad-hoc?
Community Feedback
Initial discussion on Zulip (but not much activity).
Impact
Add 👍 to issues you consider important. If others benefit from the changes in this proposal being added, please ask them to add 👍 to it.
The text was updated successfully, but these errors were encountered: