Skip to content

[vernac] expose API to elaborate declarations #15872

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

Merged
merged 8 commits into from
Apr 27, 2022

Conversation

gares
Copy link
Member

@gares gares commented Mar 30, 2022

In the context of Elpi I want to let coq elaborate an inductive declaration (up to the constr level) without necessarily adding it to the environment, the same for records and context.
This PR exports the first half of do_mutual_inductive (the part which is side effect free) and do_record and do_context.

Fixes #14737

TODO:

  • overlay for coq-elpi using the new API

@gares gares requested a review from a team as a code owner March 30, 2022 14:03
@gares gares added the needs: overlay This is breaking external developments we track in CI. label Mar 30, 2022
@gares gares added this to the 8.16+rc1 milestone Mar 30, 2022
@ejgallego ejgallego self-assigned this Mar 30, 2022
@ejgallego
Copy link
Member

Modulo the two nits this looks good to me.

@ejgallego ejgallego added kind: internal API, ML documentation... part: vernac High level command interpretation. labels Mar 30, 2022
@gares gares force-pushed the interp-inductive branch from 08f72bf to 443bc65 Compare March 30, 2022 18:37
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.

LGTM, @gares I assume you want to wait for the overlay, so I will wait to merge.

@ejgallego ejgallego self-requested a review March 31, 2022 15:20
@gares
Copy link
Member Author

gares commented Mar 31, 2022

I'm not sure the code the last commit exposes is in the right place. Happy to move it elsewhere, just tell me where.
I did hoist ".glob handling" since that part has nothing to do with pre-processing (which clients may want to call without necessarily emitting glob data).

@gares gares force-pushed the interp-inductive branch 2 times, most recently from 3e1794e to 003fb0c Compare April 1, 2022 09:31
@gares gares force-pushed the interp-inductive branch 2 times, most recently from 6d3dcd2 to d1d74f5 Compare April 1, 2022 15:05
@gares
Copy link
Member Author

gares commented Apr 1, 2022

So, after more work on the overlay (which is not ready yet, but is converging), the exposed API is:

  • pre processing of the data carried by VernacInductive, which gives two distinct ASTs: inductive or record
  • elaboration of inductive
  • elaboration of record

The latter one has a very nasty type, suggestions are welcome.

The overall feeling is that having just one VernacInductive node is wrong, since one later has to unshare options/flags, and finally take two very different code paths. But that would be another PR (changing the grammar). The funny thing is that the grammar entries for records and general inductive shares almost nothing already, and the two productions are easy for an LL parser, since it is the first word that choose (Record & co, vs Inductive & co). So I really don't get why the code is done this way.

@gares gares force-pushed the interp-inductive branch from d1d74f5 to f10383e Compare April 1, 2022 15:07
@ejgallego
Copy link
Member

Thanks @gares will have a deeper look when the overlay is ready, but indeed I agree with your comments, we can do a lot of improvements in this area. For sure this PR will improve things already.

@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 Apr 2, 2022
@gares gares force-pushed the interp-inductive branch from f10383e to 4de4597 Compare April 3, 2022 09:07
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Apr 3, 2022
@gares
Copy link
Member Author

gares commented Apr 5, 2022

I'm almost done with the overlay, sorry for taking so long, but it was not trivial. So far the API is sufficient, but I'm not done.

@ejgallego
Copy link
Member

I'm almost done with the overlay, sorry for taking so long, but it was not trivial. So far the API is sufficient, but I'm not done.

No worries at all take your time!

@gares gares changed the title [vernac] expose API to elaborate an inductive declaration [vernac] expose API to elaborate declarations Apr 7, 2022
@gares gares force-pushed the interp-inductive branch 2 times, most recently from 23ea0b2 to f1e2d0c Compare April 7, 2022 11:28
@coqbot-app

This comment was marked as outdated.

@gares gares force-pushed the interp-inductive branch from 9647493 to f2ad35d Compare April 22, 2022 18:26
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 22, 2022

🔴 CI failure at commit f2ad35d without any failure in the test-suite

✔️ Corresponding job for the base commit f9c9ee5 succeeded

❔ Ask me to try to extract a minimal test case that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following target: ci-analysis
  • You can also pass me a specific list of targets to minimize as arguments.

1 similar comment
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 23, 2022

🔴 CI failure at commit f2ad35d without any failure in the test-suite

✔️ Corresponding job for the base commit f9c9ee5 succeeded

❔ Ask me to try to extract a minimal test case that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following target: ci-analysis
  • You can also pass me a specific list of targets to minimize as arguments.

@gares
Copy link
Member Author

gares commented Apr 23, 2022

fiat failure seems unrelated. I can't say about analysis.

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 25, 2022

🔴 CI failure at commit f2ad35d without any failure in the test-suite

✔️ Corresponding job for the base commit f9c9ee5 succeeded

❔ Ask me to try to extract a minimal test case that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following target: ci-analysis
  • You can also pass me a specific list of targets to minimize as arguments.

@gares gares force-pushed the interp-inductive branch from f2ad35d to 6294bf6 Compare April 26, 2022 08:27
@gares
Copy link
Member Author

gares commented Apr 26, 2022

CI was green but for fiat which is fixed in master, so I did rebase to get a fully green CI.
After that, please merge.

@gares
Copy link
Member Author

gares commented Apr 26, 2022

@SkySkimmer would you mind taking care of this? I'd like to first merge this elpi overlay, and then prepare the one for #15946

@SkySkimmer
Copy link
Contributor

@ejgallego is still assigned though

@Alizter
Copy link
Contributor

Alizter commented Apr 26, 2022

@SkySkimmer I think it is OK to take over in this case. The assignment was a month ago, so I don't think we can reasonably expect it to be looked at.

@SkySkimmer
Copy link
Contributor

Last activity was just before holidays though

@gares
Copy link
Member Author

gares commented Apr 26, 2022

I think @ejgallego was OK with the PR both in principle and with the current code, I did only add a few comments after his review.

@ejgallego
Copy link
Member

ejgallego commented Apr 26, 2022

The assignment was a month ago, so I don't think we can reasonably expect it to be looked at.

I am not sure why, my last activity was 11 days ago, then I went on holidays [and I did a review but the PR was not ready, so really I could only have reacted on the weekend] . So yes, the assignement date is one month, but how it is relevant?

That being said, the PR looks good to me, so go ahead and take over. A think to keep in mind tho is the conflict with #15877 , that one I did have a look but I didn't manage to understand the code. I'd suggest pinging @proux01 in case he'd like to comment anything about this one.

@gares
Copy link
Member Author

gares commented Apr 27, 2022

If you are back from VAC you can also merge it yourself. I'd like to merge overlay, since I then have to make another one which will generate conflicts, so a quick merge would be appreciated

@SkySkimmer SkySkimmer self-assigned this Apr 27, 2022
@SkySkimmer
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 455d176 into rocq-prover:master Apr 27, 2022
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 27, 2022

@SkySkimmer: Please take care of the following overlays:

  • 15872-gares-interp-inductive.sh

@gares gares deleted the interp-inductive branch April 27, 2022 13:20
@gares
Copy link
Member Author

gares commented Apr 27, 2022

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal API, ML documentation... part: vernac High level command interpretation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package configuration of inductives in vernac/record.ml
4 participants