Skip to content

Fix missing open line between multi-line let-binding with poly-typexpr#1372

Merged
jberdine merged 5 commits intomasterfrom
let-and-poly
Jun 26, 2020
Merged

Fix missing open line between multi-line let-binding with poly-typexpr#1372
jberdine merged 5 commits intomasterfrom
let-and-poly

Conversation

@jberdine
Copy link
Copy Markdown
Collaborator

@jberdine jberdine commented May 22, 2020

The is_simple check for let bindings only considers the location of
the body exp. This location happens to usually be large enough. But
for one of a group of let bindings which has an explicit polymorphic
type constraint, the location of the body exp contains only the body,
and does not include the location of the pattern. This means that
cases such as:

[@@@ocamlformat "let-binding-spacing=compact"]

let f x = x

and g : 'a. (_ -> _ -> _ -> 'a) -> _ -> _ -> _ -> 'a =
 fun h a b -> h (i a) (i b) (i c)

get misformatted since g is misidentified as being a one-liner, and
the open line between f and g gets removed.

This patch fixes this by using the location of the entire let-binding
instead of the body expression.

jberdine
jberdine commented May 22, 2020
View reviewed changes
@jberdine jberdine added this to the 0.15.0 milestone Jun 11, 2020
gpetiot
gpetiot approved these changes Jun 12, 2020
View reviewed changes
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, thanks! Can you add a test for that, and a changelog entry, and then we can merge.

@jberdine
Copy link
Copy Markdown
Collaborator Author

jberdine commented Jun 12, 2020

There is a test in the PR description that I tried and failed to understand how to add to the test suite as it currently is set up. Where / how should that be added, considering that let-binding-spacing=compact is needed to see the difference before vs after?

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jun 12, 2020

I think you can do the same thing as for test/passing/module_item_spacing.ml and its variants

jberdine added 3 commits June 26, 2020 00:33
@jberdine
Fix missing open line between multi-line let-binding with poly-typexpr
96f0966 
The is_simple check for let bindings only considers the location of
the body exp. This location happens to usually be large enough. But
for one of a group of let bindings which has an explicit polymorphic
type constraint, the location of the body exp contains only the body,
and does not include the location of the pattern. This means that
cases such as:
```ocaml
[@@@ocamlformat "let-binding-spacing=compact"]

let f x = x

and g : 'a. (_ -> _ -> _ -> 'a) -> _ -> _ -> _ -> 'a =
 fun h a b -> h (i a) (i b) (i c)
```
get misformatted since `g` is misidentified as being a one-liner, and
 the open line between `f` and `g` gets removed.

This patch fixes this by using the location of the entire let-binding
instead of the body expression.
@jberdine
add value_binding case to Ast.t
906c979
@jberdine
add test
672035a
@jberdine
Copy link
Copy Markdown
Collaborator Author

jberdine commented Jun 25, 2020

@gpetiot could you check the added test?

gpetiot
gpetiot approved these changes Jun 26, 2020
View reviewed changes
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.

The tests look good, I added the results for other values of this option.

@jberdine jberdine merged commit 1dbe53a into master Jun 26, 2020
@gpetiot gpetiot deleted the let-and-poly branch June 26, 2020 11:49
@emillon emillon mentioned this pull request Aug 6, 2020
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Aug 6, 2020
@emillon
[new release] ocamlformat (0.15.0)
0dc47f1 
CHANGES:

#### Changes

  + Do not break inline elements such as `{i blah}` in docstrings (ocaml-ppx/ocamlformat#1346, @jberdine)

  + Distinguish hash-getter from hash-comparison infix operators. Operators of the form `#**#` or `#**.` where `**` can be 0 or more operator chars are considered getter operators and are not surrounded by spaces, as opposed to regular infix operators (ocaml-ppx/ocamlformat#1376, @gpetiot)

  + Type constraint on return type of functions is now always printed before the function body (ocaml-ppx/ocamlformat#1381, ocaml-ppx/ocamlformat#1397, @gpetiot)

#### Bug fixes

  + Restore previous functionality for pre-post extension points (ocaml-ppx/ocamlformat#1342, @jberdine)

  + Fix extra break before `function` body of a `fun` (ocaml-ppx/ocamlformat#1343, @jberdine)
    Indent further args of anonymous functions (ocaml-ppx/ocamlformat#1440, @gpetiot)

  + Do not clear the emacs `*compilation*` buffer on successful reformat (ocaml-ppx/ocamlformat#1350, @jberdine)

  + Fix disabling with attributes on OCaml < 4.08 (ocaml-ppx/ocamlformat#1322, @emillon)

  + Preserve unwrapped comments by not adding artificial breaks when `wrap-comments=false` and `ocp-indent-compat=true` are set to avoid interfering with ocp-indent indentation. (ocaml-ppx/ocamlformat#1352, @gpetiot)

  + Break long literal strings at the margin (ocaml-ppx/ocamlformat#1367, @gpetiot)

  + Break after a multiline argument in an argument list (ocaml-ppx/ocamlformat#1360, @gpetiot)

  + Remove unnecessary parens around object (ocaml-ppx/ocamlformat#1379, @gpetiot)

  + Fix placement of comments on constants (ocaml-ppx/ocamlformat#1383, @gpetiot)

  + Do not escape arguments of some Odoc tags (ocaml-ppx/ocamlformat#1391, 1408, @gpetiot, @Julow)
    The characters `[]{}` must not be escaped in the arguments of `@raise`, `@author`, `@version` and others.

  + Fix missing open line between multi-line let-binding with poly-typexpr (ocaml-ppx/ocamlformat#1372, @jberdine)

  + Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases (ocaml-ppx/ocamlformat#1382, @gpetiot)

  + Do not add a space to minimal comments `(* *)`, `(** *)` and `(*$ *)` (ocaml-ppx/ocamlformat#1407, @gpetiot)

  + Fix attributes position in labelled arguments type (ocaml-ppx/ocamlformat#1434, @gpetiot)

  + Add missing parens around type annotation in anonymous function (ocaml-ppx/ocamlformat#1433, @gpetiot)

  + Fix alignment of 'then' keyword in parenthesised expression (ocaml-ppx/ocamlformat#1421, @gpetiot)

#### New features

  + Support quoted extensions (added in ocaml 4.11) (ocaml-ppx/ocamlformat#1405, @gpetiot)

  + Recognise eliom file extensions (ocaml-ppx/ocamlformat#1430, @jrochel)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpetiot gpetiot gpetiot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jberdine @gpetiot @facebook-github-bot