Shorter MaxLineLength with long variable name yields invalid F# code according to fantomas

[knocte]

Issue created from fantomas-online

Code

namespace SomeNamespace

module SomeModule =

    let SomeFunc () =
        let someLocalFunc someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam =
            async {
                if (someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam = 1) then
                    return failwith "xxx"
            }
        ()

Error

Fantomas was able to format the code but the result appears to be invalid F# code.
Please open an issue.

Formatted result:

namespace SomeNamespace

module SomeModule =

    let SomeFunc () =
        let someLocalFunc someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam =
            async {
                if (someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam =
                    1) then
                    return failwith "xxx"
            }

        ()

Problem description

Please describe here the Fantomas problem you encountered.
Check out our Contribution Guidelines.

Extra information

• The formatted result breaks by code.
• The formatted result gives compiler warnings.
• [maybe] I or my company would be willing to help fix this.

Options

Fantomas Master at 11/07/2020 09:02:01 - de55bd9

    { config with
                MaxLineLength = 80 }

[knocte]

@nojaf we're working on this but are confused as to what should be the expected result. Would it be this?:

module SomeModule =

    let SomeFunc () =
        let someLocalFunc
            someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam
            =
            async {
                if (someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam =
                     1) then
                    return failwith "xxx"
            }

        ()

The above doesn't make the compiler fail for me, but strucks me as strange. Is this maybe an F#-compiler bug?

/cc @btzo

[nojaf]

Hey @knocte, I would expect the output to be:

module SomeModule =

    let SomeFunc () =
        let someLocalFunc
            someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam
            =
            async {
                if (someVeryLooooooooooooooooooooooooooooooooooooooooooooooooongParam = 1) then
                    return failwith "xxx"
            }

        ()

I noticed the condition is being mistaken for a named argument from a constructor call in

fantomas/src/Fantomas/CodePrinter.fs

Line 1644 in d5f4d9f

| InfixApp (equal, operatorExpr, e1, e2) when (equal = "=") ->

Something like A(B = b).

Constructor invocations may need their own active pattern to be captured and processed accordingly in CodePrinter.
I'll try to revisit this as it was introduced during an infix operation refactoring.

[knocte]

I would expect the output to be:

I don't see the if's condition split in 2 lines, in your snippet; are you sure that respects the MaxLineLength?

[nojaf]

It should follow:

fantomas/src/Fantomas/CodePrinter.fs

Line 1715 in d5f4d9f

genMultilineInfixExpr astContext e1 operatorText operatorExpr e2

And then go here

fantomas/src/Fantomas/CodePrinter.fs

Line 2613 in d5f4d9f

genOnelinerInfixExpr astContext e1 operatorText operatorExpr e2

as equals is part of

fantomas/src/Fantomas/Context.fs

Line 1076 in d5f4d9f

let internal noBreakInfixOps = set [ "="; ">"; "<"; "%" ]

[knocte]

@nojaf let's agree on the expected results first, before discussing implementation. The snippet that you have posted in #1241 (comment) doesn't split the if X then line in two, but fantomas has to do it, otherwise the MaxLineLength is not respected (whole point about this bug is related to this).

[nojaf]

MaxLineLength cannot always be respected in combination with all other rules/settings across the entire tool.
= is part of noBreakInfixOps and that is not respected here as I explained, that is the problem for me.

Trying to split noBreakInfixOps operator to respect max line length will introduce a whole set of different issues in regards to operator precedence.

[btzo]

Hey @nojaf ,

Constructor invocations may need their own active pattern to be captured and processed accordingly in CodePrinter.

You could elaborate this part please.
Today at SourceParser.fs, the only reference for a constructor I see is the MFConstructor.

fantomas/src/Fantomas/SourceParser.fs

Lines 545 to 554 in b36e5a5

	/// This pattern finds out which keyword to use
	let (|MFMember|MFStaticMember|MFConstructor|MFOverride|) (mf: MemberFlags) =
	match mf.MemberKind with
	| MemberKind.ClassConstructor
	| MemberKind.Constructor -> MFConstructor()
	| MemberKind.Member
	| MemberKind.PropertyGet
	| MemberKind.PropertySet
	| MemberKind.PropertyGetSet as mk ->

but it seems to me that it only serves to decide which keyword to use in each case in a Pattern.

Also, I can't find anything explicit for the treatment of a Constructor in the documentation of the SynExpr.

Maybe the New and ObjExpr expressions?

[nojaf]

Hey Mike, I'll get back to you on Friday.

[nojaf]

Hey @btzo, I added the fix myself, as well I'm the culprit who introduced it.
I've explained a bit about what happened in the PR description if you are interested.
Thanks again for showing interest in solving this.

[btzo]

Cool, I'll read the PR