Skip to content

Conversation

@gares
Copy link
Member

@gares gares commented Mar 19, 2021

Turn the error about an inconsistent inference of scopes into a warning.
We enable Notation abbr X Y Z := t (X, Z in scope foo_scope) to force a scope and silence the warning.

Internally Abbreviations and Notations accept the same modifiers, but "in scope" is for now only supported by Abbreviations. At the grammar level I've shared the whole syntax of modifiers, even if abbreviations only support "in scope" and "only parsing" (only parsing was duplicate). It seems the right thing to do if we consider supporting "in scope" also in regular Notations, or conversely any other modifier on Abbreviations.

Overlay PRs:
rocq-community/paramcoq#65
mattam82/Coq-Equations#355
LPCIC/coq-elpi#224

Fix #13957

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Mar 19, 2021

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

@gares gares force-pushed the syndef-principal-scope branch 4 times, most recently from f1a78e8 to d5ddc3e Compare March 19, 2021 18:21
@gares
Copy link
Member Author

gares commented Mar 19, 2021

OK, I think it is ready for review. I'm not very happy with my first commit, the old code was a spaghetti of control flow, but it is not the case that the new one is much better.

@gares gares marked this pull request as ready for review March 19, 2021 20:31
@gares gares requested review from a team as code owners March 19, 2021 20:31
gares added a commit to gares/Coq-Equations that referenced this pull request Mar 22, 2021
gares added a commit to gares/paramcoq that referenced this pull request Mar 22, 2021
Copy link
Member

@herbelin herbelin left a comment

Choose a reason for hiding this comment

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

No strong opinion. The syntax Notation abbr X%foo := ... looks rather intuitive and I'd be rather happy to have this feature. Don't know if others have more to comment.

No strong opinion either on what to do in the presence of a clash, but if a warning, it should tell which is finally chosen.

One could wonder what to do with Notation "..." then?

Notation "2⨉ y%nat" := (Nat.add y y).

or

Notation "2⨉ y" := (Nat.add y y) (y in scope nat_scope).

The former may (rarely?) break existing notations? The latter would suggest that the syntax for Notation abbr should also be:

Notation abbr X := ... (X in scope foo_scope).

Both are ok to me, but it is might be worth to anticipate a consistency.

@gares
Copy link
Member Author

gares commented Mar 22, 2021

Notation abbr X := ... (X in scope foo_scope).

This is a good suggestion, I may give it a try.

What about:

[ ] decide what to do when scopes are inconsistent, right now we keep the one we found first

I don't know enough scopes to know if other options are reasonable, eg concatenating the lists

@herbelin
Copy link
Member

[ ] decide what to do when scopes are inconsistent, right now we keep the one we found first

I don't know enough scopes to know if other options are reasonable, eg concatenating the lists

Since a list is ordered, there would still be an arbitrariness when the notation exists in both scopes, however, concatenating the lists could indeed be done. Some work would be needed too, because there is currently an optional "temporary" scope and it would have to be changed everywhere in constrintern.ml and constrextern.ml to a list of "temporary" scopes.

@gares
Copy link
Member Author

gares commented Mar 22, 2021

Oh, I see, then it seems too hard. I'll just fix the warning message to spell out which scope is finally chosen.

@gares
Copy link
Member Author

gares commented Mar 24, 2021

done. I've adopted the syntax you proposed and shared it with standard notations (that for now don't support it)

gares added a commit to gares/coq-elpi that referenced this pull request Mar 24, 2021
@gares gares requested a review from a team as a code owner March 24, 2021 17:57
@gares
Copy link
Member Author

gares commented Mar 24, 2021

@herbelin I did change the parser so that both notations and syndefs accepts all modifiers, then syndefs only really accept two and notations don't implement yet "in scope" (but you seemed to like the idea, so I paved the way for this).

At the grammar level, this means that both commands parse a superset of the options they really accept, the error comes later.

@gares gares changed the title [syndef] user syntax to force scope on argument [abbreviation] user syntax to set interp scope of argument Mar 24, 2021
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.

Let me know if you have questions about using make doc_gram_rsts.

Copy link
Contributor

@jashug jashug left a comment

Choose a reason for hiding this comment

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

The examples look good, and I think the feature moves in the right direction. For the future, there are probably questions about how to set multiple scopes on an argument. I didn't review the code; herbelin is the expert here.

@gares gares force-pushed the syndef-principal-scope branch from fdd7513 to 72e2ac0 Compare March 25, 2021 08:06
@coqbot-app coqbot-app bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Mar 25, 2021
@gares
Copy link
Member Author

gares commented Mar 30, 2021

Anyone taking care of this PR?

@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 Mar 30, 2021
@ppedrot ppedrot self-assigned this Mar 31, 2021
@ppedrot
Copy link
Member

ppedrot commented Apr 7, 2021

@gares this needs a rebase

@gares gares force-pushed the syndef-principal-scope branch from 18eabab to b57538a Compare April 7, 2021 17:59
@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 Apr 7, 2021
@gares
Copy link
Member Author

gares commented Apr 7, 2021

done

@ppedrot ppedrot requested a review from JasonGross April 13, 2021 11:12
@ppedrot
Copy link
Member

ppedrot commented Apr 19, 2021

Merging soon then.

@herbelin
Copy link
Member

Even if the decision is to no change, if I could get an answer to my comment above just to be sure that it has been seen, that'll be nice.

@gares
Copy link
Member Author

gares commented Apr 19, 2021

You are right, I did forget to implement the empty scope syntax.
I did open #14134 .
I think it is better to merge this one, which has overlays, ASAP, and leave #14134 as a bonus. (I don't have much time these days)

@ppedrot
Copy link
Member

ppedrot commented Apr 20, 2021

@herbelin is that fine with you?

@herbelin
Copy link
Member

@ppedrot: Yes.

@ppedrot ppedrot requested review from JasonGross and removed request for JasonGross April 23, 2021 14:33
@ppedrot ppedrot merged commit a0c3ebf into rocq-prover:master Apr 23, 2021
ppedrot added a commit to rocq-community/paramcoq that referenced this pull request Apr 23, 2021
adapt to rocq-prover/rocq#13965 (manual scope for notation arguments)
ppedrot added a commit to mattam82/Coq-Equations that referenced this pull request Apr 23, 2021
@gares gares deleted the syndef-principal-scope branch April 23, 2021 14:47
gares added a commit to LPCIC/coq-elpi that referenced this pull request Apr 23, 2021
mattam82 added a commit to mattam82/Coq-Equations that referenced this pull request May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: feature New user-facing feature request or implementation. part: notations The notation system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-linearity of variables and notation scopes

6 participants