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

RFC: dsimp should not simplify proofs #6960

Open
JovanGerb opened this issue Feb 5, 2025 · 3 comments · May be fixed by #6973
Open

RFC: dsimp should not simplify proofs #6960

JovanGerb opened this issue Feb 5, 2025 · 3 comments · May be fixed by #6973
Labels
RFC Request for comments

Comments

@JovanGerb
Copy link
Contributor

Proposal

unlike simp, dsimp traverses subexpressions that are proofs, and simplifies them. This is very bad for performance, particularly in Category Theory, where aesop_cat uses simp, which uses dsimp, and this is run on the result of other aesop_cat tactic calls.

I propose that when encountering a proof, dsimp does one of two things:

  • leave the proofs as-is, without simplification
  • simplify the type of the proof and wrap the proof in a cast so that it gets the new type.

Minimal working example

@[simp]
theorem foo (i : Nonempty α) : Nonempty.intro (Classical.choice i) = i := rfl

set_option pp.proofs true

example : Classical.choice (Nonempty.intro (Classical.choice instNonemptyFloat)) = 0 := by
  dsimp

Expected behaviour: dsimp fails
Actual behaviour: dsimp applies the simp lemma

Community Feedback

I first suggested this on Zulip.

Impact

Add 👍 to issues you consider important. If others benefit from the changes in this proposal being added, please ask them to add 👍 to it.

@JovanGerb JovanGerb added the RFC Request for comments label Feb 5, 2025
@nomeata
Copy link
Collaborator

nomeata commented Feb 5, 2025

Oh, dsimp rewrites proofs? I didn't know that! In calcify, I wrote my own little simplifier engine because simp wouldn't rewrite proofs; I maybe could have used dsimp. Thanks for the hint, will try one day.

(Not really relevant here, of course.)

@fpvandoorn
Copy link
Contributor

I'm quite sure that some of the performance issues observed by @sven-manthe are because of this.

@kim-em
Copy link
Collaborator

kim-em commented Feb 6, 2025

If we change this, there should be a flag to re-enable the current behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comments
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants