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] - fix(init/meta/converter/interactive): Do not ignore errors and tactic state changes in for and find #482

Closed
wants to merge 21 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Oct 12, 2020

An example of the type of code that used to work and now fails:

import algebra.ring

example {a b c : ℕ} : (a * b) * c = (a * b) * c :=  begin
  conv {
    for (_ * _) [1] {
      rw this_isnt_a_lemma,  -- no error message?
    },
  },
end

example {a b c : ℕ} : (a * b) * c = (a * b) * c :=  begin
  conv {
    find (_ * _) {
      rw this_isnt_a_lemma,  -- no error message?
    },  -- error: pattern not found (not true!)
  },
end

Without this PR, users like me get no information on why their tactic failed, which is a very frustrating experience.

The problem here is that ext_simplify_core ignores all failures in its pre and post arguments - failures are treated as "there was nothing to simplify here, move on to the next one". In find and for, we need to distinguish the cases of failing due to the pattern not matching and failuring due to an error in the user-provided tactic.

This PR adds three new tactic primitives, tactic.capture, tactic.unwrap, and tactic.resume, the first two of which are inspired by the python functions from the trio-outcome package with the same name. capture can be used to run the user-provided tactic and keep track of any errors that occured. unwrap is then used to re-throw the error once ext_simplify_core is done.

It turns out that not only does ext_simplify_core ignore all failures, but it resets the tactic state after it completes - which clears populated metavariables. To preserve the tactic state, we need resume.


A replacement for #476, since that PR does not have CI runs

@gebner
Copy link
Member

gebner commented Oct 12, 2020

Could you please add the other test case from #476 (comment) and copy the PR description from #476 (bors uses it in the commit message).

Otherwise LGTM.

bors d+

@bors
Copy link

bors bot commented Oct 12, 2020

✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@eric-wieser
Copy link
Member Author

Where should the passing test-cases go?

@gebner
Copy link
Member

gebner commented Oct 12, 2020

Oh, I didn't look where you put the test case. Typically these test cases (where the error message is important) go into tests/lean. You can then also put the passing test case in the same file.

Some explanation here: https://github.com/leanprover-community/lean/blob/master/doc/fixing_tests.md

@eric-wieser
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Oct 12, 2020
… state changes in `for` and `find` (#482)

An example of the type of code that used to work and now fails:
```lean
import algebra.ring

example {a b c : ℕ} : (a * b) * c = (a * b) * c :=  begin
  conv {
    for (_ * _) [1] {
      rw this_isnt_a_lemma,  -- no error message?
    },
  },
end

example {a b c : ℕ} : (a * b) * c = (a * b) * c :=  begin
  conv {
    find (_ * _) {
      rw this_isnt_a_lemma,  -- no error message?
    },  -- error: pattern not found (not true!)
  },
end
```
Without this PR, users like me get no information on why their tactic failed, which is a very frustrating experience.

The problem here is that `ext_simplify_core` ignores all failures in its `pre` and `post` arguments - failures are treated as "there was nothing to simplify here, move on to the next one". In `find` and `for`, we need to distinguish the cases of failing due to the pattern not matching and failuring due to an error in the user-provided tactic.

This PR adds three new tactic primitives, `tactic.capture`, `tactic.unwrap`, and `tactic.resume`, the first two of which are inspired by the python functions [from the `trio-outcome` package](https://outcome.readthedocs.io/en/latest/api.html) with the same name. `capture` can be used to run the user-provided tactic and keep track of any errors that occured. `unwrap` is then used to re-throw the error once `ext_simplify_core` is done.

It turns out that not only does `ext_simplify_core` ignore all failures, but it resets the tactic state after it completes - which clears populated metavariables. To preserve the tactic state, we need `resume`.

---

A replacement for #476, since that PR does not have CI runs
@eric-wieser
Copy link
Member Author

Thanks for the review, and for pointing me towards the better fix!

@bors
Copy link

bors bot commented Oct 13, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(init/meta/converter/interactive): Do not ignore errors and tactic state changes in for and find [Merged by Bors] - fix(init/meta/converter/interactive): Do not ignore errors and tactic state changes in for and find Oct 13, 2020
@bors bors bot closed this Oct 13, 2020
@bors bors bot deleted the eric-wieser/conv-tactic_state branch October 13, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants