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(logic/basic): add eq_iff_true_of_subsingleton #3308

Closed
wants to merge 9 commits into from

Conversation

gebner
Copy link
Member

@gebner gebner commented Jul 7, 2020

I'm surprised we didn't have this already.

example (x y : unit) : x = y := by simp

I'm not sure if this breaks anything.

@ChrisHughes24
Copy link
Member

bors merge

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 7, 2020
bors bot pushed a commit that referenced this pull request Jul 7, 2020
I'm surprised we didn't have this already.
```lean
example (x y : unit) : x = y := by simp
```
@gebner
Copy link
Member Author

gebner commented Jul 7, 2020

bors r-

@bors
Copy link

bors bot commented Jul 7, 2020

Canceled.

@gebner
Copy link
Member Author

gebner commented Jul 7, 2020

Ah this broke a bit more than expected. Apparently fin_cases calls norm_num on the new hypothesis, which in turn calls simp. If the new hypothesis is (i : fin 1) = 0, then simp will now rewrite it to true, and the subst call in fin_cases no longer works.

I weakened the norm_num a bit so that it is only run on the rhs, but this broke interval_cases. Tomorrow I'll try moving the norm_num call entirely to interval_cases.

@gebner gebner removed the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 8, 2020
@@ -46,7 +46,8 @@ meta def fin_cases_at_aux : Π (with_list : list expr) (e : expr), tactic unit
(to_rhs >> conv.interactive.change (to_pexpr h))
-- Otherwise, call `norm_num`. We let `norm_num` unfold `max` and `min`
-- because it's helpful for the `interval_cases` tactic.
| _ := try $ tactic.interactive.norm_num
Copy link
Member

Choose a reason for hiding this comment

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

What if you made this norm_num1 instead of norm_num to avoid calling simp? I'm not sure if it's powerful enough for whatever norm_num is intended to do here, but maybe it is, and that would be an easier fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this as well, but interval_cases seems to heavily rely on the simplifier. Otherwise you get goals like this:

w₂: ⊥ < 2
⊢ ⊥ = 0 ∨ ⊥ = 1
w₂: ⊥ + 1 < 2
⊢ ⊥ + 1 = 0 ∨ ⊥ + 1 = 1

Just to recap the issues:

  • fin_cases should use subst on hypotheses like i = 0
  • the new simp lemma simplifies those to true
  • however in general we want the full simplifier because of interval_cases

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I think I've fixed this. The issue was that I've been using tactic.interactive.norm_num in the conv monad, which (in hindsight) obviously doesn't work. Then I tried conv.interactive.norm_num, which also failed because it used the simplifier from the tactic monad. After fixing conv.interactive.norm_num, the tests pass. I'm keeping my fingers crossed.

@gebner gebner added the awaiting-review The author would like community review of the PR label Jul 8, 2020
@gebner
Copy link
Member Author

gebner commented Jul 8, 2020

It finally builds!

test/tidy.lean Outdated
intro1,
induction x,
refl
simp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this test be testing something about tidy in some way? @semorrison

Copy link
Collaborator

Choose a reason for hiding this comment

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

It had been a test about chain, checking that it failed when it was meant to fail. The test has become a bad one, because now the underlying tactic is succeeding (yay!). I propose we just delete this test entirely.

To quote @semorrison:
> It had been a test about `chain`, checking that it failed when it was
> meant to fail. The test has become a bad one, because now the underlying
> tactic is succeeding (yay!). I propose we just delete this test
> entirely.
@jcommelin
Copy link
Member

bors merge

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Jul 9, 2020
@bryangingechen
Copy link
Collaborator

Wait, did you mean to delete the entire test file?

bors r-

@bryangingechen
Copy link
Collaborator

I think @semorrison was just suggesting to delete tidy_test_0 in test/tidy.lean

@bryangingechen bryangingechen removed the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 9, 2020
@jcommelin
Copy link
Member

Ooops! I should learn to be more careful!

@bryangingechen
Copy link
Collaborator

LGTM!
bors r+

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 9, 2020
bors bot pushed a commit that referenced this pull request Jul 9, 2020
I'm surprised we didn't have this already.
```lean
example (x y : unit) : x = y := by simp
```


Co-authored-by: Johan Commelin <johan@commelin.net>
@bors
Copy link

bors bot commented Jul 9, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(logic/basic): add eq_iff_true_of_subsingleton [Merged by Bors] - feat(logic/basic): add eq_iff_true_of_subsingleton Jul 9, 2020
@bors bors bot closed this Jul 9, 2020
@bors bors bot deleted the ss_elim_simp branch July 9, 2020 07:36
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…ity#3308)

I'm surprised we didn't have this already.
```lean
example (x y : unit) : x = y := by simp
```


Co-authored-by: Johan Commelin <johan@commelin.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants