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

bugfix/issue-2485-sequentials-stack-overflow #2495

Merged

Conversation

njlr
Copy link
Contributor

@njlr njlr commented Sep 9, 2022

  • Switches Sequentials to an imperative impl for efficiency and to not blow up stack

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you for taking the initiative to solve this problem.

I'm going to sound very purist here but I'm not a fan of the solution.
We have solved these problems in the past via tail recursion, I typically use this blog post as a reference.

This PR also misses a test case to illustrate the problem is indeed solved.
Could you add a new test file ASTTransformerTests.fs (module Fantomas.Core.Tests.ASTTransformerTests)

with some unit test like:

#if RELEASE
open FSharp.Compiler.Text
open FSharp.Compiler.Xml
open FSharp.Compiler.Syntax
open FSharp.Compiler.SyntaxTrivia

[<Test>]
let ``avoid stack-overflow in long array/list, 2485`` () =
    let testIdent = Ident("Test", Range.Zero)

    let mkStringExpr () =
        SynExpr.Const(
            SynConst.String((System.Guid.NewGuid().ToString("N"), SynStringKind.Regular, Range.Zero)),
            Range.Zero
        )

    let longArrayExpr: SynExpr =
        let rec mkArray count childExpr =
            if count = 20_000 then
                childExpr
            else
                mkArray
                    (count + 1)
                    (SynExpr.Sequential(
                        DebugPointAtSequential.SuppressNeither,
                        true,
                        mkStringExpr (),
                        childExpr,
                        Range.Zero
                    ))

        SynExpr.ArrayOrListComputed(true, mkArray 0 (mkStringExpr ()), Range.Zero)

    let rootNode =
        Fantomas.Core.AstTransformer.astToNode
            Range.Zero
            []
            [ SynModuleOrNamespace(
                  [ testIdent ],
                  false,
                  SynModuleOrNamespaceKind.AnonModule,
                  [ SynModuleDecl.Expr(longArrayExpr, Range.Zero) ],
                  PreXmlDoc.Empty,
                  [],
                  None,
                  Range.Zero,
                  { ModuleKeyword = None
                    NamespaceKeyword = None }
              ) ]

    Assert.Pass()
#endif

Notice that #if RELEASE will be required for that test to succeed.

Also please take a look at the PR checklist

Many thanks

src/Fantomas.Core/SourceParser.fs Outdated Show resolved Hide resolved
@njlr
Copy link
Contributor Author

njlr commented Sep 9, 2022

Thanks @nojaf I have followed the steps in the checklist 👍

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nojaf nojaf merged commit e1bd963 into fsprojects:master Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants