[WIP][RFC] Native Java support

[puremourning]

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:

• I have read and understood YCM's CONTRIBUTING document.
• I have read and understood YCM's CODE_OF_CONDUCT document.
• I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
• I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.

[Edit: I haven't really supplied tests for this, so checking the box is a lie!]

Why this change is necessary and useful

This is the client side implementation of the features required for Java support based on ycm-core/ycmd#857. The rationale for most of the decisions is over there.

This PR is really just to open up discussion and facilitate testing/feedback on ycm-core/ycmd#857.

It is incomplete and not fully tested yet, but I thought I would get it out there to add some context to ycm-core/ycmd#857 and also to gather thoughts on some of the changes (which are nontrivial).The PR is actually multiple commits, and if anyone is going to look at the code, i'd recommend looking at it commit-wise (as they will form the individual PRs proper).

The main changes are:

• Suport for the asynchronous message poll and asynchronous diagnostics. These use a 250ms timer that runs (roughly) constantly when a filetype that supports it is open. This has some nasties, such as - for no good reason - Vim redrawing the screen whenever a timer fires. I added Suspend/Resume for the timers for certain YCM requests, but this is only supported in a newer version of Vim :(
• Rewriting the way we disable syntastic. This is now conditional on the native filetype available request. Note: again this is incomplete as this makes a blocking call on the first OnFileReadyToParse
• support for post complete actions to apply FixIts for Java

---

This change is 

[puremourning]

This looks like a merge error

[puremourning]

Comment is old and needs removing

[puremourning]

Comment is legacy and needs to be removed.

[bstaletic]

It's 03:00 am and I'm too tired to look at code right now, but perhaps this is a goot time to test Reviewable's feater of "per commit reviews" @Valloric once mentioned.

---

Review status: 0 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #2827 into master will decrease coverage by 2%.
The diff coverage is 58.26%.

@@            Coverage Diff             @@
##           master    #2827      +/-   ##
==========================================
- Coverage   92.36%   90.36%   -2.01%     
==========================================
  Files          20       21       +1     
  Lines        1965     2044      +79     
==========================================
+ Hits         1815     1847      +32     
- Misses        150      197      +47

[puremourning]

Good point. I have set this:

Don't know if it affects everyone or just me.

---

Review status: 0 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[puremourning]

TBH, I don't think it worked. I still see the commits smooshed together.

---

Review status: 0 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[zzbot]

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

[bstaletic]

I have set this:

It's apparently set for everyone, but the feature does seem broken and as if it doesn't work.

---

Reviewed 8 of 11 files at r1.
Review status: 8 of 11 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

autoload/youcompleteme.vim, line 50 at r1 (raw file):

      \   'receive_messages': {
      \     'id': -1,
      \     'wait_milliseconds': 250

250 milliseconds is noticeable. Should we go for something lower?

---

autoload/youcompleteme.vim, line 839 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This looks like a merge error

That does look out of place.

---

python/ycm/vimsupport.py, line 160 at r1 (raw file):

def GetFilepathForBufferNumber( num ):
  return GetBufferFilepath( vim.buffers[ num ] )

Is this used anywhere?

---

python/ycm/vimsupport.py, line 704 at r1 (raw file):

def ReplaceChunks( chunks, silent=False ):

Is this ever called with silent = True?

---

python/ycm/tests/vimsupport_test.py, line 37 at r1 (raw file):

from hamcrest import assert_that, calling, equal_to, has_entry, raises
from mock import MagicMock, call, patch
from ycmd.utils import ToBytes, ToUnicode

Why is ToUnicode() needed now if it wasn't needed before?

---

Comments from Reviewable

[bstaletic]

Reviewed 3 of 11 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.

---

python/ycm/vimsupport.py, line 704 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is this ever called with silent = True?

My bad, it is in _OnCompleteDone_Java().

---

python/ycm/youcompleteme.py, line 106 at r1 (raw file):

from ycm.buffer import ( DIAGNOSTIC_UI_FILETYPES,
                         DIAGNOSTIC_UI_ASYNC_FILETYPES )

Why aren't there imports higher up with other imports?

---

python/ycm/youcompleteme.py, line 387 at r1 (raw file):

      # - Use the QuickFix list to report project errors?
      # - Use a special buffer for project errors?
      # - Put them in the location list of whatever the "current" buffer is

We could also just store the diagnostics for all files a LSP server reports, but that would potentially make memory usage explode.

---

python/ycm/youcompleteme.py, line 611 at r1 (raw file):

      # offer some sort of choice here, like in command_request.py.
      for fixit in fixit_completion:
        vimsupport.ReplaceChunks( fixit[ 'chunks' ], silent=True )

Are we sure that letting the user have a single choice here is a good idea?
Right now, I'm leaning towards this staying as is.

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

              notification[ 'filepath' ],
              notification[ 'diagnostics' ] )
      elif not response:

Can response be truthy but not an instance of list?

---

Comments from Reviewable

[puremourning]

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

---

autoload/youcompleteme.vim, line 50 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

250 milliseconds is noticeable. Should we go for something lower?

TBH it isn't noticeable in actual usage. I mean it doesn't feel laggy, and it also doesn't spin around constantly polling ... I feel 4 times per second is a reasonable balance of CPU wastage (wakeup, battery life, etc.) vs. being really snappy.

---

autoload/youcompleteme.vim, line 839 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

That does look out of place.

this is actually fixing a bug. this is not reset when the server is restarted, which means the file is not reparsed. should be a separate commit at least and should have a test.

---

python/ycm/vimsupport.py, line 160 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is this used anywhere?

No, no it is not.

---

python/ycm/youcompleteme.py, line 106 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why aren't there imports higher up with other imports?

copypasta

---

python/ycm/youcompleteme.py, line 387 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

We could also just store the diagnostics for all files a LSP server reports, but that would potentially make memory usage explode.

yeah, that's also an option (have the client remember, then do the fan out later, if say a new buffer is opened). But still it's all for the future to solve i think.

---

python/ycm/youcompleteme.py, line 611 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Are we sure that letting the user have a single choice here is a good idea?
Right now, I'm leaning towards this staying as is.

I haven't come across a situation where it causes problems, and there seems to be legitimacy in applying any additionalTextEdit without choice (there is no such choice mechanism in VSCode), so I think i agree

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Can response be truthy but not an instance of list?

Not legally

---

python/ycm/tests/vimsupport_test.py, line 37 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is ToUnicode() needed now if it wasn't needed before?

There was a very subtle bug before in that the code requires unicode input, but here we were passing arbitrary input.

---

Comments from Reviewable

[bstaletic]

Reviewed 2 of 10 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 4 files at r6, 2 of 3 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

autoload/youcompleteme.vim, line 50 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

TBH it isn't noticeable in actual usage. I mean it doesn't feel laggy, and it also doesn't spin around constantly polling ... I feel 4 times per second is a reasonable balance of CPU wastage (wakeup, battery life, etc.) vs. being really snappy.

The following steps show the lag, but are not a realistic scenario.

• Input a character that would cause an error.
• Exit insert mode and wait for the diagnostics to refresh.
• Delete the character.
• Wait for diagnostics to refresh.

While this isn't a realistic scenario, a user would have a similar experience when there are multiple diagnostics on the same line and the user fixes those one at a time.

Anyway, I, in no way, consider this to be a blocker and would like to hear more opinions.

---

autoload/youcompleteme.vim, line 803 at r8 (raw file):

  if s:completion.start_column > col( '.' ) || empty( s:completion.candidates )
    call s:CloseCompletionMenu()
  else

I'm assuming this is a result of git rebase -i?

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Not legally

In that case, the code is fine, but maybe it should have a comment stating that truthy non-list objects are invalid.
If we try to integrate a LSP server that violates this, it will be nice to know who's to blame (ie not us).

---

Comments from Reviewable

[puremourning]

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

---

autoload/youcompleteme.vim, line 803 at r8 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm assuming this is a result of git rebase -i?

erm, what is?

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

In that case, the code is fine, but maybe it should have a comment stating that truthy non-list objects are invalid.
If we try to integrate a LSP server that violates this, it will be nice to know who's to blame (ie not us).

It would still be us - this is the ycmd client->server interface. Any truthy value other than a list means "poll again".

---

Comments from Reviewable

[bstaletic]

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

---

autoload/youcompleteme.vim, line 803 at r8 (raw file):

Previously, puremourning (Ben Jackson) wrote…

erm, what is?

Like I said in the gitter room, nevermind.

---

python/ycm/client/messages_request.py, line 69 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Comment is legacy and needs to be removed.

I believe you forgot to remove the comment.

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

It would still be us - this is the ycmd client->server interface. Any truthy value other than a list means "poll again".

Then I guess this is fine.

---

Comments from Reviewable

[puremourning]

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

---

autoload/youcompleteme.vim, line 803 at r8 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Like I said in the gitter room, nevermind.

:D

---

python/ycm/client/messages_request.py, line 69 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I believe you forgot to remove the comment.

I did!

---

python/ycm/client/messages_request.py, line 85 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Then I guess this is fine.

I added a comment anyway

---

Comments from Reviewable

[bstaletic]

Reviewed 1 of 1 files at r9, 5 of 5 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

autoload/youcompleteme.vim, line 56 at r10 (raw file):

" timers.
function! s:CanPauseTimers()
  return exists( '*timer_info' ) && exists( '*timer_pause' )

Is has('timers') enough?

---

python/ycm/youcompleteme.py, line 421 at r10 (raw file):

      vimsupport.DisableSyntasticForFiletype( filetype )

    vimsupport.DisableSyntasticInCurrentBuffer()

Why is this needed?

---

Comments from Reviewable

[puremourning]

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

---

autoload/youcompleteme.vim, line 56 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is has('timers') enough?

No, because timer_info and timer_pause were added after timers. The minimum Vim version requirement we have supports timers, but not these functions.

---

python/ycm/youcompleteme.py, line 421 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is this needed?

There's a race condition with syntactic. We've subtly changed where we hack the syntactic config, so that it happens asynchronously on startup. This happened after syntactic has started up and read the user's startup config.

If we just change its config after it has loaded, nothing happens, so we have to tell it that we've changed the config by calling :SyntasticReset or whatever the command is.

I think I now understand why Val originally hard-coded YCM to disable Syntastic irrespective of the server:

1. There is a blocking server call to the server to get the supported filetypes

2. The YCM viml code runs around the same time as the syntactic code on startup, so we actually get to change its config before it runs off and blocks Vim running javac and opens the location list.

This are all irksome, but I don't feel like the current solution is all that appropriate for Java. Many people will have working eclim installations, or simply working syntactic support which we would blindly turn off, even if ycmd doesn't have java support built in.

As mentioned above, this doesn't "just work" as we would like:

• syntactic wakes up, blocks Vim to call javac, then populated the location list
• YCM kicks in and says, hey no need, we have something better, and shuts off syntactic and hides the location list

I don't yet have good a good story here and i'm still playing with this commit. TBH i think in the YCM docs for Java I will write that you should disable syntactic manually when you add --java-completer for the best experience.

---

Comments from Reviewable

[zzbot]

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

[puremourning]

Thanks so much for the early-review comments on this. Closing now in favour of more focussed PRs.

---

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

---

Comments from Reviewable