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

[notations] Warn about incompatible prefixes #19049

Merged
merged 3 commits into from
May 31, 2024

Conversation

proux01
Copy link
Contributor

@proux01 proux01 commented May 17, 2024

Add a warning for notations with incompatible common prefixes, for instance

Reserved Notation "#0 #1" (at level 30).
Reserved Notation "#0 #1 #2" (at level 40).
(* Warning: Notations "#0 #1" defined at level 30 and "#0 #1 #2" defined at level 40 have incompatible prefixes. One of them will likely not work. [prefix-incompatible-level,parsing,default] *)

Reserved Notation "#20 #21 x #3 y" (x at level 30, at level 50).
Reserved Notation "#20 #21 x #34" (x at level 40, at level 50).
(* Warning: Notations "#20 #21 _ #3 _" defined at level 50 with arguments constr at level 30 and "#20 #21 _ #34" defined at level 50 with arguments constr at level 40 have incompatible prefixes. One of them will likely not work. [prefix-incompatible-level,parsing,default] *)
  • 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.

@proux01 proux01 added needs: documentation Documentation was not added or updated. kind: user messages Improvement of error messages, new warnings, etc. part: notations The notation system. needs: changelog entry This should be documented in doc/changelog. labels May 17, 2024
@proux01 proux01 added this to the 8.20+rc1 milestone May 17, 2024
@proux01 proux01 requested review from a team as code owners May 17, 2024 16:51
@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 May 17, 2024
@proux01
Copy link
Contributor Author

proux01 commented May 17, 2024

@herbelin this is the first step toward the at existing level notation option we already discussed and that I'd like to add in a further PR.

@SkySkimmer
Copy link
Contributor

Maybe the warning should explain what the consequences of incompatible prefixes are (I don't know myself)

@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from e51214c to 4c25ad7 Compare May 18, 2024 10:39
@proux01 proux01 requested a review from a team as a code owner May 18, 2024 10:39
@proux01 proux01 removed needs: documentation Documentation was not added or updated. needs: changelog entry This should be documented in doc/changelog. labels May 18, 2024
@proux01
Copy link
Contributor Author

proux01 commented May 18, 2024

Indeed, done, as well as the documentation.

Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

wording and other suggestions

@jfehrle
Copy link
Member

jfehrle commented May 18, 2024

@SkySkimmer > Maybe the warning should explain what the consequences of incompatible prefixes are (I don't know myself)

IIUC the parser would only recognize one of the notations (and report syntax errors on the others), depending on the order in which the parser processes the rules defined for each notation (first one wins). Our parser really doesn't do lookahead--it looks at only the next symbol to make a parsing decision. Once the first token of a rule is accepted, the parser is unable to switch to a different rule. Not like LALR(k) parsers such as yacc and bison. (There is a hard coded exception to this behavior for command names.)

@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from 4c25ad7 to 63eb074 Compare May 19, 2024 19:03
@SkySkimmer
Copy link
Contributor

@jfehrle please don't guess

There is a hard coded exception to this behavior for command names

is certainly not true so I don't trust the rest of your post.

@proux01
Copy link
Contributor Author

proux01 commented May 20, 2024

@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 May 20, 2024
@jfehrle
Copy link
Member

jfehrle commented May 20, 2024

@SkySkimmer The exceptional code is Grammar.parser_of_token_list, which does multisymbol lookahead among prefixes containing only terminal symbols. Most relevant to command names, option names and tactic names and not used for most other nonterminals.

@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from 63eb074 to 2f4b54e Compare May 23, 2024 15:47
@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 May 23, 2024
@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from 2f4b54e to 02b7903 Compare May 24, 2024 11:48
@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 29, 2024
@herbelin herbelin self-assigned this May 30, 2024
@herbelin
Copy link
Member

Going to merge, after rebase and clean CI.

@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label May 30, 2024
@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from 02b7903 to c30fca0 Compare May 30, 2024 07:49
@coqbot-app coqbot-app bot removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. 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 May 30, 2024
For instance

Reserved Notation "#0 coq#1" (at level 30).
Reserved Notation "#0 coq#1 coq#2" (at level 40).
(* Warning: Notations "#0 coq#1" defined at level 30 and "#0 coq#1 coq#2" defined
at level 40 have incompatible prefixes.
[prefix-incompatible-level,parsing,default] *)

Reserved Notation "coq#20 coq#21 x coq#3 y" (x at level 30, at level 50).
Reserved Notation "coq#20 coq#21 x coq#34" (x at level 40, at level 50).
(* Warning: Notations "coq#20 coq#21 _ coq#3 _" defined at level 50 with
arguments constr at level 30 and "coq#20 coq#21 _ coq#34" defined at level 50
with arguments constr at level 40 have incompatible
prefixes. [prefix-incompatible-level,parsing,default] *)
@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label May 31, 2024
@proux01 proux01 force-pushed the warn_notation_incompatible_prefix branch from c30fca0 to bd48bda Compare May 31, 2024 08:12
@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 May 31, 2024
@proux01
Copy link
Contributor Author

proux01 commented May 31, 2024

@herbelin rebased, CI green

@herbelin
Copy link
Member

herbelin commented May 31, 2024

@coqbot merge now

1 similar comment
@herbelin
Copy link
Member

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 4750d53 into coq:master May 31, 2024
5 of 6 checks passed
@proux01 proux01 deleted the warn_notation_incompatible_prefix branch June 5, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: user messages Improvement of error messages, new warnings, etc. part: notations The notation system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants