fantomas #if in mutually recursive class definition throws

Code



type X = int
and
#if NET5_0_OR_GREATER
    [<System.Runtime.CompilerServices.IsReadOnly>]
#endif
    [<CustomEquality; NoComparison; Struct>] Y = int

Error

System.FormatException: Fantomas is trying to format the input multiple times due to the detection of multiple defines.
There is a problem with merging all the code back together.
[] has 5 fragments
[NET5_0_OR_GREATER] has 3 fragments
Please raise an issue at https://fsprojects.github.io/fantomas-tools/#/fantomas/preview.
   at Fantomas.Core.MultipleDefineCombinations.mergeMultipleFormatResults(FormatConfig config, FSharpList`1 results) in /_//src/Fantomas.Core/MultipleDefineCombinations.fs:line 211
   at [email protected](FSharpList`1 results) in /_//src/Fantomas.Core/CodeFormatterImpl.fs:line 99
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, b result1, FSharpFunc`2 userCode) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 528
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 112

Problem description

See error: as far as I know this is perfectly reasonable code :)

This is a link to the original file where this was encountered

Extra information

[ ] The formatted result breaks my code.
[ ] The formatted result gives compiler warnings.
[ ] I or my company would be willing to help fix this.
[ ] I would like a release if this problem is solved.

Options

Fantomas main branch at 2025-07-04T12:03:12Z - fab1bd8fbb2023eeab53efbacff1ed93a1346597

Default Fantomas configuration

_{Did you know that you can ignore files when formatting by using a .fantomasignore file?} _{PS: It's unlikely that someone else will solve your specific issue, as it's something that you have a personal stake in.}

Jul 30 '25 07:07 tboby

Hello, thank you for reporting this issue!

The second trivia (#endif) is lost because we are merging the two AttributeListNodes into a single one. (sample) It would be safer to never do that when there is trivia involved.

Are you interested in submitting a PR for this?

Jul 30 '25 07:07 nojaf

Hello, thank you for reporting this issue!

The second trivia (#endif) is lost because we are merging the two AttributeListNodes into a single one. (sample) It would be safer to never do that when there is trivia involved.

Are you interested in submitting a PR for this?

Sure, I can give it a go!

Jul 30 '25 08:07 tboby

Great! At first glance, I think an extra check in https://github.com/fsprojects/fantomas/blob/0e925b44d92800bc6017677931988a96138d6f77/src/Fantomas.Core/CodePrinter.fs#L3476

could be relevant. genOnelinerAttributes will merge the attributes, which is not what we want in this case.

Jul 30 '25 08:07 nojaf

@nojaf I've managed to find a change that works, but it's leaking into related behaviour where I'm not sure what the expected formatting is.

This sample shows what I'm unclear on. The one-way fix of merging into a single attribute in one direction, but not splitting out into separate lines in the other direction feels weird to me.

The style guide suggests one attribute per line, but https://github.com/search?q=repo%3Adotnet%2Ffsharp+Struct%3B+CustomEquality%3B+NoComparison&type=code is a very common counter-example.

I think I just try to retain as close to current behaviour as I can?

Jul 31 '25 08:07 tboby

Hmm, the style guide seems a little incomplete on this topic. The original issue doesn't mention multiple attributes.

And I believe, this is one of those early Fantomas things, which has always been there and never been broadly discussed with the community. You could open an issue on https://github.com/dotnet/docs and ask for further clarification.

In the meantime, to just fix the problem at hand. I probably would be inclined to merge

    //But do we want this to merge?
#if !TEST
    [<CustomEquality; NoComparison; Struct; test>]
#endif
    Y = int

because we can merge, and that seems to be the behaviour today.

Try and keep the constraint for not merging as high as possible. We don't merge when we technically can't, and that is the trade-off.

How does that sound?

Jul 31 '25 08:07 nojaf

Sounds good to me!

Jul 31 '25 09:07 tboby

Came across two related issues.

Sample Two moves the type out of the define.

Sample three isn't correctly indenting.

They both look like they might be due to an error that I suspect must be in the AST -> Oak conversion: The attributes (or define trivia) seem to be ending up before the leading keyword.

From the way the AST keeps the trivia separate to the typedef I can see how it might be fiddly to reassemble.

Jul 31 '25 20:07 tboby

Yes, these things can happen as the trivia assignment process is not without its flaws. This is hard to get 100% right, and in an extreme example, there will always be an edge case.

There is a curious range problem in this example. TypeDefnExplicitNode doesn't start at the type keyword for some reason.

We accept PRs if you want to fix things, but if there are no real-life examples, I wouldn't bother reporting issues.

Aug 01 '25 05:08 nojaf

OK, so, I managed to get something that solved all three of those problems, but it just kept unravelling more gaps, especially when I realised attributes could be both before and after the leading keyword and have trivia forcing them to remain in place.

So, I tried to reduce it down to just the attribute compacting, which is a bit messy as it still needs to know whether to indent.

https://github.com/fsprojects/fantomas/compare/main...tboby:fantomas:issue3174-simple

I'll put this up for MR but it still has a few bits that need improving.

On the general concept of attributes:

Should Oak model which attributes are before and after the leading keyword? People do seem to do this
Are the Children on Oak node supposed to be in position order?
Technically I think this issue might come up in other places genOnelinerAttributes is used
This feels like an area to return to if/when there is an up-to-date style guide for attributes!

My previous write-up of my larger attempt:

https://github.com/fsprojects/fantomas/compare/main...tboby:fantomas:issue3174

The main parts of the fix are:

Expanding the range of the typenamenode to include the both the attributes and leading keyword for type

Checking which way round the attributes and leading keyword are to decide whether to put them before or after

Checking if there is any trivia in the attributes in order to decide if to indent

Compacting the attribute list as much as possible (in-between trivia)

I can still find plenty of edge cases this doesn't handle though, so it's a much bigger task, that probably involves tracking which attributes are before/after.

Aug 02 '25 12:08 tboby

Should Oak model which attributes are before and after the leading keyword?

Not sure what you are asking here.

Are the Children on Oak node supposed to be in position order?

I believe so.

Technically I think this issue might come up in other places genOnelinerAttributes is used

We can most likely cross that bridge when we get there. It's perfectly fine to concentrate on the issue present in your codebase right now. This is somewhat of an edge case.

This feels like an area to return to if/when there is an up-to-date style guide for attributes!

Yes, if someone has the heart to figure out the style guide, it would be fair game to revisit in a next minor.

Aug 05 '25 08:08 nojaf