agda / agda-stdlib

The Agda standard library
https://wiki.portal.chalmers.se/agda/Libraries/StandardLibrary
Other
575 stars 237 forks source link

More list properties about `catMaybes/mapMaybe` #2389

Closed omelkonian closed 3 months ago

omelkonian commented 4 months ago
omelkonian commented 4 months ago

@JacquesCarette To sum up, let's agree that there are both pros and cons to using old-school eliminators in contrast to immediately using (dependent) pattern matching:

Given that there is only one lemma about maybe :disappointed: (maybe-map) and that the inductive hypotheses in the proofs above can easily be shared using with IH ← ..., I have not followed your suggestion to use eliminators and stuck with matching on the maybes and so forth.

Feel free to push another commit on top of my branch to convert to eliminator-style proofs though...

JacquesCarette commented 4 months ago

@omelkonian I'll comment on various of the details a bit later, but long story short: you make a very convincing case. I was overly optimistic.

JacquesCarette commented 4 months ago

As I agree with @jamesmckinna 's comments, I won't approve this PR (yet), even though I'm not happy with the parts that I had commented on myself.

jamesmckinna commented 4 months ago

Thanks for the continued updates, but I won't re-review until you signal it's time to do so though!

omelkonian commented 4 months ago

@jamesmckinna ready for review

omelkonian commented 4 months ago

@JacquesCarette @jamesmckinna Ready to merge?

omelkonian commented 4 months ago

I don't think the new with-based code (and the accompanying code motion) is an improvement over the existing pattern-matching based definitions, but you have two approvals from others, so I'm not going to stand in the way.

Isn't the reuse of the inductive hypothesis (ih) clearly an improvement?

[sorry, hit edit instead of quote reply]

jamesmckinna commented 4 months ago

I don't think the new with-based code (and the accompanying code motion) is an improvement over the existing pattern-matching based definitions, but you have two approvals from others, so I'm not going to stand in the way.

Isn't the reuse of the inductive hypothesis (ih) clearly an improvement?

In this case, I (personally) actually think no, it's not, at least not enough to justify the rewriting of existing code. Funnily enough, I think that both you and @JacquesCarette have convinced me, in different ways, and from different starting points, to view such code style with... scepticism for the time being. And one reason I dismissed my review was to avoid going back over this ground...so: I stand aside, as previously.

andreasabel commented 4 months ago

Mmh, I tend to agree with James @jamesmckinna that with should be avoided where possible. The reason is that it internally generates a helper function which Agda might sometimes leak in printing. Also, it bears some hazards in proving things about the parent function as the name of the auxiliary function is not available to the user. So, I think the general thrust should be to get rid of with where possible rather than introducing more withs.

JacquesCarette commented 4 months ago

While I very much agree with @andreasabel 's general stance, especially for any definition that computes, I've gotten more relaxed about "pure proofs".

Really what's we're sorely missing (and this is, I believe, what @omelkonian was using it for) is a proper pattern-matching let that doesn't unfold eagerly. This is a real wart in the current Agda language (along with the same issue wrt module parameters).

omelkonian commented 4 months ago

While I very much agree with @andreasabel 's general stance, especially for any definition that computes, I've gotten more relaxed about "pure proofs".

See my comment above on "meta-properties", which nullifies the distinction between programs and "pure proofs".

Really what's we're sorely missing (and this is, I believe, what @omelkonian was using it for) is a proper pattern-matching let that doesn't unfold eagerly. This is a real wart in the current Agda language (along with the same issue wrt module parameters).

Isn't the upcoming using syntax that I mentioned above already what you wish for (although I'm not sure about its unfolding behaviour, but the opaque feature should cover that).

JacquesCarette commented 3 months ago

Yes, meta-properties, being proofs-about-proofs, do blur things. I've hated doing such proofs so much (in MLTT) that I block them out; when done 'properly', a la category theory where one adds higher-level cells, then these become sane again. subst hell is really the worst hell of them all.

I don't understand (yet!) enough of the details of using to know if it really going to be 'the' feature needed. I suspect that as long as there isn't some kind of let-like construct in the internal AST, it won't be, it'll merely be pleasant syntactic sugar.

jamesmckinna commented 3 months ago

Shall we close the discussion and merge?

andreasabel commented 3 months ago

@omelkonian: Thanks, Orestis, for your patience and willingness to revert changes!
(with is gone again)

jamesmckinna commented 3 months ago

@omelkonian: Thanks, Orestis, for your patience and willingness to revert changes! (with is gone again)

Seconded!