Conversation
cda08c0 to
26bf881
Compare
|
@coqbot run full ci |
4cfacbd to
3f8c2f6
Compare
Currently behind a Sort Polymorphism flag. fix: Generate proper sort variable in record feat: Add sort elab for interactive definitions fix: Thread sort poly flag more correctly It was being passed as unconstrained_sorts in some cases. refactor: Remove unnecessary sort_poly flags fix: Add sort_poly to prepare_obligations refactor: Remove remaining fixmes and todos refactor: General cleanup refactor: Get ?evar_handler from evd refactor: Remove search_fix_guard doc: comment collapse refactor: Remove unnecessary unconstrained_sorts refactor: Remove unnecessary check for collapse_sort_variables revert: Some changes in unify quality feat: Add elim constraints to non-prim records with sort variables
|
I found a bug when declaring a variable in Set Universe Polymorphism.
Unset Collapse Sorts ToType.
Section Test.
Variable (A : Type).
(* Anomaly "File "kernel/safe_typing.ml", line 698, characters 11-17: Assertion failed." Please report at http://rocq-prover.org/bugs/. *)Which comes from this check: let check_quality q =
Sorts.QVar.is_global q &&
not (QGraph.is_declared (Sorts.Quality.QVar q) (Environ.qualities senv.env))
in
let () = assert (Sorts.QVar.Set.for_all check_quality (fst qctx)) inApart from that, I think the projections are now currently elaborated with their appropriate elimination constraints, not just collecting. |
|
Currently we expect that all qvars in the global env (including section qualities) are QVar.is_global, this check enforces that. |
|
@ppedrot Why should sort variables be only global inside a section? According to the assertion: let check_quality q =
Sorts.QVar.is_global q &&
not (QGraph.is_declared (Sorts.Quality.QVar q) (Environ.qualities senv.env))
in
let () = assert (Sorts.QVar.Set.for_all check_quality (fst qctx)) inWhat cases is this assertion protecting from? |
vernac/declare.ml
Outdated
| (* We need sigma to check for elimination constraints. In most cases it's None, except for | ||
| [declare_mutual_definitions] where we get it from UState. *) | ||
| let sigma = Option.default (Evd.from_env env) sigma in | ||
| let _, indexes = Pretyping.search_guard env sigma possible_guard rec_declaration in |
There was a problem hiding this comment.
The change in the output for FixpointNoElim is because in master this search_guard fails, and in this PR we ignore the returned evar map (which would have an added s -> Prop) and then the kernel fails.
I'm not sure to what extent this is a problem, but it doesn't seem great.
vernac/record.ml
Outdated
| match Constr.kind c with | ||
| | Cast (c, _, _) -> aux c | ||
| | Prod (_, t, b) | Lambda (_, t, b) -> | ||
| let t_elim_cstrs = aux t in | ||
| let b_elim_cstrs = aux b in |
There was a problem hiding this comment.
why isn't this using Constr.fold?
There was a problem hiding this comment.
Because I didn't notice there was that function 🤷
test-suite/success/sort_poly_elab.v
Outdated
| Arguments inl {A B} _ , [A] B _. | ||
| Arguments inr {A B} _ , A [B] _. | ||
| Definition qsort := Type. | ||
| (* qsort@{α ; u |} = Type@{α ; u} : Type@{u+1} *) |
There was a problem hiding this comment.
instead of comments which are not checked by anything, this should be an output test (with an added About instead of the comment)
alternatively we could write some code to check that the expected thing is produced but it would get verbose, eg to check that an expected definition foo@{s s'; | s -> s'} we have to do
Fail Definition foo_check@{s s';|} := foo@{s s';}. (* missing elim constraint *)
Definition foo_check@{s s';|s -> s'} := foo@{s s';}.and if there is more than 1 elim constraint fully checking it gets exponential (we have to check that every strict subset of the constraints is rejected)
There was a problem hiding this comment.
Yup, those were more for documentation, if someone looks at the test without needing to run them. I'll add the output tests.
There was a problem hiding this comment.
I'd prefer for the whole test to be moved to output and every comment be accompanied by a About
|
|
||
| (** Universe minimization (collapse_sort_variables is true by default) *) | ||
| val minimize_universes : ?collapse_sort_variables:bool -> evar_map -> evar_map | ||
| val minimize_universes : ?collapse_sort_variables:bool -> ?to_type:bool -> evar_map -> evar_map |
There was a problem hiding this comment.
this API seems a bit awkward (to_type is ignored when collapse = false) but IDK what would be better
There was a problem hiding this comment.
Yeah, I agree. The only use I've seen for collapse is for template I think.
| val check_fix : ?evars:evar_handler -> ?elim_to:(Sorts.Quality.t -> Sorts.Quality.t -> bool) -> env -> fixpoint -> unit | ||
| val check_fix_pre_sorts : ?evars:evar_handler -> env -> fixpoint -> (Sorts.t * Sorts.t) option array | ||
| (** Checks fixpoint without checking sort elimination constraints. | ||
| Returns an array of possible sorts (None if Type in Type) *) |
There was a problem hiding this comment.
not sure what "possible sorts" means, is it "for each fixpoint the pair of structural argument's sort and output's sort"?
also since type in type isn't per-fixpoint it should be _ array option not _ option array
There was a problem hiding this comment.
I'll edit it, don't actually remember.
It stops us from putting |
How is it handled for universe levels ? Can we use the same approach? |
|
For univ levels unif and global are the same |
No, we shouldn't reflect the level implementation for sorts, and this is a hill I will die on. Monomorphic sorts are like constants, not unification variables. For sections I'm open to discussion, but I'm still quite convinced that allowing unification of section-local sorts is a bad idea. It's less crazy than unification of global sorts since there is an explicit scope, but still. As for the precise way to implement it, I don't know whether we should use QVars or globals, but this is mostly an implementation detail, both are observationally equivalent except maybe for some printing functions. |
Sounds fair.
I'm unfamiliar with how exactly the section context works wrt. to definitions in the section, so in order to clarify a little bit: With the local assumption
Why do you think it's bad ? |
|
Maybe we could have version of the PR where section variables do not support elaborating qualities (error not yet implemented if there are non empty implicit qualities / elim constraints to add to the section context), then I think it would be mergeable and we can think about sections separately. |
This PR depends on #21417 .
This PR adds elaboration of (implicit) qualities, which is enabled by setting the flag
Universe Polymorphismand disabling a new flagCollapse Sorts ToType.Associated RFC : rocq-prover/rfcs#111.
Examples
The test file
test-suite/success/sort_poly_elab.vhas been updated with more examples.Overlays