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

add lemma length_app_comm #75

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

NicholasBHubbard
Copy link
Contributor

@NicholasBHubbard NicholasBHubbard commented Jan 15, 2025

This PR adds to List.v a lemma: Lemma length_app_comm : forall l l' : list A, length (l++l') = length (l'++l).

I'd like to preface by saying that I am new to Coq so may be ignorant of many things. However, I was in need of such a lemma in one of my proofs and was surprised that an obvious fact such as this did not already exist in the stdlib. I hope that length_app_comm is a good name for this. I read over coq/coq#18564 where the naming convention of these lemmas was discussed, and I believe that I followed it correctly.

One thing I am unsure about is if I was correct to add the new lemma under the Global Hint Rewrite here:

Global Hint Rewrite
  rev_involutive (* rev (rev l) = l *)
  rev_unit (* rev (l ++ a :: nil) = a :: rev l *)
  map_nth (* nth n (map f l) (f d) = f (nth n l d) *)
  length_map (* length (map f l) = length l *)
  length_seq (* length (seq start len) = len *)
  length_app (* length (l ++ l') = length l + length l' *)
  length_app_comm (* length (l ++ l') = length (l' ++ l) *)
  length_rev (* length (rev l) = length l *)
  app_nil_r (* l ++ nil = l *)
  : list.

Was this the correct thing to do?

Please let me know if you believe length_app_comm would be a good addition to List.v, and if anything else needs to be done. Thanks!

  • Added changelog.

@NicholasBHubbard NicholasBHubbard marked this pull request as ready for review January 15, 2025 22:36
@NicholasBHubbard
Copy link
Contributor Author

Not sure why the CI fails but will look into it.

@Zimmi48
Copy link
Member

Zimmi48 commented Jan 16, 2025

Coq-Elpi error is:

 File "etc/version_parser.ml", line 4, characters 14-43:
4 |   let a,b,c = Elpi.API.Utils.version_parser ~what:"elpi" v in
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound value Elpi.API.Utils.version_parser

Very unclear for Rewriter (idem for Fiat-crypto-legacy).

Equations-test error looks like a GitHub Action issue (unclear what happened from the log). Idem for Metacoq-template-coq.

@proux01 Any idea what is going on here? Should we just relaunch the failed jobs?

@proux01
Copy link
Contributor

proux01 commented Jan 16, 2025

Elpi is known to be currently broken. Fiat-crypto and rewriter might be actual overlays needed, not sure for the others.

@proux01
Copy link
Contributor

proux01 commented Jan 17, 2025

@NicholasBHubbard CI in other PR confirms that coq-elpi is unrelated but you need to investigate more the other CI failures.

@Zimmi48
Copy link
Member

Zimmi48 commented Jan 17, 2025

Something is a bit strange though: why did all these jobs fail after 45-47 minutes?

image

I suspect something was wrong with the GitHub Action runners (especially given that the logs do not give any clue). I will restart these jobs.

@Zimmi48
Copy link
Member

Zimmi48 commented Jan 17, 2025

The failures still happen and this time, I identified the cause of at least one of them:

       > make[2]: *** [Makefile:818: STLC.vo] Killed

Now, my guess is that this comes from having adding this lemma to a rewrite hint database. Probably this creates an infinite loop and that's also the reason why these jobs take such a long time to fail.

@NicholasBHubbard
Copy link
Contributor Author

NicholasBHubbard commented Jan 17, 2025

Looks like removing the lemma from the rewrite hint database fixed it

@proux01 proux01 force-pushed the add-lemma-length_app_comm branch from d1933c9 to a71c168 Compare January 18, 2025 13:38
@proux01
Copy link
Contributor

proux01 commented Jan 18, 2025

Rebased to relaunch CI now that it's fixed, just to be on the safe side.

@proux01 proux01 merged commit 22f13b5 into coq:master Jan 18, 2025
116 checks passed
@proux01
Copy link
Contributor

proux01 commented Jan 18, 2025

Thanks to both of you for the contrib and the debuging

@proux01 proux01 added this to the 9.0 milestone Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants