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

Detecting changes outside of loaded buffer #603

Open
i-sinister opened this issue Jul 2, 2020 · 28 comments
Open

Detecting changes outside of loaded buffer #603

i-sinister opened this issue Jul 2, 2020 · 28 comments

Comments

@i-sinister
Copy link

Quite often I find myself manually restarting server with 'OmniSharpRestartServer' when there are changes made outside of current buffer. Without doing this autocompletion and code-checks do not work. Examples are:

  • git operation
    • fetching changes (I prefer to close all buffers to prevent vim from asking if file should be reloaded)
    • rebasing (code-checks are a "must have" especially when resolving conflicts)
    • discarding changes in a file (may not be in a loaded buffer)
  • after dotnet cli operations:
    • adding package
    • adding reference
    • adding project
  • file operations:
    • renaming file in a vim ("Rename file" code action never worked for me)
    • adding new classes with ":e MyNewClass.cs"
  • after build when project that uses generated assets (t4 templates, api/schema definition, etc)

Is it possible to teach omnisharp to do this automagically? For example after running build it could check timestamps of files or if new files are available.

@nickspoons
Copy link
Member

Some of these things I don't have any trouble with, particularly :e NewFile.cs which works fine - OmniSharp-vim happily sends the new file along to OmniSharp-roslyn which includes it in the project.

I also think git operations have been working quite well - I'm only asked to reload files if they are modified?

Generally speaking, the idea of watching files isn't something we can do on the OmniSharp-vim end (although we can probably do a better job of keeping modified open buffers in sync with the server). I think there's been some talk of file watching on the OmniSharp-roslyn end? Not sure what the state of that is though.

  • Are you using the latest version of the stdio server?
  • Do you use highlighting?

The highlighting actually does a pretty good job of keeping open buffers synchronised with the server, since it is triggered on TextChanged (when g:OmniSharp_highlighting = 2)

Renaming files is an unfortunate one, I (or someone else) need to take a look at that open issue and get renaming via code action working.

@nickspoons
Copy link
Member

I just saw that the TextChanged autocmd is only triggered for changed to the current buffer, so that's not helping keep files synchronised with the server. Perhaps adding a FileChangedShell autocmd triggering an /updatebuffer call would help?

Also, I have set autoread in my .vimrc which helps, but this is perhaps what you try to avoid with your git operations?

@i-sinister
Copy link
Author

i-sinister commented Jul 2, 2020

Thank you for the quick response.

particularly :e NewFile.cs which works fine

I've just checked and indeed it worked - I must be remembering it wrong or it was some specific scenario - maybe new class was added by the commit pulled and usage of new class was added to loaded buffer.

I also think git operations have been working quite well - I'm only asked to reload files if they are modified?

Does it work if changed file is not loaded into buffer?

Generally speaking, the idea of watching files isn't something we can do on the OmniSharp-vim

Correct, I guess most of this issue is about server side

Are you using the latest version of the stdio server?

VIM - Vi IMproved 8.1, OmniSharp version "1.35.0.0", stdio is enabled

Do you use highlighting?

Yes, g:OmniSharp_highlighting = 3

Also, I have set autoread in my .vimrc which helps,

Thanks for the hint - I guess this is what I needed. I'll check if it works for me.

@i-sinister
Copy link
Author

One more case:

  • moved file with NERDTree to different folder (old buffer removed, new created)
  • executing global check returns errors/warning with the former file path, and trying to navigate opens empty buffer

@nickspoons
Copy link
Member

nickspoons commented Jul 3, 2020

Right, in that case we haven't told the server about the removed file, so it thinks it's still there.

A server-side file watcher would solve it (and all the other issues). I'm not sure how feasible it otherwise is to catch file movements like that in Vim...

Interestingly, navigating to the missing file ought to fix the problem, because we'll send the now empty buffer to the server.

@DasCleverle
Copy link

It might be a good thing to checkout how omnisharp-vscode handles file renaming/moving. In my experince it worked fine.

@nickspoons
Copy link
Member

I've just fixed #454 - "rename" code actions now work correctly. Is that what you're referring to, @DasCleverle, or more general handling of renames in vim?

@DasCleverle
Copy link

I mean the general things, like moving a file in NERDTree. IMO omnisharp-vim should detect the moved file and send the change to the server. Or better, the server should detect changes to the project files and update its state internally.

@nickspoons
Copy link
Member

We might be able to detect non-focused loaded files being moved with autocmds on the vim side, but we won't be able to detect changes to files that haven't been loaded yet, or detect what a file has been renamed to if it happens externally.

I agree this should be happening on the server, but don't know if there are plans for file watchers on all C# files in OmniSharp-roslyn though.

@i-sinister
Copy link
Author

i-sinister commented Aug 7, 2020

Provided that we can not reliably detect all changes, does server api supports an "intelligent refresh" of the state? Currently I am doing 'server restart' but it is slow on a large code base and "intelligent" refresh could work faster if only few files were change - server knows files of the project/solution so it could check cached versus actual file content.

@nickspoons
Copy link
Member

As soon as you open a buffer in a window, OmniSharp-vim updates its state on the server. So if you have updated 2 or 3 files, you can quickly open them all in Vim and that will sync them with the server.

We could add functionality to make it easier to do this by calling a function with a filename or list of filenames to sync them, we just need to work out what this should look like. I suspect it's mostly possible already but I'm mobile right now and can't check the code.

@nickspoons
Copy link
Member

I spoke to David Driscoll on slack the other day and he told me this:

omnisharp-vscode does setup a watcher for the workspaces, so for now I'm more or less mimicking that, that gets piped to the fileschanges api at the moment.

So vscode does perform its own file watching. We can't do that in Vim, although we should be able to have autocmds handle changes to .cs buffers which have been opened. This is something I haven't had time to look into yet.

@zatherz
Copy link

zatherz commented Aug 19, 2020

I still have to run :OmniSharpRestartAllServers, create a new blank buffer, delete all previous buffers and then reopen files when I add a file, otherwise I get bogus "class already defined" errors. Is not having to restart the server every single time you add or rename a file really such low priority?
Edit: I've looked around omnisharp-roslyn, and apparently there's a 5 year old pull request to add at the very least a /reloadsolution endpoint. Never merged. It fascinates me what a mess .NET is outside of the official sanctioned Windows/Visual Studio environment, despite the whole Microsoft heart emoji Linux campaign and .NET Core.

@nickspoons
Copy link
Member

@zatherz I'm not sure who you think you're talking to, but there's no Microsoft here, just users contributing to a tool we like.

Why would you restart all servers instead of just the one you're currently in? What do you want a /reloadsolution endpoint for, that :OmniSharpRestartServer doesn't do?

There's certainly no need to restart the server when you add a file, that works fine. The issue is with renaming a file, we don't have a good system in place for detecting that that is what happened, when it is done outside OmniSharp-vim.

I had a bit of a look into autocmds and I'm no longer so sure we can detect external changes as I had hoped, so restarting the server after a git checkout may be unavoidable. There are vim processes for detecting when the current file has changed externally, but as far as I could see these don't apply to non-active buffers. I would love to be proven wrong here.

As for renaming files in vim, you just need to be aware of what is happening. OmniSharp-roslyn will still think the file exists until you tell it it doesn't. One way to do this is to manually open the old filename, which will presumably be empty now (depending on how you performed your rename). The empty buffer will be sent to the server, to update its internal representation of the file, after which you won't see duplicates any more. If, however, your buffer is not empty (because you renamed it externally or used a tool which didn't clean up after itself), then you can ggdG your buffer (again, to update the server state) and remove it yourself with :bw.

@zatherz
Copy link

zatherz commented Aug 24, 2020

I was under the impression that this issue was also about adding files, since that by itself produces a redefinition error for me.
nvim-qt

let g:OmniSharp_highlight_types = 2
let g:OmniSharp_server_stdio = 1
let g:ale_linters = {
\ 'cs': ['OmniSharp']
\}

@nickspoons
Copy link
Member

OK I don't experience that. I will try to build a very minimal vim config later on but here's a short demo of me trying to recreate your demo.

asciicast

@nickspoons
Copy link
Member

@zatherz I just tried a very similar test (without NERDtree) using this .vimrc:

filetype off

let &runtimepath .= ',~/.vim/plugged/ale'
let &runtimepath .= ',~/.vim/plugged/omnisharp-vim'

filetype plugin indent on
syntax on

let g:ale_linters = {
\ 'cs': ['OmniSharp'],
\}

I still get OmniSharp support for the new file, and no warning about the global namespace already containing Test.

Can we see your full .vimrc?

@zatherz
Copy link

zatherz commented Aug 24, 2020

Sure: init.vim. OmniSharp version is 1.37.0, NeoVim version is 0.4.3-3 and apparently someone is experiencing this in VSCode, although I don't know how relevant that is.

@nickspoons
Copy link
Member

This is an asciinema of me using your config, in the project I created in the previous asciinema:

asciicast

Clearly there's something else going on here. How did you create your project? Mine is:

mkdir tmp
cd tmp
mkdir test_project
cd test_project
dotnet new console
cd ..
dotnet new sln
dotnet sln add test_project/test_project.csproj
dotnet build
nvim test_project/Program.cs

In your demo, is there any chance you have previously created another file, filled it with class Test {} and then deleted it? Because that would result in this situation, as we haven't told OmniSharp-roslyn that that file should be removed from the workspace. That's the only way I can see how to reproduce what happens in your demo.

@zatherz
Copy link

zatherz commented Aug 25, 2020

asciicast

@nickspoons
Copy link
Member

So weird! I don't know what else I can do to repro this, we seem to be doing exactly the same things.

@nickspoons
Copy link
Member

@zatherz since we've come this far, do you want to try to add let g:OmniSharp_loglevel = 'debug' to your config, run your demo again, and share the :OmniSharpOpenLog? Then I can do a direct comparison with what happens on my system and try to drill down into where they are diverging.

@zatherz
Copy link

zatherz commented Aug 27, 2020

stdio.log

@nickspoons
Copy link
Member

@zatherz I think you might be using a very outdated version of OmniSharp-vim. The logs now look like this:

NVIM v0.5.0-635-g8c49e3d50

OmniSharp server started.
    Path: /home/nickspoon/.cache/omnisharp-vim/omnisharp-roslyn/run
    Target: /mnt/c/code/test/unittests/tmp.sln
    PID: 18498

[info]: OmniSharp.Stdio.Host
        Starting OmniSharp on arch 0.0 (x64)
[info]: OmniSharp.Services.DotNetCliService
        DotNetPath set to dotnet
[info]: OmniSharp.MSBuild.Discovery.MSBuildLocator
        Located 1 MSBuild instance(s)
            1: StandAlone 16.8.0 - "/home/nickspoon/.cache/omnisharp-vim/omnisharp-roslyn/omnisharp/.msbuild/Current/Bin"
[info]: OmniSharp.MSBuild.Discovery.MSBuildLocator
        MSBUILD_EXE_PATH environment variable set to '/home/nickspoon/.cache/omnisharp-vim/omnisharp-roslyn/omnisharp/.msbuild/Current/Bin/MSBuild.exe'
...

@zatherz
Copy link

zatherz commented Aug 27, 2020

Here's me wiping the entire omnisharp-roslyn dir and letting omnisharp-vim reinstall OmniSharp: asciicast

@nickspoons
Copy link
Member

Right, that's the server, OmniSharp-roslyn.

But the logs are written by OmniSharp-vim, the client. Can you try to git pull or :PlugUpdate or however you update your vim plugins? I don't know if it will help this situation but a lot has changed since our logs looked like the one you posted.

@zatherz
Copy link

zatherz commented Aug 27, 2020

I guess I'm still too much of a newbie to vim/neovim because I assumed vundle's PluginInstall also updated plugins, and somehow did not notice PluginUpdate existed. Indeed after updating I'm no longer experiencing the issue with creating new files. Sorry for all the trouble.

@nickspoons
Copy link
Member

Oh that's great to hear, it's always good to be able to get to the bottom of these things.

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

No branches or pull requests

4 participants