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

Typecheck the result of term unquotation. #1098

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

ppedrot
Copy link
Collaborator

@ppedrot ppedrot commented Aug 30, 2024

In some cases, unquoting could generate terms that were ill-typed, typically when unquoting [I @hole] for some template polymorphic inductive type [I]. Similarly universe levels were not computed at all, which could have led to bizarre failures.

To ensure that the unquoted term makes sense at all, we simply perform a call to Typing before returning it. This has a non-trivial cost but since building the term is already linear, this should not hopefully matter that much in practice.

cc @tabareau

In some cases, unquoting could generate terms that were ill-typed, typically
when unquoting [I @hole] for some template polymorphic inductive type [I].
Similarly universe levels were not computed at all, which could have led
to bizarre failures.

To ensure that the unquoted term makes sense at all, we simply perform a
call to Typing before returning it. This has a non-trivial cost but since
building the term is already linear, this should not hopefully matter that
much in practice.
@ppedrot ppedrot force-pushed the typecheck-unquote branch from d809598 to 7bfa901 Compare August 30, 2024 15:35
@ppedrot
Copy link
Collaborator Author

ppedrot commented Aug 30, 2024

CI green and this works on my branch, so let's merge.

@ppedrot ppedrot merged commit 3067209 into MetaCoq:main Aug 30, 2024
5 checks passed
@ppedrot ppedrot deleted the typecheck-unquote branch August 30, 2024 17:05
@JasonGross
Copy link
Contributor

Does this fix #842 ? What about #853 ? #862 ?

@JasonGross
Copy link
Contributor

And maybe we can remove

(* Hack around https://github.com/MetaCoq/metacoq/issues/853 *)
Definition tmRetypeAroundMetaCoqBug853 (prefix : string) (t : typed_term) : TemplateMonad typed_term
:= let '{| my_projT1 := ty ; my_projT2 := v |} := t in
ty <- tmRetypeRelaxOnlyType prefix ty;;
v <- tmRetypeMagicRelaxOnlyType prefix ty v;;
ret {| my_projT1 := ty ; my_projT2 := v |}.

@ppedrot
Copy link
Collaborator Author

ppedrot commented Aug 30, 2024

This only fixes the Ltac binding for denote_term and does not do anything for the vernac commands or the monad evaluator. Typing unconditionally in the constr denoter uncovers bugs in some parts where the environment passed is not the correct one. That is, it produces unbound rels errors. This can be fixed but it requires more work and I wanted the least disruptive change to proceed with coq/coq#19228.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants