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

Add coercion hook #17794

Merged
merged 1 commit into from Jul 5, 2023
Merged

Add coercion hook #17794

merged 1 commit into from Jul 5, 2023

Conversation

proux01
Copy link
Contributor

@proux01 proux01 commented Jun 30, 2023

This adds a hook in pretyping/coercion.ml to enable programming more elaborate coercions in an external metalanguage (Ltac, Ltac2, Elpi,...). This was developped during CUDW 2023 along with a coq-elpi interface: https://github.com/proux01/coq-elpi/tree/coercion_hook

Fixes / closes #????

  • Added / updated test-suite.
  • Added changelog.
  • Added / updated documentation.
    • Documented any new / changed user messages.
    • Updated documented syntax by running make doc_gram_rsts.
  • Opened overlay pull requests.

@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 Jun 30, 2023
@proux01 proux01 added kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: coercions The coercion mechanism. labels Jun 30, 2023
@proux01 proux01 added this to the 8.18+rc1 milestone Jun 30, 2023
pretyping/coercion.ml Outdated Show resolved Hide resolved
@proux01 proux01 marked this pull request as ready for review June 30, 2023 12:29
@proux01 proux01 requested a review from a team as a code owner June 30, 2023 12:29
pretyping/coercion.ml Outdated Show resolved Hide resolved
Copy link
Member

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be made safe wrt backtracking

@gares
Copy link
Member

gares commented Jun 30, 2023

Needs to be made safe wrt backtracking

I don't think so. This pr is a twin of

#16654

@ejgallego
Copy link
Member

Needs to be made safe wrt backtracking

I don't think so. This pr is a twin of

#16654

I understood that that PR was only for "experimentation", but see my review.

I think that both that PR and this PR need amending as they are breaking a critical use case, breaking unrelated files / backtrack of the plugin require. Both of the approaches hinted above should work (indirection or ~local:true)

pretyping/coercion.ml Outdated Show resolved Hide resolved
@gares
Copy link
Member

gares commented Jul 2, 2023

I think you want to error if the same hook is declared twice.

pretyping/coercion.ml Outdated Show resolved Hide resolved
@proux01
Copy link
Contributor Author

proux01 commented Jul 3, 2023

@ejgallego your request should now be fullfilled, could you please update your review if you agree this is indeed the case?

@ejgallego
Copy link
Member

@ejgallego your request should now be fullfilled, could you please update your review if you agree this is indeed the case?

Thanks @proux01 , I'm happy with any mechanism that works with the current VernacState approach, so loading a plugin in a document doesn't leak to other unrelated documents. If @SkySkimmer and/or @ppedrot are OK with the approach here please feel free to dismiss my review.

[I would prefer if we had an API that was actually always safe instead of placing the burden in the plugin writer, but as long as things work properly on backtrack I don't mind so much here. But indeed, if we are gonna start making these parts of Coq extensible IMHO we need a principled approach]

@gares
Copy link
Member

gares commented Jul 3, 2023

This PR seems to validate that property.

@ejgallego
Copy link
Member

This PR seems to validate that property.

Yes you are correct I somehow misread it. I think we have some similar code already in the system so at some point it could be nice to refactor this pattern into something that could be shared.

@gares
Copy link
Member

gares commented Jul 3, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed 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 3, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 3, 2023

I think we have some similar code already in the system

Any idea where? I thought the same but couldn't find any example.

@ejgallego
Copy link
Member

I think we have some similar code already in the system

Any idea where? I thought the same but couldn't find any example.

We don't need to solve this now, but I think a couple of places where we do something similar is in the primitive notations and grammar extension code, but @SkySkimmer @herbelin know better.

@SkySkimmer
Copy link
Contributor

Those have a similar principle of indirection through strings, but they're different enough that it's not clearly factorizable IMO

@ejgallego
Copy link
Member

Those have a similar principle of indirection through strings, but they're different enough that it's not clearly factorizable IMO

Too bad :( For #17807 we want to copy this code but I'm unsure if that's enought as to make this "Summary-friendly" Hook API available in as Summary.Hook.Make(F)

@gares gares self-assigned this Jul 5, 2023
@gares
Copy link
Member

gares commented Jul 5, 2023

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 44a5a6c into coq:master Jul 5, 2023
7 checks passed
@coqbot-app coqbot-app bot added this to Request 8.18+rc1 inclusion in Coq 8.18 Jul 5, 2023
@proux01 proux01 deleted the coercion_hook branch July 5, 2023 09:05
gares added a commit to gares/coq that referenced this pull request Jul 7, 2023
@coqbot-app coqbot-app bot moved this from Request 8.18+rc1 inclusion to Shipped in 8.18+rc1 in Coq 8.18 Jul 8, 2023
proux01 added a commit to proux01/coq that referenced this pull request Jul 17, 2023
When doing the following since coq#17794

```Coq
Declare ML Module "module-declaring-a-coercion-hook".
(* <backtrack> *)
(* <reexecute previous command> *)
```

Coq complained about "Hook already registered". To avoid that,
this commit puts the set of registered hooks under a summary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: coercions The coercion mechanism.
Projects
Coq 8.18
Shipped in 8.18+rc1
Development

Successfully merging this pull request may close these issues.

None yet

5 participants