Only wrap Pexp_function in parens when necessary

[anmonteiro]

fixes #2032

before:

let predicate =
  predicate === Functions.alwaysTrue1 ?
    defaultPredicate :
    (
      fun
      | None => false
      | Some(exn) => predicate(exn)
    );

after:

let predicate =
  predicate === Functions.alwaysTrue1 ?
    defaultPredicate :
    fun
    | None => false
    | Some(exn) => predicate(exn);

[anmonteiro]

this case was useless. Pexp_function would be caught by expression_immediate_extension_sugar

[jordwalke]

I've found it very difficult to prove that it is safe to omit parenthesis around functions when they are embedded in infix expressions, and when they return infix function applications. To merge this I think we want to have a lot more test cases where the function returns something that applies infix x >>= y, and also when the function itself is the right hand/left hand side of similar infix. (Also, your ternary test is good too - let's combine it with a bunch of the infix tests I mentioned as well).

[anmonteiro]

@jordwalke thanks for the feedback.

In coming up with test cases, the following example does indeed print incorrectly with the current changes:

(fun
| None => x >>= y
| Some(x) => x >>= y)
>>= bar ;

/* becomes this, which doesn't pass idempotency */
fun
| None => x >>= y
| Some(x) => x >>= y
>>= bar;

I wonder if we can play with the precedence rues to make it work? I'm trying that right now.

[anmonteiro]

@jordwalke I think I added your suggested test cases, let me know if you'd like to see more (and which).

The latest changes introduce a custom "fun" precedence (in the printer) that's lower than e.g. bind operators (>..) but higher than the ternary operators, making Pexp_function print without paren wrapping in the ternary case (which is what the original issue is suggesting).

[IwanKaramazow]

@chenglou I don't immediately see anything wrong with this PR, let's merge?

[chenglou]

Thanks. Though I’ll hold off meeting for a day or two. New release now

[chenglou]

Thanks!