[READY] Omnisharp roslyn

[mispencer]

See #213 for prior pull request.

Allow using omnisharp-roslyn. See #209 (comment).

This pull request does not make roslyn the default (yet). Rather, it must be switched to the roslyn implementation by running the UseRoslynOmnisharp completer subcommand. This is per-solution, and pre-runtime.

---

This change is 

[mispencer]

Current status:

• Windows is failing the Shutdown_test.FromHandlerWithSubservers_test and I am not sure why
• Travis hasn't been running the Mac tests, so I'm not sure if they are passing
• There are a lot of skipped tests, some which should be fixed
• Fixit in new backend isn't working, and I believe it is implementable
• The libuv check in build.py should be improved
• Dotnet Roslyn version is in the build, but isn't supported in the runtime

[codecov-io]

Codecov Report

Merging #885 into master will decrease coverage by 0.04%.
The diff coverage is 96.05%.

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
- Coverage   96.18%   96.13%   -0.05%     
==========================================
  Files          83       83              
  Lines        6468     7098     +630     
==========================================
+ Hits         6221     6824     +603     
- Misses        247      274      +27

[zzbot]

☔ The latest upstream changes (presumably #883) made this pull request unmergeable. Please resolve the merge conflicts.

[bstaletic]

Thanks for once again investing the time into this.

You're probably aware that we don't like the idea of supporting multiple engines for a single language.
So what is stopping us from ditching the legacy engine?
Sorry if this has been asked in the previous attempt.

---

Reviewed 10 of 15 files at r1, 2 of 2 files at r2.
Review status: 12 of 17 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

build.py, line 499 at r3 (raw file):

          '/v', 'version' ] )
      dotnet_46_pattern = re.compile( 'version\s*REG_SZ\s*4.[67].\d*' )
      

Trailing spaces.

---

ycmd/tests/cs/debug_info_test.py, line 59 at r3 (raw file):

        'address': instance_of( str ),
        'port': instance_of( int ),
        'logfiles': empty(),

Does roslyn actually have no logs?

---

Comments from Reviewable

[mispencer]

Merge conflicts have been resolved.

[zzbot]

☔ The latest upstream changes (presumably #857) made this pull request unmergeable. Please resolve the merge conflicts.

[mispencer]

Merge conflicts have been resolved again.

Could I get a review?

---

Comments from Reviewable

[bstaletic]

First, thank you for your patience and your effort.

Here's what we all have been thinking about this.

Supporting two C# servers would be a burden. Especially considering none of the maintainers is a C# developer.
I vaguely remember some troubles with Omnisharp just because none of us regularly uses C#.

Maintaining two servers shall be hard. I'm also not sure what I think about the approach of trying and seeing which server works - using try/except everywhere just because of two backends.

On the other hand Roslyn is supposed to have a LSP server capabilities and that should be fairly easy to use now that #857 has been merged. @micbou @puremourning and I have all been able to hack a random LSP server in a short amount of time.

@puremourning @mispencer @Valloric @vheon
Thoughts?

---

Reviewed 1 of 15 files at r1, 2 of 3 files at r5, 1 of 2 files at r6, 3 of 4 files at r7, 8 of 8 files at r9.
Review status: 16 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 36 at r9 (raw file):

import requests
import threading
from subprocess import ( PIPE )

Parentheses aren't needed here.

---

ycmd/completers/cs/cs_completer.py, line 118 at r9 (raw file):

  def ShouldUseNowInner( self, request_data ):
    """ Preempt the identity completer always, since the C# completer is fast

Are we sure this is a good idea?
How fast is omnisharp for really big projects?

---

ycmd/tests/cs/init.py, line 65 at r9 (raw file):

def WaitUntilOmniSharpServerReady( app, filepath ):
  retries = 2000

Why does this need so many tries?

---

ycmd/tests/cs/debug_info_test.py, line 59 at r3 (raw file):

Previously, mispencer wrote…

As part of this change, I had piped the log output into ycmd logs. However, I believe this is the cause of the Window's failures, so I have just reverted this back to logging to separate log files.

I think it's preferable to have separate files for separate logs.

---

Comments from Reviewable

[mispencer]

The double backend is needed for me because there were some legacy solutions, which I am actively working on, that I could not get Roslyn Omnisharp to work properly with, and there were some other newer solutions, which I am also actively working on, that the legacy Omnisharp could not open. Thus, to support all my use cases, YCMD needed to support both the legacy and Roslyn Omnisharp. Furthermore, there are still some features from the legacy Omnisharp which I would miss that are not supported in Roslyn Omnisharp.

However, having said all that, I have noticed a issue which could have caused my issues with legacy solutions and it is supposedly fixed in the latest version (1.28). I will have to re-test. If bumping the Roslyn Omnisharp version resolves my issue, I would (reluctantly) consent to a single backend with Roslyn Omnisharp.

LSP is still an open issue in Roslyn Omnisharp. I know the initial implementation was included in the latest beta of Roslyn Omnisharp, but I wasn't under the impression that it was stable yet.

Though LSP may be a simple implementation, it would still a decent implementation effort, and I'm not sure what we would gain. The existing Omnisharp interface is not deprecated, and I don't believe it will be any time soon.

---

Review status: 16 of 17 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cs/cs_completer.py, line 36 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Parentheses aren't needed here.

Done.

---

ycmd/completers/cs/cs_completer.py, line 118 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Are we sure this is a good idea?
How fast is omnisharp for really big projects?

Fast enough. It normally shows up by the time I type 3-4 characters, which is about the minimum needed to be typed to select the correct result anyway.

Note, please, that this is the way that the C# completer currently works, so this would be expected behavior by anyone using the C# completion. See #116 for justification why it currently functions this way.

---

ycmd/tests/cs/init.py, line 65 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why does this need so many tries?

It probably doesn't need to so high. I'm pretty sure I just got frustrated by CI failures from timeout and set it to a excessively high number.

---

Comments from Reviewable

[bstaletic]

Roslyn Omnisharp doesn't support Fixit yet, so that will remain skipped until this is implemented in Roslyn.

Apart from us not wanting to support two backends, this is the only concern left. I am not sure if we want to take features away from the users. Is there an effort to make FixIts work with Roslyn?

LSP is still an open issue in Roslyn Omnisharp. I know the initial implementation was included in the latest beta of Roslyn Omnisharp, but I wasn't under the impression that it was stable yet.

Though LSP may be a simple implementation, it would still a decent implementation effort, and I'm not sure what we would gain. The existing Omnisharp interface is not deprecated, and I don't believe it will be any time soon.

Alright, sounds good to me. I'm fine with sticking to this API.

---

Review status: 16 of 17 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 118 at r9 (raw file):

Previously, mispencer wrote…

Fast enough. It normally shows up by the time I type 3-4 characters, which is about the minimum needed to be typed to select the correct result anyway.

Note, please, that this is the way that the C# completer currently works, so this would be expected behavior by anyone using the C# completion. See #116 for justification why it currently functions this way.

Alright, then I guess this is fine.

---

ycmd/tests/cs/init.py, line 65 at r9 (raw file):

Previously, mispencer wrote…

It probably doesn't need to so high. I'm pretty sure I just got frustrated by CI failures from timeout and set it to a excessively high number.

Okay. Can you add a comment stating this is because of the CI timing out?

---

Comments from Reviewable

[mispencer]

I have opened a issue in Omnisharp-Roslyn (OmniSharp/omnisharp-roslyn#1098) about the fixits (code issues in Omnisharp jargon). We'll see what they say.

Should I be concerned about the coverage failures?

---

Review status: 15 of 17 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

ycmd/tests/cs/init.py, line 65 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Okay. Can you add a comment stating this is because of the CI timing out?

Done.

---

Comments from Reviewable

[bstaletic]

Thanks for opening the issue.

Cygwin

@puremourning @micbou @Valloric What are your opinions on this?
I personally feel that if cygwin needs this much "hand holding", it should be tested. And as far as Iknow appveyor can run cygwin builds too.

On the other hand, testing on Cygwin means we actually support it, which hasn't been the case before.

Coverage

Codecov has been reporting nonsense lately and we should probably start investigating.
But codecov still reports sensible "patch". So getting that part of codecov checks to pass would be great, especially for a code this complex.
Generally, @Valloric likes to see coverage over 95% and @micbou is usually happy with 90% coverage.

---

Reviewed 1 of 2 files at r10.
Review status: 16 of 17 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 691 at r10 (raw file):

    response = requests.post( target,
                                   json = parameters,
                                   timeout = timeout )

Last two parameters are misaligned.

---

ycmd/completers/cs/cs_completer.py, line 805 at r10 (raw file):

def _ConvertFilenameForCygwin( filename, direction ):

Why is the second parameter called direction?

---

ycmd/completers/cs/cs_completer.py, line 819 at r10 (raw file):

        dir_arg = '-w' if direction else '-u'
        command = [ 'cygpath', '-f', '-', dir_arg ]
        cygpath = utils.SafePopen( command , stdout = PIPE, stdin = PIPE )

@micbou Were there any caveats/gotchas in using SafePopen on Windows?
I remember there was something whe I made the rustup PR.

---

Comments from Reviewable

[mispencer]

I personally feel that if cygwin needs this much "hand holding", it should be tested. And as far as Iknow appveyor can run cygwin builds too.
On the other hand, testing on Cygwin means we actually support it, which hasn't been the case before.

If there is any interest in this, I can look into it.

Codecov has been reporting nonsense lately and we should probably start investigating.
But codecov still reports sensible "patch". So getting that part of codecov checks to pass would be great, especially for a code this complex.
Generally, @Valloric likes to see coverage over 95% and @micbou is usually happy with 90% coverage.

I have made some changes to increase the coverage number, though I kinda feel I'm maximizing a measurement, rather than code quality.

---

Review status: 14 of 17 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cs/cs_completer.py, line 691 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Last two parameters are misaligned.

Done.

---

ycmd/completers/cs/cs_completer.py, line 805 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is the second parameter called direction?

It's the direction of conversion (window-to-cygwin or cygwin-to-window).

---

ycmd/completers/cs/cs_completer.py, line 819 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

@micbou Were there any caveats/gotchas in using SafePopen on Windows?
I remember there was something whe I made the rustup PR.

I don't think any of those apply in Cygwin, which is the only OS this runs in.

---

Comments from Reviewable

[bstaletic]

If there is any interest in this, I can look into it.

Great, but let's not rush. I'd like to see what others have to say about it.

I have made some changes to increase the coverage number, though I kinda feel I'm maximizing a measurement, rather than code quality.

We shouldn't be trying to hide unlikely and uncovered branches with # pragma: no cover. Ideally, code should be covered, but in case that is hard or even impossible for some reason, we should still be aware that some branch isn't covered.

If you need something to return a specific value or throw a specific exception, python provides @patch decorator for testing that.

Here's an example of overridden return value: https://github.com/Valloric/ycmd/blob/master/ycmd/tests/rust/get_completions_test.py#L58

---

Reviewed 3 of 3 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/completers/cs/cs_completer.py, line 805 at r10 (raw file):

Previously, mispencer wrote…

It's the direction of conversion (window-to-cygwin or cygwin-to-window).

It's still not clear which direction is which. So this would benefit a lot from a comment explaining what direction = True, as opposed to direction = False means.

---

ycmd/completers/cs/cs_completer.py, line 819 at r10 (raw file):

Previously, mispencer wrote…

I don't think any of those apply in Cygwin, which is the only OS this runs in.

That might as well be true, I know next to nothing about Cygwin.

For reference: #785 (comment)
Specifically, the part where @micbou mentions CREATE_NO_WINDOWS flag.

---

Comments from Reviewable

[puremourning]

Re: Cygwin

You won't be surprised to hear that I have no particular interest in supporting Cygwin, and personally see it as more things I have to worry about for future changes. But that's a super selfish viewpoint and should not be taken seriously :)

What I would say IMO is that we should only support Cygwin (properly) if the maintenance burden is low, and that we have good evidence that we have strong demand for it, and a ahem willing volunteer to own the integration and maintenance. We currently support it on a best-efforts basis, but if that's not enough then it is worth a discussion.

---

Reviewed 2 of 15 files at r1.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

appveyor.yml, line 8 at r5 (raw file):

  # We dropped MSVC 11 support because it lacks features from C++11 and C99.
  # We dropped MSVC 13 support because it lacks .NET 4.6 on that image
  #- APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013

we can probably remove these lines. i have no issue with dropping older msvc as it is trivial to acquire new versions and you can aiui have multiple versions installed.

---

build.py, line 85 at r5 (raw file):

def OnCygwin():
  return platform.system().startswith("CYGWIN")

is this adding support for cygwin ? My recollection is that we don't (officially) support it. if so is there value in doing that in a different PR ?

---

build.py, line 164 at r5 (raw file):

def CheckOutput( args, **kwargs ):

i'm not clear why this is necessary.

---

build.py, line 283 at r5 (raw file):

  parser.add_argument( '--js-completer', action = 'store_true',
                       help = 'Enable JavaScript semantic completion engine.' ),
  parser.add_argument( '--new-cs-completer', action = 'store_true',

if we are really going to support two completers in parallel i wonder if it would be better to use --cs-completer=roslyn or something? New is only new as long as the old one exists. Usually best to avoid distinctions of "new" and "old" because new becomes old very quickly

---

build.py, line 551 at r5 (raw file):

            seven_zip_path = _winreg.QueryValueEx( SZ, 'Path' )[ 0 ]

      extract_command = [ p.join( seven_zip_path, '7z.exe' ), 'x', file_path ]

don't we already use 7zip in the make? I seem to recall that we don't have to look in the registry for this?

---

build.py, line 555 at r5 (raw file):

      extract_command = [ 'tar', 'xfv', file_path ]

    subprocess.check_call( extract_command )

what's extract command on linux/macOS?

---

ycmd/completers/cs/cs_completer.py, line 59 at r5 (raw file):

ROSLYN_OMNISHARP_BINARY = 'OmniSharp'
MONO_MODE = 1 # TODO
if utils.OnWindows() or utils.OnCygwin() or MONO_MODE:

Any reason not to use PathToFirstExistingExecutable which sort out this .exe stuff for us?

---

ycmd/completers/cs/cs_completer.py, line 207 at r5 (raw file):

      'SetOmnisharpPath'                 : ( lambda self, request_data, args:
         self._SetOmnisharpPath( request_data, args[ 0 ] ) ),
      'UseLegacyOmnisharp'                 : ( lambda self, request_data, args:

is there really a need to support both engines and toggle between them? this sounds painfully coplicated and worse, how many users are really going to understand this and be able to actually do it?

---

ci/travis/travis_install.linux.sh, line 29 at r5 (raw file):

# Libuv is required for Omnisharp-Roslyn and isn't in accessible repos
curl -sSL https://github.com/libuv/libuv/archive/v1.18.0.tar.gz | tar zxfv - -C /tmp && cd /tmp/libuv-1.18.0/

80 characters

---

ci/travis/travis_install.linux.sh, line 36 at r5 (raw file):

export LD_LIBRARY_PATH="$HOME/libuvinstall/lib"
export LIBRARY_PATH="$HOME/libuvinstall/lib"
cd $OLDPWD

what sets OLDPWD ? is that a shell builtin?

i suspect we really want pushd and popd here, as this seems breakable.

---

Comments from Reviewable

[bstaletic]

Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

ci/travis/travis_install.linux.sh, line 36 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

what sets OLDPWD ? is that a shell builtin?

i suspect we really want pushd and popd here, as this seems breakable.

For the record OLDPWD is a shell builtin and cd $OLDPWD is equivalent to cd -, or rather cd - is equivalend to cd $OLDPWD.

---

Comments from Reviewable

[zzbot]

☔ The latest upstream changes (presumably #924) made this pull request unmergeable. Please resolve the merge conflicts.

[Valloric]

Closing PR as it's not moving forward; can reopen in the future if desired.

[insidewhy]

I am not sure if we want to take features away from the users. Is there an effort to make FixIts work with Roslyn?

I doubt there are very many C# developers who would be content to use a version of C# that is several years old just to be able to use fixits.