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

Clarifying the logic of levels in custom and constr entry rules #13025

Closed

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Sep 15, 2020

Kind: cleanup + small enhancements

Note: This PR was originally to fix #13018 and #12775 but those were finally fixed by #13073. This PR is thus now about small enhancements and a more principled code. The header has been updated.

Synchronous overlay for elpi at LPCIC/coq-elpi#184

@herbelin herbelin added kind: fix This fixes a bug or incorrect documentation. part: notations The notation system. part: printer The printing mechanism of Coq. labels Sep 15, 2020
@herbelin herbelin added this to the 8.13+beta1 milestone Sep 15, 2020
@herbelin herbelin requested review from a team as code owners September 15, 2020 16:28
herbelin added a commit to herbelin/coq-elpi that referenced this pull request Sep 16, 2020
…no level).

There is now a new specific type "notation_subentry_level" for
remembering the level or absence of explicit level of the variables of
a grammar rule.

The type "notation_entry_level" still exists but is only for
characterizing the level (and custom entry name) in which a rule lives.
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from 9334617 to f713a54 Compare September 16, 2020 06:01
@LasseBlaauwbroek
Copy link
Member

LasseBlaauwbroek commented Sep 21, 2020

I just ran into the same issue as #13018. I tested your branch, but it seems that there has been a regression. The following prints nicely in 8.12, but does not in master and this branch:

Declare Custom Entry S.
Notation "[ x ]" := x (x custom S at level 0).
Notation "[ x # .. # y ]" := (plus x .. (plus y 0) ..)
  (in custom S at level 0, x constr, y constr).

Check [[1 # 2 # 3 # 4]].
(* v8.12: [[1# 2# 3# 4]]
  master: 1 + (2 + (3 + [[4]])) *)

And now that I am here, there is another printing problem (let me know if I should open a separate issue):

Declare Custom Entry S.

Notation "[ x ]" := x (x custom S at level 0).
Notation "# x" := (id x) (in custom S at level 0, x bigint).
Notation "& x" := (id x) (in custom S at level 0, x global).

Check [# 4]. (* [& 4] *)
Check [& I]. (* [& I] *)

Two problems here:

  • Note that I have to wrap variables in id. This is needed because otherwise, Coq won't print them. This seems unnecessary in a custom entry.
  • When printing, Coq is not checking that a variable in a notation has the correct entry. Therefore, [# 4] gets printed as [& 4], which is not parsable.

@herbelin
Copy link
Member Author

I'd bet you're using CoqIDE! The first problem is related to option Printing Parentheses being on by default, thus bypassing the use of the recursive notation. See #13067 for a fix.

The second issue is known: there is no support for bigint and numeral simultaneously, sorry (this should eventually be addressed by #11561).

Thanks for your feedback.

@LasseBlaauwbroek
Copy link
Member

Ah, I see, that explains it. I'll change my settings. Thanks.

@ejgallego
Copy link
Member

cc @maximedenes as I understand he's familiar with the code.

Would be good not to delay this a bit as otherwise 8.12 will have the fix but not master.

@maximedenes maximedenes self-assigned this Sep 23, 2020
@Zimmi48 Zimmi48 added needs: changelog entry This should be documented in doc/changelog. priority: high The priority for inclusion in the next release is high. labels Sep 23, 2020
@Zimmi48

This comment has been minimized.

@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 Sep 23, 2020
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from f713a54 to edd94e1 Compare September 24, 2020 15:57
@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 Sep 24, 2020
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from edd94e1 to ec56e36 Compare September 24, 2020 15:58
herbelin added a commit to herbelin/github-coq that referenced this pull request Sep 24, 2020
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from ec56e36 to 86ffca5 Compare September 24, 2020 16:02
@herbelin herbelin removed the needs: changelog entry This should be documented in doc/changelog. label Sep 24, 2020
@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 Oct 3, 2020
herbelin added a commit to herbelin/github-coq that referenced this pull request Oct 4, 2020
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from 86ffca5 to 5a7572b Compare October 4, 2020 13:19
@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 Oct 4, 2020
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from f88feec to 078d6d1 Compare July 3, 2021 09:27
herbelin added a commit to herbelin/coq-elpi that referenced this pull request Jul 3, 2021
…notations).

There is now a new specific type "notation_subentry_level" for
remembering the level or absence of explicit level of the variables of
a grammar rule.

The type "notation_entry_level" still exists but is only for
characterizing the level (and custom entry name) in which a rule lives.
@herbelin herbelin changed the title Fix printing bugs in custom entry rules with no explicit level Clarifying the logic of levels in custom and constr entry rules Jul 3, 2021
@herbelin
Copy link
Member Author

herbelin commented Jul 3, 2021

I updated the title and the header to adjust to the new status of this PR. It is now going out of the list of PRs suspended for a while, that were needing an update and an extra rebase.

@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 Sep 29, 2021
@SkySkimmer SkySkimmer added the needs: test-suite update Test case should be added to / updated in the test-suite. label Oct 22, 2021
@SkySkimmer
Copy link
Contributor

IIUC this is just missing review/assignee.
@maximedenes if you have more time now can you self-assign again?

informations about the subentries: we remember whether we are at any
level, at the next level or at the self level.

Incidentally, we also take into account option Printing Parentheses in
custom entries.

This is a more principled fix to coq#12775/coq#13018 than coq#13073 (printing
bugs in custom entry rules with no explicit level).
This insists on the syntactic structure of the type rather than on its
semantic origin, since, after all, subentries are promoted as entries
in the recursive printing loop.
We hard-wire a coercion in constr from level 0 to the top level.
@herbelin herbelin force-pushed the master+fix-printing-custom-no-level branch from 078d6d1 to e396bc1 Compare October 22, 2021 19:19
@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 Oct 22, 2021
herbelin added a commit to herbelin/coq-elpi that referenced this pull request Oct 22, 2021
…notations).

There is now a new specific type "notation_subentry_level" for
remembering the level or absence of explicit level of the variables of
a grammar rule.

The type "notation_entry_level" still exists but is only for
characterizing the level (and custom entry name) in which a rule lives.
@SkySkimmer SkySkimmer removed this from the 8.15+rc1 milestone Nov 8, 2021
@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 Dec 14, 2021
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jan 13, 2022

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 Jan 13, 2022
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Feb 14, 2022

This PR was not rebased after 30 days despite the warning, it is now closed.

@coqbot-app coqbot-app bot closed this Feb 14, 2022
herbelin added a commit to herbelin/coq-elpi that referenced this pull request Jan 14, 2023
…notations).

There is now a new specific type "notation_subentry_level" for
remembering the level or absence of explicit level of the variables of
a grammar rule.

The type "notation_entry_level" still exists but is only for
characterizing the level (and custom entry name) in which a rule lives.
@herbelin
Copy link
Member Author

Reopened in #17117.

coqbot-app bot added a commit that referenced this pull request Jun 1, 2023
…entry rules (reopening of #13025)

Reviewed-by: proux01
Ack-by: SkySkimmer
Co-authored-by: proux01 <proux01@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. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. needs: test-suite update Test case should be added to / updated in the test-suite. part: notations The notation system. part: printer The printing mechanism of Coq. stale This PR will be closed unless it is rebased.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom entry without explicit level prints as constr instead
7 participants