[READY] Java support using jdt.ls #857

puremourning · 2017-10-12T22:50:24Z

OK take a deep breath, this is a monster PR.

First and foremost, massive thanks to @bstaletic and @micbou for helping not
only with the development and testing, but also for moral support!

Overview

This PR implements native support in ycmd for the Java language, based on
jdt.ls. In summary, the following key features are supported:

Installation of jdt.ls (built from source with build.py)
Management of the jdt.ls server instance, projects etc.
A generic (ish) implementation of a Language Server Protocol client so
far as is required for jdt.ls (easily extensible to other engines)
Support for the following Java semantic engine features:
- Semantic code completion, including automatic imports
- As-you-type diagnostics
- GoTo including GoToReferences
- FixIt
- RefactorRename
- GetType
- GetDoc

See the trello board for a more complete picture.

Show me the demo, already.

OK sure.

Startup and completion

Rename

FixIt

GoToReferences

Why is this necessary and useful?

The current state of the plugin for YCM support for Java is basically eclim
or javacomplete2, neither of which work perfectly with ycmd, and neither of
which are quite as well integrated as we might like.

eclim

Eclim is actually pretty good and I have used it for a long time. However, the
experience isn't optimal for YCM:

Pros

Pretty good project creation, management functions
Lots of languages (supports more than just Java)
Lots of features (supports nearly all the standard YCMD commands)
New class creation, and lots of other stuff like that

Cons

It's a pain to set up (I know of only 1 other person at my company who has
successfully got it working), requiring a Vimball, installation into an
existing eclipse installation, manually starting the server, and other fiddly
stuff.
Diagnostics are delivered via Syntastic, so block the UI on save, and at a
number of other times.
Requires eclipse installation and understanding

javacomplete2

I have not extensively used javacomplete2, but I know there have been issues
reported that its omnifunc doesn't always play nice with ycmd.

Pros?

Very simple to get going
Seems to give completion suggestions reasonably well

Cons?

Lacks features
No diagnostics ?
Lots of vimscript (java parser in vimscript?)

Notable mentions

[vim-lsp][] seems to be a plugin for generic LSP support. I haven't
tried it but my experience with jdt.ls is that there is no way to make a fully
working LSP client without server-specific knowledge. I don't know if this
works with jdt.ls

Why jdt.ls?

I tried 2 different Java LSP engines:

vscode-javac: javac (the Java compiler) is the backend. Actually,
this is a nice and simple server and has decent features.
jdt.ls: Eclipse JDT as a server daemon. This appears to be the most
popular Java plugin for VSCode, and has the full power of RedHat and
Microsoft investing in bringing JDT (the eclipse Java engine) to VSCode and
the LSP-using community.

Initially, for its simplicity, I was drawn to vscode-javac
for its simplicity and ease of integration with ycmd. However, I also
implemented jdt.ls and found this, while more work, to be a superior
experience. It also has a very active development community, which bodes well
for the future.

Why such a big PR, man. Come on!

I'm sorry. I really really am. I honestly tried to rebase and rewrite the ~150
commits over the last 1 year or so, but as with many such things, a lot of the
functionality that would make "Phase 1" ended up relying on functionality and
test framework that came in "Phase N". So I'm dumping this on you in one
unwieldy heap and hoping that you can find it in your hearts to forgive me. I
honestly spent hours trying :(

OK, but come on, give us a hand...

Sure thing! I've tried to give below a "review guide" to help you get your heads
around what's going on an why some of the design decisions where made.

Overall design/goals

Key goals:

Support Java in ycmd and YCM; make it good enough to replace eclim and
javacomplete2 for most people
Make it possible/easy to support other lsp servers in future (but, don't
suffer from yagni); prove that this works.

An overview of the objects involved can be seen on this
card. In short:

2 classes implement the language server protocol in the
language_server_completer.py module:
LanguageServerConnection - an abstraction of the comminication with the
server, which may be over stdio or any number of TCP/IP ports (or a domain
socket, etc.). Only a single implementation is included (stdio), but
implementations for TCP/IP
were written originally and dropped in favour of stdio's simplicity.
LanguageServerCompleter - an abstract base for any completer based on LSP,
which implements as much standard functionality as possible including
completions, diagnostics, goto, fixit, rename, etc.
The java_completer itself implements the LanguageServerCompleter, boots
the jdt.ls server, and instantiates a LanguageServerConnection for
communication with jdt.ls.

The overall plan and some general discussion around the project can be found on
the trello board I used for development.

The proof? I implemented a fully working clangd completer for ycmd in about 30 minutes. The only wrinkles were clangd bugs!

Threads, why oh why so many threads.

OK chill. It's not thhaartd.

LSP is by its nature an asyncronous protocol. There are request-reply like
requests and unsolicited notifications. Receipt of the latter is mandatory,
so we cannot rely on there being a bottle thread executing a client request.

So we need a message pump and despatch thread. This is actually the
LanguageServerConnection, which implements thread. It's main method simply
listens on the socket/stream and despatches complete messages to the
LanguageServerCompleter. It does this:

For requests: similarly to the TypeScript completer, using python event
objects, wrapped in our Response class
For notifications: via a synchronised queue. More on this later.

A very poor representation of this is on the "Requests and notifications" page
of the design, including a rough sketch of the thread interaction.

Some handling is done in the message pump.

That's right. There are certain notifications which we have to handle when we
get them, such as:

Initialisation messages
Diagnostics

In these cases, we allow some code to be executed inline within the message pump
thread, as there is no other thread guaranteed to execute. These are handled by
callback functions and mutexes.

Startup sequence

See the 'initialisation sequence' tab on the design for a bit of background.

In standard LSP, the initialisation sequence consists of an initialise
request-reply, followed by us sending the server an initialised notification. We
must not send any other requests until this has completed.

An additional wrinkle is that jdt.ls, being based on eclipse has a whole other
initialisation sequence during which time it is not fully functional, so we have
to determine when that has completed too. This is done by jdt.ls-specific
messages and controls the ServerIsReady response.

In order for none of these shenanigans to block the user, we must do them all
asynchronously, effectively in the message pump thread. In addition, we must
queue up any file contents changes during this period to ensure the server is up
to date when we start processing requests proper.

This is unfortunately complicated, but there were early issues with really bad
UI blocking that we just had to get rid of.

Completion foibles

Language server protocol requires that the client can apply textEdits,
rather than just simple text. This is not an optional feature, but ycmd
clients do not have this ability.

The protocol, however, restricts that the edit must include the original
requested completion position, so we can perform some simple text
manipulation to apply the edit to the current line and determine the
completion start column based on that.

In particular, the jdt.ls server returns textEdits that replace the
entered text for import completions, which is one of the most useful
completions.

We do this super inefficiently by attempting to normalise the TextEdits
into insertion_texts with the same start_codepoint. This is necessary
particularly due to the way that eclipse returns import completions for
packages.

We also include support for "additionalTextEdits" which
allow automatic insertion of, e.g., import statements when selecting
completion items. These are sent on the completion response as an
additional completer data item called 'fixits'. The client applies the
same logic as a standard FixIt once the selected completion item is
inserted.

Diagnostics foibles

Diagnostics in LSP are delivered asynchronously via notifications. Normally,
we would use the OnFileReadyToParse response to supply diagnostics, but due to
the lag between refreshing files and receiving diagnostics, this leads to a
horrible user experience where the diagnostics always lag one edit behind.

To resolve this, we use the long-polling mechanism added here (ReceiveMessages
request) to return diagnostics to the client asynchronously.

We deliver asynchronous diagnostics to the client in the same way that the
language server does, i.e. per-file. The client then fans them out or does
whatever makes sense for the client. This is necessary because it isn't possible
to know when we have received all diagnostics, and combining them into a single
message was becoming clunky and error prone.

In order to be relatively compatible with other clients, we also return
diagnostics on the file-ready-to-parse event, even though they might be
out of date wrt the code. The client is responsible for ignoring these
diagnostics when it handles the asynchronously delivered ones. This requires
that we hold the "latest" diagnostics for a file. As it turns out, this is also
required for FixIts.

Projects

jdt.ls is based on eclipse. It is in fact an eclipse plugin. So it requires an
eclipse workspace. We try and hide this by creating an ad-hoc workspace for each
ycmd instance. This prevents the possibility of multiple "eclipse" instances
using the same workspace, but can lead to unreasonable startup times for large
projects.

The jdt.ls team strongly suggest that we should re-use a workspace based on the
hash of the "project directory" (essentially the dir containing the project
file: .project, pom.xml or build.gradle). They also say, however, that
eclipse frequently corrupts its workspace.

So we have a hidden switch to re-use a workspace as the jdt.ls devs suggest. In
testing at work, this was mandatory due to a slow SAN, but at home, startup
time is not an issue, even for large projects. I think we'll just have to see
how things go to decide which one we want to keep.

Subcommand foibles

GetDoc/GetType

There is no GetType in LSP. There's only "hover". The hover response is
hilariously server-specific, so in the base LanguageServerCompleter we just
provide the ability to get the hover response and JavaCompleter extracts the
appropriate info from there. Thanks to @bstaletic for this!

FixIt

FixIts are implemented as code actions, and require the diagnostic they relate
to to be send from us to the server, rather than just a position. We use the
stored diags and find the nearest one based on the request_data.

What's worse is that the LSP provides no documentation for what the "Code
action" response should be, and it is 100% implementation-specific. They just
have this command abstraction which is basically "do a thing".

From what I've seen, most servers just end up with either a WorkspaceEdit or a
series of TextEdits, which is fine for us as that's what ycmd's protocol looks
like.

The solution is that we have a callback into the JavaCompleter to handle the
(custom) java.apply.workspaceEdit "command".

GoToReferences

Annoyingly, jdt.ls sometimes returns references to .class files within jar
archives using a custom jdt:// protocol. We can't handle that, so we have to
dodge and weave so that we don't crash.

Stopping the server

Much like the initialisation sequence, the LSP shutdown sequence is a bit
fiddly. 2 things are required:

A shutdown request-reply. The server tides up and prepares to die!
An exit notification. We tell the server to die.

This isn't so bad, but jdt.ls is buggy and actually dies without responding to
the shutdown request. So we have a bunch of code to handle that and to ensure
that the server dies eventually, as it had a habbit of getting stuck running,
particularly if we threw an exception.

Closing remarks

Phew. That was a lot of caveats! I have personally used this quite a lot now,
and it has proven to be really useful. In particular the full index and
GoToReferences, etc.

I think the tests are comprehensive, but there is probably more work to do on
coverage. Some of it will be tricky (particularly exceptions around jdt: and
other edge cases in jdt.ls that I've come across).

Ship it as experimental ?

Due to the rapid pace of development of jdt.ls and the scale of this change, one
thought I had was to mark Java support in YCM as exprrimental and gather
feedback via an open github issue. I'm certain there will be issues, and
preferences, etc. so this might be a good way to tackle that.

Where's the client PR ?

I haven't finished polishing the client yet, but you can get use it by checking
out my language-server-java branch of YCM.

This change is

bstaletic · 2017-10-13T07:34:19Z

Just a few comments to start the discussion. Far from a finished review.

Also, if we are going to keep JAVA_SUPPORT.md we should fix typos.

Reviewed 54 of 60 files at r1.
Review status: 54 of 60 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

JAVA_SUPPORT.md, line 246 at r1 (raw file):

ycmd instance. This prevents the possibility of multiple "eclipse"  instances
using the same workspace, but can lead to unreasonable startup times for large
projects.

How often did it happen to you that the workspace became corrupt? I'm wondering what "often" actually means.
In my opinion, start up time is bearable, but it's far from snappy like I'm used to.

JAVA_SUPPORT.md, line 265 at r1 (raw file):

hilariously server-specific, so in the base `LanguageServerCompleter` we just
provide the ability to get the `hover` response and `JavaCompleter` extracts the
appropriate info from there. Thanks to @bstaletic for this!

While all this is true and can even be verified in microsoft/language-server-protocol#291, jdt.ls here does the same as the python language server and, if I remember correctly, the same as clangd.

A simplified explanation:

The response to hover request is an array of something called MarkedText.
The first MarkedText is a dictionary looking like this { 'language': 'java', 'value': 'whatever GetType returns usually' }
The second to last MarkedText is just a string containing lines of documentation (for our GetDoc).

Once again there's no guarantee that this will be done for every LSP server we decide to integrate.
On the other hand, perhaps all interesting servers will actually work the same, so maybe we can implement GetType and GetDoc in language_server.py and be done.

ycmd/completers/completer.py, line 386 at r1 (raw file):

  def PollForMessagesInner( self, request_data ):
    # Most completers don't implement this

I'd say this needs another sentence or two. Why most completers don't implement this? Or what completers do implement this.

Comments from Reviewable

puremourning · 2017-10-13T15:37:36Z

Review status: 54 of 60 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

JAVA_SUPPORT.md, line 246 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

How often did it happen to you that the workspace became corrupt? I'm wondering what "often" actually means.
In my opinion, start up time is bearable, but it's far from snappy like I'm used to.

It's happened to me only when multiple instances use the same workspace (i.e. it would happen frequently when running the tests before i went back to ad-hoc workspaces). TBH it's a complete unknown.

JAVA_SUPPORT.md, line 265 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

While all this is true and can even be verified in microsoft/language-server-protocol#291, jdt.ls here does the same as the python language server and, if I remember correctly, the same as clangd.

A simplified explanation:

The response to hover request is an array of something called MarkedText.

The first MarkedText is a dictionary looking like this { 'language': 'java', 'value': 'whatever GetType returns usually' }

The second to last MarkedText is just a string containing lines of documentation (for our GetDoc).

Once again there's no guarantee that this will be done for every LSP server we decide to integrate.
On the other hand, perhaps all interesting servers will actually work the same, so maybe we can implement GetType and GetDoc in language_server.py and be done.

OK so maybe it is a little more standardised. Thanks for the explanation.

ycmd/completers/completer.py, line 386 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'd say this needs another sentence or two. Why most completers don't implement this? Or what completers do implement this.

Good point indeed. It does.

Comments from Reviewable

puremourning · 2017-10-13T17:04:32Z

FWIW I think the test failures are flakes.

JAVA_SUPPORT.md was added mainly for you to comment on :) But we can keep it if we care to.

Review status: 54 of 60 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

micbou · 2017-10-14T21:04:51Z

I started to write a PHP completer relying on the PHP Language Server (PLS) in order to test your LSP implementation. I encountered several issues:

adding \r\n at the end of the request crashes PLS; it only expects \r\n inside the request itself. This newline is not needed anyway (see my comment);
as suggested by that section in PLS docs, the stdio connection doesn't work on Windows (or maybe I am doing something wrong but I couldn't make it work and I know it's not your implementation). I replaced it with the TCP connection and it works great;
PLS returns null for optional fields instead of omiting them if they are not used. It's illegal if I am reading the protocol correctly. I though about raising an issue on PLS repository but given the implementation, I doubt this will be changed. So, we should take care of that.

You can find the implementation in this branch. You should be able to test it by adding the --php-completer option to the build.py script. Completions, GoTo, GoToReferences, and diagnostics are working. Note that stopping the server is not yet implemented so you'll have to terminate the process yourself.

In conclusion, this PR looks really promising. Nice work.

FWIW I think the test failures are flakes.

We shouldn't ignore the OSError: [WinError 6] The handle is invalid errors. They are often a sign that a process was not properly terminated.

Reviewed 17 of 60 files at r1.
Review status: 57 of 60 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

build.py, line 100 at r1 (raw file):

  if not path:
    sys.exit( "ERROR: Unabel to find executable '{0}'. {1}".format(

Typo in Unable.

ycmd/completers/java/java_completer.py, line 34 at r1 (raw file):

from subprocess import PIPE

from ycmd import ( utils, responses )

Parentheses are not needed.

ycmd/completers/java/java_completer.py, line 75 at r1 (raw file):

#
# Cons:
#  - A little more complexity (we has the project path to create the workspace

"we has"? Missing word?

ycmd/completers/java/java_completer.py, line 83 at r1 (raw file):

# So:
#  - By _default_ we use a clean workspace (see default_settings.json) on each
#    Vim instance

We shouldn't talk about Vim. Let's say client instead.

ycmd/completers/java/java_completer.py, line 190 at r1 (raw file):

        # this requires some additional complexity and state management.
        self._StartServer()
      except:

Should be at least except Exception but ideally a more specific exception.

ycmd/completers/java/java_completer.py, line 277 at r1 (raw file):

  def ServerIsHealthy( self, request_data = {} ):

This function doesn't have a request_data parameter.

ycmd/completers/java/java_completer.py, line 281 at r1 (raw file):

      return False

    return True

Can be simplified to return self._ServerIsRunning().

ycmd/completers/java/java_completer.py, line 369 at r1 (raw file):

        _logger.info( 'jdt.ls Language Server started' )
      else:
        _logger.warning( 'jdt.ls Language Server failed to start' )

I would log this as an error.

ycmd/completers/java/java_completer.py, line 370 at r1 (raw file):

      else:
        _logger.warning( 'jdt.ls Language Server failed to start' )
        return

This could be rewritten without the else:

if not self._ServerIsRunning():
  _logger.warning( 'jdt.ls Language Server failed to start' )
  return
_logger.info( 'jdt.ls Language Server started' )

ycmd/completers/java/java_completer.py, line 385 at r1 (raw file):

      except language_server_completer.LanguageServerConnectionTimeout:
        _logger.warn( 'jdt.ls failed to start, or did not connect '
                      'successfully' )

Should be logged as an error.

ycmd/completers/java/java_completer.py, line 397 at r1 (raw file):

      # connected to our server connector. Just close stderr.
      if self._server_handle and self._server_handle.stderr:
        self._server_handle.stderr.close()

Not closing stdin and stdout may be the cause of the errors on AppVeyor. Are you sure we can't use CloseStandardStreams after stopping the server? I tried and didn't encounter issues restarting the server.

ycmd/completers/java/java_completer.py, line 407 at r1 (raw file):

      # If the server is still running, e.g. due to errors, kill it
      self._StopServerForecefully()

_StopServerForcefully

ycmd/completers/java/java_completer.py, line 410 at r1 (raw file):

      # Tidy up our internal state
      self._Reset()

Could we define a CleanUp function instead like we do for other completers?

ycmd/completers/java/java_completer.py, line 433 at r1 (raw file):

  def _StopServerForecefully( self ):

_StopServerForcefully

ycmd/completers/language_server/language_server_completer.py, line 458 at r1 (raw file):

  def _Write( self, data ):
    bytes_to_write = data + utils.ToBytes( '\r\n' )

Appending \r\n is not needed. In fact, this can prevent other language servers from working (e.g. PHP Language Server) because they only expect it in the request itself.

ycmd/completers/language_server/language_server_completer.py, line 673 at r1 (raw file):

      'File',
      'Reference',
    ]

Should be defined at the top of this file. There is no reason to recompute it each time we call this function.

ycmd/completers/language_server/language_server_completer.py, line 748 at r1 (raw file):

        detailed_info = ( item[ 'label' ] +
                          '\n\n' +
                          item.get( 'documentation', '' ) ),

You are not going to like this but some language servers return null for this field (e.g. PHP Language Server). This results in concatenating a string with None which is illegal. We need to check that item[ 'documentation' ] is not None.

ycmd/completers/language_server/language_server_completer.py, line 1305 at r1 (raw file):

      raise IncompatibleCompletionException( textEdit[ 'newText' ] )

  additional_text_edits.extend( item.get( 'additionalTextEdits', [] ) )

The additionalTextEdits field can be null and Python doesn't like extending an array with None. We need to check that item[ 'additionalTextEdits' ] is not None.

ycmd/tests/java/init.py, line 113 at r1 (raw file):

def WaitUntilCompleterServerReady( app, timeout = 30 ):

This function is already defined in tests/test_utils.py.

Comments from Reviewable

puremourning · 2017-10-14T23:54:40Z

Thanks so much for trying it out and taking a look!

Review status: 57 of 60 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

build.py, line 100 at r1 (raw file):

Previously, micbou wrote…

Typo in Unable.

Done.

ycmd/completers/java/java_completer.py, line 34 at r1 (raw file):

Previously, micbou wrote…

Parentheses are not needed.

Done.

ycmd/completers/java/java_completer.py, line 75 at r1 (raw file):

Previously, micbou wrote…

"we has"? Missing word?

"hash"

ycmd/completers/java/java_completer.py, line 83 at r1 (raw file):

Previously, micbou wrote…

We shouldn't talk about Vim. Let's say client instead.

Done.

ycmd/completers/java/java_completer.py, line 190 at r1 (raw file):

Previously, micbou wrote…

Should be at least except Exception but ideally a more specific exception.

Done.

Also noted why it is a generic catch.

ycmd/completers/java/java_completer.py, line 277 at r1 (raw file):

Previously, micbou wrote…

This function doesn't have a request_data parameter.

Good point. Removed this from here and also tern_completer which I copied it from !

ycmd/completers/java/java_completer.py, line 281 at r1 (raw file):

Previously, micbou wrote…

Can be simplified to return self._ServerIsRunning().

Done.

ycmd/completers/java/java_completer.py, line 369 at r1 (raw file):

Previously, micbou wrote…

I would log this as an error.

Done.

ycmd/completers/java/java_completer.py, line 370 at r1 (raw file):

Previously, micbou wrote…

This could be rewritten without the else:

if not self._ServerIsRunning():
  _logger.warning( 'jdt.ls Language Server failed to start' )
  return
_logger.info( 'jdt.ls Language Server started' )

Done.

ycmd/completers/java/java_completer.py, line 385 at r1 (raw file):

Previously, micbou wrote…

Should be logged as an error.

Done.

ycmd/completers/java/java_completer.py, line 397 at r1 (raw file):

Previously, micbou wrote…

Not closing stdin and stdout may be the cause of the errors on AppVeyor. Are you sure we can't use CloseStandardStreams after stopping the server? I tried and didn't encounter issues restarting the server.

Well, you could be right that there are still shutdown races. I'll review it again.

But I can't use CloseStandardStreams because that completely prevents the server shutting down cleanly. Doing so breaks all the tests ! :)

ycmd/completers/java/java_completer.py, line 407 at r1 (raw file):

Previously, micbou wrote…

_StopServerForcefully

Done.

ycmd/completers/java/java_completer.py, line 410 at r1 (raw file):

Previously, micbou wrote…

Could we define a CleanUp function instead like we do for other completers?

Renamed to _CleanUp

ycmd/completers/java/java_completer.py, line 433 at r1 (raw file):

Previously, micbou wrote…

_StopServerForcefully

Done.

ycmd/completers/language_server/language_server_completer.py, line 458 at r1 (raw file):

Previously, micbou wrote…

Appending \r\n is not needed. In fact, this can prevent other language servers from working (e.g. PHP Language Server) because they only expect it in the request itself.

OK sure. Done.

ycmd/completers/language_server/language_server_completer.py, line 673 at r1 (raw file):

Previously, micbou wrote…

Should be defined at the top of this file. There is no reason to recompute it each time we call this function.

I've moved the ls-specific stuff into lsapi.py and the non-LS specific stuff to the top.

ycmd/completers/language_server/language_server_completer.py, line 748 at r1 (raw file):

Previously, micbou wrote…

You are not going to like this but some language servers return null for this field (e.g. PHP Language Server). This results in concatenating a string with None which is illegal. We need to check that item[ 'documentation' ] is not None.

You're right. I don't like that because it seems dodgy. I also can't really test it.

ycmd/completers/language_server/language_server_completer.py, line 1305 at r1 (raw file):

Previously, micbou wrote…

The additionalTextEdits field can be null and Python doesn't like extending an array with None. We need to check that item[ 'additionalTextEdits' ] is not None.

As above, this is not legal in LSP, and doesn't appear to be done by jdt.ls. Perhaps we can defer making this change until we need it ?

ycmd/tests/java/init.py, line 113 at r1 (raw file):

Previously, micbou wrote…

This function is already defined in tests/test_utils.py.

There is a small difference in the implementation (it uses a different ping timeout), but it also did some other stuff in the past. I have replaced with a call to test_utils

Comments from Reviewable

codecov-io · 2017-10-14T23:54:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@89db48f). Click here to learn what that means.
The diff coverage is 98.08%.

@@            Coverage Diff            @@
##             master     #857   +/-   ##
=========================================
  Coverage          ?   96.18%           
=========================================
  Files             ?       83           
  Lines             ?     6468           
  Branches          ?        0           
=========================================
  Hits              ?     6221           
  Misses            ?      247           
  Partials          ?        0

Valloric · 2017-10-17T19:55:11Z

This is an ungodly amount of work; thanks a ton for doing this!

I don't have any time to review this now, I'll have to block off some time on the weekend. I read through the PR description and it all seems very sensible. I do agree that we might want to declare this as experimental in the docs to begin with; should provide @puremourning with the flexibility of breaking backcompat if necessary as the completer develops.

I'd say a max 6 month experimental baking period is reasonable.

Review status: 54 of 61 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

puremourning · 2017-10-19T11:55:29Z

So somehow the latest patch really broken shutdown on Windows. I'll have to debug it.

Who woulda thought shutting down was so hard :)

Valloric · 2017-10-22T20:54:54Z

@puremourning Did a review pass. This is a criminally big PR! :P Thanks for doing all this work though, it's hugely appreciated. ;)

Review status: 51 of 61 files reviewed at latest revision, 18 unresolved discussions.

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

def EnableGoCompleter():
  go = FindExecutableOrDie( 'go', 'go is required to build gocode.' )

Oh I like this OrDie version, very nice!

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

                    'eclipse.jdt.ls' ) )

  if OnWindows():

Perhaps mvnw = X if OnWindows() else Y? Just a thought; up to you.

ycmd/completers/java/java_completer.py, line 107 at r5 (raw file):

def _PathToLauncherJar():
  # The file name changes between version of eclipse, so we use a glob as
  # recommended by the language server developers. There should only be one.

I would rephrase the last sentence to "There can be only one" so as to make a Highlander reference, but that's just me. :D

ycmd/completers/language_server/language_server_completer.py, line 159 at r5 (raw file):

    This connection runs as a thread and is generally only used directly by
    LanguageServerCompleter, but is instantiated, startd and stopped by Concrete

startd -> started

ycmd/completers/language_server/language_server_completer.py, line 163 at r5 (raw file):

    Implementations of this class are required to provide the following methods:
      - _TryServerConnectionBlocking: Connect to the server and return when the

If implementations need to provide some methods, why do those methods need to have the _ prefix which denotes a private method? If a method is a part of a contract, it should be public, no?

ycmd/completers/language_server/language_server_completer.py, line 183 at r5 (raw file):

      LanguageServerCompleter.ShutdownServer to do that part)
    - Call Close() to close any remaining streams. Do this in a request thread.
      DO NOT CALL THIS FROM THE DISPATCH THREAD.

You should clarify what the dispatch thread is here (I can guess, but it's not really clear).

ycmd/completers/language_server/language_server_completer.py, line 267 at r5 (raw file):

  def NextRequestId( self ):
    with self._responseMutex:
      self._lastId += 1

I think our code style is _last_id, not _lastId.

ycmd/completers/language_server/language_server_completer.py, line 419 at r5 (raw file):

  def _DespatchMessage( self, message ):

"Dispatch" is the preferred spelling, even in British English.

http://grammarist.com/spelling/dispatch-despatch/

ycmd/completers/language_server/language_server_completer.py, line 659 at r5 (raw file):

  def ResolveCompletionItems( self, items, request_data ):

This is a pretty big method; could some parts be extracted into helper methods?

ycmd/completers/language_server/language_server_completer.py, line 731 at r5 (raw file):

      # Build a ycmd-compatible completion for the text as we received it. Later
      # we might mofify insertion_text should we see a lower start codepoint.
      completions.append( responses.BuildCompletionData(

This item -> completion mapping looks like it could be extracted into a helper method.

ycmd/completers/language_server/language_server_completer.py, line 907 at r5 (raw file):

  def _RefreshFiles( self, request_data ):

The name is a bit vague; where are the files being refreshed? Why are they being refreshed? What does refresh even mean?

Until I read the doc comment below, I didn't even think this would talk to the server.

Maybe UpdateFilesStateOnServer? Just a thought; better names probably exist.

ycmd/completers/language_server/language_server_completer.py, line 1092 at r5 (raw file):

  def CodeAction( self, request_data, args ):

Vague name. It also contains no verbs so I don't know what is being performed here.

ycmd/completers/language_server/language_server_completer.py, line 1157 at r5 (raw file):

  def Rename( self, request_data, args ):

Vague name. Rename what to what? Where is it being renamed? Does it send a request to the server or the client? It's not really clear.

ycmd/completers/language_server/language_server_completer.py, line 1212 at r5 (raw file):

def InsertionTextForItem( request_data, item ):

This is rather big; could it be broken up into helpers?

ycmd/completers/language_server/lsapi.py, line 1 at r5 (raw file):

# Copyright (C) 2016 ycmd contributors

lsapi is a very confusing name. :) Let's pick something more obvious.

Also, it hasn't been 2016 for a while now. :D

Comments from Reviewable

puremourning · 2017-10-22T23:05:31Z

Thanks so much for looking at it.

Review status: 51 of 61 files reviewed at latest revision, 18 unresolved discussions.

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

Previously, Valloric (Val Markovic) wrote…

Perhaps mvnw = X if OnWindows() else Y? Just a thought; up to you.

Yeah, that's better. Done.

ycmd/completers/language_server/language_server_completer.py, line 159 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

startd -> started

Done.

ycmd/completers/language_server/language_server_completer.py, line 163 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

If implementations need to provide some methods, why do those methods need to have the _ prefix which denotes a private method? If a method is a part of a contract, it should be public, no?

I was somewhat torn about this. In the end I went with non-underscore methods are public in the sense that you should call then in the form "my_thing.MyMethod()" and any "protected" method (either used by, or implemented by concrete types) gets an underscore prefix. For example:

The user of the LanguageServerConnection (i.e. the person who instantiates a StandardIOLanguageServerConnection calls the Close() method, but the implementer of the LanguageServerConnection API (i.e. StandardIOLanguageServerConnection) implements the _Close() method. The user of the class must not call _Close() directly.

Perhaps I should go with CloseInner() like we do in other places? I thought that felt a little awkward, but happy to take a consensus.

On reflection, I didn't completely consistently apply that rule, despite revising it multiple times. What do you think is the "right thing" for these cases:

methods that are for users of a class to call. I think this is clear-cut public method: my object.DoAThing()
methods that are not for users to call, but for concrete classes to implement and for the base class to call
methods that are on base class, but for concrete classes to call (sort if like protected methods), GetAThing()

ycmd/completers/language_server/language_server_completer.py, line 183 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

You should clarify what the dispatch thread is here (I can guess, but it's not really clear).

I've included some of the text from the PR description in the doc string explaining what the dispatch thread is and made this more explicit.

ycmd/completers/language_server/language_server_completer.py, line 267 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

I think our code style is _last_id, not _lastId.

yes it is, fixed.

ycmd/completers/language_server/language_server_completer.py, line 419 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

"Dispatch" is the preferred spelling, even in British English.

http://grammarist.com/spelling/dispatch-despatch/

Moreover, we've agreed that we should use US English in all the code and comments! Fixed.

ycmd/completers/language_server/language_server_completer.py, line 659 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

This is a pretty big method; could some parts be extracted into helper methods?

As below, I've extracted a piece, but this whole "resolve and then mess around with completions" stuff is just frustratingly complicated due to the protocol. Happy to iterate on this if it needs to go further.

ycmd/completers/language_server/language_server_completer.py, line 731 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

This item -> completion mapping looks like it could be extracted into a helper method.

Done.

The rest of the method is 40 lines removing comments and empty lines. Seem ok ?

ycmd/completers/language_server/language_server_completer.py, line 907 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

The name is a bit vague; where are the files being refreshed? Why are they being refreshed? What does refresh even mean?

Until I read the doc comment below, I didn't even think this would talk to the server.

Maybe UpdateFilesStateOnServer? Just a thought; better names probably exist.

Went with _UpdateServerWithFileContents

ycmd/completers/language_server/language_server_completer.py, line 1092 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

Vague name. It also contains no verbs so I don't know what is being performed here.

Changed to GetCodeActions (codeAction is the LSP term for something like what we call FixIts; it's like "stuff you can do with the current selection").

ycmd/completers/language_server/language_server_completer.py, line 1157 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

Vague name. Rename what to what? Where is it being renamed? Does it send a request to the server or the client? It's not really clear.

Changed to RefactorRename to match the subcommand that we use (it's like GoToDefinition etc.).

ycmd/completers/language_server/language_server_completer.py, line 1212 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

This is rather big; could it be broken up into helpers?

I pulled out the validation of the textEdit as that is a chunk of annoying code that distracts from the important stuff.

ycmd/completers/language_server/lsapi.py, line 1 at r5 (raw file):

Also, it hasn't been 2016 for a while now

Tell me about it. Then again, it was 2016 when I wrote it, and first pushed it to github, so strictly that's from when I (we) claim copyright on it, and is also the year of first "publication". Then again, IANAL as a wise man once said.

Then again, perhaps the year it is merged is the year of "first publication". I really don't know.

lsapi is a very confusing name. :) Let's pick something more obvious.

I renamed the module to language_server_protocol and imported it as lsp because it is used a lot.

Comments from Reviewable

puremourning · 2017-10-22T23:38:24Z

Review status: 50 of 61 files reviewed at latest revision, 17 unresolved discussions.

ycmd/completers/completer.py, line 386 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Good point indeed. It does.

Done. I also tidied up the API and updated the Completer API which was also missing.

And fixed a bug refactoring error. Yeah, that too.

Comments from Reviewable

Valloric · 2017-10-26T03:09:55Z

Review status: 46 of 66 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

ycmd/completers/language_server/language_server_completer.py, line 163 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I was somewhat torn about this. In the end I went with non-underscore methods are public in the sense that you should call then in the form "my_thing.MyMethod()" and any "protected" method (either used by, or implemented by concrete types) gets an underscore prefix. For example:

The user of the LanguageServerConnection (i.e. the person who instantiates a StandardIOLanguageServerConnection calls the Close() method, but the implementer of the LanguageServerConnection API (i.e. StandardIOLanguageServerConnection) implements the _Close() method. The user of the class must not call _Close() directly.

Perhaps I should go with CloseInner() like we do in other places? I thought that felt a little awkward, but happy to take a consensus.

On reflection, I didn't completely consistently apply that rule, despite revising it multiple times. What do you think is the "right thing" for these cases:

methods that are for users of a class to call. I think this is clear-cut public method: my object.DoAThing()

methods that are not for users to call, but for concrete classes to implement and for the base class to call

methods that are on base class, but for concrete classes to call (sort if like protected methods), GetAThing()

Well, the FooInner convention is for when Foo exists in the base class, but the subclass needs to plug into that logic with something custom. So FooInner. It's not really a great pattern, I was just lazy. :D

WRT underscore, I understand your reasoning. I thought that might have been it. To think of it in terms of other OOP languages like say Java, a method that needs to be implemented in a subclass is pretty much always public. Java won't even let you create a private abstract method, and method prototypes declared in interfaces can't even be anything except public.

There's a fairly strong culture of "public" meaning "this is the interface/API surface of this class" (which does include methods meant to be overridden), which is a wider definition than "clients are allowed to call this." Prominent voices in Java land also claim that protected was a mistake because it still means public (anyone can subclass a class) just in a roundabout way.

I think ultimately it would be very confusing to readers to see private (i.e. underscore) methods be overridden. Maybe I'm wrong, but it did make me go "wait, wtf" when I first saw it. :)

ycmd/completers/language_server/language_server_completer.py, line 731 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

The rest of the method is 40 lines removing comments and empty lines. Seem ok ?

It's ok (but barely) for now; I'm more concern with this method getting more code added to it in the future and becoming a monster. It has the potential.

Keep an eye on it in future PRs. If it gets any more code added to it, it really needs splitting up.

ycmd/completers/language_server/language_server_completer.py, line 907 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Went with _UpdateServerWithFileContents

Nice name!

ycmd/completers/language_server/language_server_protocol.py, line 1 at r5 (raw file):

so strictly that's from when I (we) claim copyright on it,

I feel it's important to point out it's not "we" who have copyright on your changes, only you do. :) The whole "ycmd contributors" thing does not change who owns the copyright to this code (only you do), it merely makes it so that we don't have to list each and every single contributor in the header.

The changelog still has authorship info which is why we can do this.

Comments from Reviewable

puremourning · 2017-10-26T21:27:47Z

Thanks again for the thoughts!

Review status: 46 of 66 files reviewed at latest revision, 6 unresolved discussions.

ycmd/completers/language_server/language_server_completer.py, line 163 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

Well, the FooInner convention is for when Foo exists in the base class, but the subclass needs to plug into that logic with something custom. So FooInner. It's not really a great pattern, I was just lazy. :D

WRT underscore, I understand your reasoning. I thought that might have been it. To think of it in terms of other OOP languages like say Java, a method that needs to be implemented in a subclass is pretty much always public. Java won't even let you create a private abstract method, and method prototypes declared in interfaces can't even be anything except public.

There's a fairly strong culture of "public" meaning "this is the interface/API surface of this class" (which does include methods meant to be overridden), which is a wider definition than "clients are allowed to call this." Prominent voices in Java land also claim that protected was a mistake because it still means public (anyone can subclass a class) just in a roundabout way.

I think ultimately it would be very confusing to readers to see private (i.e. underscore) methods be overridden. Maybe I'm wrong, but it did make me go "wait, wtf" when I first saw it. :)

OK thanks! I change it to: non-private (i.e. no leading _) as meaning either "use by instantiators of the thingy" or "provide by implementations of a Thingy`. Everything else is private.

I also made everything private that isn't currently relied upon to be public. This restricts the API to "what's required" and we can open it up if subsequent servers require it.

I decided to completely ignore the thing i just learned about python name mangling and methods starting (but not ending) with __. Because I can unread it. I'm sure.

ycmd/completers/language_server/language_server_completer.py, line 731 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

It's ok (but barely) for now; I'm more concern with this method getting more code added to it in the future and becoming a monster. It has the potential.

Keep an eye on it in future PRs. If it gets any more code added to it, it really needs splitting up.

I looked again and decided that you're right; there are 2 other multi-call-blocks which could be broken out. the entire method without comments is now:

  def ResolveCompletionItems( self, items, request_data ):
    do_resolve = self._ShouldResolveCompletionItem( )
    completions = list()
    start_codepoints = list()
    min_start_codepoint = request_data[ 'start_codepoint' ]
    for item in items:
      if do_resolve:
        item = self._ResolveCompletionItem( item )
      try:
        ( insertion_text, fixits, start_codepoint ) = (
          _InsertionTextForItem( request_data, item ) )
      except IncompatibleCompletionException:
        _logger.exception( 'Ignoring incompatible completion suggestion '
                           '{0}'.format( item ) )
        continue
      min_start_codepoint = min( min_start_codepoint, start_codepoint )
      completions.append( _CompletionItemToCompletionData( insertion_text,
                                                           item,
                                                           fixits ) )
      start_codepoints.append( start_codepoint )
    if ( len( completions ) > 1 and
         min_start_codepoint != request_data[ 'start_codepoint' ] ):
      return _FixUpCompletionPrefixes( completions,
                                       start_codepoints,
                                       request_data,
                                       min_start_codepoint )
    request_data[ 'start_codepoint' ] = min_start_codepoint
    return completions

ycmd/completers/language_server/language_server_protocol.py, line 1 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

so strictly that's from when I (we) claim copyright on it,

I feel it's important to point out it's not "we" who have copyright on your changes, only you do. :) The whole "ycmd contributors" thing does not change who owns the copyright to this code (only you do), it merely makes it so that we don't have to list each and every single contributor in the header.

The changelog still has authorship info which is why we can do this.

Heh, ok, sure. Then again, you gotta be some level of nuts to copy my code :)

Comments from Reviewable

bstaletic · 2017-10-27T05:09:53Z

Here are my thoughts after playing around with a python language server.

First, jdt.ls probably wasn''t the best choice to showcase ycmd's LSP support. To be fair, jdt.ls was chosen for a completely different reason.
The working implementation of python-language-server was 40% smaller than java_completer.py, because it doesn't have all the nonsense regarding start up or workspace dir.
Once I figured out what are jdt.ls oddities in java_completer.py it was easy to get everything working.

What was working:

Completion
Diagnostics
GoTo, GoToDefinition, GoToDeclaration (all are the same)
GoToReferences
GetDoc

Bottom line is that it was a neat API to work with.

Reviewed 3 of 61 files at r1, 5 of 7 files at r2, 3 of 4 files at r3, 2 of 2 files at r5, 1 of 6 files at r6, 15 of 16 files at r7.
Review status: 64 of 66 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

JAVA_SUPPORT.md, line 224 at r3 (raw file):

horrible user experience where the diagnostics always lag one edit behind.

To resolve this, we use the long-polling mechanism added here (`ReceiveMessages`

How does long-polling mechanism solve this problem? Later in the code a comment says it polls once every hour.
I'm probably being blind, but where is the period defined?
I've tried jdt.ls and the diags work, but how does it handle changes in diagnostics that happen quicker than an hour?

third_party/eclipse.jdt.ls, line 1 at r7 (raw file):

Subproject commit aec31fbb8fa88bc5c6c205ac517a770ad035b38d

This is my biggest concern about this PR. It had to be changed more than once and we haven't even had a single LGTM.
They seriously need to solve the stable builds.

ycmd/completers/java/java_completer.py, line 107 at r5 (raw file):

Previously, Valloric (Val Markovic) wrote…

I would rephrase the last sentence to "There can be only one" so as to make a Highlander reference, but that's just me. :D

One more in favour of the Highlander reference.

Comments from Reviewable

Valloric · 2017-10-30T21:39:52Z

Review status: 64 of 66 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

ycmd/completers/language_server/language_server_completer.py, line 731 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I looked again and decided that you're right; there are 2 other multi-call-blocks which could be broken out. the entire method without comments is now:

  def ResolveCompletionItems( self, items, request_data ):
    do_resolve = self._ShouldResolveCompletionItem( )
    completions = list()
    start_codepoints = list()
    min_start_codepoint = request_data[ 'start_codepoint' ]
    for item in items:
      if do_resolve:
        item = self._ResolveCompletionItem( item )
      try:
        ( insertion_text, fixits, start_codepoint ) = (
          _InsertionTextForItem( request_data, item ) )
      except IncompatibleCompletionException:
        _logger.exception( 'Ignoring incompatible completion suggestion '
                           '{0}'.format( item ) )
        continue
      min_start_codepoint = min( min_start_codepoint, start_codepoint )
      completions.append( _CompletionItemToCompletionData( insertion_text,
                                                           item,
                                                           fixits ) )
      start_codepoints.append( start_codepoint )
    if ( len( completions ) > 1 and
         min_start_codepoint != request_data[ 'start_codepoint' ] ):
      return _FixUpCompletionPrefixes( completions,
                                       start_codepoints,
                                       request_data,
                                       min_start_codepoint )
    request_data[ 'start_codepoint' ] = min_start_codepoint
    return completions

👍

Comments from Reviewable

micbou · 2017-10-31T02:21:56Z

Reviewed 36 of 61 files at r1, 4 of 7 files at r2, 1 of 1 files at r4, 2 of 2 files at r5, 4 of 6 files at r6, 13 of 16 files at r7.
Review status: 65 of 66 files reviewed at latest revision, 43 unresolved discussions, some commit checks failed.

JAVA_SUPPORT.md, line 224 at r3 (raw file):
Long-polling involves sending a request with an arbitrarily long timeout (e.g. 1 hour) and resending it when the server responds; repeating that process ad vitam æternam.

I'm probably being blind, but where is the period defined?

In the client.

README.md, line 93 at r7 (raw file):

OmniSharp-based completer for C#, a [Gocode][gocode]-based completer for Go
(using [Godef][godef] for jumping to definitions), and a TSServer-based
completer for TypeScript. More will be added with time.

The Java completer is not mentioned.

examples/example_client.py, line 489 at r7 (raw file):

  # Send the long poll 10 times (only the first N will return any useful
  # messages)
  for i in range(1, 6):

Are we polling 6 or 10 times?

examples/example_client.py, line 508 at r7 (raw file):

  # Send the long poll 10 times (only the first N will return any useful
  # messages)
  for i in range(1, 6):

Same comment.

ycmd/completers/completer.py, line 154 at r6 (raw file):

  Language Server Protocol, then you can override the PollForMessagesInner
  method. This method is called by the client in the "long poll" fashion to
  receive unsolicted messages. The method should block until a message is

unsolicited

ycmd/completers/java/java_completer.py, line 358 at r7 (raw file):

      self._server_stderr = utils.CreateLogfile(
          LOGFILE_FORMAT.format( pid = os.getpid(), std = 'stderr' ) )

Using ycmd pid in the logfile name seems out of place. Not even sure why we need to put stderr. Could simply be jdt.ls_.

ycmd/completers/language_server/language_server_completer.py, line 1305 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

As above, this is not legal in LSP, and doesn't appear to be done by jdt.ls. Perhaps we can defer making this change until we need it ?

Yes, this can wait.

ycmd/completers/language_server/language_server_completer.py, line 85 at r6 (raw file):

  LanguageServerCompleter handles create an instance of this class for each
  request that expects a response and wait for its response synchonously by

Missing r in synchronously.

ycmd/completers/language_server/language_server_completer.py, line 119 at r6 (raw file):

  def AwaitResponse( self, timeout ):
    """Called by clients to wait syncronously for either a response to be

Missing h in synchronously.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

class LanguageServerConnection( threading.Thread ):
  """
    Abstract language server communication object.

Why is this indented with 4 spaces?

ycmd/completers/language_server/language_server_completer.py, line 160 at r6 (raw file):

    This connection runs as a thread and is generally only used directly by
    LanguageServerCompleter, but is instantiated, started and stopped by
    Concrete LanguageServerCompleter implementations.

Should concrete be capitalized?

ycmd/completers/language_server/language_server_completer.py, line 172 at r6 (raw file):

    Threads:

    LSP is by its nature an asyncronous protocol. There are request-reply like

Missing h in asynchronous.

ycmd/completers/language_server/language_server_completer.py, line 442 at r6 (raw file):

    return ( data, read_bytes, headers )

Parentheses are not needed.

ycmd/completers/language_server/language_server_completer.py, line 603 at r6 (raw file):

  def ShutdownServer( self ):
    """Send the shutdown and possibly exit request to the server.
    Implemenations must call this prior to closing the LanguageServerConnection

Typo in Implementations.

ycmd/completers/language_server/language_server_completer.py, line 756 at r6 (raw file):

      # Build a ycmd-compatible completion for the text as we received it. Later
      # we might mofify insertion_text should we see a lower start codepoint.

Typo in modify.

ycmd/completers/language_server/language_server_completer.py, line 930 at r6 (raw file):

    close any buffers no longer open.

    This method should be called frequently and in any event before a syncronous

Missing h in synchronous.

ycmd/completers/language_server/language_server_completer.py, line 973 at r6 (raw file):

    """Return the directory in which the server should operate. Language server
    protocol and most servers have a concept of a 'project directory'. By
    default this is the working directory of the ycmd server, but implemenations

Missing t in implementations.

ycmd/completers/language_server/language_server_completer.py, line 1042 at r6 (raw file):

    """Register a function to be called when the initialize exchange completes.
    The function |handler| will be called on successful completion of the
    initalize exchange with a single argument |self|, which is the |self| passed

Missing i in initialize.

ycmd/completers/language_server/language_server_completer.py, line 54 at r7 (raw file):

  'Warning': 'WARNING',
  'Information': 'WARNING',
  'Hint': 'WARNING'

I feel we should add INFORMATION and HINTto our diagnostic severities and convert them to WARNING in YCM.

ycmd/completers/language_server/language_server_completer.py, line 299 at r7 (raw file):

    with self._response_mutex:
      self._last_id += 1
      return str( self._last_id )

The protocol supports a number for the id field. Are we converting to a string because jdt.ls does not support numbers in that field? If that's the case, shouldn't we report the issue on the jdt.ls tracker?

ycmd/completers/language_server/language_server_completer.py, line 311 at r7 (raw file):

    with self._response_mutex:
      assert request_id not in self._responses

If the assertion fails, the thread will be killed and the completer won't work anymore. I think we should do our best to always keep the connection alive. I would replace the assertion with a _logger.error statement and let the existing response be overwritten by the new one.

ycmd/completers/language_server/language_server_completer.py, line 360 at r7 (raw file):

    data = bytes( b'' )
    while True:
      ( data, read_bytes, headers ) = self._ReadHeaders( data )

These parentheses are not needed.

ycmd/completers/language_server/language_server_completer.py, line 458 at r7 (raw file):

    if 'id' in message:
      with self._response_mutex:
        assert str( message[ 'id' ] ) in self._responses

Same comment as above about the assert statement. In that case, we should bail out by returning early.

ycmd/completers/language_server/language_server_completer.py, line 600 at r7 (raw file):

      self._serverFileState = {}
      self._latest_diagnostics = collections.defaultdict( list )
      self._syncType = 'Full'

Should be named _sync_type.

ycmd/completers/language_server/language_server_completer.py, line 603 at r7 (raw file):

      self._initialise_response = None
      self._initialise_event = threading.Event()
      self._on_initialise_complete_handlers = list()

initialize instead of initialise for these three variables (that's the spelling of the request in the protocol).

ycmd/completers/language_server/language_server_completer.py, line 761 at r7 (raw file):

      try:
        ( insertion_text, fixits, start_codepoint ) = (

Parentheses around insertion_text, fixits, start_codepoint are not needed.

ycmd/completers/language_server/language_server_completer.py, line 994 at r7 (raw file):

  def SendInitialise( self ):

SendInitialize? Could be renamed to SendInitializeRequest.

ycmd/completers/language_server/language_server_completer.py, line 1019 at r7 (raw file):

  def _HandleInitialiseInPollThread( self, response ):

_HandleInitializeInPollThread.

ycmd/completers/language_server/language_server_completer.py, line 1198 at r7 (raw file):

                        'Usage: RefactorRename <new name>' )

Two blank lines.

ycmd/completers/language_server/language_server_completer.py, line 1267 at r7 (raw file):

  # Abort this request if the server is buggy and ignores us.
  assert lsp.INSERT_TEXT_FORMAT[
    item.get( 'insertTextFormat', 1 ) ] == 'PlainText'

We should avoid assert statements altogether. They can be optimized away.

ycmd/completers/language_server/language_server_completer.py, line 1272 at r7 (raw file):

  start_codepoint = request_data[ 'start_codepoint' ]
  # We will alwyas have one of insertText or label

always.

ycmd/completers/language_server/language_server_completer.py, line 1325 at r7 (raw file):

      [ responses.FixIt( chunks[ 0 ].range.start_, chunks ) ] )

  return ( insertion_text, fixits, start_codepoint )

Parentheses are not needed.

ycmd/completers/language_server/language_server_completer.py, line 1373 at r7 (raw file):

    raise RuntimeError( 'Cannot jump to location' )
  except KeyError:
    raise RuntimeError( 'Cannot jump to location' )

Both except statements can be merged.

ycmd/completers/language_server/language_server_completer.py, line 1386 at r7 (raw file):

    filename = ''
    file_contents = []
  except IOError:

No logging here? Also, filename is not defined in that case.

ycmd/completers/language_server/language_server_completer.py, line 1423 at r7 (raw file):

    file_contents = utils.SplitLines( GetFileContents( request_data,
                                                       filename ) )
  except IOError:

Should we log something here?

ycmd/completers/language_server/language_server_completer.py, line 1446 at r7 (raw file):

  r = _BuildRange( request_data, filename, diag[ 'range' ] )

  return responses.BuildDiagnosticData ( responses.Diagnostic(

Extra space before parenthesis.

ycmd/completers/language_server/language_server_completer.py, line 1481 at r7 (raw file):

  # We sort the filenames to make the response stable. Edits are applied in
  # strict sequence within a file, but apply to files in arbitrary order.
  # However, it's important for the response to be stable for the tests.

This doesn't look like a good reason. We can always sort the response in the tests.

ycmd/completers/language_server/language_server_protocol.py, line 253 at r7 (raw file):

  data = ToBytes( json.dumps( message, sort_keys=True ) )
  packet = ToBytes( 'Content-Length: {0}\r\n'
                    'Content-Type: application/vscode-jsonrpc;charset=utf8\r\n'

According to the protocol, we should use utf-8 instead of utf8. We may also consider removing this line since this is the default. This would save bandwidth.

ycmd/tests/clang/init.py, line 31 at r7 (raw file):

from ycmd.tests.test_utils import TemporaryTestDir as TemporaryClangTestDir # noqa

Let's replace all instances of TemporaryClangTestDir instead of using that trick.

ycmd/tests/java/diagnostics_test.py, line 27 at r7 (raw file):

from future.utils import iterkeys
from hamcrest import ( assert_that, contains, contains_inanyorder, has_entries )

Parentheses are not needed.

ycmd/tests/java/diagnostics_test.py, line 38 at r7 (raw file):

from ycmd.tests.test_utils import ( BuildRequest,
                                    LocationMatcher )

Can be rewritten on one line (without the parentheses).

ycmd/tests/java/get_completions_test.py, line 37 at r7 (raw file):

import requests

from ycmd.tests.java import ( DEFAULT_PROJECT_DIR, PathToTestFile, SharedYcmd )

Parentheses are not needed.

Comments from Reviewable

bstaletic · 2017-10-31T09:17:30Z

Reviewed 1 of 6 files at r6, 1 of 16 files at r7.
Review status: all files reviewed at latest revision, 43 unresolved discussions, some commit checks failed.

JAVA_SUPPORT.md, line 224 at r3 (raw file):

Previously, micbou wrote…

Long-polling involves sending a request with an arbitrarily long timeout (e.g. 1 hour) and resending it when the server responds; repeating that process ad vitam æternam.

I'm probably being blind, but where is the period defined?

In the client.

Thanks for the explanation.

ycmd/completers/language_server/language_server_completer.py, line 1481 at r7 (raw file):

Previously, micbou wrote…

This doesn't look like a good reason. We can always sort the response in the tests.

This was added after I wrote the test for GoToReferences. In the test we use contains even though contains_inanyorder exists. We've discussed it on ycmd gitter room at the time and I remember @puremourning having a reason why the response itself needs to be stable and why it is not enough to just do it in the tests.

At the time I did not quite understand the reasoning, so @puremourning should be able to explain why tthis was done this way.

Comments from Reviewable

puremourning · 2017-11-01T00:53:52Z

Review status: 59 of 68 files reviewed at latest revision, 44 unresolved discussions.

README.md, line 93 at r7 (raw file):

Previously, micbou wrote…

The Java completer is not mentioned.

Done.

examples/example_client.py, line 489 at r7 (raw file):

Previously, micbou wrote…

Are we polling 6 or 10 times?

Erm. 5 it would seem!

examples/example_client.py, line 508 at r7 (raw file):

Previously, micbou wrote…

Same comment.

Erm. 5 it would seem!

third_party/eclipse.jdt.ls, line 1 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

This is my biggest concern about this PR. It had to be changed more than once and we haven't even had a single LGTM.
They seriously need to solve the stable builds.

I don't see that we have any other choice. I asked about stable releases in the chat but didn't really get a straight answer.

It's possible that the problems before were transient. They seem to at least be using stable tags now, which is what we are pegging to.

ycmd/completers/completer.py, line 154 at r6 (raw file):

Previously, micbou wrote…

unsolicited

Done.

ycmd/completers/java/java_completer.py, line 107 at r5 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

One more in favour of the Highlander reference.

I'm not confident enough that the jdt.ls people and/or the dark magic of maven wouldn't break the declaration, which would be tragic for more than one reason.

ycmd/completers/java/java_completer.py, line 358 at r7 (raw file):

Previously, micbou wrote…

Using ycmd pid in the logfile name seems out of place. Not even sure why we need to put stderr. Could simply be jdt.ls_.

My assumption was that if we didn't use this, then wouldn't we get collisions in the case of multiple Vim instances all running an instance of jdt.ls? We often use the port of the server, but as we're not using TCP, we don't have a port to use. Using the ycmd pid at least means there's only one process writing to the file....

But on reflection, the return from NamedTemporaryFile is sufficiently unique so this is not a good reason.

I kept stderr in the name because it makes it clear that this is the server's standard error, as opposed to its log file (which is .log in the workspace directory).

ycmd/completers/language_server/language_server_completer.py, line 85 at r6 (raw file):

Previously, micbou wrote…

Missing r in synchronously.

Done. I ran through everything that Vim reports as misspelled in this file and the other main python files. I didn't do all of the tests, I confess.

ycmd/completers/language_server/language_server_completer.py, line 119 at r6 (raw file):

Previously, micbou wrote…

Missing h in synchronously.

Done.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, micbou wrote…

Why is this indented with 4 spaces?

Done.

ycmd/completers/language_server/language_server_completer.py, line 160 at r6 (raw file):

Previously, micbou wrote…

Should concrete be capitalized?

Nope. Done.

ycmd/completers/language_server/language_server_completer.py, line 172 at r6 (raw file):

Previously, micbou wrote…

Missing h in asynchronous.

Done.

ycmd/completers/language_server/language_server_completer.py, line 442 at r6 (raw file):

Previously, micbou wrote…

Parentheses are not needed.

Done.

ycmd/completers/language_server/language_server_completer.py, line 603 at r6 (raw file):

Previously, micbou wrote…

Typo in Implementations.

Done.

ycmd/completers/language_server/language_server_completer.py, line 756 at r6 (raw file):

Previously, micbou wrote…

Typo in modify.

Done.

ycmd/completers/language_server/language_server_completer.py, line 930 at r6 (raw file):

Previously, micbou wrote…

Missing h in synchronous.

Done.

ycmd/completers/language_server/language_server_completer.py, line 973 at r6 (raw file):

Previously, micbou wrote…

Missing t in implementations.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1042 at r6 (raw file):

Previously, micbou wrote…

Missing i in initialize.

Done.

ycmd/completers/language_server/language_server_completer.py, line 54 at r7 (raw file):

Previously, micbou wrote…

I feel we should add INFORMATION and HINTto our diagnostic severities and convert them to WARNING in YCM.

I considered that, but decided that it was yet another (mandatory) API change, and the YCM code for this is quite fiddly (with all of the filtering stuff). It seemed easier to keep the API the same. But sure, why not.

ycmd/completers/language_server/language_server_completer.py, line 299 at r7 (raw file):

Previously, micbou wrote…

The protocol supports a number for the id field. Are we converting to a string because jdt.ls does not support numbers in that field? If that's the case, shouldn't we report the issue on the jdt.ls tracker?

At least one server which I tested completely broke if the ID wasn't a string. There's no reason for us to be particular (we don't actually really care about what type the ID is, and a number is arbitrary). Given that this works, and we're already working around at least 3 other jdt.ls bugs, I think this is probably ok?

ycmd/completers/language_server/language_server_completer.py, line 311 at r7 (raw file):

Previously, micbou wrote…

If the assertion fails, the thread will be killed and the completer won't work anymore. I think we should do our best to always keep the connection alive. I would replace the assertion with a _logger.error statement and let the existing response be overwritten by the new one.

This code happens in a bottle thread, so bottle will catch the exception.
In any case I think it is better to die so that we know about the bug (because people are reporting it), rather than have it dormant forever and then bite us in some unexpected way later on. Better to fail early than late.

ycmd/completers/language_server/language_server_completer.py, line 360 at r7 (raw file):

Previously, micbou wrote…

These parentheses are not needed.

OK fine

ycmd/completers/language_server/language_server_completer.py, line 458 at r7 (raw file):

Previously, micbou wrote…

Same comment as above about the assert statement. In that case, we should bail out by returning early.

I think if we receive a response to a request that we didn't make, then it is perfectly reasonable to kill the dispatch thread and for it to stop working. It's highly unlikely that this is a recoverable situation - it means there's a serious protocol error or a serious bug. I'd honestly prefer to know about it at the point it is detected rather than at some point later.

This does indeed kill the message pump, and so stuff will just hang. I've added a catch and log to the dispatch thread main loop to at least signpost this in the log and give some advice to fix it.

We can perhaps, if this happens often, change this to automatically restart the connection, but as I said i would like to know about the bugs for now.

ycmd/completers/language_server/language_server_completer.py, line 600 at r7 (raw file):

Previously, micbou wrote…

Should be named _sync_type.

Done.

ycmd/completers/language_server/language_server_completer.py, line 603 at r7 (raw file):

Previously, micbou wrote…

initialize instead of initialise for these three variables (that's the spelling of the request in the protocol).

Done.

ycmd/completers/language_server/language_server_completer.py, line 761 at r7 (raw file):

Previously, micbou wrote…

Parentheses around insertion_text, fixits, start_codepoint are not needed.

Done.

ycmd/completers/language_server/language_server_completer.py, line 994 at r7 (raw file):

Previously, micbou wrote…

SendInitialize? Could be renamed to SendInitializeRequest.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1019 at r7 (raw file):

Previously, micbou wrote…

_HandleInitializeInPollThread.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1198 at r7 (raw file):

Previously, micbou wrote…

Two blank lines.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1267 at r7 (raw file):

Previously, micbou wrote…

We should avoid assert statements altogether. They can be optimized away.

I tend to disagree on this point, and agree (mostly) with what is said (at the start) here: https://wiki.python.org/moin/UsingAssertionsEffectively

If it gets optimised out by some JIT optimiser, then so be it, the assertions have no side-effects, so there's no harm no foul.

IMO anyway.

ycmd/completers/language_server/language_server_completer.py, line 1272 at r7 (raw file):

Previously, micbou wrote…

always.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1325 at r7 (raw file):

Previously, micbou wrote…

Parentheses are not needed.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1373 at r7 (raw file):

Previously, micbou wrote…

Both except statements can be merged.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1386 at r7 (raw file):

Previously, micbou wrote…

No logging here? Also, filename is not defined in that case.

No logging because it's a legit scenario. I was sure I'd written a comment about this, but clearly not here.

filename is defined because the IOError is thrown by GetFileContents after setting it to UriToFilePath. We could split the try/catch into two, but I found that made the code seem frustratingly overspecified.

Anyway I added both a comment and log.

ycmd/completers/language_server/language_server_completer.py, line 1423 at r7 (raw file):

Previously, micbou wrote…

Should we log something here?

Done.

ycmd/completers/language_server/language_server_completer.py, line 1446 at r7 (raw file):

Previously, micbou wrote…

Extra space before parenthesis.

Done.

ycmd/completers/language_server/language_server_completer.py, line 1481 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

This was added after I wrote the test for GoToReferences. In the test we use contains even though contains_inanyorder exists. We've discussed it on ycmd gitter room at the time and I remember @puremourning having a reason why the response itself needs to be stable and why it is not enough to just do it in the tests.

At the time I did not quite understand the reasoning, so @puremourning should be able to explain why tthis was done this way.

It's crucially important that the FixIt chunks are applied in a particular sequence. I felt that giving users of ycmd API a guaranteed stable interface (file-wise) seemed reasonable. I'd like to understand your objection to this before I change it as it is a lot of work, and as @bstaletic said, I spent days on this area getting it solid and well tested :/

ycmd/completers/language_server/language_server_protocol.py, line 253 at r7 (raw file):

Previously, micbou wrote…

According to the protocol, we should use utf-8 instead of utf8. We may also consider removing this line since this is the default. This would save bandwidth.

it appears they have changed the protocol since I wrote this.

i also think it is correct to be explicit as the protocol says "only supported right now" and hasn't been shy about changing it! At least one completer server that I tested required this, and another, which I fixed, barfed on it.

Unfortunately I wasn't able to easily see if VSCode sends this header or not, as there doesn't appear to be a full protocol tracing facility (and stdio can't be snooped aiui).

Anyway I've removed the header because it works, we can add it if it breaks.

ycmd/tests/clang/init.py, line 31 at r7 (raw file):

Previously, micbou wrote…

Let's replace all instances of TemporaryClangTestDir instead of using that trick.

Done.

ycmd/tests/java/diagnostics_test.py, line 27 at r7 (raw file):

Previously, micbou wrote…

Parentheses are not needed.

Fine

ycmd/tests/java/diagnostics_test.py, line 38 at r7 (raw file):

Previously, micbou wrote…

Can be rewritten on one line (without the parentheses).

Done.

ycmd/tests/java/get_completions_test.py, line 37 at r7 (raw file):

Previously, micbou wrote…

Parentheses are not needed.

Done.

Comments from Reviewable

bstaletic · 2017-11-01T07:21:04Z

Reviewed 1 of 6 files at r6, 11 of 11 files at r8.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

third_party/eclipse.jdt.ls, line 1 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I don't see that we have any other choice. I asked about stable releases in the chat but didn't really get a straight answer.

It's possible that the problems before were transient. They seem to at least be using stable tags now, which is what we are pegging to.

Okay, guess we just have to accept that our "old commits" might break eventually. Which kind of breaks the claim that every commit is a release.

Maybe we should consider something like --system-jdt.ls.

Comments from Reviewable

micbou · 2017-11-02T16:31:10Z

Reviewed 1 of 6 files at r6, 8 of 11 files at r8.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

ycmd/completers/java/java_completer.py, line 487 at r8 (raw file):

      try:
        get_type_java = hover_response[ 0 ][ 'value' ]
      except( TypeError ):

Unneeded parentheses.

ycmd/completers/java/java_completer.py, line 500 at r8 (raw file):

    if isinstance( hover_response, list ):
      if not len( hover_response ):
        raise RuntimeError( 'No information' )

NO_DOCUMENTATION_MESSAGE?

ycmd/completers/java/java_completer.py, line 512 at r8 (raw file):

    if not get_doc_java:
      raise ValueError( NO_DOCUMENTATION_MESSAGE )

Why a ValueError exception instead of RuntimeError?

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

      raise ValueError( NO_DOCUMENTATION_MESSAGE )

    return responses.BuildDisplayMessageResponse( get_doc_java.rstrip() )

BuildDetailedInfoResponse should be used here.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

Still indented with 4 spaces instead of 2.

ycmd/completers/language_server/language_server_completer.py, line 299 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

At least one server which I tested completely broke if the ID wasn't a string. There's no reason for us to be particular (we don't actually really care about what type the ID is, and a number is arbitrary). Given that this works, and we're already working around at least 3 other jdt.ls bugs, I think this is probably ok?

Fair enough.

ycmd/completers/language_server/language_server_completer.py, line 311 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This code happens in a bottle thread, so bottle will catch the exception.
In any case I think it is better to die so that we know about the bug (because people are reporting it), rather than have it dormant forever and then bite us in some unexpected way later on. Better to fail early than late.

Right. I thought this was in the same thread as the other assert statement. I am fine with it then since we'll see it in the logs if the assertion fails.

ycmd/completers/language_server/language_server_completer.py, line 458 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think if we receive a response to a request that we didn't make, then it is perfectly reasonable to kill the dispatch thread and for it to stop working. It's highly unlikely that this is a recoverable situation - it means there's a serious protocol error or a serious bug. I'd honestly prefer to know about it at the point it is detected rather than at some point later.

This does indeed kill the message pump, and so stuff will just hang. I've added a catch and log to the dispatch thread main loop to at least signpost this in the log and give some advice to fix it.

We can perhaps, if this happens often, change this to automatically restart the connection, but as I said i would like to know about the bugs for now.

I experienced this while implementing RLS (Rust Language Server) support. The server is incorrectly returning two responses with the same id. It took me some time to find the cause as there was nothing in the logs. Now that we are logging the assertion, I am fine with keeping the assert. I'll report the id issue on RLS repository.

Just one thing. Can we introduce an intermediate variable message_id to avoid doing str( message[ 'id' ] ) three times?

ycmd/completers/language_server/language_server_completer.py, line 1267 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I tend to disagree on this point, and agree (mostly) with what is said (at the start) here: https://wiki.python.org/moin/UsingAssertionsEffectively

If it gets optimised out by some JIT optimiser, then so be it, the assertions have no side-effects, so there's no harm no foul.

IMO anyway.

That link changed my mind. Let's keep the assert.

ycmd/completers/language_server/language_server_completer.py, line 1481 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

It's crucially important that the FixIt chunks are applied in a particular sequence. I felt that giving users of ycmd API a guaranteed stable interface (file-wise) seemed reasonable. I'd like to understand your objection to this before I change it as it is a lot of work, and as @bstaletic said, I spent days on this area getting it solid and well tested :/

I missed the first sentence in the comment. I thought the only reason to sort was to help writing the tests. I am fine with sorting by filename.

ycmd/completers/language_server/language_server_completer.py, line 1261 at r8 (raw file):

     - start_codepoint  = the start column at which the text should be inserted
  )"""
  # We explicitly state that we do not support completion types of "Snippet".

Where do we tell the server that we don't support snippets? I can't find it.

ycmd/completers/language_server/language_server_protocol.py, line 253 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

it appears they have changed the protocol since I wrote this.

i also think it is correct to be explicit as the protocol says "only supported right now" and hasn't been shy about changing it! At least one completer server that I tested required this, and another, which I fixed, barfed on it.

Unfortunately I wasn't able to easily see if VSCode sends this header or not, as there doesn't appear to be a full protocol tracing facility (and stdio can't be snooped aiui).

Anyway I've removed the header because it works, we can add it if it breaks.

I am almost certain that VS Code doesn't send this header because RLS doesn't support it (I reported the issue on RLS tracker) and there is a VS Code extension for it.

Comments from Reviewable

bstaletic · 2017-11-02T17:00:17Z

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

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

      raise ValueError( NO_DOCUMENTATION_MESSAGE )

    return responses.BuildDisplayMessageResponse( get_doc_java.rstrip() )

I know this was my code more or less, but...

There's get_doc_java = get_doc_java.rstrip() four lines above (three not counting an empty line). So can we just do the following (I've taken @micbou's comment into account)?

  return responses.BuildDetailedInfoResponse( get_doc_java )

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, micbou wrote…

Still indented with 4 spaces instead of 2.

To calrify, @micbou is refering to the text inside the tripple quotes.

Comments from Reviewable

micbou · 2017-11-02T21:39:46Z

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

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I know this was my code more or less, but...

There's get_doc_java = get_doc_java.rstrip() four lines above (three not counting an empty line). So can we just do the following (I've taken @micbou's comment into account)?
  return responses.BuildDetailedInfoResponse( get_doc_java )

Right. I noticed that we were removing trailing characters twice but forgot to mention it.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

To calrify, @micbou is refering to the text inside the tripple quotes.

Yes, the docstring.

Comments from Reviewable

puremourning · 2017-11-02T22:07:59Z

Review status: 64 of 69 files reviewed at latest revision, 10 unresolved discussions.

third_party/eclipse.jdt.ls, line 1 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Okay, guess we just have to accept that our "old commits" might break eventually. Which kind of breaks the claim that every commit is a release.

Maybe we should consider something like --system-jdt.ls.

They just today announced a "stable" repo. This is about the 2nd such repo, so i'm keeping an eye on it.

At the least we could just wget the tarball and extract it, rather than build it. But I think this is part of the experimental parts - the server is still pre v1.0.0

ycmd/completers/java/java_completer.py, line 487 at r8 (raw file):

Previously, micbou wrote…

Unneeded parentheses.

Done.

ycmd/completers/java/java_completer.py, line 500 at r8 (raw file):

Previously, micbou wrote…

NO_DOCUMENTATION_MESSAGE?

Done.

ycmd/completers/java/java_completer.py, line 512 at r8 (raw file):

Previously, micbou wrote…

Why a ValueError exception instead of RuntimeError?

Dunno; went with RuntimeError.

We should really not throw exceptions for these things! It's really not an error.

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

Previously, micbou wrote…

BuildDetailedInfoResponse should be used here.

Done.

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I know this was my code more or less, but...

There's get_doc_java = get_doc_java.rstrip() four lines above (three not counting an empty line). So can we just do the following (I've taken @micbou's comment into account)?
  return responses.BuildDetailedInfoResponse( get_doc_java )

Done.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

To calrify, @micbou is refering to the text inside the tripple quotes.

It is indented 2 (i.e. once) from the """" ?

Is it a problem ? It seems pretty readable to me.

ycmd/completers/language_server/language_server_completer.py, line 458 at r7 (raw file):

Just one thing. Can we introduce an intermediate variable message_id to avoid doing str( message[ 'id' ] ) three times?

Yes, we should totally do that. Done!

ycmd/completers/language_server/language_server_completer.py, line 1261 at r8 (raw file):
Well, the comment is a little misleading. This is in the capabilities section of the initialise request.

Actually, we don't specify any client capabilities, which means:

A missing property should be interpreted as an absence of the capability. If a property is missing that defines sub properties all sub properties should be interpreted as an absence of the capability.

So it would be illegal to supply snippets, as we don't set the the capabilities.textDocument.completion.completionItem.snippetSupport to true.

I updated the comment to be more clear.

Comments from Reviewable

puremourning · 2017-11-02T22:09:24Z

Review status: 64 of 69 files reviewed at latest revision, 10 unresolved discussions.

ycmd/completers/language_server/language_server_completer.py, line 156 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

It is indented 2 (i.e. once) from the """" ?

Is it a problem ? It seems pretty readable to me.

Nvm, i changed it to be consistent with the others.

Comments from Reviewable

puremourning · 2017-11-02T22:10:06Z

Review status: 64 of 69 files reviewed at latest revision, 10 unresolved discussions.

ycmd/completers/java/java_completer.py, line 514 at r8 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

Done.

Comments from Reviewable

Close the connection in the handler thread to avoid race condition. Test failed shutdown

Increase the timeout for the shutdown tests for our CI environment Test receive_messages handler without completer support Fix project detection tests Additional tests for things we can't easily test in the CI with real server

Rename lsapi to language_server_protocol Tidy build.py Better names for subcommand handlers Use snake case for variables Clarify the threading setup for the language server connection Despatch -> Dispatch Better name for _RefreshFiles Refactor large completion item methods Update API documentation for long poll, and fix the nonblocking pending messages handler rename textEdit to text_edit for coding style Tracing shutdown to help post mortem diagnostics Update copyright year Move all exceptions to he top of the module Public or API methods are not private. All other methods are private More refactoring of completion item resolution code Compute whether or not to resolve completions statically Remove bare except Mention jdt.ls in the ycmd README Fix up comment in example client Typos and stylistic changes Log when we get errors Don't include the optional header Remove import hack laziness for TemporaryClangTestDir Remove extraneous parentheses More trivial code tidy Use correct GoTo response and errors Don't convert the id to string more than once Clarify client capabilitites Add INFORMATION and HINT to the ycmd API rather than mapping it Use 'Unkonwn type' like clang completer

Join the dispatch thread after closing sockets Don't forcefully kill the server as this causes issues on Windows Ensure that we don't start (and not stop) a JediHTTP instance in shutdown tests Use SharedYcmd for handler tests Properly namespace server management tests

This required a number of things, not least implementing a proper model of the server file state and using that to reduce the number of didClose/didChange events to send. This fixes the interpretation of the file_data: only modified buffers are included, so we must read the contents of non-modified (but open) buffers from disk. Finally, we also move to using binary packages to improve the build times, and tidy up the third party area.

Since we start the server on the FileReadyToParse event, the client might send the initial poll request before we've attempted to start the server. So in that case we wait at least one poll interval for the initialize sequence to complete before starting the poll loop.

Per the language server protocol, we need to count UTF16 code units.

On larger projects and slower systems, resolving many items can be very costly, and not really all that useful.

puremourning · 2018-01-21T10:39:51Z

Review status: all files reviewed at latest revision, 7 unresolved discussions.

.circleci/install_dependencies.sh, line 111 at r48 (raw file):

Previously, micbou wrote…

Looks like we can remove this TODO.

Done.

ycmd/completers/language_server/language_server_completer.py, line 803 at r48 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

What happens if we don't resolve completion items?

It just doesn't include the docs and additional fixits, which isn't a big deal. Completion strings still mainly work.

ycmd/completers/language_server/language_server_completer.py, line 891 at r48 (raw file):

Previously, micbou wrote…

started?

Done.

ycmd/completers/language_server/language_server_completer.py, line 924 at r48 (raw file):

Previously, micbou wrote…

started?

Done.

ycmd/completers/language_server/language_server_completer.py, line 929 at r48 (raw file):

Previously, micbou wrote…

I don't think we need that #.

Done.

ycmd/tests/language_server/language_server_protocol_test.py, line 1 at r48 (raw file):

Previously, micbou wrote…

Tildes are not needed.

Done.

ycmd/tests/language_server/language_server_protocol_test.py, line 193 at r48 (raw file):

Previously, micbou wrote…

Do we need the prints? Seems like they were added for debugging the tests.

Done.

Comments from Reviewable

micbou · 2018-01-21T22:18:57Z

@zzbot r+

Reviewed 3 of 3 files at r49.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

zzbot · 2018-01-21T22:18:58Z

📌 Commit e03836f has been approved by micbou

zzbot · 2018-01-21T22:19:01Z

⌛ Testing commit e03836f with merge 960080d...

@bstaletic

[READY] Java support using jdt.ls OK take a deep breath, this is a monster PR. First and foremost, massive thanks to @bstaletic and @micbou for helping not only with the development and testing, but also for moral support! # Overview This PR implements native support in ycmd for the Java language, based on [jdt.ls][]. In summary, the following key features are supported: * Installation of jdt.ls (built from source with `build.py`) * Management of the jdt.ls server instance, projects etc. * A generic (ish) implementation of a [Language Server Protocol][lsp] client so far as is required for jdt.ls (easily extensible to other engines) * Support for the following Java semantic engine features: * Semantic code completion, including automatic imports * As-you-type diagnostics * GoTo including GoToReferences * FixIt * RefactorRename * GetType * GetDoc See the [trello board][trello] for a more complete picture. # Show me the demo, already. OK sure. ## Startup and completion ![ycm-java-startup-debug](https://user-images.githubusercontent.com/10584846/31523935-54ddcdc2-afae-11e7-9ea5-766a8fde3279.gif) ## Rename ![ycm-java-refactorrename](https://user-images.githubusercontent.com/10584846/31523924-4dd8fce0-afae-11e7-9859-70ea162ced31.gif) ## FixIt ![ycm-java-fixit](https://user-images.githubusercontent.com/10584846/31523868-0c3fd362-afae-11e7-9d5f-f1448aee8fcb.gif) ## GoToReferences ![ycm-java-gotorefs](https://user-images.githubusercontent.com/10584846/31523876-13ae72c0-afae-11e7-9ef0-7804b07c2f04.gif) # Why is this necessary and useful? The current state of the plugin for YCM support for Java is basically [eclim][] or [javacomplete2][], neither of which work perfectly with ycmd, and neither of which are quite as well integrated as we might like. ## eclim Eclim is actually pretty good and I have used it for a long time. However, the experience isn't optimal for YCM: ### Pros * Pretty good project creation, management functions * Lots of languages (supports more than just Java) * Lots of features (supports nearly all the standard YCMD commands) * New class creation, and lots of other stuff like that ### Cons * It's a pain to set up (I know of only 1 other person at my company who has successfully got it working), requiring a Vimball, installation into an existing eclipse installation, manually starting the server, and other fiddly stuff. * Diagnostics are delivered via Syntastic, so block the UI on save, and at a number of other times. * Requires eclipse installation and understanding ## javacomplete2 I have not extensively used javacomplete2, but I know there have been issues reported that its omnifunc doesn't always play nice with ycmd. ### Pros? * Very simple to get going * Seems to give completion suggestions reasonably well ### Cons? * Lacks features * No diagnostics ? * Lots of vimscript (java parser in vimscript?) ## Notable mentions * [vim-lsp][] seems to be a plugin for generic [LSP][lsp] support. I haven't tried it but my experience with jdt.ls is that there is no way to make a fully working LSP client without server-specific knowledge. I don't know if this works with jdt.ls # Why jdt.ls? I tried 2 different Java LSP engines: * [vscode-javac][]: javac (the Java compiler) is the backend. Actually, this is a nice and simple server and has decent features. * [jdt.ls][]: Eclipse JDT as a server daemon. This appears to be the most popular Java plugin for [VSCode][], and has the full power of RedHat and Microsoft investing in bringing JDT (the eclipse Java engine) to VSCode and the LSP-using community. Initially, for its simplicity, I was [drawn to vscode-javac](puremourning@9180e96) for its simplicity and ease of integration with ycmd. However, I also implemented [jdt.ls][] and found this, while more work, to be a superior experience. It also has a very active development community, which bodes well for the future. # Why such a big PR, man. Come on! I'm sorry. I really really am. I honestly tried to rebase and rewrite the ~150 commits over the last 1 year or so, but as with many such things, a lot of the functionality that would make "Phase 1" ended up relying on functionality and test framework that came in "Phase N". So I'm dumping this on you in one unwieldy heap and hoping that you can find it in your hearts to forgive me. I honestly spent hours trying :( # OK, but come on, give us a hand... Sure thing! I've tried to give below a "review guide" to help you get your heads around what's going on an why some of the design decisions where made. ## Overall design/goals Key goals: 1. Support Java in ycmd and YCM; make it good enough to replace eclim and javacomplete2 for most people 2. Make it possible/easy to support other [lsp][] servers in future (but, don't suffer from yagni); prove that this works. An overview of the objects involved can be seen on [this card][design]. In short: * 2 classes implement the language server protocol in the `language_server_completer.py` module: * `LanguageServerConnection` - an abstraction of the comminication with the server, which may be over stdio or any number of TCP/IP ports (or a domain socket, etc.). Only a single implementation is included (stdio), but [implementations for TCP/IP](puremourning@f3cd062) were written originally and dropped in favour of stdio's simplicity. * `LanguageServerCompleter` - an abstract base for any completer based on LSP, which implements as much standard functionality as possible including completions, diagnostics, goto, fixit, rename, etc. * The `java_completer` itself implements the `LanguageServerCompleter`, boots the jdt.ls server, and instantiates a `LanguageServerConnection` for communication with jdt.ls. The overall plan and some general discussion around the project can be found on the [trello board][trello] I used for development. The proof? I implemented a fully working [clangd][] [completer for ycmd][clangd-completer] in about 30 minutes. The only wrinkles were clangd bugs! ## Threads, why oh why so many threads. OK chill. It's not thhaartd. LSP is by its nature an asyncronous protocol. There are request-reply like `requests` and unsolicited `notifications`. Receipt of the latter is mandatory, so we cannot rely on there being a `bottle` thread executing a client request. So we need a message pump and despatch thread. This is actually the `LanguageServerConnection`, which implements `thread`. It's main method simply listens on the socket/stream and despatches complete messages to the `LanguageServerCompleter`. It does this: * For `requests`: similarly to the TypeScript completer, using python `event` objects, wrapped in our `Response` class * For `notifications`: via a synchronised `queue`. More on this later. A very poor representation of this is on the "Requests and notifications" page of the [design][], including a rough sketch of the thread interaction. ### Some handling is done in the message pump. That's right. There are certain notifications which we have to handle when we get them, such as: * Initialisation messages * Diagnostics In these cases, we allow some code to be executed inline within the message pump thread, as there is no other thread guaranteed to execute. These are handled by callback functions and mutexes. ## Startup sequence See the 'initialisation sequence' tab on the [design][] for a bit of background. In standard LSP, the initialisation sequence consists of an initialise request-reply, followed by us sending the server an initialised notification. We must not send any other requests until this has completed. An additional wrinkle is that jdt.ls, being based on eclipse has a whole other initialisation sequence during which time it is not fully functional, so we have to determine when that has completed too. This is done by jdt.ls-specific messages and controls the `ServerIsReady` response. In order for none of these shenanigans to block the user, we must do them all asynchronously, effectively in the message pump thread. In addition, we must queue up any file contents changes during this period to ensure the server is up to date when we start processing requests proper. This is unfortunately complicated, but there were early issues with really bad UI blocking that we just had to get rid of. ## Completion foibles Language server protocol requires that the client can apply textEdits, rather than just simple text. This is not an optional feature, but ycmd clients do not have this ability. The protocol, however, restricts that the edit must include the original requested completion position, so we can perform some simple text manipulation to apply the edit to the current line and determine the completion start column based on that. In particular, the jdt.ls server returns textEdits that replace the entered text for import completions, which is one of the most useful completions. We do this super inefficiently by attempting to normalise the TextEdits into insertion_texts with the same start_codepoint. This is necessary particularly due to the way that eclipse returns import completions for packages. We also include support for "additionalTextEdits" which allow automatic insertion of, e.g., import statements when selecting completion items. These are sent on the completion response as an additional completer data item called 'fixits'. The client applies the same logic as a standard FixIt once the selected completion item is inserted. ## Diagnostics foibles Diagnostics in LSP are delivered asynchronously via `notifications`. Normally, we would use the `OnFileReadyToParse` response to supply diagnostics, but due to the lag between refreshing files and receiving diagnostics, this leads to a horrible user experience where the diagnostics always lag one edit behind. To resolve this, we use the long-polling mechanism added here (`ReceiveMessages` request) to return diagnostics to the client asynchronously. We deliver asynchronous diagnostics to the client in the same way that the language server does, i.e. per-file. The client then fans them out or does whatever makes sense for the client. This is necessary because it isn't possible to know when we have received all diagnostics, and combining them into a single message was becoming clunky and error prone. In order to be relatively compatible with other clients, we also return diagnostics on the file-ready-to-parse event, even though they might be out of date wrt the code. The client is responsible for ignoring these diagnostics when it handles the asynchronously delivered ones. This requires that we hold the "latest" diagnostics for a file. As it turns out, this is also required for FixIts. ## Projects jdt.ls is based on eclipse. It is in fact an eclipse plugin. So it requires an eclipse workspace. We try and hide this by creating an ad-hoc workspace for each ycmd instance. This prevents the possibility of multiple "eclipse" instances using the same workspace, but can lead to unreasonable startup times for large projects. The jdt.ls team strongly suggest that we should re-use a workspace based on the hash of the "project directory" (essentially the dir containing the project file: `.project`, `pom.xml` or `build.gradle`). They also say, however, that eclipse frequently corrupts its workspace. So we have a hidden switch to re-use a workspace as the jdt.ls devs suggest. In testing at work, this was _mandatory_ due to a slow SAN, but at home, startup time is not an issue, even for large projects. I think we'll just have to see how things go to decide which one we want to keep. ## Subcommand foibles ### GetDoc/GetType There is no GetType in LSP. There's only "hover". The hover response is hilariously server-specific, so in the base `LanguageServerCompleter` we just provide the ability to get the `hover` response and `JavaCompleter` extracts the appropriate info from there. Thanks to @bstaletic for this! ### FixIt FixIts are implemented as code actions, and require the diagnostic they relate to to be send from us to the server, rather than just a position. We use the stored diags and find the nearest one based on the `request_data`. What's worse is that the LSP provides _no documentation_ for what the "Code action" response should be, and it is 100% implementation-specific. They just have this `command` abstraction which is basically "do a thing". From what I've seen, most servers just end up with either a `WorkspaceEdit` or a series of `TextEdits`, which is fine for us as that's what ycmd's protocol looks like. The solution is that we have a callback into the `JavaCompleter` to handle the (custom) `java.apply.workspaceEdit` "command". ### GoToReferences Annoyingly, jdt.ls sometimes returns references to .class files within jar archives using a custom `jdt://` protocol. We can't handle that, so we have to dodge and weave so that we don't crash. ### Stopping the server Much like the initialisation sequence, the LSP shutdown sequence is a bit fiddly. 2 things are required: 1. A `shutdown` request-reply. The server tides up and _prepares to die!_ 2. An `exit` notification. We tell the server to die. This isn't so bad, but jdt.ls is buggy and actually dies without responding to the `shutdown` request. So we have a bunch of code to handle that and to ensure that the server dies eventually, as it had a habbit of getting stuck running, particularly if we threw an exception. # Closing remarks Phew. That was a lot of caveats! I have personally used this quite a lot now, and it has proven to be really useful. In particular the full index and GoToReferences, etc. I think the tests are comprehensive, but there is probably more work to do on coverage. Some of it will be tricky (particularly exceptions around `jdt:` and other edge cases in jdt.ls that I've come across). ## Ship it as experimental ? Due to the rapid pace of development of jdt.ls and the scale of this change, one thought I had was to mark Java support in YCM as _exprrimental_ and gather feedback via an open github issue. I'm certain there will be issues, and preferences, etc. so this might be a good way to tackle that. # Where's the client PR ? I haven't finished polishing the client yet, but you can get use it by checking out [my `language-server-java` branch of YCM][client]. [jdt.ls]: https://github.com/eclipse/eclipse.jdt.ls [lsp]: https://github.com/Microsoft/language-server-protocol/ [eclim]: http://eclim.org [javacomplete2]: https://github.com/artur-shaik/vim-javacomplete2 [vscode-javac]: https://github.com/georgewfraser/vscode-javac [VSCode]: https://code.visualstudio.com [design]: https://trello.com/c/78IkFBzp [trello]: https://trello.com/b/Y6z8xag8/ycm-java-language-server [client]: https://github.com/puremourning/YouCompleteMe/tree/language-server-java [clangd]: https://clang.llvm.org/extra/clangd.html [clangd-completer]: puremourning@787cc88  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/857)

zzbot · 2018-01-21T23:26:09Z

💔 Test failed - status-appveyor

zzbot · 2018-01-22T01:41:37Z

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 960080d to master...

[READY] Update API documentation The website containing the API docs wasn't updated when merging PR #857 (the `/receive_messages` handler is missing). This PR fixes some typos, renames `openapi.yaml` to `openapi.yml`, and updates the website.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/911)

[READY] Update ycmd Include the following changes: - PR ycm-core/ycmd#789: add support for Windows flags when --driver-mode=cl is given; - PR ycm-core/ycmd#848: hide C++ symbols by default; - PR ycm-core/ycmd#857: add Java support using jdt.ls; - PR ycm-core/ycmd#861: translate libclang error codes to exceptions; - PR ycm-core/ycmd#880: support downloading Clang binaries on ARM systems; - PR ycm-core/ycmd#883: handle zero column diagnostic from OmniSharp; - PR ycm-core/ycmd#884: specify Platform property when compiling OmniSharp; - PR ycm-core/ycmd#886: use current working directory in JavaScript completer; - PR ycm-core/ycmd#887: update Boost to 1.66.0; - PR ycm-core/ycmd#888: update JediHTTP; - PR ycm-core/ycmd#889: update Clang to 5.0.1; - PR ycm-core/ycmd#891: fix building with system libclang on Gentoo amd64; - PR ycm-core/ycmd#904: drop Python 2.6 and Python 3.3 support; - PR ycm-core/ycmd#905: calculate the start column when items are not resolved in the language server completer; - PR ycm-core/ycmd#912: download Clang binaries from HTTPS; - PR ycm-core/ycmd#914: do not try to symlink libclang on Windows.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2902)

[READY] Java support with asynchronous diagnostics and messages # PR Prelude Thank you for working on YCM! :) **Please complete these steps and check these boxes (by putting an `x` inside the brackets) _before_ filing your PR:** - [x] I have read and understood YCM's [CONTRIBUTING][cont] document. - [x] I have read and understood YCM's [CODE_OF_CONDUCT][code] document. - [x] I have included tests for the changes in my PR. If not, I have included a rationale for why I haven't. - [x] **I understand my PR may be closed if it becomes obvious I didn't actually perform all of these steps.** # Why this change is necessary and useful This change is required for a better user experience when using native java support This implements an asynchronous message system using a long-poll request to the server. The server provides an endpoint /receive_messages which blocks until either a timeout occurs or we receive a batch of asynchronous messages. We send this request asynchronously and poll it 4 times a second to see if we have received any messages. The messages may either be simply for display (such as startup progress) or diagnostics, which override the diagnostics returned by OnFileReqdyToParse. In the former case, we simply display the message, accepting that this might be overwritten by any other message (indeed, requiring this), and for the latter we fan out diagnostics to any open buffer for the file in question. Unfortunately, Vim has bugs related to timers when there is something displayed (such as a "confirm" prompt or other), so we suspend background timers when doing subcommands to avoid vim bugs. NOTE: This requires a new version of Vim (detected by the presence of the particular functions used). NOT_READY because: - the submodule commit points at my repo and requires ycm-core/ycmd#857 to be merged - my spider sense suggest i have more testing to do... Notes: - Part 3 (I think) of the Java support PRs. This one actually adds the minimal changes for working java support - There are about 2 or 3 other PRs to come to add things like automatic module imports, etc. [Please explain **in detail** why the changes in this PR are needed.] [cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md [code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2863)

micbou mentioned this pull request Oct 14, 2017

[Possible Feature]: Add Ruby completion #314

Closed

puremourning force-pushed the java-language-server branch from 116144a to 507f00d Compare October 19, 2017 22:20

puremourning force-pushed the java-language-server branch from 326bfcc to f5f2863 Compare October 22, 2017 22:43

puremourning force-pushed the java-language-server branch from ed2fdcb to f1d8a59 Compare October 26, 2017 21:27

puremourning force-pushed the java-language-server branch from 2b266de to 2471e31 Compare November 1, 2017 00:50

puremourning and others added 10 commits January 21, 2018 10:39

Improve the API and stability for shutdown

072cfd1

Close the connection in the handler thread to avoid race condition. Test failed shutdown

Improve test coverage

8c2f192

Increase the timeout for the shutdown tests for our CI environment Test receive_messages handler without completer support Fix project detection tests Additional tests for things we can't easily test in the CI with real server

Start jdt.ls on FileReadyToParse event

1f8adc4

Update API docs for mesage poll request

c42d5e8

Use UTF16 code unit offsets not codepoint offsets

feba227

Per the language server protocol, we need to count UTF16 code units.

Only resolve up to 100 items

e03836f

On larger projects and slower systems, resolving many items can be very costly, and not really all that useful.

puremourning force-pushed the java-language-server branch from 83ea5d7 to e03836f Compare January 21, 2018 10:39

zzbot merged commit e03836f into ycm-core:master Jan 22, 2018

zzbot mentioned this pull request Jan 22, 2018

[READY] Omnisharp roslyn #885

Closed

micbou mentioned this pull request Jan 24, 2018

[READY] Update API documentation #911

Merged

micbou mentioned this pull request Feb 9, 2018

[READY] Update ycmd ycm-core/YouCompleteMe#2902

Merged

micbou mentioned this pull request Feb 12, 2018

Java backend #45

Closed

puremourning mentioned this pull request Feb 15, 2018

Shutdown ycmd if the editor is gone? #927

Closed

bstaletic mentioned this pull request Mar 26, 2019

Some input Avi-D-coder/lsp-range-unit-survey#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Java support using jdt.ls #857

[READY] Java support using jdt.ls #857

puremourning commented Oct 12, 2017 •

edited

Loading

bstaletic commented Oct 13, 2017

puremourning commented Oct 13, 2017

puremourning commented Oct 13, 2017

micbou commented Oct 14, 2017

puremourning commented Oct 14, 2017

codecov-io commented Oct 14, 2017 •

edited by codecov bot

Loading

Valloric commented Oct 17, 2017

puremourning commented Oct 19, 2017

Valloric commented Oct 22, 2017

puremourning commented Oct 22, 2017

puremourning commented Oct 22, 2017

Valloric commented Oct 26, 2017

puremourning commented Oct 26, 2017

bstaletic commented Oct 27, 2017

Valloric commented Oct 30, 2017

micbou commented Oct 31, 2017

bstaletic commented Oct 31, 2017

puremourning commented Nov 1, 2017

bstaletic commented Nov 1, 2017

micbou commented Nov 2, 2017

bstaletic commented Nov 2, 2017

micbou commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Jan 21, 2018

micbou commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 22, 2018

[READY] Java support using jdt.ls #857

[READY] Java support using jdt.ls #857

Conversation

puremourning commented Oct 12, 2017 • edited Loading

Overview

Show me the demo, already.

Startup and completion

Rename

FixIt

GoToReferences

Why is this necessary and useful?

eclim

Pros

Cons

javacomplete2

Pros?

Cons?

Notable mentions

Why jdt.ls?

Why such a big PR, man. Come on!

OK, but come on, give us a hand...

Overall design/goals

Threads, why oh why so many threads.

Some handling is done in the message pump.

Startup sequence

Completion foibles

Diagnostics foibles

Projects

Subcommand foibles

GetDoc/GetType

FixIt

GoToReferences

Stopping the server

Closing remarks

Ship it as experimental ?

Where's the client PR ?

bstaletic commented Oct 13, 2017

puremourning commented Oct 13, 2017

puremourning commented Oct 13, 2017

micbou commented Oct 14, 2017

puremourning commented Oct 14, 2017

codecov-io commented Oct 14, 2017 • edited by codecov bot Loading

Codecov Report

Valloric commented Oct 17, 2017

puremourning commented Oct 19, 2017

Valloric commented Oct 22, 2017

puremourning commented Oct 22, 2017

puremourning commented Oct 22, 2017

Valloric commented Oct 26, 2017

puremourning commented Oct 26, 2017

bstaletic commented Oct 27, 2017

Valloric commented Oct 30, 2017

micbou commented Oct 31, 2017

bstaletic commented Oct 31, 2017

puremourning commented Nov 1, 2017

bstaletic commented Nov 1, 2017

micbou commented Nov 2, 2017

bstaletic commented Nov 2, 2017

micbou commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Nov 2, 2017

puremourning commented Jan 21, 2018

micbou commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 21, 2018

zzbot commented Jan 22, 2018

puremourning commented Oct 12, 2017 •

edited

Loading

codecov-io commented Oct 14, 2017 •

edited by codecov bot

Loading