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

Stronger invariants in the structure of Declare.proof_entry #19237

Merged

Conversation

herbelin
Copy link
Member

The PR enforces stronger invariants in the different uses of proof_entry.

Using a sum type Default(Transparent|Opaque)|DeferredOpaque, the type proof_entry is morally divided into the following variants:

  • constr pproof_entry: no side effect, no local universes, not deferred
  • 'eff default_proof_body pproof_entry, parameterized by effects: not deferred, possibly transparent, possibly with local universes
  • 'eff proof_entry, parameterized by effects, the most general: possibly deferred, possibly transparent, possibly with local universes

The following excerpt of functions have then refined types:

  • cast_entry_proof
  • cast_opaque_entry_proof
  • declare_private_constant
  • build_constant_by_tactic
  • etc.

Together with a forthcoming PR which will merge close_proof and close_proof_delayed around a type Immediate(body,typ,uctx)|Delayed(body,type,uctx), we should be able to make progresses in the direction of coq/rfcs#89, that is to have both close_proof and immediate definitions producing an Immediate|Delayed which is then turned into a Default(Transparent|Opaque)|DeferredOpaque proof entry, then processed by declare_entry (to distinguish between in or outside a section) and finally by declare_constant.

The main commit is the 2nd one. The 3rd commit is mostly about the structuration of the code.

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. kind: internal API, ML documentation... labels Jun 19, 2024
@herbelin herbelin added this to the 8.21+rc1 milestone Jun 19, 2024
@herbelin herbelin requested a review from a team as a code owner June 19, 2024 08:34
@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 19, 2024
let force_entry_body entry =
match entry.proof_entry_body with
| Default { body; opaque } -> body, opaque
| DeferredOpaque { body; feedback_id = Some _ } -> CErrors.anomaly (str "Forcing a proof with feedback")
Copy link
Member

Choose a reason for hiding this comment

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

I think you should enforce this statically, since several callers below are using Future.from_val for no good reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

It remains 3 Future.from_val in the current PR:

  • one is to use cast_opaque_proof_entry and we could have a third case to it to avoid the from_val,
  • one is to declare an opaque Let: I don't understand well the code but it can probably be avoided,
  • the third one is to declare deferred univs for a non-deferred constant, and this could be avoided too.

So, all three could be avoided (assuming you are ok to have a third variant of cast_opaque_proof_entry).

Regarding force_entry_body, this is used for obligations and for Derive. I have the feeling that we rely here on stm invariants, that is that the stm will not defer proofs of obligations and proofs of Derive. I don't know how to ensure that (except by duplicating or annotating the type proof_object so that close_proof and close_future_proof have different return types). In any cases, it would modify the API of declare.ml and I'm not very comfortable at doing it now, at least until the whole coq/rfcs#89 is in place.

Copy link
Member

Choose a reason for hiding this comment

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

third variant of cast_opaque_proof_entry

GADTs can do wonder to prevent code duplication.

I'm not very comfortable at doing it now

No problem, it's better to do it incrementally.

Copy link
Member Author

Choose a reason for hiding this comment

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

If one wants to avoid futures as much as possible, one would also need a version of Opaque.declare_defined_opaque that takes a non deferred body. Do you want that?

Copy link
Member Author

Choose a reason for hiding this comment

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

...or a dependently-typed version of Opaque.declare_defined_opaque.

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jun 19, 2024
@herbelin herbelin force-pushed the master+proof-entry-stronger-invariants branch from 4ebee40 to 2021c18 Compare June 19, 2024 18:20
@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 Jun 19, 2024
@ppedrot
Copy link
Member

ppedrot commented Jun 22, 2024

@coqbot run full ci

@ppedrot
Copy link
Member

ppedrot commented Jun 25, 2024

@coqbot run full ci

@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 26, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jun 26, 2024
@herbelin herbelin force-pushed the master+proof-entry-stronger-invariants branch from 2021c18 to d05a2c1 Compare June 26, 2024 16:40
@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. labels Jun 26, 2024
@SkySkimmer
Copy link
Contributor

@ppedrot are you assigning?

@ppedrot ppedrot self-assigned this Jun 27, 2024
@ppedrot
Copy link
Member

ppedrot commented Jun 27, 2024

I think I had a merge conflict with master when trying to run this locally after the merge of another declare cleanup PR. Let's just @coqbot run full ci just in case

@ppedrot
Copy link
Member

ppedrot commented Jun 28, 2024

@coqbot merge now

@coqbot-app coqbot-app bot merged commit f6fdc81 into coq:master Jun 28, 2024
6 checks passed
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: internal API, ML documentation...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants