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

Discharge on the fly #17888

Closed
wants to merge 8 commits into from
Closed

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Jul 25, 2023

This is a proof of concept for discharge on the fly, that is for being able to access all generalizations of a declaration in a section at the time of declaration.

Example:

Section S.
Context {A B:Type}.
Inductive option := None | Some (a:A).
Definition map (f : A -> B) (o : option) :=
  match o with
  | None => Top.None
  | Some a => Top.Some (f a)
  end.
End S.

The new model is basically the following:

  • one declaration in a nested sequence of sections S1, ..., Sn induces n+1 declarations, corresponding to the original declaration and its n generalizations
  • at the time of closing Si, we morally just drop the copy of the declaration in Si (even if, technically, we reset and replay the i generalizations from level 0 to i-1)

There is basically one main change of structure:

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

The changes are otherwise dispatched into three parts:

  • in the kernel (safe_typing.ml): add all generalizations of a declaration at declaration time
  • in library (lib.ml): add all discharges of objects at declaration time
  • adapt all discharge functions to take into account that names now include a section segment and that all discharges virtually live together in the section

This raises questions about the levels at which a declaration has to be done:

  • most declarations, starting with definitions and inductive types, as well as their attributes (implicit arguments, scopes, etc.) have to be done at all levels, i.e., for the example above, in section S, both option and Top.option have to be defined
  • some declarations should be done only in one level, e.g. for Require, or (mono) Universe only the copy living at the toplevel has to be registered
  • for some declarations, such as coercions and canonical structures, it is unclear: do we want to have all discharged version available from section i (even if it is somehow redundant) or only the version discharged at level i?

Incidentally, since the discharge functions have to be adapted anyway, that might be a right time to change the type of Libobject functions (e.g. so that they take an environment and a summary as arguments?).

I open the PR as draft to get some first comments.

Current status: PRs #18062 (preliminatory work on discharge) and #18065 (discharge on the fly in the kernel) have been made out of this PR, which now depends on them.

  • Added / updated test-suite.
  • Added changelog.
  • Added / updated documentation.

Related discussion: Future of sections #6254 and coq/ceps#72.

@herbelin herbelin added kind: feature New user-facing feature request or implementation. part: kernel labels Jul 25, 2023
@herbelin herbelin added this to the 8.19+rc1 milestone Jul 25, 2023
@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 25, 2023
@herbelin herbelin force-pushed the master+discharge-on-the-fly branch from 7eed083 to b21e6d6 Compare July 25, 2023 19:42
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 25, 2023
@herbelin

This comment was marked as outdated.

@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 25, 2023
@coqbot-app

This comment was marked as outdated.

@JasonGross
Copy link
Member

This is nice!

for some declarations, such as coercions and canonical structures, it is unclear: do we want to have all discharged version available from section i (even if it is somehow redundant) or only the version discharged at level i?

I think we want all versions (unless it's declared Local). For example, we might have a coercion from option A >-> A + unit. If this coercion is declared in the same section that the option inductive is created, we might still want access to the coercion from Top.option B >-> B + unit. By contrast, it might be the case that the global coercion isn't good enough, for example if we have Context {A : Type} (default : A)., define Inductive option, and then have a coercion in the section option >-> A (which won't work globally without default).

I worry that doing this for instances is going to blow up typeclass resolution, though? In general, it seems like we only want the more global version of a canonical structure/coercion/instance to trigger if the section-local one cannot apply.

@CohenCyril
Copy link
Contributor

@gares

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 26, 2023
@herbelin herbelin force-pushed the master+discharge-on-the-fly branch from b21e6d6 to 6fc1ad4 Compare July 26, 2023 20:24
@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 Jul 26, 2023
@coqbot-app

This comment was marked as outdated.

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 27, 2023
@herbelin herbelin force-pushed the master+discharge-on-the-fly branch from 6fc1ad4 to 3c89247 Compare July 27, 2023 07: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 Jul 27, 2023
@coqbot-app

This comment was marked as outdated.

m-lindgren pushed a commit to UniMath/UniMath that referenced this pull request Jul 27, 2023
This PR is in anticipation of name collisions that would appear with the
introduction of discharge on the fly in Coq (coq/coq#17888).

In the present case, the collisions would come from the introduction of
new discharged names of the form `pullbacks.PullbackArrow` in `graphs`
which would collide with the same names in `limits`.

Apparently, the convention was anyway that names from `limits` had an
explicit `limits` prefix up to the four exceptions of this PR. So, I
believe that this PR can be merged anyway, as soon as now, independently
of coq/coq#17888.
@gares
Copy link
Member

gares commented Jul 27, 2023

I like the feature, I've always thought that this is how sections should work.
But I think you took a very hard path, making the kernel aware of the the two copies (the section one and the discharged one) of the same constant. I think the section one should not exist in the first place, at best emulated via the nametab, as if we could put modules inside sections.

Section S.
Variable A : Type.
Definition f : T :=. .. A ...
Module Aux. Let f := f A. End Aux. Import Aux. (* pseudo code for nametab *)
Check f : T.
Check Top.f : Type -> T.

or said otherwise

Section S.
Variable A : Type.
Definition f : T :=. .. A ...
Let f_aux := f A.
(* nametab making f -> f_aux *)
Check f : T.
Check f_aux. (* error, no nametab entry *)
Check Top.f : Type -> T.
End S.
(* nametab entry for f -> f_aux gone *)

Modifying the kernel is a regression to me, it has been a while since we managed to get rid of section components in kernel names. But maybe your change amounts to implementing the module-in-section thing just for constants, in that case I stand corrected, but you mark it as the main change which seems a bit odd it is amounts to generate a f_aux name.

@gares
Copy link
Member

gares commented Jul 27, 2023

In any case, I think this is a huge change deserving a CEP (like the one about levels in grammar entries, which studies all details and possible angles to tackle it)

@gares
Copy link
Member

gares commented Jul 27, 2023

I worry that doing this for instances is going to blow up typeclass resolution, though? In general, it seems like we only want the more global version of a canonical structure/coercion/instance to trigger if the section-local one cannot apply.

I think that generating the metadata twice would be an error, e.g. the discharged copy should stay naked.
IMO being able to refer to the discharged constant (and manually adding metadata if one wants) is sufficient to improve the current situation.

@SkySkimmer
Copy link
Contributor

The test suite error is a new warning in an output test, seems quite unlike the other CI errors. Therefore
@coqbot ci minimize

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 27, 2023

I have initiated minimization at commit 3c89247 for the suggested targets ci-equations, ci-fiat_crypto_legacy, ci-flocq, ci-hott, ci-iris, ci-mathcomp, ci-perennial, ci-rewriter as requested.
However, you may want to try again once the pipeline for the head commit (3c89247) finishes.

@SkySkimmer
Copy link
Contributor

also @coqbot bench

@coqbot-app

This comment was marked as outdated.

@coqbot-app

This comment was marked as outdated.

About declaring scopes, databases for hints, and overriden notations.
That means canonically relying on discharge functions for global
references.
There is basically one change of structure:
- kernel names include a section path in their name (encoded using MPdot)

There are basically three kinds of algorithmic change:
- in the kernel (safe_typing.ml): add all generalisations of a declaration at declaration time
- in library (lib.ml): add all discharges of a declaration at declaration time
- adapt all discharge functions to take into account that names now include a section segment

There are three kinds of behaviors for objects:
- registered only in the innermost section (e.g. coercions, canonical projections, instances, ...)
- registered only in the outermost section (e.g. Require)
- registered only in all section (e.g. implicit arguments, arguments scopes, ...)
(Note that the Univ object is implemented in an ad hoc way so that bound
universes are innermost and monomorphic universes are outermost)

TODO:
- double-check what really needs to be in Safe_typing.section_data
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 2, 2023
@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 Sep 2, 2023
@JasonGross
Copy link
Member

JasonGross commented Sep 3, 2023

@JasonGross: Here is a log (https://gitlab.com/coq/coq/-/jobs/4765018144), with a failure on example 008.

I see. The minimization strategy for removing sections indeed becomes more fragile with this change, since there are more cases where removing End section will change the behavior if Section section is not removed at the same time. I guess I should either adapt the test suite file check or add a pass for removing paired section vernaculars.

@JasonGross
Copy link
Member

Actually, sorry, I think I misdiagnosed the failure, and in fact the issue is that minimization got stronger / easier, now that absolute names are available inside sections. I guess the test case check should be adapted

@herbelin
Copy link
Member Author

For the record, a comment of Théo reporting about the experience in Lean of simulating sections with implicit arguments.

@JasonGross
Copy link
Member

Is this currently blocked on me writing an overlay for coq-tools, or something else?

@herbelin
Copy link
Member Author

herbelin commented Sep 19, 2023

Is this currently blocked on me writing an overlay for coq-tools, or something else?

Nothing strictly blocking yet.

We had a discussion at the Coq call today and decided to merge already the internal (kernel) part (#18065).

For the rest, we decided to collect examples of needs so that we understand better what to exactly implement user-side (e.g. whether the discharge is made on demand or systematically, or how to deal with different versions of a hint, or whether we recover the specific versions from the most general ones, etc.)

@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 25, 2023
@SkySkimmer SkySkimmer removed this from the 8.19+rc1 milestone Nov 22, 2023
Copy link
Contributor

coqbot-app bot commented Nov 24, 2023

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 24, 2023
Copy link
Contributor

coqbot-app bot commented Dec 25, 2023

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

@coqbot-app coqbot-app bot closed this Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature request or implementation. 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: kernel stale This PR will be closed unless it is rebased.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants