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

[Merged by Bors] - feat: recover as a tactic combinator #336

Closed

Conversation

siddhartha-gadgil
Copy link
Collaborator

Following a suggestion of @digama0 recover is implemented as wrapping tactics so that if goals are closed incorrectly they are reopened. Below are examples (test suggested by @gebner). Some code is from Aesop.

/-- problematic tactic for testing recovery -/
elab "this" "is" "a" "problem" : tactic =>
  Lean.Elab.Tactic.setGoals []

/- The main test-/
example : 1 = 1 := by
  recover this is a problem
  rfl

/- Tests that recover does no harm -/
example : 3 < 4 := by
    recover decide

example : 1 = 1 := by
    recover skip ; rfl

example : 2 = 2 := by
    recover skip
    rfl

Copy link
Member

@gebner gebner left a comment

Choose a reason for hiding this comment

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

The actual tactic implementation looks good.

Mathlib/Tactic/Recover.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Recover.lean Outdated Show resolved Hide resolved
@gebner gebner added the awaiting-author A reviewer has asked the author a question or requested changes label Jul 20, 2022
instantiateMVarDeclMVars mvarId

/-- Returns unassigned metavariable dependencies (from Aesop) -/
partial def getUnassignedGoalMVarDependencies (mvarId : MVarId) :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think about it: does recover actually want to recurse into the found metavariables? If we make the 'dropped' metavariables goals, any mvars occurring in them are not dropped any more, so they don't need to also become goals. cc @gebner

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A situation I can imagine: a new metavariable is created, a dropped mvar assigned to it and removed from goals but the new mvar is not added to goals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, your situation looks like this:

  • initial goal: ?g[?m1] (this is my notation for '?g contains ?m')
  • tactic introduces ?m2 and assigns ?m1 := ?m2
  • tactics returns goals: []

Now if we just collect unassigned mvars in ?g, we correctly find ?m2 (and not ?m1 since it gets substituted away by instantiateMVars). So we set goals:

  • goals after recover: [?m2]

If there's another mvar ?m3 with ?m2[?m3], this is fine because ?m3 is already reachable from the tactic state after recover. So we don't need to recursively get mvars from ?m2. However, maybe we want to do it anyway because it's nicer to have an mvar in the goal list than to not have it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I misunderstood your point. Is what you are saying that we should collect unassigned mvars in the original goals list, but no need to do this recursively. That sounds reasonable. I will make the change if you confirm that is what you mean and feel it will make things better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes exactly, but I'm not sure about this. Let's wait and see what Gabriel thinks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Member

Choose a reason for hiding this comment

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

It's not actually necessary to recurse into the initial goals at all, that was just an optimization. It suffices to just hold on to the list of goals itself and do all recursing through assignments at the end of the block.

@siddhartha-gadgil siddhartha-gadgil changed the title recover as a tactic combinator feat: recover as a tactic combinator Jul 20, 2022
@semorrison semorrison added the awaiting-review The author would like community review of the PR label Jul 20, 2022
@siddhartha-gadgil siddhartha-gadgil removed the awaiting-author A reviewer has asked the author a question or requested changes label Jul 25, 2022
@gebner
Copy link
Member

gebner commented Aug 24, 2022

Thanks!

bors r+

bors bot pushed a commit that referenced this pull request Aug 24, 2022
Following a suggestion of @digama0 `recover` is implemented as wrapping tactics so that if goals are closed incorrectly they are reopened. Below are examples (test suggested by @gebner). Some code is from Aesop.

```lean
/-- problematic tactic for testing recovery -/
elab "this" "is" "a" "problem" : tactic =>
  Lean.Elab.Tactic.setGoals []

/- The main test-/
example : 1 = 1 := by
  recover this is a problem
  rfl

/- Tests that recover does no harm -/
example : 3 < 4 := by
    recover decide

example : 1 = 1 := by
    recover skip ; rfl

example : 2 = 2 := by
    recover skip
    rfl
```

Co-authored-by: Jannis Limperg <jannis@limperg.de>
@bors
Copy link

bors bot commented Aug 24, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: recover as a tactic combinator [Merged by Bors] - feat: recover as a tactic combinator Aug 24, 2022
@bors bors bot closed this Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants