WIP: Improve sln scope handling

[Melandel]

Given a server was requested to start
When the projects are done loading
Then any existing buffer whose path is under one of the projects should be associated with the server

Given a server was running
When a new buffer is edited
Then omnisharp should try to associate it with a running server before trying to find a potential sln_or_dir in its parent folders

[nickspoons]

Hi @Melandel,

Can you give an example of a folder/solution structure where this works differently to the current behaviour?

Or is this about performance benefits, not changing behaviour?

[Melandel]

Sure.

root
|-- dirA
    |-- module1 (referenced by every sln)
    |-- module2 (referenced by every sln1 only)
    `-- sln1.sln
|-- dirB
    |-- module3 (referenced by every sln)
    |-- module4 (referenced by every sln2 only)
    `-- sln2.sln
|-- module5 (referenced by every sln)
|-- module6 (referenced by every sln3 only)
`-- sln3.sln

After starting vim

ie, no omnisharp server running.
If we take a look at b:OmniSharp_host.sln_or_dir:

	sln_or_dir == sln1.sln	sln_or_dir == sln2.sln	sln_or_dir == sln3.sln
Equality is true for files inside...	module1 module2	module3 module4	module5 module6

I'm not changing this behavior.

After :OmniSharpStartServer dirA/sln1.sln

ie, when sln1's projects are done loading

Before the PR

	sln_or_dir == sln1.sln	sln_or_dir == sln2.sln	sln_or_dir == sln3.sln
Equality is true for files inside...	module1 module2	module3 module4	module5 module6

Pain point

I don't have access to the intellisense from a file inside module3 or module5, despite the fact that they are referenced by sln1

After the PR

	sln_or_dir == sln1.sln	sln_or_dir == sln2.sln	sln_or_dir == sln3.sln
files inside...	module1 module2 module3 module5	module4	module6

Value

I have access to the intellisense from a file inside any module that is referenced by sln1

[nickspoons]

I'm not sure about this last commit.

After the PR

	sln_or_dir == sln1.sln	sln_or_dir == sln2.sln	sln_or_dir == sln3.sln
files inside...	module1 module2 module3 module5	module4	module6

It seems like this last commit will close sln2.sln and sln3.sln, so module4 and module6 will no longer have a running server?

What about situations where you are working in module6 and you are finding the places where code from module5 is used in module6 (let's say you're finding code test coverage, where module5 is a library and module6 is a test class). You then go to module1 for something, triggering a load of sln1 and module5 files are now associated with sln1 instead of sln3. This means that you can no longer find usages in module6, only in other sln1 solutions. I realise that this is the behaviour you want, but how can we be sure that this is what everyone wants?

[nickspoons]

OK I've been thinking about it a bit more and I think that, while it should be possible to do what you're doing here, I don't think it should be default behaviour. Having buffers change from one solution to another may not fit with other users' expectations.

How about this. We keep the changes that have been made to autoload/OmniSharp-vim, but instead of the autoload/OmniSharp/actions/workspace.vim changes, we add a variable callback that users can configure, e.g.:

  " autoload/OmniSharp/actions/workspace.vim line 33
  if exists('g:OmniSharp_server_loaded_callback')
    call function(g:OmniSharp_server_loaded_callback, [a:job])
  endif
endfunction

Then in a user config, I could do this:

" .vimrc
let g:OmniSharp_server_loaded_callback = 'OnSolutionLoaded'
function! OnSolutionLoaded(job)
  echmsg 'Solution ' . a:job.sln_or_dir . ' loaded, with ' . len(a:job.projects) . 'projects'
endfunction

And you could implement the autoload/OmniSharp/actions/workspace.vim code from this PR in a similar way.

What do you think?

[nickspoons]

I've tried out the latest commits and immediately hit 2 issues.

The first is a breaking behavioural change which is incorrect.

I have a project structure like this:

root
|-- test
    |-- test.csproj
    `-- test.sln
|-- main.csproj
`-- main.sln

Each solution is distinct - main.sln does not include test.csproj.

I open a file under main.csproj which launches main.sln, as expected. I then open a file under test.csproj. This is associated with main.sln and doesn not start a new OmniSharp-roslyn server for test.sln, which it should.

---

The second issue is a bug, causing a vim error. I opened a fresh vim, and opened a file under test.csproj, which loaded test.sln. I then opened a file under main.csproj, which loaded main.sln. This matches current behaviour and was expected. I went back to the test file which now should have 2 available running servers it can switch between. I entered :OmniSharpPickRunningServer <Tab> expecting tab completion between the 2 servers, and got this error:

Error detected while processing function OmniSharp#CompleteOtherRunningSlnOrDirCoveringCurrentFile:
line    5:
E716: Key not present in Dictionary: "projects, "fnamemodify(v:val.path, ':p:h')")"
E116: Invalid arguments for function mapnew(OmniSharp#proc#GetJob(runningJob).projects, "fnamemodify(v:val.path, ':p:h')")

I have also struck similar errors when loading 2 servers simultaneously.

I also note that you have replaced mapnew() with map(copy(...)) in 2 locations in order to support older vims, but not the usage mentioned in the above error message.

---

My conclusion at this point is to come back to my previous comment: I think it's good to be able to manage this stuff programatically and OmniSharp-vim should provide open APIs and callbacks to allow coding up custom workflows, but I still think a lot of this behaviour is too niche and workflow-specific to be included as standard behaviour.

I like :OmniSharpPickRunningServer, I can see that being useful in various situations, including refactoring a file from one solution to another (we move the file using e.g. fugitive's :GMove ../SomeOtherSolution/ but the buffer is still associated with the old server). However, in this case the :OmniSharpPickRunningServer command as it is now won't work, as the new solution will probably not share paths with the old one, and when there is only 1 running solution covering the current file, no completion options are returned for this command. So I think this command should actually return all running servers, not just those which cover this file path, but sorted with the "closest" solutions first, so the top server will normally be the correct option.

Having said that, it is again probably quite niche behaviour. How many people will use it, or even learn that this behaviour exists? :OmniSharpPickRunningServer is entirely implemented using public APIs - it could just as well be part of a personal config, with an article in the wiki showing the code and describing how to use it?

[Melandel]

I reproduced your example layout using

dotnet new sln -n main -o root
dotnet new classlib -n main -o root
dotnet sln root add root
dotnet new sln -n test -o root/test
dotnet new classlib -n test -o root/test
dotnet sln root/test add root/test

Is it a folder layout that makes sense in real life, or was it created only for the purpose of testing this feature?

It is the very first time I see a project contain a folder holding a csproj file - not to mention a sln file, I did not know it was a thing.

I am highly intrigued - what is the expected behavior on compilation of main.csproj? Does it include the source code from the test folder?

This is the solution explorer view on Visual Studio:

If I write code that does not compile inside test/Class1.cs, Visual Studio says main.csproj refuses to build.

Based on this observation, my understanding is that given this folder structure, main.csproj includes the source files inside the test directory.

Because test/Class1.cs is considered to be part of main.sln (according to Visual Studio), I would expect the currently running server to be reused.

Perhaps you meant to reproduce the folder structure I described in this PR, which would be the following? Please let me know.

root
|-- test
    |-- testmodule
        `-- testmodule.csproj
    `-- test.sln
|-- main
    |-- mainmodule
        `-- mainmodule.csproj
    `-- main.sln
`-- root.sln

---

I entered :OmniSharpPickRunningServer expecting tab completion between the 2 servers, and got this error

Strange, it works on my machine.
Which version of vim are you using?

I also note that you have replaced mapnew() with map(copy(...)) in 2 locations in order to support older vims, but not the usage mentioned in the above error message.

I modified the necessary calls in order to pass the vader tests indeed, but am still not clear on which syntax to use, and was waiting for guidance in the code review.

---

I am waiting for your answer in order to acknowledge that the following structure is valid (my current understanding being that it is not, as described earlier in this post with the Visual Studio test):

root
|-- test
    |-- test.csproj
    `-- test.sln
|-- main.csproj
`-- main.sln

If this is indeed a valid structure, I would agree that the behaviour should be coded programmatically outside of omnisharp-vim.

However, if it is not a valid structure, I would argue that reusing existing servers makes more sense than firing up new servers, when applicable

In any case, it would definitely be an interesting option to have a user-defined callback entry at that location - whether or not we decide to use the behavior implemented in this PR as default behavior or not

[Melandel]

we move the file using e.g. fugitive's :GMove ../SomeOtherSolution/ but the buffer is still associated with the old server

I personally had issues with dirvish where I rename a file, and OmniSharp complains that there are duplicate classes in my code. I found that annoying, and found it more practical to delete the buffer when changing a filename, rather than restarting the server. I suppose the same can be applied with GMove, but I do not know fugitive's api very well.

as the new solution will probably not share paths with the old one, and when there is only 1 running solution covering the current file, no completion options are returned for this command. So I think this command should actually return all running servers

My understanding is that in this PR, we use all the running servers except the one in OmniSharp#GetHost().sln_or_dir.

If your assumption is correct (but the buffer is still associated with the old server), then I do not understand the issue, since all running servers except the former -now incorrect- will be displayed in the autocompletion.

[nickspoons]

I apologise, no I was using an over-simplified example to explain the scenario.

Here is a "real" version of what I was describing:

mkdir main
dotnet new sln
dotnet new classlib -n main
dotnet sln add main
dotnet new mstest -n test
dotnet new sln -n test -o test
dotnet sln test add test/test.csproj

I can now build main with dotnet build and build the test sln with dotnet build test. After cleaning it up it looks like this:

$ rm -rf **/{bin,obj}
$ tree
.
├── main
│   ├── Class1.cs
│   └── main.csproj
├── main.sln
└── test
    ├── test.csproj
    ├── test.sln
    └── UnitTest1.cs

2 directories, 6 files

Now, 2 scenarios.

1. I open Class1.cs first with vim main/Class1.cs, then UnitTest1.cs with :vs test/UnitTest1.cs. Both files are associated with solution main.sln.
2. I open UnitTest1.cs first with vim test/UnitTest1.cs, then Class1.cs with :vs main/Class1.cs. Each file is associated with its closest solution, which is the same as our current behaviour.

In scenario 1, this demonstrates a breaking change from current OmniSharp-vim behaviour - currently UnitTest1.cs would be associated with main.sln. It is also wrong - UnitTest1.cs is not currently part of main.sln at all.

Also, when I run scenario 2 quickly, opening Class1.cs before the test solution has finished loading, I hit this error (not the same as the previously reported error):

Error detected while processing function OmniSharp#proc#vimOutHandler[11]..OmniSharp#stdio#HandleResponse[42]..<SNR>203_ProjectsRH:
line   35:
E716: Key not present in Dictionary: "projects), "fnamemodify(v:val.path, ':p:h')")"
E116: Invalid arguments for function copy(OmniSharp#proc#GetJob(runningJob).projects), "fnamemodify(v:val.path, ':p:h')")
E116: Invalid arguments for function map(copy(OmniSharp#proc#GetJob(runningJob).projects), "fnamemodify(v:val.path, ':p:h')")

---

Now, we can add the test project to main.sln as well, so the project is now part of both solutions:

dotnet sln add test/test.csproj

Scenario 1 from above works the same. Note though that I would expect to be able to use :OmniSharpPickRunningServer to move the file to test.sln since it is part of both solutions, but that is impossible, as that solution has not been started, and will never be started in this situation except by manually running :OmniSharpStartServer.

Re-running scenario 2, UnitTest1.cs starts in test.sln and then is moved to main.sln which is what you want here. And, once both projects are fully loaded so no errors are raised, I can move UnitTest1.sln back to test.sln if I choose with :OmniSharpPickRunningServer. Note that I could not do this before adding the test project to the main solution.

---

I entered :OmniSharpPickRunningServer expecting tab completion between the 2 servers, and got this error

Strange, it works on my machine.
Which version of vim are you using?

8.2.3516 - I built it on Friday. The error occurs when using :OmniSharpPickRunningServer before the server is completely loaded.

---

we move the file using e.g. fugitive's :GMove ../SomeOtherSolution/ but the buffer is still associated with the old server

I personally had issues with dirvish where I rename a file, and OmniSharp complains that there are duplicate classes in my code. I found that annoying, and found it more practical to delete the buffer when changing a filename, rather than restarting the server. I suppose the same can be applied with GMove, but I do not know fugitive's api very well.

Yes I currently do this too, I :bw the buffer and then re-open the file, and OmniSharp-vim can then re-associate the file with the correct project. Shouldn't :OmniSharpPickRunningServer be able to do this, a bit more cleanly?

My understanding is that in this PR, we use all the running servers except the one in OmniSharp#GetHost().sln_or_dir.

My point is that, because the server in OmniSharp#GetHost().sln_or_dir is excluded, no completions are provided after moving the file to a new solution/project path. And therefore the command cannot be easily executed, the user has to manually type out the path to the sln.

I think the completion function should be simplified to just include all running solutions, OmniSharp#CompleteRunningServers(). Then users can use it the way they like - maybe for other purposes than :OmniSharpPickRunningServer.