Skip to content

Fix parentheses around infix operators with attributes#1306

Merged
Julow merged 5 commits intoocaml-ppx:masterfrom
craigfe:fix-infix-apply-with-attributes
Apr 1, 2020
Merged

Fix parentheses around infix operators with attributes#1306
Julow merged 5 commits intoocaml-ppx:masterfrom
craigfe:fix-infix-apply-with-attributes

Conversation

@craigfe
Copy link
Copy Markdown
Contributor

@craigfe craigfe commented Mar 20, 2020

This resolves #1250.

As a side-effect, there are now unnecessary parentheses around infix application chains with attributes attached, e.g.:

-;; a + b + c [@attr]
+;; (a + b + c) [@attr]

This is a breaking change. I chose to propose the change anyway, since I find the precedence binding of attributes to infix application confusing. For instance:

(* Here the attribute binds to [(a::b)] *)
let _ = (a :: b [@attr])

(* Here the attribute binds to [b] only *)
let _ = (a @ b [@attr])

If we decide to keep only the parentheses necessary, I think it's necessary to add attributes to the infix precedence logic. In that case, we might want to consider what the interaction with the infix-precedence option should be.

Let me know what you think.

@facebook-github-bot
Copy link
Copy Markdown

facebook-github-bot commented Mar 20, 2020

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!

@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Mar 20, 2020

This is already disambiguated by parentheses:

let _ = a @ (b [@a])
let _ = (a + b) [@a]

I think disambiguation for attributes is very important for readability. This is a fix for consistency and readability:

-;; a + b + c [@attr]
+;; (a + b + c) [@attr]

infix-precedence could be extended to attributes but that's not related, it's used to control disambiguation parentheses when the expression is on several lines.

These are the breaking changes, and I think the parentheses are not wanted here:

-let _ = ([] @ []) [@a]
+let _ = (([] @ []) [@a])
 
-let _ = a := (b + c) [@d]
+let _ = a := ((b + c) [@d])
 
-let _ = (a + b) [@a]
+let _ = ((a + b) [@a])

@gpetiot gpetiot force-pushed the fix-infix-apply-with-attributes branch from a6eecba to 5769f44 Compare March 31, 2020 11:41
@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Mar 31, 2020 •
edited
Loading

I removed the unnecessary parentheses, no regression with test_branch. I added an ugly test of the context that should be merged with the parenze functions in Ast but it's such a mess right now that I didn't manage to do it.
Anyway it looks good to me.

@gpetiot gpetiot requested a review from Julow March 31, 2020 11:42
Julow
Julow approved these changes Mar 31, 2020
View reviewed changes
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.

Let's merge !

craigfe
craigfe commented Mar 31, 2020
View reviewed changes
@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Apr 1, 2020

Thanks for the work !

@Julow Julow merged commit 585b126 into ocaml-ppx:master Apr 1, 2020
@Julow Julow mentioned this pull request Apr 2, 2020
Merged
Julow added a commit to Julow/opam-repository that referenced this pull request Apr 2, 2020
@Julow
[new release] ocamlformat (0.14.0)
f11f376 
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)
@craigfe craigfe mentioned this pull request Jun 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Julow Julow Julow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: attributes attached to certain kinds of binops

4 participants

@craigfe @facebook-github-bot @Julow @gpetiot