Check guardedness of fixpoints also in erasable subterms#15434
Conversation
|
The job library:ci-fiat_crypto_legacy has failed in allow failure mode |
|
🔴 CI failures at commit 75e78c7 without any failure in the test-suite ✔️ Corresponding jobs for the base commit 4d78b1f succeeded ❔ Ask me to try to extract minimal test cases that can be added to the test-suite 🏃
|
1 similar comment
|
🔴 CI failures at commit 75e78c7 without any failure in the test-suite ✔️ Corresponding jobs for the base commit 4d78b1f succeeded ❔ Ask me to try to extract minimal test cases that can be added to the test-suite 🏃
|
75e78c7 to
e765aba
Compare
|
🔴 CI failures at commit e765aba without any failure in the test-suite ✔️ Corresponding jobs for the base commit 9dbb613 succeeded ❔ Ask me to try to extract minimal test cases that can be added to the test-suite 🏃
|
e765aba to
d05cbf4
Compare
| Fixpoint F (n : nat) (A : Type) {struct n} : nat | ||
| with G (n : nat) (A:Type@{_}) {struct n} : nat. | ||
| Proof. | ||
| 1: pose (G n A). |
There was a problem hiding this comment.
Why is this no longer accepted?
There was a problem hiding this comment.
I've noticed you've expanded a lot of these kinds of terms, that seems a bit strange. Were we doing something wrong before?
There was a problem hiding this comment.
d05cbf4 to
e28d141
Compare
|
🔴 CI failures at commit e28d141 without any failure in the test-suite ✔️ Corresponding jobs for the base commit 1b1206e succeeded ❔ Ask me to try to extract minimal test cases that can be added to the test-suite 🏃
|
e28d141 to
a731603
Compare
|
🔴 CI failure at commit a731603 without any failure in the test-suite ✔️ Corresponding job for the base commit 1b1206e succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
|
The failure in fiat-crypto is of the form: Fixpoint f n :=
match n with
| 0 => 0
| S n =>
(fix aux n :=
match n with
| 0 => f n
| S n => aux n
end) (S n)
end.which indeed is not strongly normalizing (unless we decide that unfolding a |
a731603 to
7746fec
Compare
7746fec to
6e3757f
Compare
|
🔴 CI failure at commit 6e3757f without any failure in the test-suite ✔️ Corresponding job for the base commit bd6cead succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
1 similar comment
|
🔴 CI failure at commit 6e3757f without any failure in the test-suite ✔️ Corresponding job for the base commit bd6cead succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
|
@herbelin can you get around defining The example currently guard checks because it reduces the definition to: Right? |
I did not try yet, but yes, one could certainly unroll the loop one step further (as you did in your redefinition of
I think so, yes, the notion of K-redex / erasable-subterm should be generalized to |
6e3757f to
8f0863f
Compare
|
@ppedrot: you mentioned a slow guard checking at |
|
Bench did not go the end. Before failing on installing the fiat-crypto overlay it gave: Should we investigate more what happens in e.g. fiat-core or coq-performance-tests-lite? (And if yes, what are the best tools to investigate?) |
The bench doesn't use overlays, you have to upstream it to bench it. |
…subterms, Checking guard in previously ignored erasable or inert subterms of fixpoints. We do that by splitting guard errors into categories: - fatal recursive calls to non guarded arguments - recursive calls on descendants of variables bound in subterms of a redex and thus liable to be instantiated by reduction of the redex + not applied enough recursive calls which also may become enough applied after reduction. We also maintain a stack of redexes so that when a variable is bound in the subterm of a redex, we directky know which redex needs to be reduced to trigger the instantiation of the variable. In situations of the form "C(if c then x else y)" where x is a recursion variable, y needs the unfolding of a redex C[], we use a rich notion of subterm which remembers both that we need to reduce the redex to check a possible recursive call on y, but that we need to check that we don't also make a recursive call on x even w/o reducing the redex. So, the union of specifications of subterms of a match/if is now a set. For the beta and zeta-redex cases, we choose to check the argument for possibly fatal non guarded calls and reduce anyway. Another strategy could have been to reduce only when a subterm of the redex needs a reduction (but the stack call is then a bit complicated to manage). Note: We could have been more tolerant in subterms with recursive calls on internal uninstantiatable variables and on not applied enough recursive calls which cannot be applied more (e.g. "Fixpoint foo (n : nat) := forall x : foo = foo, True"). To accept these calls, "check_erasable_inert_subterm_rec_call" should be renamed into "check_inert_subterm_rec_call" to emphasize that it becomes more general and checks in types (Ind, Prod, types in Fix/Cofix/Lambda/Array) should call it in place of "check_rec_call".
Indeed, if the "match" needs to be reduced but its argument is not a constructor, it can eventually be reduced by any arbitrary term and there is no hope to be guarded at all, even by reducing outer redexes.
169ac11 to
17ade41
Compare
|
@herbelin I took the liberty to rebase the PR myself. |
|
@coqbot bench |
|
🏁 Bench results: 🐢 Top 25 slow downs🐇 Top 25 speed ups |
|
🔴 CI failure at commit 17ade41 without any failure in the test-suite ✔️ Corresponding job for the base commit be7781b succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
|
@coqbot ci minimize ci-fiat_crypto |
|
I am now running minimization at commit 17ade41 on requested target ci-fiat_crypto. I'll come back to you with the results once it's done. |
|
Minimized File /github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/src/Util/ListUtil.v (from ci-fiat_crypto) (full log on GitHub Actions) We are collecting data on the user experience of the Coq Bug Minimizer. Minimized Coq File (consider adding this file to the test-suite)(* -*- mode: coq; coq-prog-args: ("-emacs" "-q" "-w" "+implicit-core-hint-db,+implicits-in-term,+non-reversible-notation,+deprecated-intros-until-0,+deprecated-focus,+unused-intro-pattern,+variable-collision,+omega-is-deprecated,+deprecated-instantiate-syntax,+non-recursive" "-w" "-notation-overridden,-undeclared-scope,-deprecated-hint-rewrite-without-locality,-deprecated-hint-constr,-fragile-hint-constr,-native-compiler-disabled,-ambiguous-paths" "-w" "-deprecated-native-compiler-option" "-R" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/src" "Crypto" "-Q" "/github/workspace/cwd" "Top" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/deps/coqutil/src/coqutil" "coqutil" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/src/Rupicola" "Rupicola" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/bedrock2/src/bedrock2" "bedrock2" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/bedrock2/src/bedrock2Examples" "bedrock2Examples" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/compiler/src/compiler" "compiler" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/compiler/src/compilerExamples" "compilerExamples" "-Q" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/deps/riscv-coq/src/riscv" "riscv" "-Q" "/github/workspace/builds/coq/coq-failing/_install_ci/lib/coq/user-contrib/Bignums" "Bignums" "-Q" "/github/workspace/builds/coq/coq-failing/_install_ci/lib/coq/user-contrib/Coqprime" "Coqprime" "-Q" "/github/workspace/builds/coq/coq-failing/_install_ci/lib/coq/user-contrib/Ltac2" "Ltac2" "-Q" "/github/workspace/builds/coq/coq-failing/_install_ci/lib/coq/user-contrib/Rewriter" "Rewriter" "-I" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/deps/coqutil/src" "-I" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/src" "-I" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/bedrock2/src" "-I" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/compiler/src" "-I" "/github/workspace/builds/coq/coq-failing/_build_ci/fiat_crypto/rupicola/bedrock2/deps/riscv-coq/src" "-top" "ListUtil" "-native-compiler" "ondemand") -*- *)
(* File reduced by coq-bug-minimizer from original input, then from 3112 lines to 25 lines, then from 30 lines to 26 lines *)
(* coqc version 8.16+alpha compiled with OCaml 4.13.0
coqtop version runner-nthfetyx-project-6138686-concurrent-0:/builds/coq/coq/_build/default,(HEAD detached at 19a6ae0) (19a6ae06ebc93ba02c6183df9ce9cecb197c655c)
Expected coqc runtime on this file: 0.191 sec *)
Require Coq.Lists.List.
Import Coq.Lists.List.
Import ListNotations.
Definition span_cps' {A} (f : A -> bool) {T} (k : list A * list A -> T)
:= fix span_cps' (ls : list A) (prefix : list A) : T
:= match ls with
| nil => k (List.rev prefix, nil)
| x :: xs => if f x then span_cps' xs (x :: prefix) else k (List.rev prefix, ls)
end.
Definition groupBy' {A} (f : A -> A -> bool)
:= fix groupBy' (ls : list A) (prefix : list A) : list (list A)
:= match ls with
| [] => []
| x :: xs => span_cps'
(f x) (fun '(xs, ys)
=> (x :: xs) :: match ys with
| [] => []
| _ => groupBy' ys []
end)
xs prefix
end.Intermediate Coq File (useful for debugging if minimization did not go as far as you wanted)Build Log (contains the Coq error message) (truncated to last 8.0KiB; full 6.7MiB file on GitHub Actions Artifacts under
|
|
The error message on the minimized example is weird. It says but I don't see where this empty list is coming from in the syntax. Maybe there is an off-by-one error somewhere. |
|
Here is a simplified example. Definition span_cps' {A} (f : A -> bool) {T} (k : list A -> T) :=
fix span_cps' (ls : list A) {struct ls} : T := match ls with
| nil => k nil
| cons x xs => if f x then span_cps' xs else k ls
end.
Definition groupBy' {A} (f : A -> A -> bool) :=
fix groupBy' (ls : list A) {struct ls} : unit := match ls with
| nil => tt
| cons x xs =>
span_cps' (f x) (fun ys => match ys with nil => tt | _ => groupBy' ys end) xs
end.Interestingly, the match in the continuation is necessary in master to make this pass the guard checking, but I don't understand why. The EDIT: this will teach me to turn seven times my fingers over my keyboard before writing a post, but it's obvious: the problem is that in the nil branch of |
|
On master it probably reduces the |
|
@mattam82 AFAICT this PR is ready and waiting for your approval / merge. |
|
@ppedrot do we need to worry about the bench failures? |
|
@mattam82 no, it was a transient failure at the time. This should be ready to merge. |
|
@coqbot merge now |
Kind: Enhancement
Includes #15451 and #15453.
We do that by splitting guard errors into two categories:
We also maintain a stack of redexes so that when a variable is bound in the subterm of a redex, we directly know which redex needs to be reduced to trigger the instantiation of the variable.
In situations of the form
C(if c then x else y)wherexis a recursion variable,yneeds the unfolding of a redexC[], we use a rich notion of subterm which remembers both that we need to reduce the redex to check a possible recursive call ony, but that we need to check that we don't also make a recursive call on x even w/o reducing the redex. So, the union of specifications of subterms of amatch/ifis now a set.We are also more tolerant in subterms with recursive calls on internal uninstantiatable variables and on not applied enough recursive calls which cannot be applied more (e.g.
Fixpoint foo (n : nat) := forall x : foo = foo, True).For the beta and zeta-redex cases, we choose to check the argument for possibly fatal non guarded calls and reduce anyway. Another strategy could have been to reduce only when a subterm of the redex needs a reduction (but the stack call is then a bit complicated to manage).
Fixes #6487
Fixes #7061
Incidentally also fixes #5702 (reporting about a non-guarded fixpoint is exponential in the number of unfoldable constants encapsulating the illegal recursive call).
See examples in file
test-suite/success/FixStronglyWf.v.