Skip to content

Support multi indices extended indexing operators#1279

Merged
Julow merged 8 commits intoocaml-ppx:masterfrom
Julow:multi-indices-extended-indexing-operators
Mar 11, 2020
Merged

Support multi indices extended indexing operators#1279
Julow merged 8 commits intoocaml-ppx:masterfrom
Julow:multi-indices-extended-indexing-operators

Conversation

@Julow
Copy link
Copy Markdown
Collaborator

@Julow Julow commented Mar 10, 2020

There was some bugs in #1277 this PR fixes them.

The module Ast.Indexing_op is added and replaces the previous code working with indexing operators.
I add unit tests for Ast.is_symbol_id, which indirectly test parsing of indexing operators.

@Julow Julow requested a review from gpetiot March 10, 2020 16:40
@Julow Julow force-pushed the multi-indices-extended-indexing-operators branch from f77ed67 to 580404f Compare March 10, 2020 16:40
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

Looks good to me, no regression with test_branch, good job!
Personally I think it would make sense to make this Index_op module into a new file if it can be isolated from the rest of Ast.ml (we would need to clean up Ast.ml sooner or later, it's difficult to maintain in its current state) but you can merge either way.

@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented Mar 11, 2020

Personally I think it would make sense to make this Index_op module into a new file if it can be isolated from the rest of Ast.ml

I wanted to wait to have a plan before doing this. I agree we should start now !

What do you think of moving this module to Sugar ?
Ast.Indexing_op would only expose parse and the type raw (renamed). Indexing_op is used later in Ast, this is nice for correctness but this is not necessary, we could use unsafe functions, as everything else in this module.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Mar 11, 2020

It would make sense to move it to Sugar but Sugar already depends on Ast, that's why I suggested having a third file (I don't know what kind of unsafe functions you are referring to but unsafe rarely sounds good)

@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented Mar 11, 2020

I don't know what kind of unsafe functions you are referring to but unsafe rarely sounds good

Unsafe meant using Indexing_op.parse (still better than previous API) instead of get_sugar, which does more checks. Those checks and types are not necessary for Ast and can be moved to Sugar. This makes Ast more consistent and defines better how it works. (if there is a "get_sugar" function in Ast, that means an abstraction is missing, here it's Indexing_op.parse)

@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented Mar 11, 2020

Unsafe also meant that we'll have to do operations like "get the last argument" that are less sound than what get_sugar does. But again, that's how Ast works.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Mar 11, 2020

Alright then this split can wait until we cleanup and improve the way Ast works. We can merge if it's ready.

@Julow Julow merged commit 19acb6e into ocaml-ppx:master Mar 11, 2020
Julow added a commit to Julow/opam-repository that referenced this pull request Apr 2, 2020
CHANGES:

#### New features

  + Add an option `--format-invalid-files` to print unparsable parts of the input as verbatim text. This feature is still experimental. (ocaml-ppx/ocamlformat#1026) (Guillaume Petiot)

  + Support multi-indices extended indexing operators (ocaml-ppx/ocamlformat#1279, ocaml-ppx/ocamlformat#1277) (Jules Aguillon, Guillaume Petiot)
    This feature has been added in OCaml 4.10.0

  + Handle OCaml 4.10.0 AST (ocaml-ppx/ocamlformat#1276) (Guillaume Petiot)

  + Preserve functor syntax for consistency (ocaml-ppx/ocamlformat#1312) (Guillaume Petiot)
    Previously both functor syntax: `module M = functor (K : S) -> struct end` and `module M (K : S) = struct end` would be formatted as the latter, the original syntax is now preserved.

#### Changes

  + Add the option `doc-comments-val=before|after` (ocaml-ppx/ocamlformat#1012) (Jules Aguillon)
    This option set the placement of documentation comment on `val` and `external` only.
    It is set to `after` by default.

  + The default for `doc-comments` is changed from `after` to `before` (ocaml-ppx/ocamlformat#1012, ocaml-ppx/ocamlformat#1325) (Jules Aguillon)
    This affects both `conventional` (default) and `ocamlformat` profiles.

  + Some options are now deprecated:
    * `doc-comments` (ocaml-ppx/ocamlformat#1293, ocaml-ppx/ocamlformat#1012)
      This option depends on a flawed heuristic.
      It is replaced by `doc-comments-val` for `val` and `external` declarations.
      There is no equivalent to this option in the general case.
    * `escape-chars`, `escape-strings` and `extension-sugar` (ocaml-ppx/ocamlformat#1293)
      These options are rarely used and their default behavior is considered to be the right behavior.

  + Add space between `row_field` attributes and the label or arguments, to be
    consistent with the non-polymorphic case. (ocaml-ppx/ocamlformat#1299) (Craig Ferguson)

#### Bug fixes

  + Fix missing parentheses around `let open` (ocaml-ppx/ocamlformat#1229) (Jules Aguillon)
    eg. `M.f (M.(x) [@attr])` would be formatted to `M.f M.(x) [@attr]`, which would crash OCamlformat

  + Remove unecessary parentheses with attributes in some structure items:
    * extensions and eval items (ocaml-ppx/ocamlformat#1230) (Jules Aguillon)
      eg. the expression `[%ext (() [@attr])]` or the structure item `(() [@attr]) ;;`
    * `let _ = ...`  constructs (ocaml-ppx/ocamlformat#1244) (Etienne Millon)

  + Fix some bugs related to comments:
    * after a function on the rhs of an infix (ocaml-ppx/ocamlformat#1231) (Jules Aguillon)
      eg. the comment in `(x >>= fun y -> y (* A *))` would be dropped
    * in module unpack (ocaml-ppx/ocamlformat#1309) (Jules Aguillon)
      eg. in the module expression `module M = (val x : S (* A *))`

  + Fix formatting of empty signature payload `[%a:]` (ocaml-ppx/ocamlformat#1236) (Etienne Millon)

  + Fix parenthesizing when accessing field of construct application (ocaml-ppx/ocamlformat#1247) (Guillaume Petiot)

  + Fix formatting of attributes on object overrides `{< >}` (ocaml-ppx/ocamlformat#1238) (Etienne
    Millon)

  + Fix attributes on coercion (ocaml-ppx/ocamlformat#1239) (Etienne Millon)

  + Fix formatting of attributes on packed modules (ocaml-ppx/ocamlformat#1243) (Etienne Millon)

  + Fix parens around binop operations with attributes (ocaml-ppx/ocamlformat#1252, ocaml-ppx/ocamlformat#1306) (Guillaume Petiot, Craig Ferguson)

  + Remove unecessary parentheses in the argument of indexing operators (ocaml-ppx/ocamlformat#1280) (Jules Aguillon)

  + Retain attributes on various AST nodes:
    * field set expressions, e.g. `(a.x <- b) [@A]` (ocaml-ppx/ocamlformat#1284) (Craig Ferguson)
    * instance variable set expressions, e.g. `(a <- b) [@A]` (ocaml-ppx/ocamlformat#1288) (Craig Ferguson)
    * indexing operators, e.g. `(a.(b)) [@A]` (ocaml-ppx/ocamlformat#1300) (Craig Ferguson)
    * sequences, e.g. `(a; b) [@A]` (ocaml-ppx/ocamlformat#1291) (Craig Ferguson)

  + Avoid unnecessary spacing after object types inside records and polymorphic variants,
    e.g. `{foo : < .. > [@A]}` and `{ foo : < .. > }` (ocaml-ppx/ocamlformat#1296) (Craig Ferguson)

  + Fix missing parentheses around tuples with attributes. (ocaml-ppx/ocamlformat#1301) (Craig Ferguson)
    Previously, `f ((0, 0) [@A])` would be formatted to `f (0, 0) [@A]`, crashing OCamlformat.

  + Avoid emitting `>]` when an object type is contained in an extension point
    or attribute payload (ocaml-ppx/ocamlformat#1298) (Craig Ferguson)

  + Fix crash on the expression `(0).*(0)` (ocaml-ppx/ocamlformat#1304) (Jules Aguillon)
    It was formatting to `0.*(0)` which parses as an other expression.

  + Preserve empty doc-comments syntax. (ocaml-ppx/ocamlformat#1311) (Guillaume Petiot)
    Previously `(**)` would be formatted to `(***)`.

  + Do not crash when a comment contains just a newline (ocaml-ppx/ocamlformat#1290) (Etienne Millon)

  + Handle lazy patterns as arguments to `class` (ocaml-ppx/ocamlformat#1289) (Etienne Millon)

  + Preserve cinaps comments containing unparsable code (ocaml-ppx/ocamlformat#1303) (Jules Aguillon)
    Previously, OCamlformat would fallback to the "wrapping" logic, making the comment
    unreadable and crashing in some cases.

  + Fix normalization of attributes, fixing the docstrings in attributes (ocaml-ppx/ocamlformat#1314) (Guillaume Petiot)

  + Add missing parentheses around OR-patterns with attributes (ocaml-ppx/ocamlformat#1317) (Guillaume Petiot)

  + Fix spacing inside parens for symbols when the spacing was handled by the englobing exp (ocaml-ppx/ocamlformat#1316) (Guillaume Petiot)

  + Fix invalid (unparsable) docstrings (ocaml-ppx/ocamlformat#1315) (Guillaume Petiot)
    When parsing a comment raises an error in odoc, it is printed as-is.

  + Fix parenthesizing of optional arguments rebound to non-variables, e.g. `let
    f ?a:(A) = ()` rather than the unparsable `let f ?a:A = ()` (ocaml-ppx/ocamlformat#1305) (Craig Ferguson)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants