Add missing parentheses around expression having attributes or commen…

…ts inside a shorthand let-open clause (ocaml-ppx#1708)
antalsz · Jul 9, 2021 · e78506b · e78506b
1 parent 9136fc1
commit e78506b
Show file tree

Hide file tree

Showing 5 changed files with 134 additions and 1 deletion.
diff --git a/CHANGES.md b/CHANGES.md
@@ -39,6 +39,8 @@
 
   + Add missing break between pattern and attribute (#1711, @gpetiot)
 
+  + Add missing parentheses around expression having attributes or comments inside a shorthand let-open clause (#1708, @gpetiot)
+
 #### Changes
 
   + Improve the diff of unstable docstrings displayed in error messages (#1654, @gpetiot)

diff --git a/lib/Fmt_ast.ml b/lib/Fmt_ast.ml
@@ -2232,8 +2232,13 @@ and fmt_expression c ?(box = true) ?pro ?epi ?eol ?parens ?(indent_wrap = 0)
         | _ -> maybe_break
       in
       let can_skip_parens =
+        (not (Cmts.has_before c.cmts e0.pexp_loc))
+        && (not (Cmts.has_after c.cmts e0.pexp_loc))
+        &&
         match e0.pexp_desc with
-        | Pexp_array _ | Pexp_record _ -> true
+        | (Pexp_array _ | Pexp_record _)
+          when List.is_empty e0.pexp_attributes ->
+            true
         | Pexp_tuple _ -> Poly.(c.conf.parens_tuple = `Always)
         | _ -> Option.is_some (Sugar.list_exp c.cmts e0)
       in

diff --git a/test/passing/tests/open-closing-on-separate-line.ml.ref b/test/passing/tests/open-closing-on-separate-line.ml.ref
@@ -296,3 +296,45 @@ let g =
      x
      ) [@attr]
     )
+
+let _ = M.({f} [@warning "foo"])
+
+let _ = M.((* A *) {f})
+
+let _ = M.({f} (* B *))
+
+let _ = M.((* A *) {f} (* B *))
+
+let _ = M.((* A *) {f} (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([|f|] [@warning "foo"])
+
+let _ = M.((* A *) [|f|])
+
+let _ = M.([|f|] (* B *))
+
+let _ = M.((* A *) [|f|] (* B *))
+
+let _ = M.((* A *) [|f|] (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([f] [@warning "foo"])
+
+let _ = M.((* A *) [f])
+
+let _ = M.([f] (* B *))
+
+let _ = M.([f] (* B *)) (* after *)
+
+let _ = M.((* A *) [f] (* B *))
+
+let _ = M.((* A *) ([f] (* B *) [@warning "foo"] (* C *)))
+
+let _ = M.((f, f) [@warning "foo"])
+
+let _ = M.((* A *) (f, f))
+
+let _ = M.((f, f) (* B *))
+
+let _ = M.((* A *) (f, f) (* B *))
+
+let _ = M.((* A *) ((f, f) (* B *) [@warning "foo"] (* C *)))
diff --git a/test/passing/tests/open.ml b/test/passing/tests/open.ml
@@ -221,3 +221,45 @@ open[@attr] (val x)
 open[@attr] [%ext]
 
 let g = M.f ((let open M in x) [@attr])
+
+let _ = M.({f} [@warning "foo"])
+
+let _ = M.((* A *) { f })
+
+let _ = M.({ f } (* B *))
+
+let _ = M.((* A *) { f } (* B *))
+
+let _ = M.((* A *) { f } (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([|f|] [@warning "foo"])
+
+let _ = M.((* A *) [| f |])
+
+let _ = M.([| f |] (* B *))
+
+let _ = M.((* A *) [| f |] (* B *))
+
+let _ = M.((* A *) [| f |] (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([f] [@warning "foo"])
+
+let _ = M.((* A *) [ f ])
+
+let _ = M.([ f ] (* B *))
+
+let _ = M.([ f ] (* B *)) (* after *)
+
+let _ = M.((* A *) [ f ] (* B *))
+
+let _ = M.((* A *) [ f ] (* B *) [@warning "foo"] (* C *))
+
+let _ = M.((f, f) [@warning "foo"])
+
+let _ = M.((* A *) ( f, f))
+
+let _ = M.((f, f ) (* B *))
+
+let _ = M.((* A *) (f, f) (* B *))
+
+let _ = M.((* A *) (f, f) (* B *) [@warning "foo"] (* C *))
diff --git a/test/passing/tests/open.ml.ref b/test/passing/tests/open.ml.ref
@@ -286,3 +286,45 @@ let g =
   M.f
     ((let open M in
      x) [@attr] )
+
+let _ = M.({f} [@warning "foo"])
+
+let _ = M.((* A *) {f})
+
+let _ = M.({f} (* B *))
+
+let _ = M.((* A *) {f} (* B *))
+
+let _ = M.((* A *) {f} (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([|f|] [@warning "foo"])
+
+let _ = M.((* A *) [|f|])
+
+let _ = M.([|f|] (* B *))
+
+let _ = M.((* A *) [|f|] (* B *))
+
+let _ = M.((* A *) [|f|] (* B *) [@warning "foo"] (* C *))
+
+let _ = M.([f] [@warning "foo"])
+
+let _ = M.((* A *) [f])
+
+let _ = M.([f] (* B *))
+
+let _ = M.([f] (* B *)) (* after *)
+
+let _ = M.((* A *) [f] (* B *))
+
+let _ = M.((* A *) ([f] (* B *) [@warning "foo"] (* C *)))
+
+let _ = M.((f, f) [@warning "foo"])
+
+let _ = M.((* A *) (f, f))
+
+let _ = M.((f, f) (* B *))
+
+let _ = M.((* A *) (f, f) (* B *))
+
+let _ = M.((* A *) ((f, f) (* B *) [@warning "foo"] (* C *)))