Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] Calculate the start codepoint when we don't resolve items in LSC #905

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

puremourning
Copy link
Member

@puremourning puremourning commented Jan 22, 2018

Resolving lots of completion items is very slow, so we only do that if
there are 100 or feweer completions.

Most unfortunately Visual Studio Code does some heuristics on completion
items, so that any overlap with existing code is replaced, rather than
appended. This happens to be how jdt.ls returns import completions.

It's common for there to be more than 100 suggestions when typing,
say 'import com.'. This leads to bad behaviour where the insertion ends
up being 'com.com.youcompletme...' which is clearly wrong.

To combad this, when we don't have a textEdit to apply, we detect any
overlap between the insertion_text and the existing text (up to the
completion location) and offset the start_codepoint by that much.

The overlap detection algorithm is efficient (see the link) and simpler
than a dynamic programming one.

Incidentally, we add the 'prefix' key to request_wrap to avoid
recalculating it for each element.


This change is Reviewable

@puremourning puremourning force-pushed the lsp-calc-start-codepoint branch from e118fa0 to 46a7b9e Compare January 22, 2018 22:07
@codecov-io
Copy link

codecov-io commented Jan 22, 2018

Codecov Report

Merging #905 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
+ Coverage   96.18%   96.19%   +0.01%     
==========================================
  Files          83       83              
  Lines        6468     6496      +28     
==========================================
+ Hits         6221     6249      +28     
  Misses        247      247

@micbou
Copy link
Collaborator

micbou commented Jan 22, 2018

Reviewed 5 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


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

    #
    # ycmd protocol only supports a single start column, so we must post-process
    # the completion items to work out a single start colum to use, as follows:

column.


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

      raise IncompatibleCompletionException( insertion_text )
  else:
    # Calculate the start codepoint based on the overlappoing text in the

overlapping


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

  #      return 4 (from previous iteration)

  # More complex examle: 'Some CoCo' vs 'CoCo Bean'

example.


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

  # More complex examle: 'Some CoCo' vs 'CoCo Bean'
  #   No trunation

truncation.


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

  #   Iter 1 (overlap = 1): p('b') = 3, overlap = 4, aaab!=caab, return 0

One blank line is enough.


ycmd/tests/request_wrap_test.py, line 63 at r1 (raw file):

         RequestWrap( PrepareJson( line_num = 1,
                                   contents = line,
                                   column_num = col ) ) [ 'prefix' ] )

Space before [ can be removed.


Comments from Reviewable

Resolving lots of completion items is very slow, so we only do that if
there are 100 or feweer completions.

Most unfortunately Visual Studio Code does some heuristics on completion
items, so that any overlap with existing code is replaced, rather than
appended. This happens to be how jdt.ls returns import completions.

It's common for there to be more than 100 suggestions when typing,
say 'import com.'. This leads to bad behaviour where the insertion ends
up being 'com.com.youcompletme...' which is clearly wrong.

To combad this, when we don't have a textEdit to apply, we detect any
overlap between the insertion_text and the existing text (up to the
completion location) and offset the start_codepoint by that much.

The overlap detection algorithm is efficient (see the link) and simpler
than a dynamic programming one.

Incidentally, we add the 'prefix' key to request_wrap to avoid
recalculating it for each element.
@puremourning puremourning force-pushed the lsp-calc-start-codepoint branch from 46a7b9e to 3209927 Compare January 22, 2018 22:11
@puremourning
Copy link
Member Author

Review status: 3 of 5 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.


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

Previously, micbou wrote…

column.

Done.


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

Previously, micbou wrote…

overlapping

Done.


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

Previously, micbou wrote…

example.

Done.


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

Previously, micbou wrote…

truncation.

Done.


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

Previously, micbou wrote…

One blank line is enough.

Done.


ycmd/tests/request_wrap_test.py, line 63 at r1 (raw file):

Previously, micbou wrote…

Space before [ can be removed.

Done.


Comments from Reviewable

@micbou
Copy link
Collaborator

micbou commented Jan 22, 2018

This works great. :lgtm_strong:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


ycmd/request_wrap.py, line 193 at r3 (raw file):

  def _Prefix( self ):
    return self[ 'line_value' ][ : ( self[ 'start_codepoint' ] - 1 ) ]

Parentheses are not needed (see the _Query function just above).


Comments from Reviewable

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


ycmd/request_wrap.py, line 193 at r3 (raw file):

Previously, micbou wrote…

Parentheses are not needed (see the _Query function just above).

I kinda like them for the clarity. The -1 is quite subtle IMO.


Comments from Reviewable

@micbou
Copy link
Collaborator

micbou commented Jan 23, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion.


ycmd/request_wrap.py, line 193 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I kinda like them for the clarity. The -1 is quite subtle IMO.

If you think this improves readability then we should keep them.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

:lgtm:

@zzbot r=micbou


Reviewed 2 of 5 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Jan 23, 2018

📌 Commit 3209927 has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Jan 24, 2018

⌛ Testing commit 3209927 with merge 5b1615a...

zzbot added a commit that referenced this pull request Jan 24, 2018
[READY] Calculate the start codepoint when we don't resolve items in LSC

Resolving lots of completion items is very slow, so we only do that if
there are 100 or feweer completions.

Most unfortunately Visual Studio Code does some heuristics on completion
items, so that any overlap with existing code is replaced, rather than
appended. This happens to be how jdt.ls returns import completions.

It's common for there to be more than 100 suggestions when typing,
say 'import com.'. This leads to bad behaviour where the insertion ends
up being 'com.com.youcompletme...' which is clearly wrong.

To combad this, when we don't have a textEdit to apply, we detect any
overlap between the insertion_text and the existing text (up to the
completion location) and offset the start_codepoint by that much.

The overlap detection algorithm is efficient (see the link) and simpler
than a dynamic programming one.

Incidentally, we add the 'prefix' key to request_wrap to avoid
recalculating it for each element.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/905)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Jan 24, 2018

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

@zzbot zzbot merged commit 3209927 into ycm-core:master Jan 24, 2018
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Feb 10, 2018
[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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2902)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants