Be more cautious when adding a sepSpace before paren lambda

[dawedawe]

Fixes #2231
Fixes #2041

Not super sure about this one and it may need more discussion, but I think we are a bit too liberal adding spaces before the lparen of lambda arguments. This can easily break calls to Linq methods. See the example in #2231

[nojaf]

Hey, sorry for the delay.
In this case, that AST node/combo is ruffly DotGet(App(longIdent, Paren(Lambda _)), longIdent2).
The space in this exact shape is something we should avoid.

This PR touches a bit more than that. Though I must admit it can be subtle.
You could try introducing a new active pattern in SourceParser to capture this.
It would be something along the lines of:

        // Foo(fun x -> y).Bar
        | DotGet(AppWithLambda(_, [], _, _, _, _), lids) ->
            !- "todo"

And to not need to repeat everything for AppWithLambda I would extract the logic in

        // functionName arg1 arg2 (fun x y z -> ...)
        | AppWithLambda (e, es, lpr, lambda, rpr, pr) -> ...

And parameterize the logic to add a space or not.
In the case of the dotget it should not add it.
For the | AppWithLambda it should use the existing logic.

I hope this helps, good luck.

[dawedawe]

I hope this helps, good luck.

It sure does. Thanks. I'll try to come up with something.

[nojaf]

Getting closer, keep it up!

[nojaf]

I wouldn't pass in the lids of the DotGet here to the genAppWithLambda function.
What you want to control from outside the genAppWithLambda is whether there should be a space or not.

I would pass in a function (that adds the space) instead of a boolean.

Something long the lines of:

        | DotGetAppWithLambda ((e, es, lpr, lambda, rpr, pr), lids) ->
            leadingExpressionIsMultiline 
                (genAppWithLambda astContext sepNone (e, es, lpr, lambda, rpr, pr))
                (fun isMultline -> // print lids and do the right thing based on isMultline)

        // functionName arg1 arg2 (fun x y z -> ...)
        | AppWithLambda (e, es, lpr, lambda, rpr, pr) ->
              let sepSpaceAfterFunctionName =
                  let sepSpaceBasedOnSetting e =
                      match e with
                      | Paren _ -> sepSpace
                      | UppercaseSynExpr -> (fun ctx -> onlyIf ctx.Config.SpaceBeforeUppercaseInvocation sepSpace ctx)
                      | LowercaseSynExpr -> (fun ctx -> onlyIf ctx.Config.SpaceBeforeLowercaseInvocation sepSpace ctx)

                  match es with
                  | [] -> sepSpaceBasedOnSetting e
                  | _ -> sepSpace

            genAppWithLambda astContext sepSpaceAfterFunctionName  (e, es, lpr, lambda, rpr, pr) 

The benefit here is that genAppWithLambda has no knowledge about lids so the function only does one thing (namely printing a function application that ends with a lambda).

Oh and in a lot of places astContext is the first argument.
This is a bit historically.

[dawedawe]

Thanks for the review and the suggestions. Really appreciate your patience.

I hope I'm not too confused here but I think this is now more in line with our default config of SpaceBeforeUppercaseInvocation = false. As a consequence there were some tests which needed adjustments.

[nojaf]

I think the changes in the existing tests are all valid.

[nojaf]

This is dangerous that have an indent without later restoring using an unindent.

[nojaf]

All tests also seem to run just fine when removing indent >> so not sure if we still need that here.

[dawedawe]

Mmh, yeah. I think that was just an artefact of me screwing around.

[nojaf]

Typo isMultiline

[dawedawe]

fixed

[nojaf]

A couple of very small things left, but this is looking good!

[nojaf]

Many thanks for this!