Don't add newline to module where the first trivia is a newline

[ErikSchierboom]

I've taken a stab at fixing #784, but I'd like to check if I'm heading in the right direction first. The problem is that the sepModuleAndFirstDecl checks for any declaration. If there is no such declaration, two newlines are added after the module declaration. As comments are no declarations, this means that the comment will also receive two newlines. The main problem is that running format one more time after that, will add another newlines (there will be three at this point), and so on.

Let me know if I'm heading in the right direction with my solution.

[nojaf]

Can be simplified to

        if hasPrintableContent node.ContentBefore || hasPrintableContent node.ContentAfter then
            ctx
        else
            sepNln ctx

Because I think this won't work if you have

module foo
// bar
// meh

[nojaf]

@ErikSchierboom you are in the right direction here.
Maybe change sepNlnConsideringTriviaContentBeforeAndAfter to sepNlnForEmptyModule because I don't think this edge case can happen anywhere else at first glance.

Please also fix this for signature files, as the bug appears there as well.

You current fix will also not work for namespaces I think.
Consider this example, notice that the namespace has a weird range in the AST.

ImplFile
  (ParsedImplFileInput
     ("tmp.fsx",true,QualifiedNameOfFile Tmp$fsx,[],[],
      [SynModuleOrNamespace
         ([Foo],false,DeclaredNamespace,[],
          PreXmlDoc ((1,9),FSharp.Compiler.XmlDoc+XmlDocCollector),[],None,
          tmp.fsx (3,6--3,6) IsSynthetic=false)],(true, true)))

The comment is linked after the ident instead there:

[ErikSchierboom]

Alright thanks! I'll work on this some more.

[nojaf]

Nitpick: I'd prefer a not using the TestCase attribute but duplicate the test to SignatureTests.fs.
To remain consistent with the rest of the codebase and to easier spot regressions.
This is clear now but it will be confusing that a test fails in ModuleTests while changing something in Signature AST for example.

[ErikSchierboom]

Will do!

[nojaf]

I would move this function to TriviaHelpers.fs

[ErikSchierboom]

Ah, I thought it should be added here due to Context.fs already having findTriviaMainNodeOrTokenOnStartFromRange and findTriviaMainNodeFromRange functions. But I can move it to TriviaHelpers.fs. Would you like it to be renamed?

[nojaf]

Yeah TriviaHelpers came after we already have some stuff in Context.fs
Not everything was perfectly moved to TriviaHelpers.
Please move and rename your function, we can move the other functions in a cleanup PR.

TriviaNodeType can only be two things so the Trivia.isMainNode and Trivia.isToken can also be left out I think.

[ErikSchierboom]

Alright, done! I've renamed the function to findInRange to better fit with the existing findByRange function in TriviaHelpers.fs. Let me know if that is okay or if you' prefer a different name.

[nojaf]

Hey @ErikSchierboom! Very readable code, many thanks for this PR.

[ErikSchierboom]

Thanks for the review! I'll apply your suggestions (tomorrow).