Skip to content

[Subsumed] Towards a single code path for the "using" clause + small improvement of "using Type" for co/fixpoints - #18742

Closed
herbelin wants to merge 4 commits into
rocq-prover:masterfrom
herbelin:master+one-copy-of-using-clause
Closed

[Subsumed] Towards a single code path for the "using" clause + small improvement of "using Type" for co/fixpoints#18742
herbelin wants to merge 4 commits into
rocq-prover:masterfrom
herbelin:master+one-copy-of-using-clause

Conversation

@herbelin

@herbelin herbelin commented Mar 4, 2024

Copy link
Copy Markdown
Member

The using clause was stored either in the Proof.t (for definitions generating proofs) or in the CInfo.t (for definitions with immediate proof, one for each component if a co/fixpoint). The PR makes a single choice by putting it instead in Info.t. In particular:

Note that we renounce to have one using per member of the CInfo.t on the assumption that the components of CInfo.t are mutually dependent, thus, having to share the same section variables (did I miss other uses of CInfo.t than for mutual definitions?).

In particular, we enhance a bit the effect of using Type on mutual definitions, taking all types together into account so that the dependencies are computed for the block of co/fixpoints as a whole.

@gares, @ejgallego: I believe it goes in the direction that you defended, I hope I'm not wrong.

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: inductives Inductive types, fixpoints, etc. labels Mar 4, 2024
@herbelin herbelin added this to the 8.20+rc1 milestone Mar 4, 2024
@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 Mar 4, 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 Mar 4, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 4, 2024
@herbelin
herbelin force-pushed the master+one-copy-of-using-clause branch from 8f6af45 to 1a3209a Compare March 4, 2024 19:02
@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. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels Mar 4, 2024
@ejgallego ejgallego self-assigned this Mar 5, 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 Mar 5, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 5, 2024
@herbelin
herbelin force-pushed the master+one-copy-of-using-clause branch from 1a3209a to 1303e48 Compare March 5, 2024 18:19
@coqbot-app coqbot-app Bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels Mar 5, 2024
@herbelin
herbelin marked this pull request as ready for review March 5, 2024 18:19
@herbelin
herbelin requested a review from a team as a code owner March 5, 2024 18:19
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 6, 2024
@herbelin
herbelin force-pushed the master+one-copy-of-using-clause branch from 1303e48 to 9575db3 Compare March 8, 2024 15:11
@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 Mar 8, 2024
@herbelin

herbelin commented Mar 8, 2024

Copy link
Copy Markdown
Member Author

I added another small using "Type" bug with Program Fixpoint: bodies were wrongly taken into account:

Require Import Program.
Section S.
Variables a : nat.
#[using="Type", warning="-non-recursive"]
Program Fixpoint b (n:nat) : nat := (fun _ => 0) a.
End S.
Print b.
(* "fun _ : nat => fix b (n : nat) : nat := 0" instead of "fix b (n : nat) : nat := 0" *)

@herbelin

herbelin commented Mar 9, 2024

Copy link
Copy Markdown
Member Author

Actually, I'm confused about the meaning to give to Type. What Type is doing in the code, before the PR, is to collect all variables statically known at Gallina declaration time, so including the body when one is present, and, indeed, this is what is interesting to do in practice. So, maybe Type should be kept as it is, and the meaning of Type in the documentation be instead clarified.

Another curiosity in passing, which impacts the computation of using, is that some commands eventually contract beta-redexes present in the body or the type:

  • Program Fixpoint beta-reduces both the bodies and types
  • Program Definition beta-reduces the type but not the body
  • Definition/Fixpoint/Theorem do not beta-reduce

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 13, 2024
@herbelin
herbelin force-pushed the master+one-copy-of-using-clause branch from 9575db3 to aa6cd4c Compare March 13, 2024 20:38
@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 Mar 13, 2024
@herbelin

Copy link
Copy Markdown
Member Author

It is still unclear what would be the best semantics of Type of using. Enrico seems ok with an interpretation of Type as variables in the type of the declaration rather than as variables occurring at definition time, that is as the PR is currently. So, maybe can we proceed?

@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 Apr 3, 2024

@ejgallego ejgallego left a comment

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.

@herbelin , thanks for the PR, the patch looks good, but we already have using in the proof info field; I wonder if we can just remove that using parameter at all from Info.t / CInfo.t ?

ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 3, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- non-interactive commands ignore the using attribute (that could be
  resurrected maybe if we add an "Opaque" form)
- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)
- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare

TODOs:

- XXX in declare.ml
- add warning when attributes are not supported
ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 3, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- non-interactive commands ignore the using attribute (that could be
  resurrected maybe if we add an "Opaque" form)
- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)
- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare

TODOs:

- XXX in declare.ml
- add warning when attributes are not supported
@ejgallego

Copy link
Copy Markdown
Contributor

Indeed, it seems to me that we have only two relevant cases:

  • proof using
  • init of interactive proofs

I've tried going deeper in #18890 , let me know what you think.

If we think the approach in that PR is better, I will finish the XXX (minor), I have to run now so I posted the current WIP code in that PR.

@herbelin

herbelin commented Apr 4, 2024

Copy link
Copy Markdown
Member Author

@ejgallego:

I wonder if we can just remove that using parameter at all from Info.t / CInfo.t ?

I put using in Info.t as a way to factorize its presence in both ProgramDecl.t and Proof.t (corresponding to prg_using and Proof.using in your PR). The way it is done in #18890 is ok to me.

@herbelin

herbelin commented Apr 4, 2024

Copy link
Copy Markdown
Member Author

Your PR is good. Once the design decision of rejecting or accepting using on non-interactive declarations is taken, I propose to merge your PR, which additionally should solve the issue of using on mutual fixpoints, once the XXX XXX are addressed there.

You can also add the tests I made to your PR.

herbelin added 4 commits April 6, 2024 11:22
…s altogether.

This is currently for non-interactive Co/Fixpoint only.

Additionally, for Co/Fixpoint, include the bodies when computing using
"Type", as it is done elsewhere.
- We keep only one copy of "using" in declare.ml, shared by all proofs
  and all statements of a block, that in once Info.t while before it
  was in each CInfo.t and in the Proof_info.t.
- We treat "using" the same way in interactive and non-interactive mode,
  gaining in uniformity and answering an open comment in vernacentries.ml.
  As a consequence, the fix for 'using="Type"' on mutual fixpoints in
  the previous commit also works in interactive mode.
@herbelin
herbelin force-pushed the master+one-copy-of-using-clause branch from 825bdc1 to b417b85 Compare April 6, 2024 09:24
@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 Apr 6, 2024
ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 11, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- non-interactive commands ignore the using attribute (that could be
  resurrected maybe if we add an "Opaque" form)
- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)
- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare
- for now we choose to warn on places where using attributes are not used
ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 11, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)
@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 Apr 21, 2024
ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 25, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)
ejgallego added a commit to ejgallego/coq that referenced this pull request Apr 26, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
ejgallego added a commit to ejgallego/coq that referenced this pull request May 2, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
ejgallego added a commit to ejgallego/coq that referenced this pull request May 2, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
ejgallego added a commit to ejgallego/coq that referenced this pull request May 2, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
@ejgallego

Copy link
Copy Markdown
Contributor

@herbelin alternative PR seems ready, with the tests developed here; I think you can close this one then; thanks for all the help and careful comments!

ejgallego added a commit to ejgallego/coq that referenced this pull request May 6, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
ejgallego added a commit to ejgallego/coq that referenced this pull request May 6, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with rocq-prover#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in rocq-prover#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from rocq-prover#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
@herbelin

herbelin commented May 6, 2024

Copy link
Copy Markdown
Member Author

Replaced by #18890.

@herbelin herbelin closed this May 6, 2024
@coqbot-app coqbot-app Bot removed this from the 8.20+rc1 milestone May 6, 2024
@herbelin herbelin changed the title Towards a single code path for the "using" clause + small improvement of "using Type" for co/fixpoints [Subsumed] Towards a single code path for the "using" clause + small improvement of "using Type" for co/fixpoints May 21, 2024
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. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. part: inductives Inductive types, fixpoints, etc.

Projects

Status: Rejected

Development

Successfully merging this pull request may close these issues.

2 participants