Fix: Selecting an unopened type in completion opens its Namespace in invalid places

[Booksbaum]

Selecting a type in an unopened namespace, auto-opens that namespace.
BUT: that open gets inserted sometimes in wrong places:

• directly before some existing statement; like directly before let -- on same line and without spaces between
  

  • Reason: open gets inserted at correct place -- but doesn't do a line break to move existing stuff a line down
    -> works when there's an empty line (by consuming it), but produces invalid result otherwise.

• incorrect indentation
  

  • Reason: TextEdit for new open wants to insert it at some Column > 0. But if there's not enough existing text, it gets inserted as earliest point (example: Column=4, but empty line (incl. no spaces) -> text gets inserted at beginning of line -> Column=0)

This PR fixes these two cases:

Note:
There are no tests, because I couldn't get the F# parser, checker, code completion, etc. in tests to work. That's already an existing issue, see tests in CI builds, like https://github.com/fsharp/FsAutoComplete/runs/2765381976#step:7:407 .

-> I tested it by hand in ionide with "FSharp.fsac.netCoreDllPath": "[..]\\FsAutoComplete\\src\\FsAutoComplete\\bin\\Debug\\net5.0\\fsautocomplete.dll" set.

Used code

(all Regex are unopened -> cursor after Regex and Ctrl+Space)

module OpenNamespace.RootModule

let foo: Regex =
  Regex

module Nested1 =
  let foo: Regex =
    Regex
  
  module Nested11 =
    let foo: Regex =
      Regex

module Nested2 =

  let foo: Regex =
    Regex

  module Nested21 =

    let foo: Regex =
      Regex

module Nested3 =
  open System

  let foo: Regex =
    Regex

  module Nested31 =
    open System

    let foo: Regex =
      Regex

module Nested4 =
  open System
  let foo: Regex =
    Regex

  module Nested41 =
    open System
    let foo: Regex =
      Regex

module Nested5 =
  let bar () = ()

  let foo: Regex =
    Regex

  module Nested51 =
    let bar () = ()

    let foo: Regex =
      Regex

module Nested6 =
  module Nested51 =
    let foo: Regex =
      Regex

  let foo: Regex =
    Regex

Note:
Code completion and code fix (Ctrl+., https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete/CodeFixes/ResolveNamespace.fs) open namespaces in different places:

• Code Completion: Nearest possible place (-> nearest module)
• Code fix: Top Level (-> root module)

Might be reasonable to unify the behaviour of these two -- or even better make it a choice with code fix (and for code completion maybe a setting?).

[baronfel]

Nice work! I can see if I can add in some tests for this specifically (the code fixes at least have a good test suite that makes this pretty easy to verify), but we can try to hit the code-completion use case as well.

Regarding your note, I've noticed that myself and haven't yet come to a decision on how to handle it. I think it's safest to go to Nearest by default, but I do agree that we could easily make it an option for the user for the codefix. For code completion I think a user setting would be the correct way to go, which we would just need to add to the config DTO and flow down into other layers as appropriate.

If you don't mind, could you write a new issue with your findings there for the mismatched behavior?

[Booksbaum]

Issue created: #789

---

About tests: yes, have seen these tests too. Can be easily adjusted too return code completion instead of code fixes.
But issue isn't creating the tests, but instead running them: Used F# parser doesn't return any diagnostics (errors) when it should. And Code Completion request just returns the default, "I found nothing -- just return the default keywords" list instead of correct completion list.

But CI build has the same issues as my local build (and had the issues for quite some time) -- so I didn't give it too much thought.

[baronfel]

What OS are you on? I ask because the macos (which I run) and ubuntu runs are generally ok, but there are a few tests on the windows run that do case-sensitive path comparisons and seem to have timing issues that I haven't been able to root out yet.

[Booksbaum]

I'm using Windows 10

---

In CI build log for Windows: (like current master: https://github.com/fsharp/FsAutoComplete/runs/2765381976#step:7:407 )
Search for Should have had errors, these are Tests in CodeFixesTests.fs with some expected pare error. In tests::

      do! server.TextDocumentDidOpen tdop
      let! diagnostics = waitForParseResultsForFile "Script.fsx" events |> AsyncResult.bimap (fun _ -> failtest "Should have had errors") (fun e -> e)

Should have an error, but was nothing.

Mac & Linux failed too, so I assumed these two have same errors -- but no: CodeFixesTests succeeded on both

Edit: It works in WSL -- so seems to be a windows issue

Running lsp.Ionide WorkspaceLoader.codefix tests.missing instance member.can add this member prefix:

• On Windows:

...
[21:19:51 ERR] lsp.Ionide WorkspaceLoader.codefix tests.missing instance member.can add this member prefix failed in 00:00:01.7530000.
Should have had errors
   at [email protected](FSharpChoice`2 _arg11) in E:\prog\fsharp\editor\ionide\FsAutoComplete\test\FsAutoComplete.Tests.Lsp\Helpers.fs:line 167
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, FSharpFunc`2 userCode, b result1) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 404
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 104 <Expecto>
...
Failed:    1
        lsp.Ionide WorkspaceLoader.codefix tests.missing instance member.can add this member prefix

• on Linux (WSL):

...
Passed:    1
        lsp.Ionide WorkspaceLoader.codefix tests.missing instance member.can add this member prefix

[Booksbaum]

Ok...updating .net version on linux is not that great...: newer .net version (5.0.3xx) is not compatible with old version (5.0.2xx) -- and unlike on windows these two are not installed side by side (at least via apt).

---

Ok, but at least I have a method to run tests.

-> I can write the Tests, no need for you to do it.
But not today.

[baronfel]

Yeah, the apt install method leaves a lot to be desired :) For my production hosts I install side by side for this reason. I appreciate you being willing to add tests, and look forward to merging this once they're present!

[Booksbaum]

Tests added.

Because of several Heisenbugs that disappeared while trying to pinpoint the issues and rewriting the code while doing this, there are quite a lot of tests... but well -- no such thing as too many tests....

Note for anybody who wants to unify open location from code completion and quick fix (#789):
The Tests contains a (disabled) comparison between open locations of code completion and quick fix. Should be useful for for implementing that issue, because it allows to reuse the tests and tests both method together.

[baronfel]

these are great, thank you very much for taking the time to write such an exhaustive suite! I'm going to merge this and one other PR and release them as 0.46.2 later today.