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

Adding support for an "only parsing" modifier in "where"-based notation #11602

Merged

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Feb 14, 2020

Kind: enhancement

To be merged synchronously with overlay for Equations (mattam82/Coq-Equations#261).

Since Reserved Notation does not support the only parsing modifier, there was no way to tell that a notation declared using a where clause is only parsing. For consistency of the picture, this PR adds this support.

While updating the refman, I fixed what seems typos but I hope I did not introduce problems in the meantime. For instance, I renamed decl_notation into decl_notations and one_decl_notation into decl_notation and I hope this is consistent with the naming policies for the reference manual (cc @jfehrle).

  • Added / updated test-suite
  • Corresponding documentation was added / updated
  • Entry added in the changelog

@herbelin herbelin added part: notations The notation system. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. labels Feb 14, 2020
@herbelin herbelin added this to the 8.12+beta1 milestone Feb 14, 2020
@herbelin herbelin requested review from a team as code owners February 14, 2020 12:18
@@ -130,7 +130,7 @@ type definition_expr =
| DefineBody of local_binder_expr list * Genredexpr.raw_red_expr option * constr_expr
* constr_expr option

type decl_notation = lstring * constr_expr * scope_name option
type decl_notation = lstring * constr_expr * bool * scope_name option
Copy link
Member

Choose a reason for hiding this comment

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

Probably the right time to turn this into a record.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is what I thought too... Maybe tonight or tomorrow (or even later if not urgent)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I experimenting using a record. It is much more verbose, but certainly also more self-explanatory. Any suggestions in passing on how formatting records?

Copy link
Member

Choose a reason for hiding this comment

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

You mean, the type declaration? I subscribe to the

type foo = {
  foo_bla : Tbla;
  foo_bar : Tbar;
  ...
}

syntax, but I am not sure that's what your asking.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I am not sure that's what your asking.

Yes, that was part of my question. In practice, I used:

type decl_notation =
  { decl_ntn_string : lstring
  ; decl_ntn_interp : constr_expr
  ; decl_ntn_only_parsing : bool
  ; decl_ntn_scope : scope_name option
  }

because that's the style which was already used nearby in vernacexpr.ml (even if other part of vernacexpr.ml use the style that you're mentioning.

On the other side, in metasyntax.ml, I used the style:

    let open Vernacexpr in
    let
      { decl_ntn_string = {CAst.loc;v=ntn};
        decl_ntn_interp = c;
        decl_ntn_only_parsing = onlyparsing;
        decl_ntn_scope = scopt } = decl_ntn in

@ppedrot ppedrot self-assigned this Feb 14, 2020
@ppedrot ppedrot added the needs: overlay This is breaking external developments we track in CI. label Feb 14, 2020
@herbelin herbelin force-pushed the master+support-only-parsing-where-clause branch from 3826b15 to 6372aaf Compare February 14, 2020 19:55
@jfehrle
Copy link
Member

jfehrle commented Feb 14, 2020

For instance, I renamed decl_notation into decl_notations and one_decl_notation into decl_notation and I hope this is consistent with the naming policies for the reference manual (cc @jfehrle).

I made the same change myself (not yet merged).

Changes made in .mlg files are easy to notice and to then make rst adjustments. In the .rsts, the productionlists will be going away entirely in favor of insertprodn/prodns in which the insertprodn automatically updates the prodn from the edited grammar. Updating the syntax in the rst directly will usually not be the preferred process. I'll explain the ins and outs of this somewhat later when it's more fully integrated into the build process.

   .. insertprodn fix_definition decl_notation

   .. prodn::
      fix_definition ::= @ident_decl {* @binder } {? @fixannot } {? : @term } {? := @term } {? @decl_notations }
      decl_notations ::= where @decl_notation {* and @decl_notation }
      decl_notation ::= @string := @term1_extended {? : @ident }

which looks like this in the HTML:

image

@herbelin
Copy link
Member Author

which looks like this in the HTML:

image

Nice.

Updating the syntax in the rst directly will usually not be the preferred process. I'll explain the ins and outs of this somewhat later when it's more fully integrated into the build process.

OK, so, if I understand correctly, for this PR, I do nothing special more at the moment.

Copy link
Member

@ppedrot ppedrot left a comment

Choose a reason for hiding this comment

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

The ML code is fine with me.

@ppedrot ppedrot removed the needs: overlay This is breaking external developments we track in CI. label Feb 15, 2020
doc/sphinx/language/gallina-extensions.rst Outdated Show resolved Hide resolved
doc/sphinx/language/gallina-extensions.rst Outdated Show resolved Hide resolved
doc/sphinx/language/gallina-extensions.rst Outdated Show resolved Hide resolved
@herbelin herbelin force-pushed the master+support-only-parsing-where-clause branch 2 times, most recently from 91a4c03 to b13ac5b Compare February 17, 2020 19:17
@herbelin herbelin force-pushed the master+support-only-parsing-where-clause branch from b13ac5b to ce0b076 Compare February 20, 2020 04:37
@herbelin
Copy link
Member Author

Same fiat-crypto error as in #10832.

@Zimmi48
Copy link
Member

Zimmi48 commented Feb 20, 2020

This error is everywhere indeed.

@ppedrot
Copy link
Member

ppedrot commented Feb 23, 2020

@herbelin can you rebase to get a clean CI just in case?

@herbelin herbelin force-pushed the master+support-only-parsing-where-clause branch from ce0b076 to dfc5795 Compare February 23, 2020 21:42
@ppedrot ppedrot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Feb 28, 2020
@herbelin herbelin force-pushed the master+support-only-parsing-where-clause branch from dfc5795 to b31767a Compare March 3, 2020 17:13
@coqbot coqbot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Mar 3, 2020
Copy link
Member

@maximedenes maximedenes left a comment

Choose a reason for hiding this comment

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

LGTM

@ppedrot
Copy link
Member

ppedrot commented Mar 4, 2020

@herbelin The Equations overlay looks like it also needs to be rebased.

@herbelin
Copy link
Member Author

herbelin commented Mar 4, 2020

@herbelin The Equations overlay looks like it also needs to be rebased.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: notations The notation system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants