Semantic C# completer #365

Merged
merged 14 commits into from Jul 18, 2013

Conversation

Projects
None yet
7 participants
Contributor

Chiel92 commented Jun 8, 2013

This patch includes a semantic completer for C#. It utilizes Omnisharp (https://github.com/nosami/Omnisharp) to provide C# completion.
Basically, the idea is that omnisharp runs a server that knows everything about C#, while vim itself does not.
So, when some semantical C# information is needed (like the current possible completions), a request is sent to the server, which returns a response containing the right info.

The vimplugin for Omnisharp should be installed besides YCM, because it will take care of starting and stopping the server, and detecting your project file.
Besides that, omnisharp comes with some other features as well, which C# programmers may wanna check out.

Though not everything maybe perfectly finetuned yet, it does a pretty good job.
Some goto-definition commands aren't yet implemented, but it should be easy to implement them using omnisharp.

@Chiel92 Chiel92 referenced this pull request in OmniSharp/omnisharp-vim Jun 8, 2013

Closed

Any plans to integrate into YouCompleteMe's Completer API? #13

Contributor

JazzCore commented Jun 8, 2013

It's better to move any calls to vim ( like vim.eval on line 66 ) to the __init__ or top-level section because vim is not threadsafe and this may cause vim segfaults

python/ycm/completers/cs/cs_completer.py
+ response = urllib2.urlopen( target, parameters )
+ return response.read()
+ except:
+ vimsupport.PostVimMessage( "Could not connect to " + target )
@Valloric

Valloric Jun 9, 2013

Owner

This is not going to work because you're in a separate thread. Vim could crash if you access vim internals from outside the GUI thread.

So it's commendable that you want to notify the user of the error, but it would do more harm than good. Remove the PostVimMessage call.

python/ycm/completers/cs/cs_completer.py
+from ycm.completers.threaded_completer import ThreadedCompleter
+from ycm import vimsupport
+
+# Import stuff for Omnisharp
@Valloric

Valloric Jun 9, 2013

Owner

You don't need this comment.

python/ycm/completers/cs/cs_completer.py
+ parameters['buffer'] = '\n'.join( vim.current.buffer )
+ parameters['filename'] = vim.current.buffer.name
+
+ js = self.getResponse( '/autocomplete', parameters )
@Valloric

Valloric Jun 9, 2013

Owner

Rename js to json.

Owner

Valloric commented Jun 9, 2013

If this code depends on the omnisharp plugin being installed, won't YCM conflict with omnisharp's code completion?

From the omnisharp docs, it seems that completions already come pre-ranked into YCM. This is less than ideal; will it conflict with YCM's completion ranking or does it not matter?

I'd be much happier about merging this if it depended on OmniSharpServer instead of OmniSharp itself. The YcmCompleter system for custom completer commands would make it very easy to bring up/shut down the server or do any of the other C#-specific things.

I really really don't want to tell the user "hey we have this awesome feature you'll love but you have to go install this other plugin too and then learn it, configure it etc". The user experience should be "install YCM; done, use it, everything has sane defaults". This is why YCM pulls is Jedi as a submodule. To get good semantic completion for Python, the user merely opens a Python file and types.

In an ideal world, you'd be pulling in OmniSharpServer as a submodule and then using that directly. No need for a separate Vim plugin.

There are also fewer dependencies in YCM, fewer moving parts and it's easy to ensure the whole user experience is sound when YCM controls all the parts.

nosami commented Jun 10, 2013

The OmniSharp server is ranking the completions, not the client side. It doesn't seem to affect how YCM does things. The main autocomplete screenshot on OmniSharp was taken whilst using the YCM plugin. I need to make sure completions are ranked for users of other plugins/editors too.

@Chiel92 wrote the YCM plugin so that it doesn't pass in the partial word to complete to the OmniSharpServer. This basically causes OmniSharp to not rank completions at all as it doesn't have the characters to complete on.

There aren't any conflicts between the YCM plugin and OmniSharp, although I can see why you would want to depend on OmniSharpServer directly. OmniSharp brings other things to the table though for C# developers that aren't currently in YCM.

Contributor

Chiel92 commented Jun 10, 2013

"The YcmCompleter system for custom completer commands would make it very easy to bring up/shut down the server or do any of the other C#-specific things."

Do you mean with this that any other C# specific thing should be integrated into YCM, instead of in a seperate plugin?
I know YCM does more than just completion, like goto definition commands etcetera. Is it YCM's policy to integrate things like project file management and project building as well?

Owner

Valloric commented Jun 11, 2013

@Chiel92 wrote the YCM plugin so that it doesn't pass in the partial word to complete to the OmniSharpServer. This basically causes OmniSharp to not rank completions at all as it doesn't have the characters to complete on.

Sounds fine then.

There aren't any conflicts between the YCM plugin and OmniSharp, although I can see why you would want to depend on OmniSharpServer directly. OmniSharp brings other things to the table though for C# developers that aren't currently in YCM.

I'd love to see those other features brought directly into this CsharpCompleter class.

Do you mean with this that any other C# specific thing should be integrated into YCM, instead of in a seperate plugin? I know YCM does more than just completion, like goto definition commands etcetera. Is it YCM's policy to integrate things like project file management and project building as well?

Yes actually, that would be awesome. I look at it this way: we already have to spin up a full semantic language parser (like libclang, Jedi, NRefactory etc) to get good code completions so since we have the full AST we might as well provide some other sensible IDE features too. The other features are opt-in and nicely segmented out under the :YcmCompleter command; every semantic completer gets to control its own set of custom subcommands (although it would be great to share some of the common ones like GoTo etc). There was a similar discussion on jedi-vim; I recommend reading my comments on that issue.

The whole point of YCM is to provide a common, cross-language code completion infrastructure so that the wheel does not need to be reinvented every time someone wants to bring semantic code completion to new language X. YCM handles the nasty VimScript stuff, the semantic completers get a nice Python class to subclass and implement an interface. What they do behind that interface is up to them. Other code-comprehension features are welcome as custom subcommands.

What Syntastic is for code diagnostics, YCM is for code completion (and other code-comprehension features).

In the future, YCM will also be split into a server & client. The server will have all of the semantic engines (possibly also internally talking to other local servers like OmniSharpServer or Tern.js) but will provide a common interface for the various clients so that Vim, Emacs, SublimeText or others can easily write one YCM client and get semantic code completion for many languages at once.

Contributor

Chiel92 commented Jun 11, 2013

Yes actually, that would be awesome. I look at it this way: we already have to spin up a full semantic language parser (like libclang, Jedi, NRefactory etc) to get good code completions so since we have the full AST we might as well provide some other sensible IDE features too.

I was hoping for this. Vim really needs a good integration base for this language specific IDE stuff.

What Syntastic is for code diagnostics, YCM is for code completion (and other code-comprehension features).

Where is it exactly Syntastic comes in? Should project building be integrated into Syntastic?

Owner

Valloric commented Jun 11, 2013

Where is it exactly Syntastic comes in? Should project building be integrated into Syntastic?

That's up to the Syntastic devs. Currently YCM pipes diagnostics it gets from libclang into Syntastic, and then syntastic handles the gutter icon display and the locationlist management.

I'm fine with a semantic completer for YCM having a custom subcommand that builds the user's project. We already have to parse most of the code to get a good AST, so might as well provide a build command.

Contributor

Chiel92 commented Jun 13, 2013

Hm, it would definitively be cool if C# had real time compiling as well, just as with clang.
But first the start/stop server commands will have to be ported to ycm custom subcommands to complete this PR.

Owner

Valloric commented Jun 21, 2013

@Chiel92 So what's the state of this PR now?

Contributor

Chiel92 commented Jun 21, 2013

I've only made some small changes, because I'm very busy at the moment. In a week or two I should have more time to finish the completer. In the meantime, however, I may still find some time to submit a basic approach to this.
Basically what has to be done now is that the vimscript part in omnisharp must be ported to the ycm completer. Also, in my opinion, the last proposal in nosami/Omnisharp#60 (comment) should be implemented in the omnisharp server, to make it easy to check if the server is running.

python/ycm/completers/cs/cs_completer.py
+ solutionfiles = glob.glob1( folder, "*.sln" )
+
+ if len( solutionfiles ) == 1:
+ pass # start server here
@Chiel92

Chiel92 Jun 21, 2013

Contributor

How should the server be started? The vim part of omnisharp relies on vim-dispatch or vim-proc for this.

@Valloric

Valloric Jun 25, 2013

Owner

You can use Python's subprocess module for this I think.

python/ycm/completers/cs/cs_completer.py
+ if len( solutionfiles ) == 1:
+ pass # start server here
+ else:
+ pass # some other stuff, like notifying
@Chiel92

Chiel92 Jun 21, 2013

Contributor

If multiple solutionfiles are found (no idea if that's a useful case to catch), we could ask the user to choose. So does vim-omnisharp. Do you think that's desirable, and if so, how should that be done without breaking vim?

@Valloric

Valloric Jun 25, 2013

Owner

It's up to you whether this should be handled or not, I don't know enough about the use case to be able to provide guidance here.

There's some code in YCM's vimsupport.py file (see the PresentDialog method) that should come in handy should you decide to ask the user.

Owner

Valloric commented Jun 25, 2013

Take as much time as you need; I was just checking to see if this PR was abandoned.

Owner

Valloric commented Jun 25, 2013

BTW feel free to ping here when you want me to take another look or need some advice/guidance.

python/ycm/completers/cs/cs_completer.py
+ # Why doesn't this work properly?
+ # When starting manually in seperate console, everything works
+ # Maybe due to bothering stdin/stdout redirecting?
+ subprocess.Popen( command, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE )
@Chiel92

Chiel92 Jun 29, 2013

Contributor

This appears not to work properly, and I've no idea why. When the server is started this way, an autocomplete request returns an error response: HTTP Error 500: Internal Server Error. But a stopserver request works as normal.

However, when the server is start manually in a different terminal (with the same command!), everything works properly.
@nosami or somebody else, any idea what's possibly going on?

@Valloric

Valloric Jun 29, 2013

Owner
  1. Why are you redirecting stdin/stdout/stderr? You don't seem to be using it.
  2. You need to call .Communicate() at least once on the object returned from .Popen() to ensure the process is actually started. You don't need to send anything in the Communicate call, but you do need to call it. I had this exact problem last week in a different codebase.
  3. Possibly look into envoy if everything fails. It uses subprocess internally but provides a better and less error-prone API.
@Chiel92

Chiel92 Jun 29, 2013

Contributor
  1. AFAIK, if I don't do that, the stdin will be inherited from the parent. Anyway, omitting these parameters results in vim's screen being overwritten by log output from the omnisharp server.
  2. When I call .communicate(), it seems to wait for process to terminate. This makes vim hang, until I manually terminate the subprocess. Notice by the way that the stopcommand is functional, and the the process appears in the process list. So it looks like the subprocess is actually started.
  3. Haven't done yet. Will do.
@Valloric

Valloric Jun 29, 2013

Owner

When I call .communicate(), it seems to wait for process to terminate.

Peculiar. I have a ycm_extra_conf.py file at work that uses the YcmCorePreload API to inject a call to a binary started with subprocess. I do popen = subprocess.Popen(path_to_binary); popen.communicate() and it works just fine. Granted, I call a binary that's very short-lived and doesn't output any data.

Popen shouldn't be blocking the Python process, it should be async. If all else fails, you may need to start a Python thread and call Popen from there. See how envoy does it.

Contributor

Chiel92 commented Jul 7, 2013

Passing the command as a one-element list plus setting shell=True appeared to make the subprocess work properly. The choice for multiple solutions is useful for people dealing with seperate solutionfiles for different versions of VS, so l implemented that as well.

For now only two things remain to be done. The check for ServerIsRunning has to be implemented properly, and automatically starting/stopping has to be decided upon.
Concerning the latter, it is easy to make the server start in the init part of the completer. However, stopping the server is a different thing. I think it is okay to have the server stopped on VimLeave (or some YCM-leave event). Do you agree with this behaviour? If so, how should I hook into this event?

Owner

Valloric commented Jul 7, 2013

Concerning the latter, it is easy to make the server start in the init part of the completer. However, stopping the server is a different thing. I think it is okay to have the server stopped on VimLeave (or some YCM-leave event). Do you agree with this behaviour? If so, how should I hook into this event?

I'd say don't start the server automatically on startup by default; create a new option flag, g:ycm_auto_start_csharp_server and have it set to 0 by default. You start the server on startup if the user set the flag.

Then again, it might make more sense to autostart the server by default... I'm not sure.

You should certainly also have subcommands for starting, stopping and restarting the server.

WRT VimLeave, I just pushed a commit that added an OnVimLeave func to the Completer class that you can use.

Contributor

Chiel92 commented Jul 7, 2013

I'm not sure how to set defaults for the vim variables properly. The other completers don't seem to do so within their code.

Owner

Valloric commented Jul 8, 2013

I'm not sure how to set defaults for the vim variables properly. The other completers don't seem to do so within their code.

Just add some code to plugin/youcompleteme.vim; you don't set the variable from the Python code, you do it in the VimScript.

@Chiel92 Chiel92 referenced this pull request in OmniSharp/omnisharp-server Jul 8, 2013

Merged

Poke module added for checking if server running #15

socketwiz and others added some commits Jul 8, 2013

Fix duplicate entries in filename completion
We could just remove the "dup: 1" part in the completion dict, but that would
leave the duplicate removal up to Vim which would be slow. Also, we might not
end up returning the correct number of results then.
Contributor

Chiel92 commented Jul 10, 2013

As far as I know, the initial features are working now. Could somebody, especially Valloric, have another look at the code?

Trevoke and others added some commits Jul 10, 2013

Update README.md
Small tweaks to the README to make extended installation steps more obvious. It's not perfect yet, but it's a start.
Owner

Valloric commented Jul 12, 2013

I'll try to find some time this weekend to review it. Thanks for all your work!

python/ycm/completers/cs/cs_completer.py
+ folder = os.path.dirname( folder )
+ if folder == lastfolder:
+ break
+ solutionfiles = glob.glob1( folder, '*.sln' )
@Valloric

Valloric Jul 15, 2013

Owner

A part of the above block of code looks like it could be split into a separate function (something like _GetSolutionFiles; it doesn't even have to be a member function), thus improving readability.

python/ycm/completers/cs/cs_completer.py
+ solutionfiles = glob.glob1( folder, '*.sln' )
+
+ if len( solutionfiles ) == 0:
+ vimsupport.PostVimMessage( 'Error starting OmniSharp server: no solutionfile found' )
@Valloric

Valloric Jul 15, 2013

Owner

Try to keep the length of lines under 80 chars. This makes it possible to view several files at once side-by-side on the same screen. The rest of YCM follows this style rule as well.

+ solutionfile = solutionfiles[ choice ]
+
+ omnisharp = os.path.join( os.path.abspath( os.path.dirname( __file__ ) ),
+ 'OmniSharpServer/OmniSharp/bin/Debug/OmniSharp.exe' )
@Valloric

Valloric Jul 15, 2013

Owner

This exe is not in the upstream OmniSharpServer repo, so I'm assuming it needs to be compiled by the user. Is there no way for us to provide the exe to the user since Mono can run .net exe's on Linux that were built on Windows (and vice-versa I think)? I'd rather not have the user experience be "and now go compile this binary" if at all possible.

If we provide the binary, then the user can just open a C# file in Vim and have everything working without any configuration, right?

@nosami

nosami Jul 15, 2013

The same exe can be used for Windows and Linux. On Linux, depending on the machine configuration, you should prepend it with 'mono'. Not all Linux systems associate exe files with Mono.

You'd also need to include a few dlls, namely NRefactory, Monodoc, Mono.Cecil, Nancy and Nancy.Hosting.Self.

The executables can be downloaded from an automated build that I have on AppHarbor. If anybody needs access to it, just ping me.

@Chiel92

Chiel92 Jul 15, 2013

Contributor

It could also be compiled by the install.sh script, just like clang. On linux one would run xbuild in the Omnisharp directory, while on windows one would run msbuild with a fallback to xbuild. This might be preferable above including the binaries.

@Valloric

Valloric Jul 16, 2013

Owner

Yes, building the required binaries with install.sh + a flag (like the --clang-completer flag) makes sense.

Owner

Valloric commented Jul 15, 2013

@Chiel92 I looked through the code and I have to say, this is great work. It's well written, simple and very sensible. I added a few code comments but they're all pretty minor.

You'll also probably want to update the README file, or you can leave that up to me. You can also write a draft of the doc changes and I'll tweak them after merging if you wish.

Thanks for working on this!

Merge pull request #436 from junkblocker/patch-1
Allow easier system libclang use
Contributor

Chiel92 commented Jul 16, 2013

I added some lines to the install script to automate building Omnisharp. You probably want to make changes there, since I'm not a shell hero.
I also added a small paragraph to the README describing this completer. For clang, you mentioned the flags explicitly in the installation details, but I don't know if that's has to be the case for any other flag.
Since you know best how you want the README to be arranged, I stuck to this little paragraph.

Owner

Valloric commented Jul 16, 2013

Github says your pull request cannot be merged cleanly; please rebase on top of master. I think the conflicts come from the recent pull request that changed install.sh too.

Don't worry about the README, I'll tweak it post-pull. Your small section is fine, thanks for writing it.

The install.sh additions look sensible too.

Again, thanks for working on this! You make this merge cleanly and I think this is ready to be pulled in, barring any problems when I try to test it out on some C# code.

Contributor

Chiel92 commented Jul 16, 2013

@Valloric conflicts solved

@@ -139,3 +141,20 @@ else
testrun $cmake_args $EXTRA_CMAKE_ARGS
fi
+if $omnisharp_completer; then
+ buildcommand="msbuild"
@nosami

nosami Jul 16, 2013

Generally speaking, msbuild won't be in the $PATH unless you use the Visual Studio Command Prompt which sets up the required environment variables. Sucks I know.

On my machine it's at C:\Windows\Microsoft.NET\Framework64\v4.0.30319
and I believe it's at C:\Windows\Microsoft.NET\Framework\v4.0.30319 for 32 bit machines.

Using cygwin you may be able to use something like :-

buildcommand="/cygdrive/c/Windows/Microsoft.NET/Framework64/v4.0.30319/MSBuild.exe"

I think that you may also need

 /p:Platform="Any CPU"

see nosami/Omnisharp#64

@Chiel92

Chiel92 Jul 17, 2013

Contributor

Thank you for noticing this. I'm not very knowledgeable about windows environments.
Would your suggest hardcoding the path? That may be problematic with different versions of .NET.
If you have other suggestions about how to accomplish this, they are very welcome.

@nosami

nosami Jul 17, 2013

I'm not really sure what else to suggest. On Windows, there is a .bat file that can set up the environment variables. You're not going to be able to run that from a bash shell. I took a look at it last night to see if it could be ported, but it reads registry settings to initialise the variables. Very sucky.

Having said that, installing YCM on Windows is pretty tricky in the first place. Installing this plugin should be a walk in the park after that :)

@Chiel92

Chiel92 Jul 17, 2013

Contributor

You're not going to be able to run that from a bash shell.

Can't it be run by passing it to cmd.exe, like /bin/shell for linux? I suppose there is some application out there which can execute these bat scripts.

@nosami

nosami Jul 17, 2013

You can't pass different shells to cmd.exe AFAIK. Scripting on Windows seems to be pretty much an afterthought.

Only thing I could think of would be something like running the install.sh file from within a bat file.

Something like

install.bat
-- set up environment variables here --

C:\cygwin\bin\bash.exe ./install.sh

But even that assumes that the user would have cygwin installed. Cygwin users are probably a minority.

I think you can 'sh' from a powershell script like you can on linux. I'm by no means a windows scripting guy though. I always use cygwin when on Windows :)

I think the safest bet would be to have an install.bat file that performs the same task as the install.sh but without any cygwin dependencies. That means having 2 install scripts though :(

Like I said before though, getting YCM setup in the first place is pretty damn difficult on Windows. It's probably not even worth worrying about. I'm sure that the Windows/C#/Vim/YCM crowd is pretty tiny, and anybody that has managed to set it up won't have any trouble setting up the OmniSharp server.

@Valloric

Valloric Jul 18, 2013

Owner

I'd agree with @nosami here; YCM has no official Windows support so I don't see much point in going to great lengths to address automatic building of the server on that platform. Hell, you can't even run install.sh on vanilla Windows, you'd need Cygwin.

If someone from the community wishes to contribute an install.bat, sure. But it shouldn't be holding up this PR.

@Valloric Valloric merged commit 66b70ee into Valloric:master Jul 18, 2013

1 check passed

default The Travis CI build passed
Details
Owner

Valloric commented Jul 18, 2013

Woohoo, merged! Thanks for working on this!

I tried it out a bit and it IMO works great.

I did notice one minor thing: the first time you request semantic completions in a file it takes a while to get the completions. After that first time, it's very fast.

I'm guessing this is because the omnisharp server compiles the file the first time we ask for completions and then stores the AST in a cache of some sort. Is there any way to tell the server to "warm up" for a given file? We could then send it such a "warm up" command when the user enters a C# file. The Completer API already has support for this.

The user would then not have to wait on the first completion request.

Owner

Valloric commented Jul 18, 2013

Oh, ideas for future pull requests: support for GoTo commands. YCM has support for this internally and it's fairly easy for Completer subclasses to tie into this. See the Jedi completer for a good example.

Contributor

Chiel92 commented Jul 18, 2013

I did notice one minor thing: the first time you request semantic completions in a file it takes a while to get the completions. After that first time, it's very fast.

I believe that's because the server has to start first, loading several things. When I edit a second file, I don't notice any delay.

Oh, ideas for future pull requests: support for GoTo commands. YCM has support for this internally and it's fairly easy for Completer subclasses to tie into this. See the Jedi completer for a good example.

Yeah, I suppose it won't be that difficult. When I've got time again I will take a look.

nosami commented Jul 18, 2013

It's actually the MonoDoc documentation system that takes a long while to warm up. On Windows it's very fast as we don't use that, although there is a very small delay on the first request. The documentation system caches responses based on the type of the thing that you are trying to complete.

I think I will try and warm the system up in the background, so you won't need to do anything (apart from maybe update the submodule reference).

bijancn pushed a commit to bijancn/YouCompleteMe that referenced this pull request Jul 26, 2016

Merge pull request #365 from micbou/windows-more-py3
[READY] Fix Python 2 support and add Python 3 support on Windows

bijancn pushed a commit to bijancn/YouCompleteMe that referenced this pull request Jul 26, 2016

Auto merge of #409 - micbou:refactor-tests, r=Valloric
[RFC] Rewrite tests to reuse server state

This PR rewrites tests from completers using a server by separating them into two classes:
 - isolated tests: each test use its own server instance;
 - persistent tests: use the same server instance.

We use classes because it provides a clean way to do this (no global variables needed).

This PR also simplifies the C♯ tests by adding filepaths used to start the server to a list and stopping the server for each filepath in the list at the end of the tests. With this approach, we don't need to expose additional functions in the C♯ completer (`GetOmnisharpPort` and `SetOmnisharpPort` are not needed). Furthermore, we don't have the issue with persistent tests (introduced by PR #339) on Windows and Python 3. See PR #365 for details.

And now, the interesting part; the execution times on different CI configurations before and after the changes:

Configuration | Before (s) | After (s)
------------- | -----------| ---------
AppVeyor Python 2.7 | 58.4 | 28.8
AppVeyor Python 3.5 | 161.1 | 32.8
Travis Linux | 49.0 | 32.6
Travis OS X | 59.4 | 42.8

Closes #338.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/409)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment