Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overly aggressive de-indentation #2110

Closed
1 of 3 tasks
SteveGilham opened this issue Feb 20, 2022 · 11 comments · Fixed by #2564
Closed
1 of 3 tasks

Overly aggressive de-indentation #2110

SteveGilham opened this issue Feb 20, 2022 · 11 comments · Fixed by #2564
Labels
bug (soundness) good first issue Long hanging fruit: easy issue to get your feet wet!

Comments

@SteveGilham
Copy link

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

  • 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 SteveGilham changed the title <Insert meaningful title> Overly aggressive de-indentation Feb 20, 2022
@nojaf
Copy link
Contributor

nojaf commented Feb 20, 2022

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

| NewlineInfixApp (operatorText, operatorExpr, (Lambda _ as e1), e2)
| NewlineInfixApp (operatorText, operatorExpr, (IfThenElse _ as e1), e2) ->
genMultilineInfixExpr astContext e1 operatorText operatorExpr e2

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

Are you interested in submitting a PR for this?

@SteveGilham
Copy link
Author

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

@nojaf
Copy link
Contributor

nojaf commented Feb 20, 2022

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
Copy link
Contributor

nojaf commented Sep 18, 2022

I believe this problem was solved via #2513.
A regression test could close this issue.

@nojaf nojaf added the good first issue Long hanging fruit: easy issue to get your feet wet! label Sep 18, 2022
@SteveGilham
Copy link
Author

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.

@josh-degraw
Copy link
Contributor

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.

@SteveGilham
Copy link
Author

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 - 11c2a86

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

@SteveGilham
Copy link
Author

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.

@nojaf
Copy link
Contributor

nojaf commented Sep 26, 2022

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.

@SteveGilham
Copy link
Author

New issue #2529 created.

@nojaf
Copy link
Contributor

nojaf commented Sep 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug (soundness) good first issue Long hanging fruit: easy issue to get your feet wet!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants