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(logic/{function}/basic): remove simp lemmas function.injective.eq_iff and imp_iff_right #6402

Closed
wants to merge 8 commits into from

Conversation

robertylewis
Copy link
Member

@robertylewis robertylewis commented Feb 24, 2021

  • imp_iff_right is a conditional simp lemma that matches a lot and should never successfully rewrite.
  • function.injective.eq_iff is a conditional simp lemma that matches a lot and is rarely used. Since you almost always need to give the proof hf : function.injective f as an argument to simp, you can replace it with hf.eq_iff.

@robertylewis robertylewis added the awaiting-review The author would like community review of the PR label Feb 24, 2021
@robertylewis robertylewis added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Feb 24, 2021
@robertylewis robertylewis changed the title fix(logic/basic): remove bad simp lemma fix(logic/{function}/basic): remove simp lemmas function.injective.eq_iff and imp_iff_right Feb 24, 2021
@robertylewis robertylewis added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Feb 24, 2021
@eric-wieser
Copy link
Member

eric-wieser commented Feb 24, 2021

Are any lemmas with statement injective f currently tagged simp? I guess those could be resolved by changing the relevant fs to embeddings, although that doesn't work for already-bundled fs

@robertylewis
Copy link
Member Author

[list.reverse_injective, list.length_injective, list.cons_injective] seem to be the only ones.

But I think I'm missing something, what's being resolved with this change? It's fine to leave those as simp lemmas.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 25, 2021

I ask only because removing [simp] from injective.eq_iff means that list.reverse_injective.eq_iff, list.length_injective.eq_iff, and list.cons_injective.eq_iff will no longer be applied by simp. I think we have aliases for those statements tagged simp anyway though, so it shouldn't matter.

@gebner
Copy link
Member

gebner commented Feb 25, 2021

Right, the aliases are reverse_inj and cons_inj. I'm not sure how useful length_inj would be.

bors r+

@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 Feb 25, 2021
bors bot pushed a commit that referenced this pull request Feb 25, 2021
…q_iff` and `imp_iff_right` (#6402)

* `imp_iff_right` is a conditional simp lemma that matches a lot and should never successfully rewrite.
* `function.injective.eq_iff` is a conditional simp lemma that matches a lot and is rarely used. Since you almost always need to give the proof `hf : function.injective f` as an argument to `simp`, you can replace it with `hf.eq_iff`.



Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
@bors
Copy link

bors bot commented Feb 25, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Feb 25, 2021
…q_iff` and `imp_iff_right` (#6402)

* `imp_iff_right` is a conditional simp lemma that matches a lot and should never successfully rewrite.
* `function.injective.eq_iff` is a conditional simp lemma that matches a lot and is rarely used. Since you almost always need to give the proof `hf : function.injective f` as an argument to `simp`, you can replace it with `hf.eq_iff`.



Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
@bors
Copy link

bors bot commented Feb 25, 2021

Build failed (retrying...):

@sgouezel
Copy link
Collaborator

bors r-
Two builds have failed with this one.

@bors
Copy link

bors bot commented Feb 25, 2021

Canceled.

@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) labels Feb 25, 2021
@robertylewis
Copy link
Member Author

I just learned that simp calls refl to try to discharge hypotheses to conditional simp lemmas, even with the default discharger failed:

constant R : ℕ → ℕ → Prop 

@[refl] -- removing this cases the example to fail
lemma R_refl (a : ℕ) : R a a := sorry

@[simp]
lemma r_simp (a : ℕ) (ha : R a a) : id a = 0 := sorry 

example : id 3 = 0 := by simp

So my claim

  • imp_iff_right is a conditional simp lemma that matches a lot and should never successfully rewrite.

is false, it can successfully rewrite. I still think it should go.

@robertylewis robertylewis removed the awaiting-author A reviewer has asked the author a question or requested changes label Feb 25, 2021
@robertylewis robertylewis added the awaiting-review The author would like community review of the PR label Feb 25, 2021
@bryangingechen
Copy link
Collaborator

bors r+

@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 Feb 26, 2021
bors bot pushed a commit that referenced this pull request Feb 26, 2021
…q_iff` and `imp_iff_right` (#6402)

* `imp_iff_right` is a conditional simp lemma that matches a lot and should never successfully rewrite.
* `function.injective.eq_iff` is a conditional simp lemma that matches a lot and is rarely used. Since you almost always need to give the proof `hf : function.injective f` as an argument to `simp`, you can replace it with `hf.eq_iff`.



Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
@bors
Copy link

bors bot commented Feb 26, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(logic/{function}/basic): remove simp lemmas function.injective.eq_iff and imp_iff_right [Merged by Bors] - fix(logic/{function}/basic): remove simp lemmas function.injective.eq_iff and imp_iff_right Feb 26, 2021
@bors bors bot closed this Feb 26, 2021
@bors bors bot deleted the bad-simps branch February 26, 2021 07:13
@gebner
Copy link
Member

gebner commented Feb 26, 2021

I just learned that simp calls refl to try to discharge hypotheses to conditional simp lemmas, even with the default discharger failed:

If the discharger tactic fails, simp calls simp to discharge hypotheses in conditional simp lemmas.

b-mehta pushed a commit that referenced this pull request Apr 2, 2021
…q_iff` and `imp_iff_right` (#6402)

* `imp_iff_right` is a conditional simp lemma that matches a lot and should never successfully rewrite.
* `function.injective.eq_iff` is a conditional simp lemma that matches a lot and is rarely used. Since you almost always need to give the proof `hf : function.injective f` as an argument to `simp`, you can replace it with `hf.eq_iff`.



Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
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

5 participants