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

Fixes #10739: anomaly with Extraction within a module #17344

Merged

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Mar 5, 2023

When inside a module, module type or functor, we do as if we had first closed the module, module type or functor before asking to extract.

This implies a small change of behavior of Recursive Extraction in a module when none of the extracted term was referring to the outside of the module: the result is now encapsulated in the given module (this changes e.g. the test for #14100). On the other side, the new behavior makes Recursive Extraction also working in the presence of references outside the module.

Fixes #10739

Incidentally, it now also supports the example in #10796.

  • Added / updated test-suite.
  • Added changelog.

@herbelin herbelin added kind: fix This fixes a bug or incorrect documentation. part: extraction The extraction mechanism. request: full CI Use this label when you want your next push to trigger a full CI. labels Mar 5, 2023
@herbelin herbelin added this to the 8.18+rc1 milestone Mar 5, 2023
@herbelin herbelin requested review from a team as code owners March 5, 2023 18:28
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 5, 2023
@SkySkimmer
Copy link
Contributor

anomaly with

Require Extraction.

Module Type T.
  Definition foo := 0.
  Extraction foo.

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 17, 2023
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 17, 2023
@herbelin herbelin force-pushed the master+fixes10739-extraction-inside-a-module branch from a43702d to 475d248 Compare March 17, 2023 17:38
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 17, 2023
@herbelin
Copy link
Member Author

@SkySkimmer: well spotted, thanks, fixed.

@SkySkimmer
Copy link
Contributor

What does it do on #17395 ?

@herbelin
Copy link
Member Author

Sorry, my previous response was not properly answering the question (then deleting it). See #17395 for an answer. (In particular, there is no relation between #17395 and extracting within a module type, iianm.)

@SkySkimmer SkySkimmer removed the request for review from a team March 28, 2023 10:34
@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 May 24, 2023
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jun 23, 2023

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Jun 23, 2023
herbelin added a commit to herbelin/github-coq that referenced this pull request Jun 26, 2023
@herbelin herbelin force-pushed the master+fixes10739-extraction-inside-a-module branch from 475d248 to c3233a6 Compare June 26, 2023 14:32
@coqbot-app coqbot-app bot added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. and removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. stale This PR will be closed unless it is rebased. labels Jun 26, 2023
@herbelin
Copy link
Member Author

herbelin commented Jul 1, 2023

As far as I remember, this was ready. Maybe @ppedrot would have some time to look at it. Basically, when inside a module, module type or functor, the PR extracts as if we had first closed the module, and the code for warning when extracting from an open module or functor, or failing when extracting from an open module type are removed.

@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 1, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make this an output test to make sure it doesn't produce garbage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's worth. Done.

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 2, 2023

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

✔️ Corresponding job for the base commit 6f1588d 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-geocoq
  • You can also pass me a specific list of targets to minimize as arguments.

@herbelin herbelin force-pushed the master+fixes10739-extraction-inside-a-module branch from c3233a6 to 6899560 Compare July 2, 2023 07:50
@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 Jul 2, 2023
@herbelin
Copy link
Member Author

herbelin 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
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 3, 2023

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@herbelin
Copy link
Member Author

herbelin commented Jul 4, 2023

Looks ready.

@gares
Copy link
Member

gares commented Jul 5, 2023

@SkySkimmer would you mind reviewing and eventually merging this one? It seems ready for 8.18

Copy link
Contributor

@SkySkimmer SkySkimmer left a comment

Choose a reason for hiding this comment

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

Seems to work

@SkySkimmer SkySkimmer self-assigned this Jul 5, 2023
@SkySkimmer
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 7912460 into coq:master Jul 5, 2023
8 checks passed
@coqbot-app coqbot-app bot added this to Request 8.18+rc1 inclusion in Coq 8.18 Jul 5, 2023
@@ -4,6 +4,6 @@ Module Example.

Definition a : nat := 0.

Fail Separate Extraction a.
Separate Extraction a.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is generating files like test-suite/datatypes.ml
Should we gitignore them or is there something better we can do?

Copy link
Member Author

Choose a reason for hiding this comment

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

With #16126 or #17392, it could be redirected to say /tmp... ?

Alternatively, a Separate Extraction TestCompile command could be provided... ?

gares pushed a commit to gares/coq that referenced this pull request Jul 7, 2023
(cherry picked from commit 6899560)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. part: extraction The extraction mechanism.
Projects
Coq 8.18
Shipped in 8.18+rc1
Development

Successfully merging this pull request may close these issues.

Extraction inside a module
3 participants