Skip to content

Add parens on labeled function argument types when they are tuples #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: jane
Choose a base branch
from
Open

Add parens on labeled function argument types when they are tuples #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/Conf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ let conventional_profile from =
; parens_ite= elt false
; parens_tuple= elt `Always
; parens_tuple_patterns= elt `Multi_line_only
; parens_tuple_labeled_arg_types= elt false
; parse_docstrings= elt true
; parse_toplevel_phrases= elt false
; sequence_blank_line= elt `Preserve_one
Expand Down Expand Up @@ -164,6 +165,7 @@ let ocamlformat_profile from =
; parens_ite= elt false
; parens_tuple= elt `Always
; parens_tuple_patterns= elt `Multi_line_only
; parens_tuple_labeled_arg_types= elt false
; parse_docstrings= elt false
; parse_toplevel_phrases= elt false
; sequence_blank_line= elt `Compact
Expand Down Expand Up @@ -231,6 +233,7 @@ let janestreet_profile from =
; parens_ite= elt true
; parens_tuple= elt `Multi_line_only
; parens_tuple_patterns= elt `Multi_line_only
; parens_tuple_labeled_arg_types= elt true
; parse_docstrings= elt false
; parse_toplevel_phrases= elt false
; sequence_blank_line= elt `Compact
Expand Down
1 change: 1 addition & 0 deletions lib/Conf_t.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ type fmt_opts =
; parens_ite: bool elt
; parens_tuple: [`Always | `Multi_line_only] elt
; parens_tuple_patterns: [`Always | `Multi_line_only] elt
; parens_tuple_labeled_arg_types: bool elt
; parse_docstrings: bool elt
; parse_toplevel_phrases: bool elt
; sequence_blank_line: [`Compact | `Preserve_one] elt
Expand Down
1 change: 1 addition & 0 deletions lib/Conf_t.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ type fmt_opts =
; parens_ite: bool elt
; parens_tuple: [`Always | `Multi_line_only] elt
; parens_tuple_patterns: [`Always | `Multi_line_only] elt
; parens_tuple_labeled_arg_types: bool elt
; parse_docstrings: bool elt
; parse_toplevel_phrases: bool elt
; sequence_blank_line: [`Compact | `Preserve_one] elt
Expand Down
14 changes: 10 additions & 4 deletions lib/Fmt_ast.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,15 @@ and fmt_arrow_param ~return c ctx
| Ptyp_tuple ((Some _, _) :: _) -> true
| _ -> false
in
let core_type =
Params.parens_if labeled_tuple_ret_parens c.conf (fmt_core_type c xtI)
Copy link

Choose a reason for hiding this comment

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

There is some logic in Params.parens_if that you're not duplicating. Is this change intentional?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah - I only recently added this call to parens_if, and in retrospect believe I should have done what is here instead (though I'm not super high confidence either way).

(* Jane Street: With our profile only, parenthesize the body of a labeled
parameter type if it is a tuple, to distinguish from labeled tuples. *)
let labeled_arg_tuple_parens =
c.conf.fmt_opts.parens_tuple_labeled_arg_types.v && (not return)
&& (match tI.ptyp_desc with Ptyp_tuple _ -> true | _ -> false)
&& match lI with Nolabel -> false | Labelled _ | Optional _ -> true
in
let parens = labeled_tuple_ret_parens || labeled_arg_tuple_parens in
let core_type = fmt_core_type ~parens c xtI in
let arg =
match arg_label lI with
| None -> core_type
Expand Down Expand Up @@ -849,7 +855,7 @@ and fmt_arrow_type c ~ctx ?indent ~parens ~parent_has_parens args fmt_ret_typ
gets support for them, we should remove tydecl_param and go with whatever
their solution is. *)
and fmt_core_type c ?(box = true) ?pro ?(pro_space = true) ?constraint_ctx
?(tydecl_param = false) ({ast= typ; ctx} as xtyp) =
?(tydecl_param = false) ?(parens = false) ({ast= typ; ctx} as xtyp) =
protect c (Typ typ)
@@
let {ptyp_desc; ptyp_attributes; ptyp_loc; _} = typ in
Expand Down Expand Up @@ -883,7 +889,7 @@ and fmt_core_type c ?(box = true) ?pro ?(pro_space = true) ?constraint_ctx
(Params.parens_if (not tydecl_param) c.conf
(k $ fmt_attributes c ~pre:Cut atrs) ) )
@@
let parens = (not tydecl_param) && parenze_typ xtyp in
let parens = parens || ((not tydecl_param) && parenze_typ xtyp) in
hvbox_if box 0
@@ Params.parens_if
(match typ.ptyp_desc with Ptyp_tuple _ -> false | _ -> parens)
Expand Down
13 changes: 13 additions & 0 deletions test/passing/tests/labeled_tuples.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,3 +269,16 @@ let _ = f (Foo (~x:5, 1))
let _ = f (Foo (5, 1))
let _ = f (Foo (5, ~x:1))
let _ = f (Foo (5, ~y:1))

(* As a result of labeled tuples, we've decided to add parens around labeled function
argument types when they are tuples, for disambiguation. *)
type t = x:(int * bool) -> string
type t = x:(int * y:bool) -> string
type t = x:(y:int * z:bool) -> string

type t =
x:
(long_type_name_for_multiline1
* long_type_name_for_multiline2
* long_type_name_for_multiline3)
-> string
2 changes: 1 addition & 1 deletion test/passing/tests/labeled_tuples_regressions.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
(* https://github.com/janestreet/ocamlformat/pull/49 *)
val f
: foo:int * very_long_type_name_so_we_get_multiple_lines
: foo:(int * very_long_type_name_so_we_get_multiple_lines)
-> (* cmt *)
bar:('long_type_var_1, 'long_type_var_2) Long_module_name.t
-> additional_somewhat_long_type_name