-
Notifications
You must be signed in to change notification settings - Fork 658
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
Major refactoring of cooking.ml #14727
Major refactoring of cooking.ml #14727
Conversation
8f40105
to
f2d4fce
Compare
kernel/cooking.mli
Outdated
val cook_constant : recipe -> cooking_info Opaqueproof.opaque result | ||
val cook_constr : cooking_info list -> | ||
(constr * unit Opaqueproof.delayed_universes) -> (constr * unit Opaqueproof.delayed_universes) | ||
val cook_opaque_proofterm : cooking_info list -> | ||
Opaqueproof.opaque_proofterm -> Opaqueproof.opaque_proofterm | ||
|
||
val cook_constant : | ||
recipe -> cooking_info Opaqueproof.opaque result | ||
|
||
val cook_inductive : | ||
cooking_info -> mutual_inductive_body -> mutual_inductive_body | ||
|
||
(** {6 Utility functions used in module [Discharge]. } *) | ||
val make_cooking_info : expand_info -> named_context -> Univ.UContext.t -> cooking_info | ||
(** Abstract a context assumed to be de-Bruijn free for terms and universes *) | ||
|
||
val lift_poly_univs : cooking_info -> Univ.AbstractContext.t -> cooking_info * Univ.AbstractContext.t | ||
|
||
val abstract_rel_context : cooking_info -> rel_context -> rel_context | ||
|
||
val expmod_constr : work_list -> constr -> constr | ||
val discharge_proj_repr : abstr_inst_info -> Names.Projection.Repr.t -> Names.Projection.Repr.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terminology, cook
, or discharge
, or abstract
, or lift
should probably be simplified.
lift_poly_univs
andabstract_rel_context
do morally the same: they traverse an already-quantified context, respectively a context of universe and a context of terms, updating the cooking info for the remainder of the judgement, and "cooking" this context at the same time.
kernel/cooking.ml
Outdated
| Opaqueproof.PrivateMonomorphic () as priv -> | ||
let () = assert (AbstractContext.is_empty info.abstr_info.abstr_uctx) in | ||
let () = assert (is_empty_level_subst info.abstr_info.abstr_usubst) in | ||
priv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't there be private universes also for monomorphic constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass them to cook_constr (see drop_mono in opaqueproof)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand well the story about PrivateMonomorphic
. Is the apparentl loss of information in drop_mono
to be related to the extraction of delayed_cst
in Safe_typing.add_constant
, so that the information is actually not lost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data is redundant for the kernel, and only used for vio computation. I have a series of patches that modify this area of the code but I've not submitted them yet because they break weird invariants in Declare that I need to fix first.
f2d4fce
to
a778cb2
Compare
a778cb2
to
810105a
Compare
810105a
to
df00348
Compare
df00348
to
cc583bd
Compare
I added two further "cleaning" commits. We could do more cleaning, like completely bypassing the type @ppedrot: would you like to assign? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok overall
kernel/section.ml
Outdated
|
||
type 'a entry_map = 'a Cmap.t * 'a Mindmap.t | ||
type section_entry = | ||
| SecDefinition of Constant.t * constant_body | ||
| SecInductive of MutInd.t * mutual_inductive_body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it makes sense to have the bodies here. It seems to be used to have access to them in close_section instead of getting them from the revstruct, but couldn't we just look them up in the env0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the hyps
of the declaration in Section.segment_entry
but not the full bodies. We could indeed use an access to the env instead. I can change it if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 9cfdd1e implements your suggestion. It looks a bit strange to recompute a data which we can have at hand for free, but I agree that it makes sense also to have the bodies stored in a unique place, even if there is a cost to get them back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SkySkimmer: I'm still a bit afraid of the extra time needed to find again the constant_body
and mutual_inductive_body
in the environment. Probably not more than a dozen of steps per definition for an environment of several thousands of declarations, so it is maybe a bit unjustified fear, especially considering how lax we are on factorizing the accesses to the environment in many parts of the code, but would you be ok if I would cache back the bodies in the section_entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am only OK if you can show an impact in the bench.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I could. That would require a huge environment and short definitions so that the time for looking in the environment dominates.
Well, ok, this is globally non-significant, so I leave the PR without the "cache".
@@ -484,6 +484,8 @@ val extend_in_context_set : 'a in_universe_context_set -> ContextSet.t -> | |||
|
|||
val empty_level_subst : universe_level_subst | |||
val is_empty_level_subst : universe_level_subst -> bool | |||
val lift_level_subst : int -> universe_level_subst -> universe_level_subst | |||
val merge_level_subst : universe_level_subst -> universe_level_subst -> universe_level_subst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should state the precondition (no common elements, even if they're mapped to the same value) explicitly.
I'm also not sure if it makes sense to have a function with such a precondition in the general API rather than having it defined near its (unique) user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. Both approaches are ok to me. In the meantime, I documented it.
kernel/names.ml
Outdated
@@ -916,14 +916,15 @@ struct | |||
module HashRepr = Hashcons.Make(Self_Hashcons) | |||
let hcons = Hashcons.simple_hcons HashRepr.generate HashRepr.hcons (hcons_ind,Id.hcons) | |||
|
|||
let map_npars f p = | |||
let map_ind_npars f p = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't exposed then it should be inlined in map_npars
and map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would lead to a duplication. Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's really duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and I agree that this is clearer by splitting them.
kernel/declarations.ml
Outdated
(** Canonical renaming represented by its domain made of the | ||
actual names of the abstracted term variables (e.g. [a,c]); | ||
the codomain made of de Bruijn indices is implicit *) | ||
abstr_usubst : Univ.universe_level_subst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not always make_instance_subst abstr_uctx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name abstr_uctx
is misleading. It is an AbstractContext
and, if I'm correct, the orginal uctx
levels have been lost. Maybe should it be renamed to abstr_auctx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
kernel/cooking.ml
Outdated
@@ -315,7 +315,7 @@ let cook_constant env info cb = | |||
(********************************) | |||
(* Discharging mutual inductive *) | |||
|
|||
let abstract_rel_context cache info ctx = | |||
let cook_rel_context cache info ctx = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I prefer abstract
to cook
but it's not a strong preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like abstract
also but cook
is used many times already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like abstract also but cook is used many times already.
Naming isn't a democracy, we can be tyrannical here
The "adding" commits should be merged with the commits introducing the use of the new API imo |
@SkySkimmer Thanks a lot for the comments. Next weeks are dense for me but I'll try to reply within a couple of days. |
kernel/declarations.ml
Outdated
expand_info : expand_info; | ||
abstr_info : abstr_info; | ||
abstr_inst_info : abstr_inst_info; (* relevant for recursive types *) | ||
names_info : Id.Set.t; (* set of generalized names *) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there is too much stuff exposed in this API. The datatypes used to live in Cooking
and were moved here by @SkySkimmer IIRC but now that we're supposed to preserve more invariants I believe this should be move back under an opaque interface. How doable is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in opaqueproof, not cooking
b1675a6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, right. But now that Opaqueproof
is parameterized by this data, it should be easier to put it in Cooking
, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency order goes opaqueproof <- declarations <- cooking
so I guess no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be tried nonetheless, I really don't like having all these internal bits exposed to everybody.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possibility would be to split cooking.ml
into two parts:
- the low-level infrastructure up to
subst_private_univs
(and with an abstract export ofcooking_info
), coming beforeopaqueproof.ml
- the high-level commands
cook_constant
andcook_inductive
coming afterdeclarations
(with possible integration toterm_typing.ml
if we don't want creating an extra file)
At worst, in the higher levels (vernac, tactics, ...) only the field abstr_inst
really needs to be visible (and it could as well be hidden behind an access function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit in this directon, providing a better abstraction of cooking_info
. We could abstract even a bit further, depending on feedback.
53f77e9
to
fcee31c
Compare
🔴 CI failure at commit cb6d6fc without any failure in the test-suite ✔️ Corresponding job for the base commit 7c608fe succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
1b4080e
to
9c1aa5f
Compare
|
9c1aa5f
to
4ef9d13
Compare
There is still room for improvement. For instance, the cooking of the variance of universes is still treated separatedly. Also, one may probably want that there is a computation of the actual dependencies in the universe variables of the section, the same way as it is done for declarations of variables. But this can probably go in other PRs. So, if nothing happens new on this PR by a fews days, I believe it can be considered ready. |
Includes a suggestion from Gaëtan Gilbert.
All technical code is now concentrated in a few functions with the rest of the code simply calling the core functions compositionally. The functions are: - expand_subst = - expand_constr: expansion of global constants - subst_univs_level_constr: name-to-de-Bruijn universe renaming - subst_vars: name-to-de-Bruijn term renaming - abstract_as_type/abstract_as_body: actual generalization It relies on the following data structures: - universe graph (abstr_auctx, or G) - term context (abstr_ctx, or Δ) - name-to-de-Bruijn universe substitution (abstr_ausubst, or τ) - name-to-de-Bruijn term substitution (abstr_subst, or σ) - instance of universe graph (abstr_uinst, or |G|) - instance of term context (abstr_inst, or |Δ|) - expansion maps (expand_info, mapping a c generalized over G' and Δ' to c |G'| |Δ'|) These data are enough to turn judgements of the form [G, Δ ⊢ J] into judgements of the form [⊢ ΠG.ΠΔ.(J[σ][τ])] It includes: - lift_poly_univs: to absorb a universe context in the transformation, that is to reduce the admissibiliy of a rule going from [G, Δ ⊢ ΠG'.J] to [⊢ ΠG.ΠΔ.(ΠG'.J)[σ][τ])] to the admissibiliy of a rule going from [G(G'[ττ']), Δ ⊢ J] to [⊢ ΠG(G'[ττ']).ΠΔ.(J[σ][τ])] for a well-chosen τ' - abstract_context: to absorb a term context in the transformation, that is to reduce the admissibiliy of a rule going from [G, Δ ⊢ ΠΩ.J] to [⊢ ΠG.ΠΔ.((ΠΩ.J)[σ][τ])] to the admissibiliy of a rule going from [G, Δ, Ω[σ][τ] ⊢ J] to [⊢ ΠG.ΠΔ.Π(Ω[σ][τ]).(J[σ'][τ])] for a well-chosen σ' Note: includes suggestions from Gaëtan Gilbert.
…oking_info. New dependency chain: cooking.ml <- opaqueproof.ml <- declarations.ml <- discharge.ml
4ef9d13
to
cb1b0ba
Compare
Merging soon. |
@coqbot merge now |
@ppedrot: Please take care of the following overlays:
|
Kind: Refactoring
Depends on #14726 (merged).
In this PR, all technical code is concentrated in a few functions with the rest of the code simply calling the core functions compositionally.
These functions are:
expand_subst
which itself includes:expand_constr
: expansion of global constantssubst_univs_level_constr
: name-to-de-Bruijn universe renamingsubst_vars
: name-to-de-Bruijn term renamingabstract_as_type
/abstract_as_body
: actual generalizationIt relies on the following data structures:
abstr_uctx
, orG
)abstr_ctx
, orΔ
)abstr_usubst
, orτ
)abstr_subst
, orσ
)abstr_uinst
, or|G|
)abstr_inst
, or|Δ|
)expand_info
, mapping ac
generalized overG'
andΔ'
toc |G'| |Δ'|
)These data are enough to turn judgements of the form
G, Δ ⊢ J
into judgements of the form⊢ ΠG.ΠΔ.(J[σ][τ])
.It includes:
lift_poly_univs
: to absorb a universe context in the transformation, that is to reduce the admissibiliy of a rule going fromG, Δ ⊢ ΠG'.J
to⊢ ΠG.ΠΔ.(ΠG'.J)[σ][τ])
to the admissibiliy of a rule going fromG(G'[ττ']), Δ ⊢ J
to⊢ ΠG(G'[ττ']).ΠΔ.(J[σ][τ])
for a well-chosenτ'
abstract_context
: to absorb a term context in the transformation, that is to reduce the admissibiliy of a rule going fromG, Δ ⊢ ΠΩ.J
to⊢ ΠG.ΠΔ.((ΠΩ.J)[σ][τ])
to the admissibiliy of a rule going fromG, Δ, Ω[σ][τ] ⊢ J
to⊢ ΠG.ΠΔ.Π(Ω[σ][τ]).(J[σ'][τ])
for a well-chosenσ'
Note that the new cooking is "incremental": the cooking data is not global to the section but attached to each declaration. So, in theory, it would be direct to precook all generalizations of a constant/inductive at the time of its declaration (and hence to use them in the section!).
Overlays (relative to the change in
PrivatePolymorphic
):