Skip to content

Use multiple statements to implement interactive fixpoints - #19091

Merged
coqbot-app[bot] merged 4 commits into
rocq-prover:masterfrom
herbelin:master+rework-interactive-fixpoint
Jul 21, 2024
Merged

Use multiple statements to implement interactive fixpoints#19091
coqbot-app[bot] merged 4 commits into
rocq-prover:masterfrom
herbelin:master+rework-interactive-fixpoint

Conversation

@herbelin

Copy link
Copy Markdown
Member

Interactive co/fixpoints (and Theorem with) used to declare a single proof starting with a dummy Fix. We reimplement it using the proof engine support for multiple statements.

This will allow to share the infrastructure for interactive fixpoints and Derive and this makes useless the copy of the types in the CInfo.t of proofs.

Depends on:

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. needs: merge of dependency This PR depends on another PR being merged first. request: full CI Use this label when you want your next push to trigger a full CI. part: fixpoints About Fixpoint, fix and mutual statements part: cofixpoints About CoFixpoint, cofix and mutual statements labels May 26, 2024
@herbelin herbelin added this to the 8.20+rc1 milestone May 26, 2024
@herbelin
herbelin requested review from a team as code owners May 26, 2024 16:41
@coqbot-app coqbot-app Bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label May 26, 2024
@ejgallego

Copy link
Copy Markdown
Contributor

I'm not sure what the status of #10363 is, but last time I looked the proof engine API for multiple statements was in need of serious work. But this may be an interesting experiment, let's see what the CI yields

(Tho I wouldn't be very surprised if we don't stress the particular corner cases that could be problematic)

@herbelin

Copy link
Copy Markdown
Member Author

the particular corner cases that could be problematic

Are you suggesting possible issues if we use abstract in a fixpoint implemented with multiple statements? I just tried with #19092 and it produces exactly the same term, that is each body of the final constants is abstracted relatively to the abstracted subproofs.

In the case of Derive, I have:

Require Import Derive.
Derive f SuchThat (f = 0) As spec.
unfold f. abstract reflexivity.
Qed.
Print spec.
(* spec = spec_subproof : f = 0 : f = 0 *)
Print f.
(* f = 0 : nat *)

Or do you mean something else?

@ejgallego

Copy link
Copy Markdown
Contributor

Back in the day I meant that we duplicated side effects on each entry, this seems un/under-specified.

@herbelin

Copy link
Copy Markdown
Member Author

I would need a definition of "side effects". If it is about abstract, is the following as expected:

Fixpoint f (n:nat) : nat with g (n:nat) : nat.
- abstract (exact 0).
- exact 0.
Qed.
Print f.
(* (fun f_subproof : nat => fix f (n : nat) : nat := f_subproof with g (n : nat) : nat := 0 for f) 0 *)
Print g.
(* (fun f_subproof : nat => fix f (n : nat) : nat := f_subproof with g (n : nat) : nat := 0 for g) 0 *)
Fixpoint f (n:nat) : nat with g (n:nat) : nat.
- abstract (exact 0).
- exact 0.
Defined.
Print f.
(* fix f (n : nat) : nat := f_subproof with g (n : nat) : nat := 0 for f *)
Print g.
(* fix f (n : nat) : nat := f_subproof with g (n : nat) : nat := 0 for g *)
Print f_subproof.
(* f_subproof = 0 *)

@github-actions github-actions Bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label May 29, 2024
@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from bb87dfa to 69366d1 Compare May 30, 2024 19:01
@coqbot-app coqbot-app Bot added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. and removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels May 30, 2024
@github-actions github-actions Bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 4, 2024
@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from 69366d1 to 9c7e05b Compare June 15, 2024 06:40
@proux01 proux01 modified the milestones: 8.20+rc1, 8.21+rc1 Jun 17, 2024
@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from 9c7e05b to 404ca57 Compare June 19, 2024 07:58
@coqbot-app coqbot-app Bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 19, 2024
@github-actions github-actions Bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 27, 2024
Comment thread test-suite/output/Fixpoint.out Outdated
Comment on lines +41 to +42
As a mutual fixpoint decreasing on the 1st argument of foo and
1st argument of bar:
Not enough abstractions in the definition.
As a mutual fixpoint decreasing on the 1st argument of foo and
2nd argument of bar:
Not enough abstractions in the definition.
As a mutual fixpoint decreasing on the 2nd argument of foo and
1st argument of bar:
Not enough abstractions in the definition.
As a mutual fixpoint decreasing on the 2nd argument of foo and
2nd argument of bar:
Not enough abstractions in the definition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it would get very long and unreadable as the number of possibilities increases.
It's already kinda bad IMO.

OTOH I don't know how to avoid listing the possibilities since the user didn't say {struct} (also I think there's currently no syntax for {struct} for Lemma with).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a bit more work, we could factorize the errors which are similar.

On the other side, #19301 adds syntax {struct} for Lemma with and for Lemma. So, an alternative could be to suggest using struct to get more information (???).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also extend the Guarded syntax to be able to say eg Guarded {struct 1, 3} {struct 3, 1} to mean "test guardedness with 1st fixpoint recursive on 1st arg and 2nd on 3rd arg, and with 1st fixpoint recursive on 3rd arg and 2nd on 1st arg"
that way in the middle of a big proof there is no need to restart it to get a better error

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it would get very long and unreadable as the number of possibilities increases.

With a bit more work, we could factorize the errors which are similar.

I pushed a new version simplifying a bit the printing so that in the "Not enough abstractions", it is printed only once.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also extend the Guarded syntax to be able to say eg Guarded {struct 1, 3} {struct 3, 1}

That's a good idea.

To depart not too much from the existing syntax, we could also say, e.g.: Guarded {struct 1} {struct 3} and repeat it for any combination we are interested in??

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to implement it? (And otherwise, I'd be happy if ever you have some time for it.)

Comment thread vernac/declare.ml
exception NotGuarded of
Environ.env * Evd.evar_map *
(Environ.env * int * EConstr.t Type_errors.pcofix_guard_error) option *
(Environ.env * int * int list * EConstr.t Type_errors.pfix_guard_error) list *

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we keeping copies of the env?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first env is the context for the whole declaration. The other envs are the contexts for the respective errors.

@coqbot-app coqbot-app Bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 15, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 15, 2024
@SkySkimmer

Copy link
Copy Markdown
Contributor

test suite not happy

@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from 07f4d69 to 1856bec Compare July 16, 2024 13:39
@coqbot-app coqbot-app Bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Jul 16, 2024
@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from 1856bec to eb47bad Compare July 17, 2024 09:40
@coqbot-app coqbot-app Bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 17, 2024
@herbelin

Copy link
Copy Markdown
Member Author

Adding -async-proofs off to see if it makes test-suite:base+async happier (full CI was otherwise ok).

@ppedrot

ppedrot commented Jul 17, 2024

Copy link
Copy Markdown
Member

Isn't this just ignoring the problem? What was the cause for the async failure?

@SkySkimmer

Copy link
Copy Markdown
Contributor

Output tests don't work well with async because it changes printing order
also IIRC it sometimes loses location info

@SkySkimmer

Copy link
Copy Markdown
Contributor

(although in https://gitlab.inria.fr/coq/coq/-/jobs/4572169 it seems it entirely lost some messages so not sure what's going on)

Comment thread test-suite/output/Fixpoint.v Outdated
- destruct n as [|n].
+ exact (bar 0 0).
Fail Guarded. (* failure is correct here *)
Undo 2.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use "-async-proofs-cache" "force" so that Undo works without repeating the previous messages

@SkySkimmer SkySkimmer Jul 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the lack of repetition with async proofs on is probably what caused the issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively write a test without Undo (I guess Abort then repeat the code for the start of the proof)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then, considering #19383, tell me when you know what the recommended approach will be.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#19383 should not really matter for this AFAIK

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so a test w/o Undo, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have the power to choose.

@ppedrot

ppedrot commented Jul 19, 2024

Copy link
Copy Markdown
Member

@coqbot run full ci

@coqbot-app coqbot-app Bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 19, 2024
@ppedrot

ppedrot commented Jul 19, 2024

Copy link
Copy Markdown
Member

@SkySkimmer if you're happy with the current status I can merge when CI finishes.

…in case of failure.

Co-authored-by: Gaëtan Gilbert <gaetan.gilbert@skyskimmer.net>
@herbelin
herbelin force-pushed the master+rework-interactive-fixpoint branch from 624efc6 to 224b21f Compare July 21, 2024 09:53
@coqbot-app coqbot-app Bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 21, 2024
@herbelin

Copy link
Copy Markdown
Member Author

@coqbot run full ci

@coqbot-app coqbot-app Bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 21, 2024
@ppedrot

ppedrot commented Jul 21, 2024

Copy link
Copy Markdown
Member

@coqbot merge now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: cleanup Code removal, deprecation, refactorings, etc. part: cofixpoints About CoFixpoint, cofix and mutual statements part: fixpoints About Fixpoint, fix and mutual statements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants