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

decreasing_by goals leak wf implementation details #4928

Closed
nomeata opened this issue Aug 6, 2024 · 1 comment · Fixed by #5016
Closed

decreasing_by goals leak wf implementation details #4928

nomeata opened this issue Aug 6, 2024 · 1 comment · Fixed by #5016
Labels
bug Something isn't working P-medium We may work on this issue if we find the time

Comments

@nomeata
Copy link
Contributor

nomeata commented Aug 6, 2024

The goals shown by decreasing_by can contain scary implementation details, especially the PSigma/PSum due to non-unary functions and mutual recursion.

The simp_wf tactic is typically used to clean that up, but, as it runs unrestricted simp, may do too much.

It seems desirable that after decreasing_by the goal shown to the user is exactly what they expect based on what they wrote – no implementation details visible, but also no additional proof steps taken.

There is a similar issue in the FunInd code, where Lean.Tactic.FunInd.cleanPackedArgs is used to clean this up; this can be inspiration.

Versions

v4.11.0-rc1

Additional Information

Zulip discussion at https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60simp_wf.60.20in.20.60decreasing_by.60

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 Aug 6, 2024
@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Aug 9, 2024
@nomeata
Copy link
Contributor Author

nomeata commented Aug 13, 2024

So after

def g (x : Nat) (y : Nat) : Nat := g (x - 1) y
decreasing_by

we have

x y : Nat
⊢ (invImage (fun x => PSigma.casesOn x fun x y => x) instWellFoundedRelationOfSizeOf).1 ⟨x - 1, y⟩ ⟨x, y⟩

If we now run

  try simp (config := { unfoldPartialApp := true, zetaDelta := true }) only [invImage, InvImage, Prod.lex, sizeOfWFRel, measure, Nat.lt_wfRel, WellFoundedRelation.rel]

which is simp_wf with an only added we get

⊢ (sizeOf (x - 1)).lt (sizeOf x)

which isn’t great. We’d have to throw in

simp only [sizeOf_nat, Nat.lt_eq]

I can thow them into the simp arguments, but this is of course optimizing for the special case of Nat arguments, and if the termination is via sizeOf or Nat then the goal would be not as clean. Probably ok.

So one fix would be to change simp_wf to run simp only, and run it always, and (for backwards compat) add a simple simp call to the beginning of decreasing_tactic. Trying that.

github-merge-queue bot pushed a commit that referenced this issue Aug 15, 2024
Previously, the tactic state shown at `decreasing_by` would leak lots of
details about the translation, and mention `invImage`, `PSigma` etc.
This is not nice.
  
So this introduces `clean_wf`, which is like `simp_wf` but using
`simp`'s `only` mode, and runs this unconditionally. This should clean
up the goal to a reasonable extent.
  
Previously `simp_wf` was an unrestricted `simp […]` call, but we
probably don’t want arbitrary simplification to happen at this point, so
this now became `simp only` call. For backwards compatibility,
`decreasing_with` begins with `try simp`. The `simp_wf` tactic
is still available to not break too much existing code; it’s docstring
suggests to no longer use it.

With `set_option cleanDecreasingByGoal false` one can disable the use of
`clean_wf`. I hope this is only needed for debugging and understanding.
  
Migration advise: If your `decreasing_by` proof begins with `simp_wf`,
either remove that (if the proof still goes through), or replace with
`simp`.
  
I am a bit anxious about running even `simp only` unconditionally here,
as it may do more than some user might want, e.g. because of options
like `zetaDelta := true`. We'll see if we need to reign in this tactic
some more.

I wonder if in corner cases the `simp_wf` tactic might be able to close
the goal, and if that is a problem. If so, we may have to promote simp’s
internal `mayCloseGoal` parameter to a simp configuration option and use
that here.
  
fixes #4928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P-medium We may work on this issue if we find the time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants