Auto merge of #681 - puremourning:js-start-col, r=micbou

[READY] Allow completers to set the start column

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).

<!-- 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/681)
<!-- Reviewable:end -->

ycmd/completers/javascript/tern_completer.py

@@ -198,9 +198,34 @@ def ComputeCandidatesInner( self, request_data ):
       'omitObjectPrototype': False
     }
 
-    completions = self._GetResponse( query,
-                                     request_data[ 'start_codepoint' ],
-                                     request_data ).get( 'completions', [] )
+    response = self._GetResponse( query,
+                                  request_data[ 'start_codepoint' ],
+                                  request_data )
+
+    completions = response.get( 'completions', [] )
+    tern_start_codepoint = response[ 'start' ][ 'ch' ]
+
+    # Tern returns the range of the word in the file which it is replacing. This
+    # may not be the same range that our "completion start column" calculation
+    # decided (i.e. it might not strictly be an identifier according to our
+    # rules). For example, when completing:
+    #
+    # require( '|
+    #
+    # with the cursor on |, tern returns something like 'test' (i.e. including
+    # the single-quotes). Single-quotes are not a JavaScript identifier, so
+    # should not normally be considered an identifier character, but by using
+    # our own start_codepoint calculation, the inserted string would be:
+    #
+    # require( ''test'
+    #
+    # which is clearly incorrect. It should be:
+    #
+    # require( 'test'
+    #
+    # So, we use the start position that tern tells us to use.
+    # We add 1 because tern offsets are 0-based and ycmd offsets are 1-based
+    request_data[ 'start_codepoint' ] = tern_start_codepoint + 1
 
     def BuildDoc( completion ):
       doc = completion.get( 'type', 'Unknown type' )

ycmd/handlers.py

@@ -121,7 +121,7 @@ def GetCompletions():
 
   return _JsonResponse(
       BuildCompletionResponse( completions if completions else [],
-                               request_data.CompletionStartColumn(),
+                               request_data[ 'start_column' ],
                                errors = errors ) )
 
 

ycmd/request_wrap.py

@@ -40,36 +40,45 @@ def __init__( self, request, validate = True ):
     if validate:
       EnsureRequestValid( request )
     self._request = request
+
+    # Maps the keys returned by this objects __getitem__ to a # tuple of
+    # ( getter_method, setter_method ). Values computed by getter_method (or set
+    # by setter_method) are cached in _cached_computed.  setter_method may be
+    # None for read-only items.
     self._computed_key = {
       # Unicode string representation of the current line
-      'line_value': self._CurrentLine,
+      'line_value': ( self._CurrentLine, None ),
 
       # The calculated start column, as a codepoint offset into the
       # unicode string line_value
-      'start_codepoint': self.CompletionStartCodepoint,
+      'start_codepoint': ( self._GetCompletionStartCodepoint,
+                           self._SetCompletionStartCodepoint ),
 
       # The 'column_num' as a unicode codepoint offset
-      'column_codepoint': (lambda:
-        ByteOffsetToCodepointOffset( self[ 'line_bytes' ],
-                                     self[ 'column_num' ] ) ),
+      'column_codepoint': ( lambda: ByteOffsetToCodepointOffset(
+                              self[ 'line_bytes' ],
+                              self[ 'column_num' ] ),
+                            None ),
 
       # Bytes string representation of the current line
-      'line_bytes': lambda: ToBytes( self[ 'line_value' ] ),
+      'line_bytes': ( lambda: ToBytes( self[ 'line_value' ] ),
+                      None ),
 
       # The calculated start column, as a byte offset into the UTF-8 encoded
       # bytes returned by line_bytes
-      'start_column': self.CompletionStartColumn,
+      'start_column': ( self._GetCompletionStartColumn,
+                        self._SetCompletionStartColumn ),
 
       # Note: column_num is the byte offset into the UTF-8 encoded bytes
       # returned by line_bytes
 
       # unicode string representation of the 'query' after the beginning
       # of the identifier to be completed
-      'query': self._Query,
+      'query': ( self._Query, None ),
 
-      'filetypes': self._Filetypes,
+      'filetypes': ( self._Filetypes, None ),
 
-      'first_filetype': self._FirstFiletype,
+      'first_filetype': ( self._FirstFiletype, None ),
     }
     self._cached_computed = {}
 
@@ -78,12 +87,23 @@ def __getitem__( self, key ):
     if key in self._cached_computed:
       return self._cached_computed[ key ]
     if key in self._computed_key:
-      value = self._computed_key[ key ]()
+      getter, _ = self._computed_key[ key ]
+      value = getter()
       self._cached_computed[ key ] = value
       return value
     return self._request[ key ]
 
 
+  def __setitem__( self, key, value ):
+    if key in self._computed_key:
+      _, setter = self._computed_key[ key ]
+      if setter:
+        setter( value )
+        return
+
+    raise ValueError( 'Key "{0}" is read-only'.format( key ) )
+
+
   def __contains__( self, key ):
     return key in self._computed_key or key in self._request
 
@@ -102,18 +122,53 @@ def _CurrentLine( self ):
     return SplitLines( contents )[ self._request[ 'line_num' ] - 1 ]
 
 
-  def CompletionStartColumn( self ):
+  def _GetCompletionStartColumn( self ):
     return CompletionStartColumn( self[ 'line_value' ],
                                   self[ 'column_num' ],
                                   self[ 'first_filetype' ] )
 
 
-  def CompletionStartCodepoint( self ):
+  def _SetCompletionStartColumn( self, column_num ):
+    self._cached_computed[ 'start_column' ] = column_num
+
+    # Note: We must pre-compute (and cache) the codepoint equivalent. This is
+    # because the value calculated by the getter (_GetCompletionStartCodepoint)
+    # would be based on self[ 'column_codepoint' ] which would be incorrect; it
+    # does not know that the user has forced this value to be independent of the
+    # column.
+    self._cached_computed[ 'start_codepoint' ] = ByteOffsetToCodepointOffset(
+      self[ 'line_value' ],
+      column_num )
+
+    # The same applies to the 'query' (the bit after the start column up to the
+    # cursor column). It's dependent on the 'start_codepoint' so we must reset
+    # it.
+    self._cached_computed.pop( 'query', None )
+
+
+  def _GetCompletionStartCodepoint( self ):
     return CompletionStartCodepoint( self[ 'line_value' ],
                                      self[ 'column_num' ],
                                      self[ 'first_filetype' ] )
 
 
+  def _SetCompletionStartCodepoint( self, codepoint_offset ):
+    self._cached_computed[ 'start_codepoint' ] = codepoint_offset
+
+    # Note: We must pre-compute (and cache) the byte equivalent. This is because
+    # the value calculated by the getter (_GetCompletionStartColumn) would be
+    # based on self[ 'column_num' ], which would be incorrect; it does not know
+    # that the user has forced this value to be independent of the column.
+    self._cached_computed[ 'start_column' ] = CodepointOffsetToByteOffset(
+      self[ 'line_value' ],
+      codepoint_offset )
+
+    # The same applies to the 'query' (the bit after the start column up to the
+    # cursor column). It's dependent on the 'start_codepoint' so we must reset
+    # it.
+    self._cached_computed.pop( 'query', None )
+
+
   def _Query( self ):
     return self[ 'line_value' ][
         self[ 'start_codepoint' ] - 1 : self[ 'column_codepoint' ] - 1

ycmd/tests/javascript/__init__.py

@@ -86,14 +86,29 @@ def IsolatedYcmd( test ):
   started, no .ycm_extra_conf.py loaded, etc).
 
   Do NOT attach it to test generators but directly to the yielded tests."""
-  @functools.wraps( test )
-  def Wrapper( *args, **kwargs ):
-    old_server_state = handlers._server_state
-    app = SetUpApp()
-    try:
-      with CurrentWorkingDirectory( PathToTestFile() ):
-        test( app, *args, **kwargs )
-    finally:
-      StopCompleterServer( app, 'javascript' )
-      handlers._server_state = old_server_state
-  return Wrapper
+  return IsolatedYcmdInDirectory( PathToTestFile() )
+
+
+def IsolatedYcmdInDirectory( directory ):
+  """Defines a decorator to be attached to tests of this package. This decorator
+  passes a unique ycmd application as a parameter running in the directory
+  supplied. It should be used on tests that change the server state in a
+  irreversible way (ex: a semantic subserver is stopped or restarted) or expect
+  a clean state (ex: no semantic subserver started, no .ycm_extra_conf.py
+  loaded, etc).
+
+  Do NOT attach it to test generators but directly to the yielded tests."""
+  def Decorator( test ):
+    @functools.wraps( test )
+    def Wrapper( *args, **kwargs ):
+      old_server_state = handlers._server_state
+      app = SetUpApp()
+      try:
+        with CurrentWorkingDirectory( directory ):
+          test( app, *args, **kwargs )
+      finally:
+        StopCompleterServer( app, 'javascript' )
+        handlers._server_state = old_server_state
+    return Wrapper
+
+  return Decorator

ycmd/tests/javascript/get_completions_test.py

@@ -28,8 +28,10 @@
 from pprint import pformat
 import requests
 
-from ycmd.tests.javascript import PathToTestFile, SharedYcmd
-from ycmd.tests.test_utils import BuildRequest, CompletionEntryMatcher
+from ycmd.tests.javascript import ( PathToTestFile, SharedYcmd,
+                                    IsolatedYcmdInDirectory )
+from ycmd.tests.test_utils import ( BuildRequest, CompletionEntryMatcher,
+                                    WaitUntilCompleterServerReady )
 from ycmd.utils import ReadFile
 
 # The following properties/methods are in Object.prototype, so are present
@@ -480,3 +482,28 @@ def GetCompletions_Unicode_InFile_test( app ):
       } )
     },
   } )
+
+
+@IsolatedYcmdInDirectory( PathToTestFile( 'node' ) )
+def GetCompletions_ChangeStartColumn_test( app ):
+  WaitUntilCompleterServerReady( app, 'javascript' )
+  RunTest( app, {
+    'description': 'the completion_start_column is updated by tern',
+    'request': {
+      'filetype'      : 'javascript',
+      'filepath'      : PathToTestFile( 'node', 'node_test.js' ),
+      'line_num'      : 1,
+      'column_num'    : 17,
+      'force_semantic': True,
+    },
+    'expect': {
+      'response': requests.codes.ok,
+      'data': has_entries( {
+        'completions': contains(
+          CompletionEntryMatcher( '"path"', 'path' )
+        ),
+        'completion_start_column': 14,
+        'errors': empty(),
+      } )
+    },
+  } )

ycmd/tests/javascript/testdata/node/.tern-project

@@ -0,0 +1,5 @@
+{
+  "plugins": {
+    "node": {}
+  }
+}

ycmd/tests/javascript/testdata/node/node_test.js

@@ -0,0 +1,2 @@
+p = require( "path" )
+p.jo

ycmd/tests/request_wrap_test.py

@@ -24,6 +24,7 @@
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
+from hamcrest import assert_that, calling, equal_to, raises
 from nose.tools import eq_
 
 from ycmd.utils import ToBytes
@@ -228,3 +229,92 @@ def Query_UnicodeSinglecharExclusive_test():
   eq_( '',
        RequestWrap( PrepareJson( column_num = 5,
                                  contents = 'abc.ø' ) )[ 'query' ] )
+
+
+def StartColumn_Set_test():
+  wrap = RequestWrap( PrepareJson( column_num = 11,
+                                   contents = 'this \'test',
+                                   filetype = 'javascript' ) )
+  eq_( wrap[ 'start_column' ], 7 )
+  eq_( wrap[ 'start_codepoint' ], 7 )
+  eq_( wrap[ 'query' ], "test" )
+
+  wrap[ 'start_column' ] = 6
+  eq_( wrap[ 'start_column' ], 6 )
+  eq_( wrap[ 'start_codepoint' ], 6 )
+  eq_( wrap[ 'query' ], "'test" )
+
+
+def StartColumn_SetUnicode_test():
+  wrap = RequestWrap( PrepareJson( column_num = 14,
+                                   contents = '†e߆ \'test',
+                                   filetype = 'javascript' ) )
+  eq_( 7,  wrap[ 'start_codepoint' ] )
+  eq_( 12, wrap[ 'start_column' ] )
+  eq_( wrap[ 'query' ], "te" )
+
+  wrap[ 'start_column' ] = 11
+  eq_( wrap[ 'start_column' ], 11 )
+  eq_( wrap[ 'start_codepoint' ], 6 )
+  eq_( wrap[ 'query' ], "'te" )
+
+
+def StartCodepoint_Set_test():
+  wrap = RequestWrap( PrepareJson( column_num = 11,
+                                   contents = 'this \'test',
+                                   filetype = 'javascript' ) )
+  eq_( wrap[ 'start_column' ], 7 )
+  eq_( wrap[ 'start_codepoint' ], 7 )
+  eq_( wrap[ 'query' ], "test" )
+
+  wrap[ 'start_codepoint' ] = 6
+  eq_( wrap[ 'start_column' ], 6 )
+  eq_( wrap[ 'start_codepoint' ], 6 )
+  eq_( wrap[ 'query' ], "'test" )
+
+
+def StartCodepoint_SetUnicode_test():
+  wrap = RequestWrap( PrepareJson( column_num = 14,
+                                   contents = '†e߆ \'test',
+                                   filetype = 'javascript' ) )
+  eq_( 7,  wrap[ 'start_codepoint' ] )
+  eq_( 12, wrap[ 'start_column' ] )
+  eq_( wrap[ 'query' ], "te" )
+
+  wrap[ 'start_codepoint' ] = 6
+  eq_( wrap[ 'start_column' ], 11 )
+  eq_( wrap[ 'start_codepoint' ], 6 )
+  eq_( wrap[ 'query' ], "'te" )
+
+
+def Calculated_SetMethod_test():
+  assert_that(
+    calling( RequestWrap( PrepareJson( ) ).__setitem__ ).with_args(
+      'line_value', '' ),
+    raises( ValueError, 'Key "line_value" is read-only' ) )
+
+
+def Calculated_SetOperator_test():
+  # Differs from the above in that it use [] operator rather than __setitem__
+  # directly. And it uses a different property for extra credit.
+  wrap = RequestWrap( PrepareJson() )
+  try:
+    wrap[ 'query' ] = 'test'
+  except ValueError as error:
+    assert_that( str( error ),
+                 equal_to( 'Key "query" is read-only' ) )
+  else:
+    raise AssertionError( 'Expected setting "query" to fail' )
+
+
+def NonCalculated_Set_test():
+  # Differs from the above in that it use [] operator rather than __setitem__
+  # directly. And it uses a different property for extra credit.
+  wrap = RequestWrap( PrepareJson() )
+  try:
+    wrap[ 'column_num' ] = 10
+  except ValueError as error:
+    assert_that( str( error ),
+                 equal_to( 'Key "column_num" is read-only' ) )
+  else:
+    raise AssertionError( 'Expected setting "column_num" to fail' )