Skip to content

Discharge on the fly in the kernel - #18065

Closed
herbelin wants to merge 6 commits into
rocq-prover:masterfrom
herbelin:master+discharge-on-the-fly-kernel
Closed

Discharge on the fly in the kernel#18065
herbelin wants to merge 6 commits into
rocq-prover:masterfrom
herbelin:master+discharge-on-the-fly-kernel

Conversation

@herbelin

Copy link
Copy Markdown
Member

Since #14727, the kernel has all the information to support discharging of declarations at the time of declaration. This was experimented in #17888 and this PR is the internal kernel part of #17888.

It implements the following:

  • kernel names include a section path in their name (encoded using MPdot)
  • in safe_typing.ml: add all generalisations of a declaration at the time of the declaration rather than at the end of the section

No user-level changes if I'm not mistaken.

Depends on #18062.

TODO: double-check what really needs to be in Safe_typing.section_data

That means canonically relying on discharge functions for global
references.
This means the following changes:
- kernel names include a section path in their name (encoded using MPdot)
- in safe_typing.ml: add all generalisations of a declaration at the time
  of the declaration rather than at the end of the section

TODO:
- double-check what really needs to be in Safe_typing.section_data
@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. part: sections The section mechanism of Coq. needs: merge of dependency This PR depends on another PR being merged first. labels Sep 19, 2023
@herbelin herbelin added this to the 8.19+rc1 milestone Sep 19, 2023
@herbelin
herbelin requested review from a team as code owners September 19, 2023 16: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 Sep 19, 2023
@SkySkimmer SkySkimmer mentioned this pull request Sep 19, 2023
3 tasks
@ppedrot

ppedrot commented Sep 20, 2023

Copy link
Copy Markdown
Member

I think that this PR is already implementing too much, insofar as it's geared towards one specific instances of the extensions we discussed yesterday. Namely,

kernel names include a section path in their name (encoded using MPdot)

is already assuming we'll be accessing the various specializations at the same time. Since we don't have any equations on these yet I think it's a bad idea to include this in this PR. Especially if we have to revert it at some point (e.g. if we go for Georges' proposal for instance).

Can we just get

add all generalisations of a declaration at the time of the declaration rather than at the end of the section

to start with something consensual?

@herbelin

herbelin commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

I'm not fully sure I understand the difference that you are making. Do you mean that you'd prefer to have a kernel name (as it is now) pointing to the list of all generalizations rather than having one kernel name per generalization? Closing a section would then be dropping the more specific body in each list vs dropping the kernel name pointing to the more specific body?

@ppedrot

ppedrot commented Sep 20, 2023

Copy link
Copy Markdown
Member

I don't want the Constant type to be changed (for now), and instead of relying on the environment to get the various constant bodies, we should have one single list of bodies per constant.

@herbelin

Copy link
Copy Markdown
Member Author

So, to confirm, you mean changing Environ.mind_key and Environ.add_mind from

type mind_key = mutual_inductive_body * link_info ref  
val add_mind : MutInd.t -> mutual_inductive_body -> env -> env

to

type mind_key = mutual_inductive_body list * link_info ref 
val add_mind : MutInd.t -> mutual_inductive_body list -> env -> env

and the same for constants?

@SkySkimmer

Copy link
Copy Markdown
Contributor

I would put the discharged versions in the section data, not the environment if they're not meant to be accessed by most things

@herbelin

Copy link
Copy Markdown
Member Author

I would put the discharged versions in the section data, not the environment if they're not meant to be accessed by most things

Note that, if, up to know, only the vm is interested in the discharge data, it can directly access the API of cooking.ml without needing a PR. From my point of view, what justifies the PR is that it eventually comes with a user access to the data, so this suggests that listing our wishes is the priority (as far as I'm concerned, my idea was the one given in the header of #17888.)

@ppedrot

ppedrot commented Sep 20, 2023

Copy link
Copy Markdown
Member

it can directly access the API of cooking.ml without needing a PR

This is expensive, since we need to recook the terms. If we do it once in a centralized place this is much simpler and faster.

@herbelin

Copy link
Copy Markdown
Member Author

if we do it once in a centralized place this is much simpler and faster.

Sure, but if it is about putting this data in the section_data, it might just be enough to add a field there with the result of Discharge.cook_inductive applied to Section.segment_of_inductive, with minimal modification to Safe_typing.close_section.

So, before unravelling what I did, and maybe having to ununravel afterwards, I would prefer to have a better view of what model we want to eventually provide.

For instance, regarding the vm, is it something of the form lookup_fully_generalized_constant : Constant.t -> env -> constant_body that would be needed?

@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 Oct 27, 2023
@SkySkimmer SkySkimmer removed this from the 8.19+rc1 milestone Nov 22, 2023
@coqbot-app

coqbot-app Bot commented Nov 27, 2023

Copy link
Copy Markdown
Contributor

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app Bot added the stale This PR will be closed unless it is rebased. label Nov 27, 2023
@coqbot-app

coqbot-app Bot commented Dec 27, 2023

Copy link
Copy Markdown
Contributor

This PR was not rebased after 30 days despite the warning, it is now closed.

@coqbot-app coqbot-app Bot closed this Dec 27, 2023
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. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. needs: merge of dependency This PR depends on another PR being merged first. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. part: sections The section mechanism of Coq. stale This PR will be closed unless it is rebased.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants