MultilineBlockBracketsOnSameColumn not working properly when calling base constructors

[knocte]

Issue created from fantomas-online

Please describe here fantomas problem you encountered

Code

type ServerCannotBeResolvedException =
    inherit CommunicationUnsuccessfulException

    new(message) =
        { inherit CommunicationUnsuccessfulException(message) }

Result

With MultilineBlockBracketsOnSameColumn disabled:

type ServerCannotBeResolvedException =
    inherit CommunicationUnsuccessfulException

    new(message) =
        { inherit CommunicationUnsuccessfulException(message) }

With MultilineBlockBracketsOnSameColumn enabled:

type ServerCannotBeResolvedException =
    inherit CommunicationUnsuccessfulException

    new(message) =
        { inherit CommunicationUnsuccessfulException(message)

        }

Expected results (with MultilineBlockBracketsOnSameColumn enabled):

type ServerCannotBeResolvedException =
    inherit CommunicationUnsuccessfulException

    new(message) =
        {
            inherit CommunicationUnsuccessfulException(message)
        }

Options

Fantomas Next - 4.0.0-alpha-001-1/1/1990

Name	Value
IndentSpaceNum	4
PageWidth	120
SemicolonAtEndOfLine	false
SpaceBeforeParameter	true
SpaceBeforeLowercaseInvocation	true
SpaceBeforeUppercaseInvocation	false
SpaceBeforeClassConstructor	false
SpaceBeforeMember	false
SpaceBeforeColon	false
SpaceAfterComma	true
SpaceBeforeSemicolon	false
SpaceAfterSemicolon	true
IndentOnTryWith	false
SpaceAroundDelimiter	true
MaxIfThenElseShortWidth	40
MaxInfixOperatorExpression	50
MaxRecordWidth	40
MaxArrayOrListWidth	40
MaxLetBindingWidth	40
MultilineBlockBracketsOnSameColumn	true
NewlineBetweenTypeDefinitionAndMembers	false
KeepIfThenInSameLine	false
StrictMode	false

[Bobface]

@nojaf In the tests I can see that sometimes the first element of a record is expected to be on the same line as the opening bracket. For example:

fantomas/src/Fantomas.Tests/MultilineBlockBracketsOnSameColumnRecordTests.fs

Lines 574 to 583 in 694f4c3

	[<Test>]
	let ``update record with standard indent`` () =
	formatSourceString false "let expected = { ThisIsAThing.Empty with TheNewValue = 1 }" config
	|> prepend newline
	|> should equal """
	let expected =
	{ ThisIsAThing.Empty with
	TheNewValue = 1
	}
	"""

Is this expected? If so, could you clarify in which cases this behaviour is expected?

[knocte]

Remembering the github issue that inspired the MultilineBlockBracketsOnSameColumn setting, I'd say that any person that enables this expects always to have things like:

StuffHere
    {
        MoreStuffHere...

So, with this setting enabled, nothing should be in the same line as the opening/closing braces.

[nojaf]

@knocte this is by design. See unit test .

In alignment with how update records work.

[knocte]

@knocte this is by design. See unit test .
In alignment with how update records work.

Those unit tests and the update records cases have more lines. Look at my original testcase, it's just an inherit line, not more lines. If anything, it should put the closing brace in the same line.

So, either this expected result:

        {
            inherit CommunicationUnsuccessfulException(message)
        }

or this one:

        { inherit CommunicationUnsuccessfulException(message) }

but not this:

        { inherit CommunicationUnsuccessfulException(message)

        }

[nojaf]

{ inherit CommunicationUnsuccessfulException(message) } is 55 characters long so that crosses the boundaries of the MaxRecordWidth (40 in your report).
Leading to the current style (where { and inherit are on the same line.

[knocte]

so that crosses the boundaries of the MaxRecordWidth (40 in your report).

But this is not a record! I use short MaxRecordWidth (in fact I set it to 0 in my project) because I don't like the use of ; to separate record members, but we're not talking about the same thing here.

Leading to the current style (where { and inherit are on the same line.

I guess you actually meant where } is on a separate line. I don't have much issue with the opening brace so long as I don't see that weird way of ending it. To me, fantomas should behave in a way that a sane developer would behave, a way which could be possible to encode in a code-style guide. But telling anyone to write this...:

        { inherit CommunicationUnsuccessfulException(message)

        }

...doesn't strike me as something sane at all. I hope you agree that we need to fix it (even if we disagree on the way to fix it).

[nojaf]

But this is not a record!

It actually is from AST perspective.

I guess we can add an exception when the inherit part is not followed by any record field names that we just go of the single line formatting.

[Bobface]

@nojaf How about if we use the Context to check if we are inside a new and apply special rules if that's the case?

[nojaf]

@Bobface you don't really need the Context here, the information is all available in the AST.

The empty list on line 57 are the record fields I believe (matching xs here)
You can do a check over there I think.

[Bobface]

@nojaf AFAIK there can also be values declared even when inherit-ing:

{
    inherit CommunicationUnsuccessfulException(message)
    SomeA = ThingA
    SomeB = ThingB
    ...
}

This would be matched as regular record again as the empty list in line 57 would (I believe?) not be empty anymore.

[nojaf]

When empty I would go for:

{ inherit CommunicationUnsuccessfulException(message) }

when not empty:

{
    inherit CommunicationUnsuccessfulException(message)
    SomeA = ThingA
    SomeB = ThingB
}