Semantic C# completer #365
Conversation
|
It's better to move any calls to vim ( like |
| response = urllib2.urlopen( target, parameters ) | ||
| return response.read() | ||
| except: | ||
| vimsupport.PostVimMessage( "Could not connect to " + target ) |
There was a problem hiding this comment.
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.
|
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 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. |
|
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. |
|
"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? |
Sounds fine then.
I'd love to see those other features brought directly into this CsharpCompleter class.
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 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. |
I was hoping for this. Vim really needs a good integration base for this language specific IDE stuff.
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. |
|
Hm, it would definitively be cool if C# had real time compiling as well, just as with clang. |
|
@Chiel92 So what's the state of this PR now? |
|
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. |
| solutionfiles = glob.glob1( folder, "*.sln" ) | ||
|
|
||
| if len( solutionfiles ) == 1: | ||
| pass # start server here |
There was a problem hiding this comment.
How should the server be started? The vim part of omnisharp relies on vim-dispatch or vim-proc for this.
There was a problem hiding this comment.
You can use Python's subprocess module for this I think.
|
Take as much time as you need; I was just checking to see if this PR was abandoned. |
|
BTW feel free to ping here when you want me to take another look or need some advice/guidance. |
| # 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 ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
- Why are you redirecting stdin/stdout/stderr? You don't seem to be using it.
- 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 theCommunicatecall, but you do need to call it. I had this exact problem last week in a different codebase. - Possibly look into envoy if everything fails. It uses subprocess internally but provides a better and less error-prone API.
There was a problem hiding this comment.
- 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.
- 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. - Haven't done yet. Will do.
There was a problem hiding this comment.
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.
|
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. |
I'd say don't start the server automatically on startup by default; create a new option 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. |
|
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 |
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.
|
As far as I know, the initial features are working now. Could somebody, especially Valloric, have another look at the code? |
Small tweaks to the README to make extended installation steps more obvious. It's not perfect yet, but it's a start.
Update README.md
build error on SmartOS
|
I'll try to find some time this weekend to review it. Thanks for all your work! |
| folder = os.path.dirname( folder ) | ||
| if folder == lastfolder: | ||
| break | ||
| solutionfiles = glob.glob1( folder, '*.sln' ) |
There was a problem hiding this comment.
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.
|
@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! |
Allow easier system libclang use
|
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. |
|
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. |
Conflicts: install.sh
Conflicts: README.md
|
@Valloric conflicts solved |
| @@ -139,3 +141,20 @@ else | |||
| testrun $cmake_args $EXTRA_CMAKE_ARGS | |||
| fi | |||
|
|
|||
| if $omnisharp_completer; then | |||
| buildcommand="msbuild" | |||
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
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. |
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.
Yeah, I suppose it won't be that difficult. When I've got time again I will take a look. |
|
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). |
[READY] Fix Python 2 support and add Python 3 support on Windows
[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 ycm-core#339) on Windows and Python 3. See PR ycm-core#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 ycm-core#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 -->
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.