Skip to content

Ensure optional argument rebindings use sufficiently many parens#1305

Merged
gpetiot merged 6 commits intoocaml-ppx:masterfrom
craigfe:fix-missing-parens-around-optional-arg-bind
Mar 31, 2020
Merged

Ensure optional argument rebindings use sufficiently many parens#1305
gpetiot merged 6 commits intoocaml-ppx:masterfrom
craigfe:fix-missing-parens-around-optional-arg-bind

Conversation

@craigfe
Copy link
Copy Markdown
Contributor

@craigfe craigfe commented Mar 20, 2020

This resolves #1260.

I chose to do this by adding the logic in Fmt_ast.fmt_fun_args. It's probably possible to do this by instead adding special cases to Ast.parenze_pat. Unfortunately fmt_pattern (via which parenze_pat is called) doesn't respect its parens argument in many cases, making that route difficult. I removed an existing special case for records in Ast.parenze_pat which is superceded by the new logic.

@facebook-github-bot
Copy link
Copy Markdown

Hi @craigfe!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@craigfe craigfe changed the title Ensure optional argument defaults use sufficiently many parens Ensure optional argument rebindings use sufficiently many parens Mar 20, 2020
@craigfe craigfe force-pushed the fix-missing-parens-around-optional-arg-bind branch 2 times, most recently from fa1b505 to d341232 Compare March 20, 2020 14:45
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Thanks !

I found unecessary parentheses in presence of an attribute:

let f ?a:((0[@a])) = ()

@craigfe craigfe force-pushed the fix-missing-parens-around-optional-arg-bind branch 2 times, most recently from 8175677 to ff05ec4 Compare March 23, 2020 09:26
@craigfe
Copy link
Copy Markdown
Contributor Author

craigfe commented Mar 23, 2020

Nice catch on the attributes interaction.

I've added a fix for that case now, although I think it's an unpleasant solution.

I also found an existing bug with attributes and Ppat_or (reported here: #1310), so one of the new tests is disabled for now.

Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

There is a bug with this example, the inner parentheses get dropped:

let f ?or_:((1, 2) [@attr]) = ()

The bug you report in #1310 seems to not behave the same here. Maybe the or pattern bug is more related to the tuple one ?


let f ?interval:('a' .. 'z') = ()

let f ?tuple:((1, 2)) = ()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is not a bug. The outer parentheses are part of the syntax so I think it's nice to not use them to also delimit the tuple (parentheses around tuples are controlled by an option).

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Mar 27, 2020

The test related to #1310 is not fixed on master despite merging #1317, I feel like we need more precise contexts to decide when parentheses can be added/skipped.

Otherwise the rest of the PR looks good to me, what do you think of merging it as-is?

@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Mar 27, 2020

The bug I mentioned above is introduced by this PR. We should fix it before merging.

I agree, we need to improve the handling of parentheses (not in this PR), it's getting harder to maintain.

@gpetiot gpetiot force-pushed the fix-missing-parens-around-optional-arg-bind branch from ff05ec4 to 5ca48ef Compare March 31, 2020 09:27
@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Mar 31, 2020

I fixed the last bugs, it looks good to me now.

@gpetiot gpetiot requested a review from Julow March 31, 2020 09:28
Copy link
Copy Markdown
Collaborator

@Julow Julow 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 :) Let's merge

@gpetiot gpetiot merged commit 4801286 into ocaml-ppx:master Mar 31, 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.

Bug: crash on optional argument rebound to non-variable

4 participants