fantomas icon indicating copy to clipboard operation
fantomas copied to clipboard

Overly aggressive de-indentation

Open SteveGilham opened this issue 3 years ago • 3 comments

Issue created from fantomas-online

Code

      let result =
        Instrument.I.instrumentationVisitor state' visited

      test
        <@ { result with
               RecordingMethodRef =
                 { Visit = null
                   Push = null
                   Pop = null } } = { state' with
                                        ModuleId = def.MainModule.Mvid.ToString()
                                        RecordingMethod = visit
                                        RecordingMethodRef =
                                          { Visit = null
                                            Push = null
                                            Pop = null } } @>

Result

let result = Instrument.I.instrumentationVisitor state' visited

test
  <@ { result with
      RecordingMethodRef =
        { Visit = null
          Push = null
          Pop = null } } = { state' with
      ModuleId = def.MainModule.Mvid.ToString()
      RecordingMethod = visit
      RecordingMethodRef =
        { Visit = null
          Push = null
          Pop = null } } @>

Problem description

Overly aggressive de-indentation leading to many offside errors

Extra information

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

Options

Fantomas master branch at 1/1/1990

    { config with
                IndentSize = 2
                MaxLineLength = 90 }

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

SteveGilham avatar Feb 20 '22 09:02 SteveGilham

Hello Steve, thank you for reporting this issue. It appears that to maintain valid code, we would need to lock the column of the record update expression.

Potentially add an additional case in https://github.com/fsprojects/fantomas/blob/ef132c18560c104a26efdac518b834fde896e150/src/Fantomas/CodePrinter.fs#L1671-L1674

I'm not quite sure about this, but that would be my starting point.

Are you interested in submitting a PR for this?

nojaf avatar Feb 20 '22 15:02 nojaf

I'd have to learn my way around the codebase -- I've not peered under the hood here before.

SteveGilham avatar Feb 20 '22 15:02 SteveGilham

In case you are interested, I've made some videos that explain a bit how the code in Fantomas works: https://www.youtube.com/playlist?list=PLvw_J2kfZCX3Mf6tEbIPZXbzJOD1VGl4K

nojaf avatar Feb 20 '22 15:02 nojaf

I believe this problem was solved via https://github.com/fsprojects/fantomas/pull/2513. A regression test could close this issue.

nojaf avatar Sep 18 '22 17:09 nojaf

The file from which this example was extracted now simply yields

Processing .\AltCover.Tests\Tests2.fs
Formatting .\AltCover.Tests\Tests2.fs lead to invalid F# code

with no further hint as to what was detected; so I cannot currently state whether this issue has been resolved.

SteveGilham avatar Sep 21 '22 17:09 SteveGilham

If you pass --force to the fantomas command, it should force it to print the output and you should then be able to see the output even if it is invalid.

josh-degraw avatar Sep 21 '22 20:09 josh-degraw

Having used --force, I see the issue still recurring with fantomas 5.0.0; presumably this is what was detected as invalid code.

So far, alas, I have not been able to create a cut-down example through the web tool. The configuration I'm using is thus -- the only customisations I recall making since it was generated are the first two lines,

indent_size=2
max_line_length=90

but I notice several differences in width parameters compared with the tool defaults. Tweaking the tool settings to match the .editorconfig still did not suffice for a repro, though.

Related indentation issue (just warnings here) this sample

Code

        DotNet.test
            (fun p ->
                (({ p.WithCommon(withWorkingDirectoryVM "_Issue23") with
                                                                         Configuration = DotNet.BuildConfiguration.Debug
                                                                         NoBuild = false })
                     .WithAltCoverOptions
                     pp0
                     cc0
                     ForceTrue)
                    .WithAltCoverImportModule()
                    .WithAltCoverGetVersion()
                |> testWithCLIArguments)
            ""

Result

DotNet.test
  (fun p ->
    (({ p.WithCommon(withWorkingDirectoryVM "_Issue23") with
        Configuration = DotNet.BuildConfiguration.Debug
        NoBuild = false })
       .WithAltCoverOptions
       pp0
       cc0
       ForceTrue)
      .WithAltCoverImportModule()
      .WithAltCoverGetVersion()
    |> testWithCLIArguments)
  ""

Fantomas master branch at 2022-09-20T21:08:34Z - 11c2a862e2b3828627c19ea339f2db48d556e268

    { config with
                IndentSize = 2
                MaxLineLength = 90
                MaxIfThenElseShortWidth = 40
                MaxInfixOperatorExpression = 50
                MaxArrayOrListWidth = 40
                MaxValueBindingWidth = 40
                MaxDotGetExpressionWidth = 50
                NewlineBetweenTypeDefinitionAndMembers = false }

SteveGilham avatar Sep 22 '22 10:09 SteveGilham

Note: fantomas 5.0.2 has resolved the original issue -- presumably why I was unable to produce a repro using the online tool.

The related issue in the comment above still remains, however.

SteveGilham avatar Sep 26 '22 10:09 SteveGilham

Note: fantomas 5.0.2 has resolved the original issue

That is good enough for me, please create new issues for new cases. What you list in your comment is a separate issue. Though both do mention a record update expression, that does not mean they are one and the same problem.

nojaf avatar Sep 26 '22 11:09 nojaf

New issue #2529 created.

SteveGilham avatar Sep 26 '22 11:09 SteveGilham

Thanks, I prefer to close issues when a regression test was added.

nojaf avatar Sep 26 '22 11:09 nojaf