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

Convert misc chapters to prodn, update syntax #12936

Merged
merged 2 commits into from Oct 25, 2020

Conversation

jfehrle
Copy link
Member

@jfehrle jfehrle commented Aug 29, 2020

This seems to be pretty straightforward. Some questions embedded in the .rst files.

After this, there's just SSR and the tactics chapters to do. Then there will be some cleanup at the end.

@jfehrle jfehrle added the kind: documentation Additions or improvement to documentation. label Aug 29, 2020
@jfehrle jfehrle added this to the 8.12.1 milestone Aug 29, 2020
@jfehrle jfehrle requested a review from Zimmi48 August 29, 2020 00:44
@jfehrle jfehrle requested a review from a team as a code owner August 29, 2020 00:44
Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

This might take a while for me to review all this PR because it modifies chapters on topics which I know less about.

doc/sphinx/addendum/extraction.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/extraction.rst Outdated Show resolved Hide resolved
Comment on lines 271 to 273
If the type scheme axiom is an arity (a sequence of products followed
by a sort), the then some type
variables have to be given (as quoted strings). The syntax is then:
Copy link
Member

Choose a reason for hiding this comment

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

I personally don't understand any of this but it should clearly be rewritten, maybe with help from @coq/extraction-maintainers.

Note the left-over "The syntax is then:" from the previous version.

Comment on lines 175 to 179
.. todo PR needs more work. The first @one_term maps to :g:`relation (A t1 ... tn)`.
I guess the second maps to :g:`Aeq: forall (y1 : β1 ... ym : βm)`, but that
doesn't match the (Aeq t′1 ... t′m) in the cmd syntax before my changes.
And then not sure how :g:`(A : αi -> ... αn -> Type)` fits in at all, e.g.
what is α?
Copy link
Member

Choose a reason for hiding this comment

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

@mattam82 Can you help (or designate someone that could help)?

Copy link
Member

Choose a reason for hiding this comment

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

The first term should be a Type (the carrier of the relation), say T, while the second should be a relation on T, i.e. of type (T -> T -> Prop). As written before, we had A and Aeq applied to some terms built from the binders. So A: forall x1 ... x2, Type and Aeq : forall x1 .. x2, (A x1 ... xn) -> A (x1 ... xn) -> Prop), or equivalently, Aeq : forall x1 ... xn, relation (A x1 ... xn).

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's my best shot:

image

How do the binders relate to the alphas? Or are they unrelated?

Copy link
Member

Choose a reason for hiding this comment

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

The binders are usually the same as the alphas, but there is no strict relationship. Maybe rather say that one_termA and one_term_Aeq should be typeable under the context binders instead.

doc/sphinx/addendum/generalized-rewriting.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/generalized-rewriting.rst Outdated Show resolved Hide resolved
@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 8, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Sep 14, 2020

Let me know when you're done with a first pass review--I assume you're not because of the small number of comments you've made.

@jfehrle

This comment has been minimized.

@Zimmi48

This comment has been minimized.

@jfehrle

This comment has been minimized.

Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

Review still not complete.

doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/micromega.rst Outdated Show resolved Hide resolved
show_zify ::= {| InjTyp | BinOp | UnOp | CstOp | BinRel | UnOpSpec | BinOpSpec | Spec }

.. todo PR: should there be Show Zify {| PropBinOp | PropOp | PropUOp | Saturate } options?
to match Add Zify ... An oversight or not?
Copy link
Member

Choose a reason for hiding this comment

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

cc @fajb

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by options? Things you can set?
Show Zify bla does not enable printing. It prints information when invoked.

I hope it answers the question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I should have said "commands" instead of "options". For symmetry with Add Zify, should Show Zify also support PropOp | PropBinOp | PropUOp | Saturate? If so, I will add them in another PR.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

@fajb says:

As the set of such operators looks fixed to me.
I'll rather remove the Add PropOp PropBinOp et PropUOp.
That is convenient for implementation but the user probably does not
care.

doc/sphinx/addendum/program.rst Outdated Show resolved Hide resolved

The optional order annotation follows the grammar:
.. todo PR: move measure and wf info to inductive.html#recursive-functions-fix?
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a different meaning to these annotations depending on whether you use them with Function or Program (and possibly with Equations). @mattam82?

Copy link
Member

Choose a reason for hiding this comment

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

The wf annotation is treated the same but for measures Program differs indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattam82 Can you elaborate on what the difference is here? Just a few lines below, I have text that says ":g:wf R x which is equivalent to :g:measure x R." which seems inconsistent with you statement (that wf is the same but measure is difterent).

Copy link
Member

Choose a reason for hiding this comment

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

What @mattam82 said is that wf annotation is treated the same way in Function and Program and that measure means something different in Function and Program. In Program, wf means the same up to the order of the arguments. This is not inconsistent with the fact that measure means something else in Function.

BTW @mattam82, this is kind of confusing, so don't you think that measure should be disallowed with Program?

Copy link
Member

@mattam82 mattam82 Oct 22, 2020

Choose a reason for hiding this comment

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

measure is nicer in Program as it allows the user not to pack the arguments in a tuple but can mention all of them.
One can write Program Fixpoint foo (l l' : list nat) {measure (length l + length l')} not having to mention that the relation should be Nat.lt.

Copy link
Member Author

@jfehrle jfehrle Oct 22, 2020

Choose a reason for hiding this comment

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

I need a bit more explanation of measure, wf and struct (not described in the doc) before I can say how they're different with or without Program. I don't have a clear grasp on their meaning. I take it measure has to decrease for each recursion in the definition (maybe not the best words), but I have no clue how that differs from the other two. struct means the number of symbols in the specified variable must decrease for each recursion? No idea how tuples fit into this. A couple very simple examples would help.

wf means the same up to the order of the arguments

Meaning ignoring the order of the arguments? Not so familiar with the "up to" idiom.

Copy link
Member

Choose a reason for hiding this comment

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

struct is for structural recursion. It means you can only call the recursive function on strict subterms (obtained by pattern-matching) of your initial argument. This is the simplest form of recursion, the only one accepted by the kernel.

Meaning ignoring the order of the arguments?

In this case, I mean that the order of arguments is inverted between the two syntax.

doc/sphinx/addendum/program.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/program.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/program.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/nsatz.rst Outdated Show resolved Hide resolved
Comment on lines 43 to 46
* strategy = 0: reverse lexicographic order and newest s-polynomial.
* strategy = 1: reverse lexicographic order and sugar strategy.
* strategy = 2: pure lexicographic order and newest s-polynomial.
* strategy = 3: pure lexicographic order and sugar strategy.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* strategy = 0: reverse lexicographic order and newest s-polynomial.
* strategy = 1: reverse lexicographic order and sugar strategy.
* strategy = 2: pure lexicographic order and newest s-polynomial.
* strategy = 3: pure lexicographic order and sugar strategy.
* `strategy := 0%Z`: reverse lexicographic order and newest s-polynomial.
* `strategy := 1%Z`: reverse lexicographic order and sugar strategy.
* `strategy := 2%Z`: pure lexicographic order and newest s-polynomial.
* `strategy := 3%Z`: pure lexicographic order and sugar strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Kind of weird you have to put "%Z" after something that's not even used as a number.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We are just documenting this but honestly, I'm seriously scared by the way this tactic is taking its arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Something more high-level could indeed be done. I don't know how much these arguments are used though in practise and whether a change could be done without too much worries. Cc @thery

doc/sphinx/addendum/nsatz.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/nsatz.rst Outdated Show resolved Hide resolved
@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 28, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Sep 28, 2020

Updated, please take a look.

@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, 2020
@Zimmi48

This comment has been minimized.

@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 29, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Sep 29, 2020

Rebased.

@jfehrle

This comment has been minimized.

doc/sphinx/addendum/generalized-rewriting.rst Show resolved Hide resolved
doc/sphinx/addendum/generalized-rewriting.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/implicit-coercions.rst Outdated Show resolved Hide resolved
doc/sphinx/conf.py Outdated Show resolved Hide resolved
doc/sphinx/language/core/assumptions.rst Outdated Show resolved Hide resolved
doc/sphinx/language/core/definitions.rst Outdated Show resolved Hide resolved
doc/sphinx/language/core/inductive.rst Outdated Show resolved Hide resolved
doc/sphinx/language/core/inductive.rst Outdated Show resolved Hide resolved
doc/sphinx/language/core/records.rst Outdated Show resolved Hide resolved
doc/sphinx/language/core/records.rst Outdated Show resolved Hide resolved
doc/sphinx/proof-engine/vernacular-commands.rst Outdated Show resolved Hide resolved
@jfehrle
Copy link
Member Author

jfehrle commented Oct 2, 2020

Updated.

@jfehrle
Copy link
Member Author

jfehrle commented Oct 20, 2020

Updated.

And then not sure how :g:`(A : αi -> ... αn -> Type)` fits in at all, e.g.
what is α?

Declares a parametric relation :g:`Aeq: forall (y1 : β1 ... ym : βm)`,
:g:`relation (A t1 ... tn)` over :g:`(A : αi -> ... αn -> Type)`.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's correct, although it's a bit strange to separate the Aeq : forall ... from the relation part.

Copy link
Member

Choose a reason for hiding this comment

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

Well, except that \beta1 ... \betan should be \alpha1 ... \alphan here.

@jfehrle
Copy link
Member Author

jfehrle commented Oct 21, 2020

Updated.

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 22, 2020

We're going to release 8.12.1 soon but given how close to ready this PR is, I'd appreciate being able to include it. Feel free to squash your last changes to spare a cycle.

@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 22, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Oct 22, 2020

Updated. Still need some help with measure/wf/struct, so not quite done.

Always generate prodn* files if edits succeed
@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, 2020
@Zimmi48
Copy link
Member

Zimmi48 commented Oct 23, 2020

We are getting blocked on some detail which is already written well enough for the manual (doesn't introduce any regression). Of course, its presentation could be improved further, but this is also the case of many other things in the manual, and in the meantime, this delays the integration of a PR which contains huge improvements. Furthermore, it would be easier to have this kind of detailed discussion on a specific aspect in a smaller PR which is only focused on this.

@jfehrle
Copy link
Member Author

jfehrle commented Oct 23, 2020

If you're satisfied that works for me. So I squashed this without further changes. We can come back to struct, wf and measure sometime in the future. Thanks for all your help reviewing this.

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 24, 2020

Thanks! Though, there is just one regression in the description of the record declaration that we would need to fix before merging this PR.

Formerly, the text said "If type is omitted, the default type is Type."

In the new version, it should say "If sort is omitted, the default sort is Type."

However, the following (which is currently written in this PR) is wrong:

If present, field_body gives the type of the field name. Otherwise, the field type defaults to Type.

There is no default field type. As I said before, field types are inferred if there is sufficient information to do so.

@jfehrle jfehrle removed the needs: squashing Some commits should be squashed together. label Oct 24, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Oct 24, 2020

Updated.

I replaced If present, field_body gives the type of the field name. Otherwise, the field type defaults to Type.
with If :token:`sort` is omitted, the default sort is Type."

@jfehrle
Copy link
Member Author

jfehrle commented Oct 25, 2020

All the refman jobs built, so I think it's good to go.

Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

@coqbot: merge now

@coqbot-app coqbot-app bot merged commit a12112c into coq:master Oct 25, 2020
@coqbot-app coqbot-app bot added this to Request 8.12.1 inclusion in Coq 8.12 Oct 25, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Oct 25, 2020

Awesome!

Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Oct 26, 2020
@coqbot-app coqbot-app bot moved this from Request 8.12.1 inclusion to Shipped in 8.12.1 in Coq 8.12 Oct 27, 2020
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.
Projects
No open projects
Coq 8.12
  
Shipped in 8.12.1
Development

Successfully merging this pull request may close these issues.

None yet

8 participants