Skip to content

Preserve unwrapped comments#1352

Merged
gpetiot merged 1 commit intoocaml-ppx:masterfrom
gpetiot:preserve-unwrapped-comments
May 20, 2020
Merged

Preserve unwrapped comments#1352
gpetiot merged 1 commit intoocaml-ppx:masterfrom
gpetiot:preserve-unwrapped-comments

Conversation

@gpetiot
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot commented Apr 20, 2020

The goal is to:

We have to insert a break to preserve #912 and #1100 but we have to do the least possible work (to trying to be too smart), as the wrapping is disabled it's okay not to be optimal and we should be able to preserve as much as possible from the comment shape of the input.

I'm not very satisfied with using break_unless_newline here as I aimed at removing every use of this function but didn't find any alternative so far.

Diff of test_branch with the janestreet profile (due to having a break):

diff --git a/vendor/ocamlformat_support/format_.ml b/vendor/ocamlformat_support/format_.ml
index 02269ab..aed7b32 100644
--- a/vendor/ocamlformat_support/format_.ml
+++ b/vendor/ocamlformat_support/format_.ml
@@ -85,7 +85,8 @@ type pp_token =
   | Pp_tbegin of tbox (* beginning of a tabulation box *)
   | Pp_tend (* end of a tabulation box *)
   | Pp_newline (* to force a newline inside a box *)
-  | Pp_if_newline (* to do something only if this very
+  | Pp_if_newline
+  (* to do something only if this very
                      line has been broken *)
   | Pp_string_if_newline of string
   (* print a string only if this very
diff --git a/infer/src/base/TaskBar.ml b/infer/src/base/TaskBar.ml
index 273d2ee22..0de25c495 100644
--- a/infer/src/base/TaskBar.ml
+++ b/infer/src/base/TaskBar.ml
@@ -72,7 +72,8 @@ let draw_top_bar fmt ~term_width ~total ~finished ~elapsed =
     ++ ( "%s"
@@ -72,7 +72,8 @@ let draw_top_bar fmt ~term_width ~total ~finished ~elapsed =
     ++ ( "%s"
        , max (String.length elapsed_string) 9
          (* leave some room for elapsed_string to avoid flicker. 9 characters is "XXhXXmXXs" so it
-            gives some reasonable margin. *) )
+            gives some reasonable margin. *)
+       )
   in
   let top_bar_size = min term_width top_bar_size_default in
   let progress_bar_size = top_bar_size - size_around_progress_bar in
diff --git a/infer/src/clang/cTrans.ml b/infer/src/clang/cTrans.ml
index 6d77c6f3b..912d8da20 100644
--- a/infer/src/clang/cTrans.ml
+++ b/infer/src/clang/cTrans.ml
@@ -3613,7 +3613,8 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s
         (* explicit with [this] or implicit with [&] *)
         | `LCK_VLAType
         (* capture a variable-length array by reference. we probably don't handle
-           this correctly elsewhere, but it's definitely not captured by value! *) -> true
+           this correctly elsewhere, but it's definitely not captured by value! *)
+          -> true
         | `LCK_ByCopy (* explicit with [x] or implicit with [=] *) ->
           (* [=] captures this by reference and everything else by value *)
           lci_capture_this
diff --git a/infer/src/nullsafe/ClassLevelAnalysis.ml b/infer/src/nullsafe/ClassLevelAnalysis.ml
index 7985bb922..af082744d 100644
--- a/infer/src/nullsafe/ClassLevelAnalysis.ml
+++ b/infer/src/nullsafe/ClassLevelAnalysis.ml
@@ -119,7 +119,8 @@ let make_meta_issue all_issues current_mode class_name =
           | NullsafeMode.Strict
           (* We don't recommend "strict" for now as it is harder to keep a class in strict mode than it "trust none" mode.
              Trust none is almost as safe alternative, but adding a dependency will require just updating trust list,
-             without need to strictify it first. *) ->
+             without need to strictify it first. *)
+            ->
             "`@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = \
              @Nullsafe.TrustList({}))`"
           | NullsafeMode.Default | NullsafeMode.Local (NullsafeMode.Trust.Only _) ->

Diff of test_branch with the conventional profile (due to having a break):

diff --git a/vendor/ocamlformat_support/format_.ml b/vendor/ocamlformat_support/format_.ml
index db25deb..2bda28a 100644
--- a/vendor/ocamlformat_support/format_.ml
+++ b/vendor/ocamlformat_support/format_.ml
@@ -93,7 +93,8 @@ type pp_token =
   | Pp_tbegin of tbox (* beginning of a tabulation box *)
   | Pp_tend (* end of a tabulation box *)
   | Pp_newline (* to force a newline inside a box *)
-  | Pp_if_newline (* to do something only if this very
+  | Pp_if_newline
+  (* to do something only if this very
                      line has been broken *)
   | Pp_string_if_newline of string
   (* print a string only if this very
diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml
index feb179d64..52742a1fc 100644
--- a/infer/src/base/Config.ml
+++ b/infer/src/base/Config.ml
@@ -1629,7 +1629,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/infer/src/base/TaskBar.ml b/infer/src/base/TaskBar.ml
index 892f93830..4672f83a7 100644
--- a/infer/src/base/TaskBar.ml
+++ b/infer/src/base/TaskBar.ml
@@ -77,7 +77,8 @@ let draw_top_bar fmt ~term_width ~total ~finished ~elapsed =
     ++ ( "%s",
          max (String.length elapsed_string) 9
          (* leave some room for elapsed_string to avoid flicker. 9 characters is "XXhXXmXXs" so it
-            gives some reasonable margin. *) )
+            gives some reasonable margin. *)
+       )
   in
   let top_bar_size = min term_width top_bar_size_default in
   let progress_bar_size = top_bar_size - size_around_progress_bar in
diff --git a/infer/src/nullsafe/ClassLevelAnalysis.ml b/infer/src/nullsafe/ClassLevelAnalysis.ml
index f6af04132..9bce6e750 100644
--- a/infer/src/nullsafe/ClassLevelAnalysis.ml
+++ b/infer/src/nullsafe/ClassLevelAnalysis.ml
@@ -103,7 +103,8 @@ let make_meta_issue all_issues current_mode class_name =
             | NullsafeMode.Strict
             (* We don't recommend "strict" for now as it is harder to keep a class in strict mode than it "trust none" mode.
                Trust none is almost as safe alternative, but adding a dependency will require just updating trust list,
-               without need to strictify it first. *) ->
+               without need to strictify it first. *)
+              ->
                 "`@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = \
                  @Nullsafe.TrustList({}))`"
             | NullsafeMode.Default
diff --git a/compiler/lib/js_output.ml b/compiler/lib/js_output.ml
index d584325ca..10a4bf9e5 100644
--- a/compiler/lib/js_output.ml
+++ b/compiler/lib/js_output.ml
@@ -973,7 +973,8 @@ struct
             PP.end_group f;
             PP.end_group f
             (* There MUST be a space between the return and its
-               argument. A line return will not work *) )
+               argument. A line return will not work *)
+        )
     | Labelled_statement (i, s) ->
         PP.string f (Javascript.Label.to_string i);
         PP.string f ":";
diff --git a/src/dune/foreign_rules.ml b/src/dune/foreign_rules.ml
index c6fe6f09..75a01d32 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 ];

Copy link
Copy Markdown
Collaborator

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

Is there any motivation for this change of behavior except when using ocamlformat in combination with ocp-indent? The functionality that adjusts the indentation of comments was a very nice addition that notably improves the output and usability. In particular, there isn't anything in #1165 that indicates a bug or even poor behavior of ocamlformat. I'm not very happy to see it go for no other reason. Could this be guarded under --ocp-indent-compat? Looking forward to when ocamlformat has editor integration, then we won't want to have lost this comment indentation functionality.

Separately, in the diff output there are a few changes similar to:

diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml
index feb179d64..52742a1fc 100644
--- a/infer/src/base/Config.ml
+++ b/infer/src/base/Config.ml
@@ -1629,7 +1629,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."

I don't know how related they are to the rest of this PR, but changes like this that add breaks after multi-line comments are a notable improvement. These changes are a bug fix.

Placement of documentation comments on `val` and `external` items is now
controled by `doc-comments` unless this option is set.

#### Bug fixes
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.

It isn't clear to me that this is a bug fix, seems more like a feature

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented May 4, 2020

Is there any motivation for this change of behavior except when using ocamlformat in combination with ocp-indent? The functionality that adjusts the indentation of comments was a very nice addition that notably improves the output and usability. In particular, there isn't anything in #1165 that indicates a bug or even poor behavior of ocamlformat. I'm not very happy to see it go for no other reason. Could this be guarded under --ocp-indent-compat? Looking forward to when ocamlformat has editor integration, then we won't want to have lost this comment indentation functionality.

That's a good point, it's better to guard this feature by the ocp-indent-compat flag indeed.

@jberdine
Copy link
Copy Markdown
Collaborator

This one is good to rebase and merge, right?

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented May 13, 2020

I have setup a branch for JaneStreet so they can test this feature (and more JaneStreet centric features in the future), I'm waiting for feedback before merging it in master.

@gpetiot gpetiot merged commit 4c08c36 into ocaml-ppx:master May 20, 2020
@gpetiot gpetiot deleted the preserve-unwrapped-comments branch May 20, 2020 18:49
emillon added a commit to emillon/opam-repository that referenced this pull request Aug 6, 2020
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: interpret normal comments like doc comments (as ocp-indent does)

3 participants