Break after a multiline argument in an argument list by gpetiot · Pull Request #1360 · ocaml-ppx/ocamlformat

gpetiot · 2020-05-05T15:03:38Z

Found while working on #1352

gpetiot · 2020-05-05T15:06:07Z

test/passing/comments_args.ml.ref

-    (* inout_wrapper *) is_interceptable false (* is_memoize_impl *) Rx.NonRx
+    (* inout_wrapper *) is_interceptable false
+    (* is_memoize_impl *)
+    Rx.NonRx


closer to the input, probably partially due to the comment attachment (will be improved by another PR)

jberdine

This seems to have an effect for multi-line strings as well as comments, which is good.

There are more changes in the diff when I test locally than I expected. For example:

modified   infer/src/unit/DifferentialFiltersTests.ml
@@ -14,7 +14,8 @@ type 'a outcome = Return of 'a | Raise of exn
 let test_file_renamings_from_json =
   let create_test test_input expected_output _ =
     let test_output input =
-      DifferentialFilters.FileRenamings.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.from_json input
+      DifferentialFilters.FileRenamings.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.from_json
+        input
     in
     let pp_diff fmt (expected, actual) =
       let pp = DifferentialFilters.FileRenamings.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.pp in

and

modified   infer/src/pulse/PulseOperations.ml
@@ -101,7 +101,10 @@ let eval location exp0 astate =
   let rec eval exp astate =
     match (exp : Exp.t) with
     | Var id ->
-        Ok (eval_var (* error in case of missing history? *) [] (Var.of_id id) astate)
+        Ok
+          (eval_var
+             (* error in case of missing history? *) []
+             (Var.of_id id) astate)
     | Lvar pvar ->
         Ok (eval_var [ValueHistory.VariableAccessed (pvar, location)] (Var.of_pvar pvar) astate)
     | Lfield (exp', field, _) ->

and

modified   infer/src/biabduction/SymExec.ml
@@ -1164,7 +1164,8 @@ let declare_locals_and_ret tenv pdesc (prop_ : Prop.normal Prop.t) =
     in
     BiabductionConfig.run_in_re_execution_mode
       (* no footprint vars for locals *)
-      sigma_locals_and_ret ()
+      sigma_locals_and_ret
+      ()
   in
   let sigma' = prop_.Prop.sigma @ sigma_locals_and_ret in
   let prop' = Prop.normalize tenv (Prop.set prop_ ~sigma:sigma') in

Do you know what is going on with them?

gpetiot · 2020-05-11T09:09:42Z

I don't know why it s breaking in these cases, the multiline check should be enough. But it's an ugly patch mixing the break_unless_newline with the fits_breaks that were already in the equation, I had low expectations on this one.

gpetiot · 2020-05-11T09:50:19Z

I managed to have something cleaner by only fixing the Ast.is_simple function (for exprs). The := changes in the tests look like a fix to me. Can you check on your codebase, and then I will check on other big projects.

edit: I'm not sure about the := part, so I reverted it and I'm now checking that it breaks instead of reaching the margin when breaking the list of arguments into different groups. Looks better.

gpetiot · 2020-05-11T10:21:22Z

test/passing/args_grouped.ml

+    BiabductionConfig.run_in_re_execution_mode
+      (* no footprint vars for locals *)
+      sigma_locals_and_ret
+      ()


the comment is only attached to sigma_locals_and_ret so this is a complex expression, causing a break before ()

jberdine

Nice, thanks, I've tested and this looks good.

The changes are almost all bug fixes, but there are a few that I don't understand based on reading the PR's code change. For example:

modified   infer/src/backend/SyntacticCallGraph.ml
@@ -112,7 +112,8 @@ let bottom_up sources : (TaskSchedulerTypes.target, Procname.t) ProcessPool.Task
     let empty = Int.equal 0 !scheduled && Queue.is_empty pending in
     if empty then (
       remaining := 0 ;
-      L.progress "Finished call graph scheduling, %d procs remaining (in, or reaching, cycles).@."
+      L.progress
+        "Finished call graph scheduling, %d procs remaining (in, or reaching, cycles).@."
         (CallGraph.n_procs syntactic_call_graph) ;
       if Config.debug_level_analysis > 0 then CallGraph.to_dotty syntactic_call_graph "cycles.dot" ;
       (* save some memory *)

Here there isn't an embedded newline. The only explanation I can guess is that this is close enough to the margin beforehand that the Cmts.preserve call ends up breaking. Not a big deal though.

gpetiot · 2020-05-19T13:07:42Z

Even if it's not a big regression I would like to fix it before merging this PR.
But I didn't find the solution yet, do you have many occurrences of this issue? do they all follow the same pattern as this one?

jberdine · 2020-05-19T13:14:05Z

From what I could see, they all seemed to have that pattern, yes. It's hard to be sure though. There were not a large number of changes. Note that infer is now using the 0.14.1-13-g000da99 (aka master) version of ocamlformat, so testing should be much easier than before.

gpetiot · 2020-05-19T14:47:30Z

I think I fixed it, there was escaped \n that didn't make the formatted string to break but lead the heuristic to guess the string was breaking.

jberdine · 2020-05-19T21:33:37Z

I tested the current version and found 2 cases that are maybe interesting:

modified   infer/src/biabduction/SymExec.ml
@@ -1161,7 +1161,8 @@ let declare_locals_and_ret tenv pdesc (prop_ : Prop.normal Prop.t) =
     in
     BiabductionConfig.run_in_re_execution_mode
       (* no footprint vars for locals *)
-      sigma_locals_and_ret ()
+      sigma_locals_and_ret
+      ()
   in
   let sigma' = prop_.Prop.sigma @ sigma_locals_and_ret in
   let prop' = Prop.normalize tenv (Prop.set prop_ ~sigma:sigma') in

Here my understanding is that the comment is attached to the sigma_locals_and_ret arg, and it is therefore considered to be a multi-line arg, and so there is a break after. This could go either way. I expect that it would be more understandable to users if the decision to break was not as dependent on internal implementation details like the particulars of comment attachment. Not a big deal though.

The other is:

modified   infer/src/unit/DifferentialFiltersTests.ml
@@ -23,8 +23,8 @@ let test_file_renamings_from_json =
     match expected_output with
     | Return exp ->
         assert_equal ~pp_diff
-          ~cmp:DifferentialFilters.FileRenamings.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.equal exp
-          (test_output test_input)
+          ~cmp:DifferentialFilters.FileRenamings.VISIBLE_FOR_TESTING_DO_NOT_USE_DIRECTLY.equal
+          exp (test_output test_input)
     | Raise exc ->
         assert_raises exc (fun () -> test_output test_input)
   in

This one I expect is a case of a bit of noise when getting very close to the margin due to conflicting with Format's max_indent. Note that the previous version reaches column 98 with a margin configured to 100.

Those are not blockers.

gpetiot · 2020-05-20T06:49:29Z

Great, before merging this one I would like to test the combination of #1352, #1360 and #1367; if the output of test_branch looks good I will integrate them in the preview janestreet branch to check we are not causing regressions to avoid a rollback later.

gpetiot · 2020-05-20T12:32:43Z

Minor update on the PR, here is now the diff of test_branch:

diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml
index 29ce48283..583da2d45 100644
--- a/infer/src/base/Config.ml
+++ b/infer/src/base/Config.ml
@@ -1596,7 +1596,8 @@ and nullsafe_optimistic_third_party_params_in_non_strict =
          Historically this is because there was no actionable way to change third party annotations.
          Now that we have such a support, this behavior should be reconsidered, provided
          our tooling and error reporting is friendly enough to be smoothly used by developers.
-      *) ~default:true
+      *)
+    ~default:true
     "Nullsafe: in this mode we treat non annotated third party method params as if they were \
      annotated as nullable."
 
diff --git a/src/dune/expander.ml b/src/dune/expander.ml
index 70eae332..2f634af2 100644
--- a/src/dune/expander.ml
+++ b/src/dune/expander.ml
@@ -463,7 +463,8 @@ let gen_with_record_deps ~expand t resolved_forms ~dep_kind =
     expand
       (* we keep the dir constant here to replicate the old behavior of: (chdir
          foo %{exe:bar}). This should lookup ./bar rather than ./foo/bar *)
-      resolved_forms ~dir:t.dir ~dep_kind ~expand_var:t.expand_var
+      resolved_forms
+      ~dir:t.dir ~dep_kind ~expand_var:t.expand_var
   in
   { t with expand_var }
 
diff --git a/src/dune/foreign_rules.ml b/src/dune/foreign_rules.ml
index 4987a6f8..3b250147 100644
--- a/src/dune/foreign_rules.ml
+++ b/src/dune/foreign_rules.ml
@@ -88,8 +88,8 @@ let build_c_file ~sctx ~dir ~expander ~include_flags (loc, src, dst) =
     (let src = Path.build (Foreign.Source.path src) in
      Command.run
      (* We have to execute the rule in the library directory as the .o is
-        produced in the current directory *) ~dir:(Path.build dir)
-       (Ok ctx.ocamlc)
+        produced in the current directory *)
+       ~dir:(Path.build dir) (Ok ctx.ocamlc)
        [ A "-g"
        ; include_flags
        ; Dyn (Build.map c_flags ~f:(fun x -> Command.quote_args "-ccopt" x))
@@ -121,7 +121,8 @@ let build_cxx_file ~sctx ~dir ~expander ~include_flags (loc, src, dst) =
      let c_compiler = Ocaml_config.c_compiler ctx.ocaml_config in
      Command.run
      (* We have to execute the rule in the library directory as the .o is
-        produced in the current directory *) ~dir:(Path.build dir)
+        produced in the current directory *)
+       ~dir:(Path.build dir)
        (Super_context.resolve_program ~loc:None ~dir sctx c_compiler)
        ( [ Command.Args.S [ A "-I"; Path ctx.stdlib_dir ]
          ; include_flags

jberdine · 2020-05-20T13:10:57Z

The one in dune/expander.ml looks like the same behavior I saw where an arg with an attached comment gets considered to be nonsimple due to the existence/length of the comment. The other changes are bug fixes.

gpetiot · 2020-05-20T18:53:31Z

This could go either way. I expect that it would be more understandable to users if the decision to break was not as dependent on internal implementation details like the particulars of comment attachment. Not a big deal though.

I also think it's not a big deal but this may cause unwanted diff the next time users upgrade ocamlformat. But I have no idea how to fix this, so if there is no objection I will merge it soon.

jberdine · 2020-05-20T19:27:10Z

Just wondering, but is leaving out preceding and trailing comments when pre-formatting in is_simple an option?

gpetiot · 2020-05-25T12:57:21Z

Just wondering, but is leaving out preceding and trailing comments when pre-formatting in is_simple an option?

I tried something in this direction in my last commit. @jberdine can you test on your codebase there is no regression? I'm dropping the comments before, to not prevent wrapping in this case:

   (* fooooooooooooo *)
   fooo foooooooooooo fooooooo

but not after, so we have a break here, and the parameters are not hidden after the comment:

   fooooooo
   (* fooooooooooooooo *)
   fooooooooooo

The test_branch diff looks good:

diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml
index bd9252ee2..9dbffe6a1 100644
--- a/infer/src/base/Config.ml
+++ b/infer/src/base/Config.ml
@@ -1646,7 +1646,8 @@ and nullsafe_optimistic_third_party_params_in_non_strict =
          Historically this is because there was no actionable way to change third party annotations.
          Now that we have such a support, this behavior should be reconsidered, provided
          our tooling and error reporting is friendly enough to be smoothly used by developers.
-      *) ~default:true
+      *)
+    ~default:true
     "Nullsafe: in this mode we treat non annotated third party method params \
      as if they were annotated as nullable."
 
diff --git a/src/dune/foreign_rules.ml b/src/dune/foreign_rules.ml
index a3fe2af7..92c14441 100644
--- a/src/dune/foreign_rules.ml
+++ b/src/dune/foreign_rules.ml
@@ -90,8 +90,8 @@ let build_c_file ~sctx ~dir ~expander ~include_flags (loc, src, dst) =
     (let src = Path.build (Foreign.Source.path src) in
      Command.run
      (* We have to execute the rule in the library directory as the .o is
-        produced in the current directory *) ~dir:(Path.build dir)
-       (Ok ctx.ocamlc)
+        produced in the current directory *)
+       ~dir:(Path.build dir) (Ok ctx.ocamlc)
        [
          A "-g";
          include_flags;
@@ -124,7 +124,8 @@ let build_cxx_file ~sctx ~dir ~expander ~include_flags (loc, src, dst) =
      let c_compiler = Ocaml_config.c_compiler ctx.ocaml_config in
      Command.run
      (* We have to execute the rule in the library directory as the .o is
-        produced in the current directory *) ~dir:(Path.build dir)
+        produced in the current directory *)
+       ~dir:(Path.build dir)
        (Super_context.resolve_program ~loc:None ~dir sctx c_compiler)
        ( [
            Command.Args.S [ A "-I"; Path ctx.stdlib_dir ];

gpetiot · 2020-06-09T08:28:40Z

Can we merge this one?

jberdine · 2020-06-09T11:19:18Z

Sorry to be slow to test, I plan to, just need a chance to context switch.

jberdine

Sorry for the delay, but this is great, thanks!

@jberdine

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)

facebook-github-bot added the CLA Signed label May 5, 2020

gpetiot commented May 5, 2020

View reviewed changes

jberdine reviewed May 5, 2020

View reviewed changes

gpetiot changed the title ~~Break after a big comment in an argument list~~ Break after a multiline argument in an argument list May 11, 2020

gpetiot requested a review from jberdine May 11, 2020 09:50

gpetiot commented May 11, 2020

View reviewed changes

gpetiot requested review from Julow and emillon May 11, 2020 11:53

jberdine approved these changes May 12, 2020

View reviewed changes

jberdine added this to the 0.15.0 milestone May 12, 2020

gpetiot marked this pull request as draft May 18, 2020 09:46

gpetiot mentioned this pull request May 19, 2020

Break long literal strings at the margin #1367

Merged

gpetiot marked this pull request as ready for review May 19, 2020 13:08

gpetiot requested a review from jberdine May 25, 2020 12:57

gpetiot added 2 commits June 9, 2020 15:25

Break after a multiline argument in an argument list

fb54416

drop immediate surrounding comments

f5cdfb9

jberdine approved these changes Jun 9, 2020

View reviewed changes

gpetiot merged commit 94cacc8 into ocaml-ppx:master Jun 10, 2020

gpetiot deleted the break-after-big-cmt branch June 10, 2020 05:02

emillon mentioned this pull request Aug 6, 2020

[new release] ocamlformat (0.15.0) ocaml/opam-repository#16972

Merged

Julow mentioned this pull request Sep 20, 2023

Break long string literal arguments #2448

Draft

Conversation

gpetiot commented May 5, 2020

Uh oh!

gpetiot May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jberdine left a comment

Choose a reason for hiding this comment

Uh oh!

gpetiot commented May 11, 2020

Uh oh!

gpetiot commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpetiot May 11, 2020

Choose a reason for hiding this comment

Uh oh!

jberdine left a comment

Choose a reason for hiding this comment

Uh oh!

gpetiot commented May 19, 2020

Uh oh!

jberdine commented May 19, 2020 via email

Uh oh!

gpetiot commented May 19, 2020

Uh oh!

jberdine commented May 19, 2020

Uh oh!

gpetiot commented May 20, 2020

Uh oh!

gpetiot commented May 20, 2020

Uh oh!

jberdine commented May 20, 2020

Uh oh!

gpetiot commented May 20, 2020

Uh oh!

jberdine commented May 20, 2020

Uh oh!

gpetiot commented May 25, 2020

Uh oh!

gpetiot commented Jun 9, 2020

Uh oh!

jberdine commented Jun 9, 2020 via email

Uh oh!

jberdine left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gpetiot May 5, 2020 •

edited

Loading

gpetiot commented May 11, 2020 •

edited

Loading