Skip to content

Make Cumulative, NonCumulative and Private attributes. - #11665

Merged
ppedrot merged 5 commits into
rocq-prover:masterfrom
Zimmi48:cumulative-attr
Mar 20, 2020
Merged

Make Cumulative, NonCumulative and Private attributes.#11665
ppedrot merged 5 commits into
rocq-prover:masterfrom
Zimmi48:cumulative-attr

Conversation

@Zimmi48

@Zimmi48 Zimmi48 commented Feb 24, 2020

Copy link
Copy Markdown
Member

Kind: feature

These are the last morally attributes which were not parsed as such.

Note that initially I wanted to make the new attributes universes(polymorphic(cumulative)) and universes(polymorphic(noncumulative) (since these attributes must be provided in a polymorphic context). But I didn't manage to find a solution that would be backward-compatible.

  • Added / updated test-suite

@Zimmi48 Zimmi48 added needs: documentation Documentation was not added or updated. needs: changelog entry This should be documented in doc/changelog. needs: test-suite update Test case should be added to / updated in the test-suite. part: attributes #[attributes] modify the behaviour of vernac sentences. part: vernac High level command interpretation. labels Feb 24, 2020
@Zimmi48
Zimmi48 requested review from a team as code owners February 24, 2020 10:30
Comment thread vernac/g_vernac.mlg Outdated
@Zimmi48

Zimmi48 commented Feb 25, 2020

Copy link
Copy Markdown
Member Author

I'll wait for #11423 to be merged before I document this PR.

@Zimmi48

Zimmi48 commented Feb 26, 2020

Copy link
Copy Markdown
Member Author

@ppedrot This PR introduces the private attribute for private inductive types. If the notion of private inductive type is fundamentally different from private as in objects that are not exported from modules, then maybe the name for this attribute should be refined. WDYT?

@SkySkimmer

Copy link
Copy Markdown
Contributor

I think it's fair enough to say that Private Inductive is about not exporting something from the module.

@maximedenes

Copy link
Copy Markdown
Member

I think it's fair enough to say that Private Inductive is about not exporting something from the module.

I also think this is much more standard in CS than the abusive use of "private" in the HoTT community.

@Zimmi48

Zimmi48 commented Feb 26, 2020

Copy link
Copy Markdown
Member Author

OK, I don't know what the use of "private" in the HoTT community is, but if this Private Inductive feature truly is about a private thing in the sense of not exporting from the module, then there is no issue with this attribute and I simply misinterpreted @ppedrot's comment during the call.

@maximedenes

Copy link
Copy Markdown
Member

I think there's a misunderstanding (but I'm not fully sure), so let's try to clarify: a private attribute should be about not being visible outside of a module. Private Inductive is something else (the type is exported but you can't match on its elements, you have to use the eliminators). I personally find the HoTT terminology confusing so I'd like to replace it with something else (also, the feature is not really implemented since it is not enforced by the kernel, I believe). What is less personal is that the other meaning of private (not being visible outside of a module) is very well established in CS, so please don't introduce a private attribute which has another meaning (such as the HoTT one).

Zimmi48 commented Feb 27, 2020

Copy link
Copy Markdown
Member Author

OK thank you very much for clearing things up. So private inductive types are a form of abstraction but more limited than the standard meaning of private. It is similar to types that are exported without their definition in OCaml. What about making the attribute private(constructors) or constructors(private)? Or do you have a suggestion for an entirely different word? The word sealed that we have talked about for opaque definitions could make sense for private inductive types as well somehow. WDYT?

@SkySkimmer

Copy link
Copy Markdown
Contributor

private(constructors) or constructors(private)

It's the elimination which is restricted, not the construction.

@Zimmi48

Zimmi48 commented Feb 27, 2020

Copy link
Copy Markdown
Member Author

OK. Please advise a good name. nonmatchable comes to my mind but that's probably too obscure.

@SkySkimmer

Copy link
Copy Markdown
Contributor

Private is fine imo

@Zimmi48

Zimmi48 commented Feb 28, 2020

Copy link
Copy Markdown
Member Author

But @maximedenes doesn't agree (and indeed using private here would prohibit giving it another meaning later). So @maximedenes do you have an alternative suggestion?

@maximedenes

Copy link
Copy Markdown
Member

But @maximedenes doesn't agree (and indeed using private here would prohibit giving it another meaning later). So @maximedenes do you have an alternative suggestion?

Not at the moment, but please don't use private. Not only it will clash with private in the abstraction meaning, but it also clashes with the OCaml notion of private sum type, which ironically is the dual (they can be destructed but not constructed from outside).

@maximedenes

Copy link
Copy Markdown
Member

I won't oppose to nonmatchable, in fact.

@Zimmi48

Zimmi48 commented Mar 7, 2020

Copy link
Copy Markdown
Member Author

New version uses nonmatchable.

Still need to write tests, doc, and a changelog entry.

@Zimmi48
Zimmi48 requested a review from a team as a code owner March 8, 2020 10:15
@Zimmi48

Zimmi48 commented Mar 8, 2020

Copy link
Copy Markdown
Member Author

The documentation is still a work-in-progress (I still want to move the definition of the template and program attributes) but already gives a pretty good idea of the final result.

@Zimmi48 Zimmi48 removed the needs: changelog entry This should be documented in doc/changelog. label Mar 8, 2020
@Zimmi48

Zimmi48 commented Mar 18, 2020

Copy link
Copy Markdown
Member Author

@ppedrot Unfortunately, I couldn't make the Export modifier a proper legacy attribute because this syntax conflicts with the Export command. So the last commit in this PR does not change the parser but still interprets the Export modifier as an attribute, i.e. Export Set and #[ export ] Set become equivalent.

@Zimmi48

Zimmi48 commented Mar 18, 2020

Copy link
Copy Markdown
Member Author

@jfehrle The first commit of this PR 04adc1d might be of interest to you (so that you don't have to do it again in your own work).

@jfehrle

jfehrle commented Mar 18, 2020

Copy link
Copy Markdown
Member

The first commit of this PR 04adc1d might be of interest to you

Thanks. FWIW, I try to avoid DELETENT when I can because it's not too safe with respect to future grammar changes that add additional productions to the nonterminal. It's safer to delete each production explicitly with | DELETE ... in that a) the tool will complain if the production doesn't exist and b) any added production in the NT would likely show up as a change in the rsts.
But I've not followed this advice everywhere I could (future cleanup).

DELETE: [ ... ] is not too safe either, but what can you do?

This form is a bit safer (when applicable) because just by looking at it, you can easily verify that this doesn't alter the language accepted:

basequalid: [
| REPLACE ident fields
| WITH ident LIST0 field_ident
| DELETE ident
]

SPLICE and RENAME are always safe. Later on I'll look at improving the edits/edit operations to make them simpler/safer with respect to changes.

@ppedrot

ppedrot commented Mar 18, 2020

Copy link
Copy Markdown
Member

@Zimmi48 is that merely a parsing problem? In particular, could that be solved using some ad-hoc lookahead?

@Zimmi48

Zimmi48 commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

@ppedrot I thought that lookaheads could help but I don't know how to use them.

@Zimmi48 Zimmi48 linked an issue Mar 19, 2020 that may be closed by this pull request
@Zimmi48

Zimmi48 commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

Everyone on Gitter seemed to prefer private(match) over nonmatchable.
Bad luck, match is a keyword so #[ private(match) ] does not actually parse.
Should I change the proposal to private(matching), hardcode the parsing of match in attribute syntax, or go back to nonmatchable?

Zimmi48 added 5 commits March 19, 2020 17:47
- Legacy attributes can now be specified in any order.
- Legacy attribute Cumulative maps to universes(cumulative).
- Legacy attribute NonCumulative maps to universes(noncumulative).
- Legacy attribute Private maps to private(matching).

We use "private(matching)" and not "private(match)" because we cannot
use keywords within attributes.
And fix documentation following the removal of the Template Check flag
in rocq-prover#11546.
following changes to attribute parsing.
Unfortunately, we cannot go further and parse Export as a legacy
attribute because this syntax conflicts with the Export command.
@Zimmi48 Zimmi48 removed the needs: test-suite update Test case should be added to / updated in the test-suite. label Mar 19, 2020
@Zimmi48 Zimmi48 added this to the 8.12+beta1 milestone Mar 19, 2020
@Zimmi48 Zimmi48 added kind: documentation Additions or improvement to documentation. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. labels Mar 19, 2020
@Zimmi48

Zimmi48 commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

This is ready. Can I get an assignee?

@jfehrle

jfehrle commented Mar 19, 2020

Copy link
Copy Markdown
Member

I thought that lookaheads could help but I don't know how to use them.

You define a lookahead function and then use the function name in the production. The following example succeeds for the input "(" ident ":=" without consuming it. And fails otherwise.

let test_lpar_id_coloneq =
  let open Pcoq.Lookahead in
  to_entry "test_lpar_id_coloneq" begin
    lk_kw "(" >> lk_ident >> lk_kw ":="
  end


appl_arg:
    [ [ test_lpar_id_coloneq; "("; id = ident; ":="; c = lconstr; ")" -> { (c,Some (CAst.make ~loc @@ ExplByName id)) }

@Zimmi48

Zimmi48 commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

Thanks. I tried using a lookahead but didn't manage to fix the issue. But anyway, this wouldn't provide much advantage because Export followed by any other legacy attribute or any non-keyword command has to be assumed to be the Export command applied to modules of such names. Therefore, keeping things as they are in the current state of the PR looks like the best path to me.

@Zimmi48

Zimmi48 commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

As this PR changes quite a lot of things in quite a number of places, I hope I won't have to wait for too long before it gets an assignee and eventually gets merged.

@ppedrot ppedrot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't check the documentation changes thoroughly, but the changes to the code look OK to me. I am a bit surprised you managed to sort the parsing issues without breaking everything, but the CI seems to be fine, so that's probably correct.

@ppedrot

ppedrot commented Mar 20, 2020

Copy link
Copy Markdown
Member

Let's merge, this PR has been discussed viva voce already, I don't think there is much point in delaying it further.

@ppedrot
ppedrot merged commit 4d025d4 into rocq-prover:master Mar 20, 2020
@Zimmi48
Zimmi48 deleted the cumulative-attr branch March 20, 2020 15:26
@Zimmi48

Zimmi48 commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

Thanks @ppedrot!

Blaisorblade added a commit to Blaisorblade/coq that referenced this pull request Mar 26, 2020
`master` has a more complete fix, but it is part of commit
2c785aa in rocq-prover#11665. Meanwhile, the current text
is nonsense.
Blaisorblade added a commit to Blaisorblade/coq that referenced this pull request Mar 26, 2020
`master` has a more complete fix, but it is part of commit
2c785aa in rocq-prover#11665. Meanwhile, the current text
is nonsense.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: documentation Additions or improvement to documentation. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: attributes #[attributes] modify the behaviour of vernac sentences. part: vernac High level command interpretation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation on attributes

6 participants