Skip to content

Merge the code paths for "CoFixpoint"/"Fixpoint" and "Theorem with" - #19259

Merged
coqbot-app[bot] merged 5 commits into
rocq-prover:masterfrom
herbelin:master+merge-fixpoint-theorem-with
Jul 3, 2024
Merged

Merge the code paths for "CoFixpoint"/"Fixpoint" and "Theorem with"#19259
coqbot-app[bot] merged 5 commits into
rocq-prover:masterfrom
herbelin:master+merge-fixpoint-theorem-with

Conversation

@herbelin

@herbelin herbelin commented Jun 22, 2024

Copy link
Copy Markdown
Member

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. part: fixpoints About Fixpoint, fix and mutual statements part: cofixpoints About CoFixpoint, cofix and mutual statements part: gallina The gallina commands labels Jun 22, 2024
@herbelin herbelin added this to the 8.21+rc1 milestone Jun 22, 2024
@herbelin
herbelin requested review from a team as code owners June 22, 2024 11:05
@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 Jun 22, 2024
@herbelin herbelin added the needs: merge of dependency This PR depends on another PR being merged first. label Jun 22, 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 24, 2024
@herbelin
herbelin force-pushed the master+merge-fixpoint-theorem-with branch from cd98a63 to d0c4875 Compare June 27, 2024 12:24
@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 27, 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 28, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 1, 2024
@herbelin
herbelin force-pushed the master+merge-fixpoint-theorem-with branch from d0c4875 to b865197 Compare July 1, 2024 15:45
@coqbot-app coqbot-app Bot removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. 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 1, 2024
@herbelin herbelin added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. request: full CI Use this label when you want your next push to trigger a full CI. labels Jul 2, 2024
@herbelin
herbelin force-pushed the master+merge-fixpoint-theorem-with branch from b865197 to 67d251b Compare July 2, 2024 06:35
@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 2, 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 Jul 2, 2024
herbelin added 2 commits July 2, 2024 14:21
Also catch OpenProof exception in Program inference hook as it can be
one of the reasons of failure.
@herbelin
herbelin force-pushed the master+merge-fixpoint-theorem-with branch from 67d251b to 5bc7f5a Compare July 2, 2024 12:22
@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 Jul 2, 2024
@herbelin

herbelin commented Jul 2, 2024

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 2, 2024
Comment thread vernac/comFixpoint.mli

val do_mutually_recursive
: ?pm:Declare.OblState.t
(* Obligation mode turns unresolved evars into obligations *)

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.

Shouldn't these be **

@herbelin herbelin Jul 2, 2024

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.

I don't think that odoc supports (** *) comments in between types. If I put (** I get this:
odoc-fixpoint that is, it keeps only the last (** comment.

Generated with dune build @doc.

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.

Hmm I guess the correct way to do this would be in the comment attached to do_mutually_recrusive. You could write something like this:

(** [do_mutually_recursive ~program_mode ~poly recursives_exp] returns open obligations and open proofs if any. It takes the following arguents:
  - [program_ode] ...
  - [poly] ...
  - [recursives_exp] ...
  - [pm] is an optional argument that turns unresolved evars into obligations.
  - [use_interferene_hook] ...
 *)
val do_mutually_recursive
  :  ?pm:Declare.OblState.t
  -> program_mode:bool
  -> ?use_inference_hook:bool
  -> ?scope:Locality.definition_scope
  -> ?clearbody:bool
  -> poly:bool
  -> ?typing_flags:Declarations.typing_flags
  -> ?user_warns:UserWarn.t
  -> ?using:Vernacexpr.section_subset_expr
  -> recursives_expr
  -> Declare.OblState.t option * Declare.Proof.t option

The reason to do it this way is so that you can see how to use the function on hover. For instance, ocaml-lsp shows the odoc description of a function and having the full manual here can help without having to jump to the mli.

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.

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.

Somehow, I'm not convinced that it is where documentation is worth. These different functions (see e.g. also ComDefinition.do_definition) have the same arguments, 8 of them being actually components of a Declare.Info.t.

I feel that it would be better to properly document Declare.Info.t and to replace most of the arguments by an Info.t.

Comment thread vernac/declare.mli

val check_program_libraries : unit -> unit

val program_inference_hook : Environ.env -> Evd.evar_map -> Evar.t -> (Evd.evar_map * EConstr.t) option

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.

Maybe an opportunity to document and give this a better name?

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.

I don't know how to call it and I don't know either if it is worth to keep it.

@ppedrot ppedrot removed the needs: merge of dependency This PR depends on another PR being merged first. label Jul 3, 2024
@SkySkimmer SkySkimmer self-assigned this Jul 3, 2024
@SkySkimmer

Copy link
Copy Markdown
Contributor

@coqbot merge now

@coqbot-app
coqbot-app Bot merged commit 4d68031 into rocq-prover:master Jul 3, 2024
coqbot-app Bot added a commit that referenced this pull request Oct 8, 2024
…with" (lost in #19259)

Reviewed-by: SkySkimmer
Co-authored-by: SkySkimmer <SkySkimmer@users.noreply.github.com>
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 part: gallina The gallina commands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants