-
Notifications
You must be signed in to change notification settings - Fork 701
Use levels for associativity in refman #21360
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
base: master
Are you sure you want to change the base?
Conversation
|
Please provide a useful succinct description for the PR that explains concretely what is being changed. The PRs you link to are too verbose and rambling for a reviewer to quickly grasp what the change does. |
|
Also, how about a change log entry? Users may want to know that the documented grammar has been corrected (maybe). |
|
I've edited the description. Not sure it's worth a log entry. |
|
No need for a changelog entry, docgram is an internal tool. |
They are included in the reference manual when the level is not empty. Look at the grammar for You could add nonterminals for all the empty levels. It would add some clutter. Including SELF or NEXT in the doc would be a mistake, would make it harder to read. |
Can you provide a link? I don't see this. Maybe you're referring to If it were the case it would be quite illogical. The |
https://rocq-prover.org/doc/V9.1.0/refman/language/core/basic.html#grammar-token-term 200, 99, 90, 8 and 1 are not currently shown for If you're a reader trying to understand the syntax, the empty levels are often irrelevant. If you're defining a notation, you do want to know them. So it does seem we should show the empty levels that appear in the mlg. My recollection is that all levels have to be defined in the initial mlg definition of the nonterminal definition, e.g. you can't add "97" as a level for |
|
200 is used as the highest level and 8 is used for "constr" but IDK why we declare 99 and 90 in the mlg.
It's never been true AFAIK |
Follows rocq-prover#21126. Reverts rocq-prover#21071. Fixes rocq-prover#21029 and rocq-prover#21072.
|
It may allow defining new levels but IINM the productions won't be processed appear in the order you expect. You do know that the following is accepted in the mlg (just tried it)? Precedence isn't limited to numeric values, nor do they have to appear in numeric order in the mlg. My recollection is that extensions can match existing labels but new labels won't be inserted where you expect. I think "NEXT" is relative to the order in which the productions appear, rather than by ordering on the precedence string. But it's been several years since I looked at these details, I may be confused. |
|
There's some AFTER / BEFORE syntax to insert levels where desired |
Thanks! I misread the "not empty" as "empty" and only searched for exact "foo4 := foo3" patterns... 😅 So indeed, the refman already takes the approach of elaborating precedence as new non-terminals. So it would be weird to talk about Other alternatives to the
But this problem is anyway not specific to NEXT on the left. We already have it with NEXT on the right. It's a problem with NEXT. That should be handled in a different PR. |
It should be done in English in one place in the manual, not in the grammar syntax. We should keep the most common use case (readers trying to understand basic syntax) simple using a familiar way of showing grammars.
I believe that's only true by convention in assigning labels and ordering productions (see my comment just above). |
It would be less confusing if these various changes were in a single PR rather than, what is it now, 3 or 4 PRs. They're quite technical and the overall change visible to users is what counts. |
I would also prefer this.
Yes, but this is what Rocq exposes to users. The fact that Gramlib uses an ordered list of strings is an implementation detail that doesn't show to users (at least until #21244 where level
I agree, but #21071 was rushed, so now it's too late to have a proper self-contained PR. And I believe some discussions regarding these questions (like whether associativity should be a rule thing instead of a level thing) won't be resolved in a matter of weeks or even months, so it would not be reasonable to delay simple fixes for the sake of major changes. |
I think you're being too hasty. That indicates that |
Ah you didn't mean in the refman. Indeed, there's a So it seems like docgram is already considering NEXT at level 100 of |
Here, where we ask to SPLICE |
|
Ok, I've pushed a commit that fixes this |
I did mean in the refman. Focus on what users should see in the manual first, not on how it happens. It feels like you're rushing to complete something tonight. Maybe take a break until tomorrow. |
Then my
That's exactly what this PR is doing, but not for everything. This PR makes users see associativity using levels (in particular left-associativity). Not all issues in the refman must be fixed in this PR.
No. But I'm making sure this gets merged before the next release of the refman. Otherwise #21071 will get visible to users which is pretty bad. Both #21071 and this PR should be in the same refman diff visible to users. |


This PR changes how SELF on the left of a rule is rendered in the refman.
Before #21071, it was unconditionally rendered as NEXT. After #21071, it was unconditionally rendered as SELF. With this PR, the associativity of the level is taken into account. See examples below.
This PR does not change how SELF on the right of a rule is rendered, since it already takes the associativity of the level into account.
For example
SELF op SELFis rendered as (where SELF is rendered asfoo4and NEXT asfoo3):foo3 op foo3foo4 op foo3foo4 op foo3foo3 op foo4foo4 op foo4foo3 op foo4foo3 op foo3foo4 op foo3foo3 op foo3Note that the refman currently (V9.1.0) renders like before #21071. So for users, the effective change is from before #21071 to this PR skipping the intermediate step. This means that only the rendering of left-associative rules is changed from
foo3 op foo3tofoo4 op foo3(which affects very few rules, essentiallyltac_expr + ltac_exprandltac_expr || ltac_exprfor Ltac1 and a few others for Ltac2). Most users probably never noticed this very minor issue, so it is probably not worth a change log entry. But I'm happy to add one if needed.Follows #21126. Reverts #21071. Fixes #21029 and #21072.