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

Fix #16288: wrongly self-referencing extraction of primitive projections in functors #17321

Merged

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Mar 1, 2023

Fixes #16288 (usual problem of imperative tables not being updated in functors).

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

@herbelin herbelin added kind: fix This fixes a bug or incorrect documentation. part: extraction The extraction mechanism. labels Mar 1, 2023
@herbelin herbelin added this to the 8.17.0 milestone Mar 1, 2023
@herbelin herbelin requested a review from a team as a code owner March 1, 2023 20:10
@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 Mar 1, 2023
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 2, 2023
Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 2, 2023
Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 2, 2023
…eld names.

Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)

Note that Haskell and Scheme do not use records and it is ok on their
side.
@herbelin
Copy link
Member Author

herbelin commented Mar 2, 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 Mar 2, 2023
@herbelin
Copy link
Member Author

herbelin commented Mar 2, 2023

The failure in itauto is because the PR now extracts the constant form of a primitive projection only if the Coq code refers to it. Previously, a projection in the primitive form was elaborated in the projection in constant form, so, even if the Coq code referred only to the primitive form (Proj), the constant form (Const) was inferred detected as used and therefore extracted. Example:

Set Primitive Projections.
Module Import A.
Record t := {a:bool;b:bool}.
End A.
Definition c x := x.(a).
Require Import Extraction.
Recursive Extraction c.
  • before:
    type bool = | True | False
    module A = struct type t = { a : bool; b : bool } (** val a : t -> bool **) let a t0 = t0.a end
    (** val c : A.t -> bool **) let c x = x.A.a
  • after:
    type bool = | True | False
    module A = struct type t = { a : bool; b : bool } end
    (** val c : A.t -> bool **) let c x = x.A.a

@coq/extraction-maintainers: that would probably not be too much a problem to say that when a projection in primitive form is used, we effectively extract the projection in constant form (even if strictly speaking not needed). In case you have an opinion, would I implement this for optimal compatibility, or do you think we can as well renounce to it (then, third-party developments mixing extracted Coq code with private OCaml code, such as itauto, will have to use in OCaml the x.a notation rather than the applicative notation a x)?

@SkySkimmer
Copy link
Contributor

I think the new behaviour is correct, it means that if using the constant form is desired then the user must say Extraction a instead of getting t by side effect extracting something that does not use it.

@herbelin
Copy link
Member Author

herbelin commented Mar 3, 2023

I should also have said that the new behavior creates a difference between "primitive" and non "primitive" projections, which maybe is weakening the flexibility in using or not the "Primitive Projections" flag.

So, maybe, the extraction of the constant form should be removed also in the non primitive case, on the basis that in the extracted OCaml code of non primitive projections, only the dot notation is used too (except in a functor, where the same limitation as in the current report applies: the table telling that a constant is a (non primitive) projections is not active in a functor, so a non primitive projection is printed applicatively in a functor).

@herbelin herbelin 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 Mar 3, 2023
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 4, 2023
…eld names.

Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)

Note that Haskell and Scheme do not use records and it is ok on their
side.
herbelin added a commit to herbelin/github-coq that referenced this pull request Mar 4, 2023
…eld names.

Fixes coq#3846
Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)

Note that Haskell and Scheme do not use records and it is ok on their
side.
@herbelin
Copy link
Member Author

herbelin commented Mar 5, 2023

After more digging, I could summarize the situation in issue #17337. In particular, prior to this PR, there is a difference of behavior wrt inlining between being at toplevel (where inlined functions are pruned) or in a module (where inlined functions, especially projections, are not pruned). I made #17341 to address this and this now shows the itauto failure even without this PR. In particular, I'm now leaning towards changing itauto.

@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 6, 2023
@herbelin herbelin force-pushed the master+fix16288-extraction-prim-field-functor branch from 9850095 to e829f33 Compare March 6, 2023 17:08
@coqbot-app coqbot-app bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Mar 6, 2023
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Mar 6, 2023
@herbelin herbelin force-pushed the master+fix16288-extraction-prim-field-functor branch from e829f33 to 8747ab0 Compare March 6, 2023 17:14
@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 6, 2023
@Zimmi48 Zimmi48 removed this from the 8.17.0 milestone Mar 9, 2023
Zimmi48 pushed a commit to Zimmi48/coq that referenced this pull request Jun 19, 2023
…eld names.

Fixes coq#3846
Fixes coq#14843
Fixes coq#16677
Fixes coq#12813

When dealing with collisions by means of encapsulation in a Coq_XX
module, a field name should be bound to the name of the corresponding
inductive type declaration.

We use Table.repr_of_r to do that, but early enough to have the
relevant information on how to bind a field to the name of the
inductive that declares it. (In particular, we cannot rely on the
global tables in structures.ml because they are not valid when inside
a functor, see coq#17321.)

Note that Haskell and Scheme do not use records and it is ok on their
side.

(cherry picked from commit c19b2cd)
@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 Nov 6, 2023
Copy link
Contributor

coqbot-app bot commented Dec 6, 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 Dec 6, 2023
@herbelin herbelin force-pushed the master+fix16288-extraction-prim-field-functor branch from 8747ab0 to afce393 Compare December 8, 2023 11:46
@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 Dec 8, 2023
- The code for extracting a primitive projection was referencing the
  constant form of the projection.
- The code for compiling the constant form of the projection was doing
  a call to the global table of projections which is however not fed in
  functors.
- The declaration of constants bound to a projection had a small size
  and were then inlined.

Instead of relying on the table of projections, we use the
"match"-expanded form of the projections which has the advantage of be
valid in all target languages. In particular, in OCaml extraction,
this is anyway reprinted as a field.

Note: we need to move the fake_match_projection function a bit
earlier.
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Dec 10, 2023
@herbelin herbelin force-pushed the master+fix16288-extraction-prim-field-functor branch from afce393 to acb2fb1 Compare December 10, 2023 18:36
@coqbot-app coqbot-app bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Dec 10, 2023
@herbelin
Copy link
Member Author

Rerunning CI, since it is a long time ago

@coqbot run full ci

@herbelin herbelin added this to the 8.20+rc1 milestone Feb 13, 2024
@herbelin
Copy link
Member Author

This is an old PR looking for an assigne. @ppedrot, would you have some time by chance?

@ppedrot
Copy link
Member

ppedrot commented Feb 13, 2024

This is a bit outside of my expertise area... There are weird spec considerations as well. I'll see if I can grok this.

Copy link
Member

@ppedrot ppedrot left a comment

Choose a reason for hiding this comment

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

I'm a bit puzzled about why you simply don't write the miniML matching directly instead of getting it by the composition of fake_match_projection + extract_term, but I guess it's a bit more uniform this way.

@ppedrot ppedrot self-assigned this Feb 13, 2024
@ppedrot
Copy link
Member

ppedrot commented Feb 13, 2024

Merging soon if nobody complains.

@herbelin
Copy link
Member Author

Thanks!

@ppedrot
Copy link
Member

ppedrot commented Feb 23, 2024

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 6208d8c into coq:master Feb 23, 2024
7 checks passed
SkySkimmer added a commit to SkySkimmer/coq that referenced this pull request Feb 27, 2024
coqbot-app bot added a commit that referenced this pull request Feb 27, 2024
Reviewed-by: ejgallego
Co-authored-by: ejgallego <ejgallego@users.noreply.github.com>
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
None yet
Development

Successfully merging this pull request may close these issues.

Extraction generates invalid code for primitive records in functors
4 participants