-
Notifications
You must be signed in to change notification settings - Fork 17
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
Introduce csimp
tactic
#311
Conversation
Signed-off-by: Craig Disselkoen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall idea seems fine to me. Personally I like having a single tactic to try (csimp
) instead of having to think about exactly which lemmas to include with simp only
.
No comments on your questions about improving the tactic syntax since I don't have experience there 🙂
I prefer My general rule of thumb for using tactics is this: (a) the tactic abstracts a specific pattern that repeats in many proofs, or (b) it's a general-purpose decision procedure (like I've only written tactics of kind (a) so far :) This one doesn't fit either (a) or (b), so my vote is to stick to the standard solution ( |
If you type |
Am I correct that, absent proof stability concerns, all of us would prefer to use If I'm correct, then I claim that @emina says she likes seeing exactly which lemmas were needed for a proof, but to me, I don't care about which basic lemmas (included in this I also claim that
For the performance argument, let's actually do some profiling before we reject this on performance grounds. We've made other decisions that favor proof readability over performance, like our heavy use of |
cases xs | ||
cases xs <;> csimp at * | ||
case nil => | ||
simp only [Option.bind_eq_bind, List.foldlM, pure, Option.some.injEq, Option.bind_some_fun] at * | ||
subst h₃; exact h₂ | ||
case cons hd tl => | ||
simp only [Option.bind_eq_bind, List.foldlM, Option.bind_eq_some] at * | ||
cases h₄ : f x₂ hd <;> simp only [h₄, false_and, exists_false, Option.some.injEq, exists_eq_left'] at h₃ | ||
cases h₄ : f x₂ hd <;> rw [h₄] at h₃ <;> csimp at h₃ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example where I think csimp
unquestionably improves readability. It also calls out much more clearly the one non-simp
lemma (h4) which was previously used in these three long simp only
invocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think that the improvement in readability is minimal here.
case true => | ||
split <;> simp only [Except.bind_ok, Except.bind_err] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally like the new proofs, but I prefer the prior version here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, reverted this bit to use the explicit h_1
and h_2
tags and renames
I prefer to use standard tooling whenever possible. I disagree that it's easier to use To be clear, this is nothing specific to |
In general, I like how the new proofs look. I agree with Craig that performance probably isn't an issue (as evidence, a lot of these used I don't think we should underestimate the value in writing idiomatic Lean. I find the argument that people will see and emulate our use of Additionally, even for new Lean users, Google (and Loogle, LeanCopilot, or whatever) will probably give better results for issues with |
^^^ Andrew said it much better than I did. 100% this. |
This is true of all standard tactics as well (and a frustration for me, but that's somewhat tangential. I have no way to discover new useful tactics other than see them used somewhere else.) I don't think it's harder to discover
I think it depends entirely on whether we're optimizing for new contributions by people familiar with Lean but not Cedar (as @emina and @andrewmwells-amazon seem to be arguing), or for new contributions by people familiar with Cedar but not Lean (which was the boat I was recently in, and longer ago, I think @khieta as well). For people in the latter boat, I think they will 100% see and emulate |
I still like |
Signed-off-by: Craig Disselkoen <[email protected]>
(I worry this sounds passive-aggressive, but that's not my intent. I'd like to hear of complaints about it if you have them.) Have you tried LeanCopilot? It's pretty great for this IMO. |
This is a good call. I have not tried it. |
I think it's fair to say we all know about And once you learn that, you don't need the custom tactic. It just adds cognitive overhead and process burden without saving much IMO. |
I think this is the core issue. I have trouble believing we'll have contributors who learn Lean just to contribute to Cedar (beyond minor changes). As evidence, think about how much effort went into #206 . So my vote is to optimize for people already familiar with Lean. |
I think we're disagreeing about where to draw the abstraction boundaries, and when the benefits of a new abstraction outweigh the cost. My take is above: either the tactic is a general decision procedure (like To give a concrete example, I have this tactic in the internal code that lets me work with the
|
My intent at the time was to apply this to particularly ugly cases within one file (so you could define the tactic within the file). I think even Emina will agree that There's a tradeoff here between being concise and being idiomatic. I'd like to be more concise in some really ugly cases, but still mostly be idiomatic. |
Couldn't agree more :) |
Yep, it's not the easiest thing on the eyes :D |
I would be curious to hear opinions from Lean experts. Do you want to post a question on Zulip, link to this PR and ask if people have recommendations? (Or I can do it). There's also a public Lean office hours every Wednesday where we can get opinions (Leo and a rotating group of Lean / Mathlib maintainers attend and answer questions). |
Asked in the Zulip at link |
Based on feedback in this thread and on the Zulip, I'll close this. We'll stick with the |
Issue #, if available:
Resolves #279
Description of changes:
Resolves #279 and also proposes a different (cleaner IMO) way to resolve the proof-stability problem with
simp
across Lean versions. Introduces a custom tacticcsimp
(Cedar-simp) that can be used likesimp
but internally uses a stable set of lemmas that will not change with the Lean version (unless we intentionally make changes).csimp
supportsat
syntax, e.g., all of the following are valid:csimp
csimp at h
csimp at *
but
csimp
does not support extending the lemmas inline, e.g.,csimp [h₂]
(instead, usecsimp ; simp only [h₂]
, or the other order)only because I couldn't get that to work. Specifically, although I was able to get
csimp
to take an optional list argument of the appropriate type, and even append those items to the lemmas such thatcsimp [h₂]
works, Lean's handling of the optional argument meant that it dropped the whole merged list of lemmas when trying to usecsimp
without the argument. Lean's documentation for macros doesn't explain details of how to use optional arguments to macros; it's very possible there's some simple syntax to accomplish this and I just don't know it. However, this PR demonstrates that even the current version ofcsimp
is still very useful and improves proofs (making them more stable and readable).