agda / agda-stdlib

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

Add proofs to Data.List.Properties #2355

Closed mildsunrise closed 2 months ago

mildsunrise commented 2 months ago

Hi! This is my first PR, apologies if I'm missing something.

This adds a series of proofs to Data.List.Properties that I consider general enough to be in the stdlib, and in most cases already exist for other list operations. This proves, in order:

It also fixes a module that was defining a parameter that was not used in the proofs. I've tried to respect conventions of that file where possible, please tell me if there's anything that could be improved :)

jamesmckinna commented 2 months ago

Meta-comment: Should we fix mapMaybe first? Cf. https://github.com/agda/agda/issues/5833 and links there to @omelkonian 's implementation?

MatthewDaggitt commented 2 months ago

What would "fixing" it involve?

jamesmckinna commented 2 months ago

What would "fixing" it involve?

As in Orestis' prelude, write catMaybe before mapMaybe as the composition of catMaybe and map... which avoids the awkward with construction... see #2361

And apologies to @mildsunrise for hijacking a very nice first PR with a hobby-horse of mine!

mildsunrise commented 2 months ago

@MatthewDaggitt thanks for the thorough review! I've responded to your points and will act on them soon-ish, probably today

@jamesmckinna is it okay if I add the proposed fix to this PR, or would you rather keeping it in a separate PR, merging that one, and then rebasing this one on top?

MatthewDaggitt commented 2 months ago

As in Orestis' prelude, write catMaybe before mapMaybe as the composition of catMaybe and map... which avoids the awkward with construction...

Is this is a backwards compatible change with respect to reduction behaviour? We shouldn't do it now, if it's not.

jamesmckinna commented 2 months ago

I'll put my hobby-horse back in the stable... @MatthewDaggitt 's comment suggests we exercise caution. I'll investigate separately, and then take the knock-on hit from your additions downstream @mildsunrise so no action required on your part from me!

mildsunrise commented 2 months ago

to my understanding it should have equivalent reduction behavior, but yes, it's good to be cautious. okay then, I'll fix the 2 remaining issues

mildsunrise commented 2 months ago

all done. let me know if you need anything else on my part, such as squashing the fixup commits into the original commits

mildsunrise commented 2 months ago

I've added a couple more properties, mapMaybe-++ and unzipWith-++ since their proof was similar enough to the existing proofs. If it feels like too much, I can leave them out of this PR

mildsunrise commented 2 months ago

(I'll be a bit busy these days, but I'd rather not block other PRs with this one, so please ping me if action is needed on my part)

jamesmckinna commented 2 months ago

@MatthewDaggitt can you re-review (re: the cong lemmas?) and/or merge now, please? I think that this one is ready now ;-)

gallais commented 2 months ago

I think we just need the d \== e assumption to be removed and then we're good to merge.

MatthewDaggitt commented 2 months ago

Yup once removed, will merge :+1:

mildsunrise commented 2 months ago

I see that #2361 was merged before this PR. given that catMaybes is no longer defined in terms of mapMaybe, it becomes necessary to prove properties about catMaybes directly. And once this is done, the properties of mapMaybe can be simplified to use those new proofs.

2361 already did that with length-mapMaybe (added length-catMaybes). I've went ahead and done the same with the other three mapMaybe properties.