[READY] Allow completers to set the start column

[puremourning]

We always calculate the start column based on our own identifier semantics.

However, some completion engines might have a different interpretation, and indeed the javascript completer does exactly that for require( '

Fixes: #671

This PR is just to demonstrate the approach for comments. It isn't really tested yet (the tests will certainly fail).

---

This change is 

[codecov-io]

Codecov Report

Merging #681 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   94.84%   94.78%   -0.06%     
==========================================
  Files          79       79              
  Lines        5296     5316      +20     
  Branches      172      172              
==========================================
+ Hits         5023     5039      +16     
- Misses        228      232       +4     
  Partials       45       45

[Valloric]

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

---

ycmd/request_wrap.py, line 112 at r1 (raw file):

  def SetCompletionStartColumn( self, column_num ):

Seems like you want to implement __setitem__ so that request_data[ 'start_codepoint' ] = 123 ends up calling SetCompletionStartColumn, which would be private.

With the current approach we implement a nice API for reading computed fields but not for setting them, which is weird.

This would need a bit of re-architecting since _computed_key only allows storing a getter function, but not a setter.

---

ycmd/completers/javascript/tern_completer.py, line 206 at r1 (raw file):

    completions = response.get( 'completions', [] )
    start_codepoint = response[ 'start' ][ 'ch' ] + 1

The +1 needs a comment.

---

ycmd/completers/javascript/tern_completer.py, line 208 at r1 (raw file):

    start_codepoint = response[ 'start' ][ 'ch' ] + 1

    request_data.SetCompletionStartCodepoint( start_codepoint )

This would need an extensive comment as I don't really understand why you're doing this. :D

---

Comments from Reviewable

[puremourning]

Thanks for the review!

I've updated it according to Val's suggestion and added some tests.

I should also note, this is only fixed in the Vim client with ycm-core/YouCompleteMe#2489 which adds some more valuable context.

---

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

---

ycmd/request_wrap.py, line 112 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Seems like you want to implement __setitem__ so that request_data[ 'start_codepoint' ] = 123 ends up calling SetCompletionStartColumn, which would be private.

With the current approach we implement a nice API for reading computed fields but not for setting them, which is weird.

This would need a bit of re-architecting since _computed_key only allows storing a getter function, but not a setter.

OK sure :) Done.

---

ycmd/completers/javascript/tern_completer.py, line 206 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

The +1 needs a comment.

Done.

---

ycmd/completers/javascript/tern_completer.py, line 208 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

This would need an extensive comment as I don't really understand why you're doing this. :D

Done.

---

Comments from Reviewable

[Valloric]

with minor comments

Thanks for the PR!

---

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

---

ycmd/request_wrap.py, line 45 at r2 (raw file):

    self._request = request

    # Keys are keys for this object (returned by __getitem__) and values are a

Keys are keys?! :D :P

---

ycmd/request_wrap.py, line 91 at r2 (raw file):

      return self._cached_computed[ key ]
    if key in self._computed_key:
      getter, setter = self._computed_key[ key ]

Since you don't actually want the setter, you should use _ for the name. That's the Pythonic way of saying "I just one of these two".

---

ycmd/request_wrap.py, line 100 at r2 (raw file):

  def __setitem__( self, key, value ):
    if key in self._computed_key:
      getter, setter = self._computed_key[ key ]

Same as above, _ for getter.

---

ycmd/completers/javascript/tern_completer.py, line 208 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Done.

Wow, you really took "extensive" to heart! Thanks for explaining this so thoroughly! :)

---

ycmd/completers/javascript/tern_completer.py, line 209 at r2 (raw file):

    # Tern returns the range of the word in the file which it is replacing. This
    # may not be the same rainge that our "completion start column" calculation

typo: "rainge"

---

ycmd/completers/javascript/tern_completer.py, line 216 at r2 (raw file):

    #
    # with the cursor on |, tern returns something like 'test' (i.e. including
    # the single-quotes. Single-quotes are not a JavaScript identifier, so

Missing closing paren.

---

ycmd/completers/javascript/tern_completer.py, line 218 at r2 (raw file):

    # the single-quotes. Single-quotes are not a JavaScript identifier, so
    # should not normally be considered an identifier character, but by using
    # our aown start_codepoint calculation, the inserted string would be:

aown

I recommend turning on spell checking in Vim :P

---

Comments from Reviewable

[vheon]

---

Reviewed 1 of 3 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[homu]

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

[puremourning]

now possible with ycm-core/YouCompleteMe#2657!

[puremourning]

@zzbot try

appveyor flaked?

[zzbot]

⌛ Trying commit 93707d2 with merge ec9087b...

[zzbot]

☀️ Test successful - status-travis
State: approved= try=True

[bstaletic]

with a minor comment.

---

Reviewed 2 of 7 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

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

        ByteOffsetToCodepointOffset( self[ 'line_bytes' ],
                                     self[ 'column_num' ] ) ),
        None ),

This should be aligned with (lambda:. Also (lambda: should have a space after opening pren.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 45 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

Keys are keys?! :D :P

I meant keys of _computed_key are the keys of this object (accessed via __getitem__ / [] operator). I have reworded to make more sense!

---

ycmd/request_wrap.py, line 91 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

Since you don't actually want the setter, you should use _ for the name. That's the Pythonic way of saying "I just one of these two".

done.

---

ycmd/request_wrap.py, line 100 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

Same as above, _ for getter.

Done.

---

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

Previously, bstaletic wrote…

This should be aligned with (lambda:. Also (lambda: should have a space after opening pren.

Done.

---

ycmd/completers/javascript/tern_completer.py, line 209 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

typo: "rainge"

Done.

---

ycmd/completers/javascript/tern_completer.py, line 216 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

Missing closing paren.

Done.

---

ycmd/completers/javascript/tern_completer.py, line 218 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

aown

I recommend turning on spell checking in Vim :P

You probably won't believe me, but I actually do use :set spell.... :/

If only there was an autocomplete plugin for Vim which supported offering dictionary suggestions when typing in comments.... OK kill me. I think that's fair.

---

Comments from Reviewable

[bstaletic]

@puremourning This has no [READY] tag. Is there anything else missing? I also don't see any unresolved discussions.

---

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

---

Comments from Reviewable

[puremourning]

I honk it is g2g from server POV. when we're happy with ycm-core/YouCompleteMe#2657 then we can merge this. It's safe to merge without it (I think) - will check

[micbou]

Reviewed 7 of 7 files at r2, 5 of 6 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

    self._cached_computed[ 'start_codepoint' ] = ByteOffsetToCodepointOffset(
      self[ 'line_value' ],
      column_num )

Why don't we just remove the start_codepoint key from the cache instead so that we only re-compute it if needed? Also, since query depends on start_codepoint (and indirectly on start_column), we should either update its cached value or as suggested, remove its key from the cache.

---

ycmd/request_wrap.py, line 148 at r4 (raw file):

    self._cached_computed[ 'start_column' ] = CodepointOffsetToByteOffset(
      self[ 'line_value' ],
      codepoint_offset )

Same comment as above.

---

ycmd/tests/request_wrap_test.py, line 284 at r4 (raw file):

def Calculated_SetMethod_test():
  assert_that(
    calling( RequestWrap( PrepareJson( ) ).__setitem__).with_args(

Missing space after __setitem__.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, micbou wrote…

Why don't we just remove the start_codepoint key from the cache instead so that we only re-compute it if needed? Also, since query depends on start_codepoint (and indirectly on start_column), we should either update its cached value or as suggested, remove its key from the cache.

Fair point. Done.

---

ycmd/request_wrap.py, line 148 at r4 (raw file):

Previously, micbou wrote…

Same comment as above.

Fair point. Done.

---

ycmd/tests/request_wrap_test.py, line 284 at r4 (raw file):

Previously, micbou wrote…

Missing space after __setitem__.

Done.

---

Comments from Reviewable

[puremourning]

Hang on. I marked comments as done, but i haven't done them. Senile moment. Lemme see what happened.

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Fair point. Done.

Correction. I tried this and it just doesn't work. It's not super intuitive, but I'll explain:

The problem is that the calculation of the fields _GetCompletionStartColumn and _GetCompletionStartCodepoint are by default dependent only on the 'column number' in the original request. This makes sense because otherwise there would be an (arbitrary) dependency ordering between the start column and start codepoint. When we override the 'start column' (be it byte offset or codepoint), we are breaking the association between 'request column number' and 'start column'. The mechanism used to enforce this is to pre-compute the dependent value, because we don't store the 'new start column' (in whatever units the user called us) anywhere on the object.

Basically:

• if nobody ever calls the __setitem__, then the values calculated are independent of each other; they are calculated based on the column_num
• if anybody calls the __setitem__, then they are now dependent on that value that was just set. As that value is not available at a later time, we must pre-compute the now-dependent value

I hope that makes sense... It took me a few mins to work it out :)

---

ycmd/request_wrap.py, line 148 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Fair point. Done.

See above. I've added a brief comment to explain.

---

Comments from Reviewable

[puremourning]

OK panic over, I wasn't going mad. I reverted it because it was wrong :)

[puremourning]

I don't know what codecov is moaning about, coverage seems good to me.

[bstaletic]

I don't know what codecov is moaning about,

Codecov is moaning (I like that word in this context) about this. At least that's the only file with negative coverage diff according to codecov, but I can't understand why.

---

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

---

Comments from Reviewable

[micbou]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Correction. I tried this and it just doesn't work. It's not super intuitive, but I'll explain:

The problem is that the calculation of the fields _GetCompletionStartColumn and _GetCompletionStartCodepoint are by default dependent only on the 'column number' in the original request. This makes sense because otherwise there would be an (arbitrary) dependency ordering between the start column and start codepoint. When we override the 'start column' (be it byte offset or codepoint), we are breaking the association between 'request column number' and 'start column'. The mechanism used to enforce this is to pre-compute the dependent value, because we don't store the 'new start column' (in whatever units the user called us) anywhere on the object.

Basically:

• if nobody ever calls the __setitem__, then the values calculated are independent of each other; they are calculated based on the column_num
• if anybody calls the __setitem__, then they are now dependent on that value that was just set. As that value is not available at a later time, we must pre-compute the now-dependent value

I hope that makes sense... It took me a few mins to work it out :)

Right. I didn't think this through. What about query? It needs to be updated so that candidates are correctly filtered. See your example in ycm-core/YouCompleteMe#2489: test and testing should be filtered against this_is_a_test..

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, micbou wrote…

Right. I didn't think this through. What about query? It needs to be updated so that candidates are correctly filtered. See your example in ycm-core/YouCompleteMe#2489: test and testing should be filtered against this_is_a_test..

Hmm do we really want to change the query in response to a completion request? I'm concerned that might lead to unpredictable results (it's a sort of feedback loop).

The original examples that i remember for the omnicompleter were:

• javacomplete2 completion of @annonations - we would end up with 2 @ signs, but the filtering was fine IIRC
• eclim completion of import declarations (eclipse insists on returning the import keyword for inexplicable reasons. eclim does a hearty job of squashing this but iirc there were cases where it wasn't perfect. anyway, again i think the filtering in that case would be screwy if we changed the query to be the import statement)

That said, I just tested it out with the javacomplete2 @annotation thing and it isn't working correctly (it's replacing in the right position, but the @ symbol is removed). Will need to drill into that.

---

Comments from Reviewable

[micbou]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Hmm do we really want to change the query in response to a completion request? I'm concerned that might lead to unpredictable results (it's a sort of feedback loop).

This is already the case for the JavaScript completer with the proposed changes. The query is still not set when the start column is updated. It's only computed when filtering the candidates. This means that when changing the start column, we are also changing the query. Try to access the query before computing the candidates. This leads to ycmd returning all candidates when inserting the second punctuation mark in a require statement:

This is because the query is empty in that case (the start column is at the second punctuation mark). However, if we access the query after computing the candidates (and thus after updating the start column), the query is 'os' as expected.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):
Actually i have debugged this, and it's the omnifunc that's wrong. For this test case:

public class Test {
  @SuppressW
  public static void main(String[] args) {
  }
}

The omnifunc returns 2 for the findstart and SuppressWarnings for the text, so our behaviour is correct and javacomplete2 is telling us lies.

2017-06-29 17:53:40,454 - DEBUG - Omnifunc returned start: 2, values: [
  {
    "menu": "com.sun.xml.internal.ws.api.message", 
    "word": "SuppressAutomaticWSARequestHeadersFeature", 
    "type": "c"
  }, 
  {
    "menu": "java.lang", 
    "word": "SuppressWarnings", 
    "type": "c"
  }
]

For confirmation, the result is:

public class Test {
  SuppressWarnings
  public static void main(String[] args) {
  }
}

and the Vim help says (of the findstart result):

On the first invocation the arguments are:
a:findstart 1
a:base empty

The function must return the column where the completion starts. It must be a
number between zero and the cursor column "col('.')". This involves looking
at the characters just before the cursor and including those characters that
could be part of the completed item. The text between this column and the
cursor column will be replaced with the matches.

So a findstart of 2 represents the @ and the replacement text is SuppressWarnings

Can you check my logic on that?

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

This is because the query is empty in that case (the start column is at the second punctuation mark). However, if we access the query after computing the candidates (and thus after updating the start column), the query is 'os' as expected.

You're right. I think we should clear the query when forcing the start codepoint/ start column. as it's calculated based on the actual return of the __getitem__, we don't need to precompute it.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This is because the query is empty in that case (the start column is at the second punctuation mark). However, if we access the query after computing the candidates (and thus after updating the start column), the query is 'os' as expected.

You're right. I think we should clear the query when forcing the start codepoint/ start column. as it's calculated based on the actual return of the __getitem__, we don't need to precompute it.

actually i can't repro your behaviour with or without the change to 'query'

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

actually i can't repro your behaviour with or without the change to 'query'

Going back to the omnifunc thing. Testing without YCM, (going straight to the omnifunc) I get more sensible results, so something is certainly fishy with the interaction with the omnifunc.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Going back to the omnifunc thing. Testing without YCM, (going straight to the omnifunc) I get more sensible results, so something is certainly fishy with the interaction with the omnifunc.

Fixed it. The two issues are totally related. The solution was to fix the 'query' parameter as you mentioned, and to assign the start column before calling the omnifunc.

---

Comments from Reviewable

[puremourning]

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Fixed it. The two issues are totally related. The solution was to fix the 'query' parameter as you mentioned, and to assign the start column before calling the omnifunc.

OK this is now done.

---

Comments from Reviewable

[micbou]

---

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

---

ycmd/request_wrap.py, line 135 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

OK this is now done.

Awesome!

---

Comments from Reviewable

[micbou]

Given the number of s, I think we can merge this.

@zzbot r+

---

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

---

Comments from Reviewable

[zzbot]

📌 Commit b5742d3 has been approved by micbou

[zzbot]

⌛ Testing commit b5742d3 with merge 1b9a7aa...

[zzbot]

💔 Test failed - status-travis

[micbou]

This bot hates me.

@zzbot retry

---

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

---

Comments from Reviewable

[zzbot]

⌛ Testing commit b5742d3 with merge 490b3ee...

[zzbot]

☀️ Test successful - status-travis
Approved by: micbou
Pushing 490b3ee to master...