Skip to content
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.

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

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

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

Conversation

ccasin
Copy link

@ccasin ccasin commented Feb 1, 2024

Per discussion, we want to try adding parens to disambiguate labeled arguments from labeled tuples.

After this PR the jane street profile prefers

x:(int * bool) -> string

Rather than:

x:int * bool -> string

Suggest testing on tree before merging, and further discussion to make sure we don't want fewer parens.

@@ -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)

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).

@tdelvecchio-jsc
Copy link

Evidence for why this change is good: the parens in the type language now directly match their corresponding expression.

-| type a = x:int * y:bool -> string
+| type a = x:(int * y:bool) -> string
 | let a = f ~x:(20, ~y:false)

 | type b = (x:int * y:bool) -> string
 | let b = f (~x:20, ~y:false)

@tdelvecchio-jsc
Copy link

tdelvecchio-jsc commented Feb 12, 2024

If a file is styled with ocamlformat at the base of this PR to look like:

val f
  :  argument1
  -> argument2
  -> argument3
  -> label:argument4a * argument4b
  (* Some multi-line
     comment *)
  -> result

Then formatting it with tip moves the comment association:

val f
  :  argument1
  -> argument2
  -> argument3
  -> label:(argument4a * argument4b)
  -> (* Some multi-line
     comment *)
     result

Similarly:

val f
  :  label:argument1a * argument1b
  (* Some multi-line
     comment *)
  -> argument2
  -> argument3
  -> argument4
  -> result

gets restyled to

val f
  :  label:(argument1a * argument1b)
  -> (* Some multi-line
     comment *)
     argument2
  -> argument3
  -> argument4
  -> result

@ccasin
Copy link
Author

ccasin commented Feb 13, 2024

If a file is styled with ocamlformat at the base of this PR to look like:

...

@tdelvecchio-jsc I spent some time looking at this, and I think it's quite hard to fix! The issue is that in the examples without the parens, the comment is considered adjacent to the argument type, and with the parens it is not. At the cost of some sanity, I figured out how to get them considered adjacent (you can play with it here, if you really want), but it breaks the formatting of other things.

Instead, I'm going to weakly claim this change is fine. One thing to observe is that today if you write

val f : foo -> lbl:(bar -> baz) (* comment *) -> boz

This gets rewritten to

val f : foo -> lbl:(bar -> baz) -> (* comment *) boz

So, it's already the case that you can't write a comment after a labeled argument with parens - it's just that we're creating some new ones of those.

As another note about current ocamlformat insanity, val g : a -> b*c (* hi *) -> d is formatted unchanged, but val g : a -> (b*c) (* hi *) -> d is rewritten to val g : a -> b*c -> (* hi *) d. So we remove the parens but move the comment anyway. Weak evidence the situation here is already pretty sad and we're not making it dramatically worse.

Copy link

@tdelvecchio-jsc tdelvecchio-jsc left a comment

Choose a reason for hiding this comment

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

Tree-smash looks good, only manual work will be moving the comments affected by the issue discussed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants