Auto merge of #1676 - micbou:fix-it-chunks-sorting, r=Valloric

Correct FixIt chunks sorting

While playing with FixIts in C++, I found the following issue. When fixing the third line in the code:
```cpp
template<int Value> struct CT { template<typename> struct Inner; };

CT<10 >> 2> ct; // expected-warning{{require parentheses}}
```
the following result is obtained:
```cpp
CT<1(0 >> 2)> ct; // expected-warning{{require parentheses}}
```
which is obviously wrong.

The issue is YouCompleteMe does not replace the chunks in the right order. It starts by adding the closing parenthesis, add one to the delta and inserts the opening parenthesis in the wrong place cause of the delta.

We actually use the expression `str(line) + ',' + str(column)` to sort the chunks by line then column whereas it should simply be `(line, column)`.

This PR fixes this issue, adds two tests which are failing in the current version, refactors the `_HandleFixitResponse` function and cleans up code.

python/ycm/client/command_request.py

@@ -22,6 +22,7 @@ from ycm.client.base_request import BaseRequest, BuildRequestData, ServerError
 from ycm import vimsupport
 from ycmd.utils import ToUtf8IfNeeded
 
+
 def _EnsureBackwardsCompatibility( arguments ):
   if arguments and arguments[ 0 ] == 'GoToDefinitionElseDeclaration':
     arguments[ 0 ] = 'GoTo'
@@ -57,6 +58,7 @@ class CommandRequest( BaseRequest ):
   def Response( self ):
     return self._response
 
+
   def RunPostCommandActionsIfNeeded( self ):
     if not self.Done() or not self._response:
       return
@@ -68,6 +70,7 @@ class CommandRequest( BaseRequest ):
     elif 'message' in self._response:
       self._HandleMessageResponse()
 
+
   def _HandleGotoResponse( self ):
     if isinstance( self._response, list ):
       defs = [ _BuildQfListItem( x ) for x in self._response ]
@@ -78,53 +81,24 @@ class CommandRequest( BaseRequest ):
                                  self._response[ 'line_num' ],
                                  self._response[ 'column_num' ] )
 
+
   def _HandleFixitResponse( self ):
     if not len( self._response[ 'fixits' ] ):
       vimsupport.EchoText( "No fixits found for current line" )
     else:
-      fixit = self._response[ 'fixits' ][ 0 ]
+      chunks = self._response[ 'fixits' ][ 0 ][ 'chunks' ]
 
-      # We need to track the difference in length, but ensuring we apply fixes
+      vimsupport.ReplaceChunksList( chunks )
-      # in ascending order of insertion point.
-      fixit[ 'chunks' ].sort( key = lambda chunk:  (
-        str(chunk[ 'range' ][ 'start' ][ 'line_num' ])
-        + ','
-        + str(chunk[ 'range' ][ 'start' ][ 'column_num' ])
-      ))
 
-      # Remember the line number we're processing. Negative line number means we
+      vimsupport.EchoTextVimWidth( "FixIt applied "
-      # haven't processed any lines yet (by nature of being not equal to any
+                                   + str( len( chunks ) )
-      # real line number).
+                                   + " changes" )
-      last_line = -1
 
-      # Counter of changes applied, so the user has a mental picture of the
-      # undo history this change is creating.
-      num_fixed = 0
-      line_delta = 0
-      for chunk in fixit[ 'chunks' ]:
-        if chunk[ 'range' ][ 'start' ][ 'line_num' ] != last_line:
-          # If this chunk is on a different line than the previous chunk,
-          # then ignore previous deltas (as offsets won't have changed).
-          last_line = chunk[ 'range' ][ 'end' ][ 'line_num' ]
-          char_delta = 0
-
-        (new_line_delta, new_char_delta) = vimsupport.ReplaceChunk(
-                                          chunk[ 'range' ][ 'start' ],
-                                          chunk[ 'range' ][ 'end' ],
-                                          chunk[ 'replacement_text' ],
-                                          line_delta, char_delta )
-        line_delta += new_line_delta
-        char_delta += new_char_delta
-
-        num_fixed = num_fixed + 1
-
-      vimsupport.EchoTextVimWidth("FixIt applied " 
-                                  + str(num_fixed) 
-                                  + " changes")
 
   def _HandleMessageResponse( self ):
     vimsupport.EchoText( self._response[ 'message' ] )
 
+
 def SendCommandRequest( arguments, completer ):
   request = CommandRequest( arguments, completer )
   # This is a blocking call.

python/ycm/tests/vimsupport_test.py

@@ -20,6 +20,7 @@
 from ycm import vimsupport
 from nose.tools import eq_
 
+
 def ReplaceChunk_SingleLine_Repl_1_test():
   # Replace with longer range
   #                  12345678901234567
@@ -75,6 +76,7 @@ def ReplaceChunk_SingleLine_Repl_1_test():
   eq_( line_offset, 0 )
   eq_( char_offset, 10 )
 
+
 def ReplaceChunk_SingleLine_Repl_2_test():
   # Replace with shorter range
   #                  12345678901234567
@@ -91,6 +93,7 @@ def ReplaceChunk_SingleLine_Repl_2_test():
   eq_( line_offset, 0 )
   eq_( char_offset, -2 )
 
+
 def ReplaceChunk_SingleLine_Repl_3_test():
   # Replace with equal range
   #                  12345678901234567
@@ -107,6 +110,7 @@ def ReplaceChunk_SingleLine_Repl_3_test():
   eq_( line_offset, 0 )
   eq_( char_offset, 0 )
 
+
 def ReplaceChunk_SingleLine_Add_1_test():
   # Insert at start
   result_buffer = [ "is a string" ]
@@ -122,6 +126,7 @@ def ReplaceChunk_SingleLine_Add_1_test():
   eq_( line_offset, 0 )
   eq_( char_offset, 5 )
 
+
 def ReplaceChunk_SingleLine_Add_2_test():
   # Insert at end
   result_buffer = [ "This is a " ]
@@ -137,6 +142,7 @@ def ReplaceChunk_SingleLine_Add_2_test():
   eq_( line_offset, 0 )
   eq_( char_offset, 6 )
 
+
 def ReplaceChunk_SingleLine_Add_3_test():
   # Insert in the middle
   result_buffer = [ "This is a string" ]
@@ -152,6 +158,7 @@ def ReplaceChunk_SingleLine_Add_3_test():
   eq_( line_offset, 0 )
   eq_( char_offset, 4 )
 
+
 def ReplaceChunk_SingleLine_Del_1_test():
   # Delete from start
   result_buffer = [ "This is a string" ]
@@ -167,6 +174,7 @@ def ReplaceChunk_SingleLine_Del_1_test():
   eq_( line_offset, 0 )
   eq_( char_offset, -5 )
 
+
 def ReplaceChunk_SingleLine_Del_2_test():
   # Delete from end
   result_buffer = [ "This is a string" ]
@@ -182,6 +190,7 @@ def ReplaceChunk_SingleLine_Del_2_test():
   eq_( line_offset, 0 )
   eq_( char_offset, -8 )
 
+
 def ReplaceChunk_SingleLine_Del_3_test():
   # Delete from middle
   result_buffer = [ "This is not a string" ]
@@ -197,6 +206,7 @@ def ReplaceChunk_SingleLine_Del_3_test():
   eq_( line_offset, 0 )
   eq_( char_offset, -4 )
 
+
 def ReplaceChunk_RemoveSingleLine_test():
   result_buffer = [ "aAa", "aBa", "aCa" ]
   start, end = _BuildLocations( 2, 1, 3, 1 )
@@ -250,7 +260,7 @@ def ReplaceChunk_SingleToMultipleLines2_test():
                                                           0,
                                                           0,
                                                           result_buffer )
-  expected_buffer = [ "aAa", "aEb" ,"bFb", "GBa", "aCa" ]
+  expected_buffer = [ "aAa", "aEb", "bFb", "GBa", "aCa" ]
   eq_( expected_buffer, result_buffer )
   eq_( line_offset, 2 )
   eq_( char_offset, 0 )
@@ -265,11 +275,12 @@ def ReplaceChunk_SingleToMultipleLines3_test():
                                                           0,
                                                           0,
                                                           result_buffer )
-  expected_buffer = [ "aAa", "aEb" ,"bFb", "bGbBa", "aCa" ]
+  expected_buffer = [ "aAa", "aEb", "bFb", "bGbBa", "aCa" ]
   eq_( expected_buffer, result_buffer )
   eq_( line_offset, 2 )
   eq_( char_offset, 2 )
 
+
 def ReplaceChunk_SingleToMultipleLinesReplace_test():
   result_buffer = [ "aAa", "aBa", "aCa" ]
   start, end = _BuildLocations( 1, 2, 1, 4 )
@@ -284,6 +295,7 @@ def ReplaceChunk_SingleToMultipleLinesReplace_test():
   eq_( line_offset, 2 )
   eq_( char_offset, 0 )
 
+
 def ReplaceChunk_SingleToMultipleLinesReplace_2_test():
   result_buffer = [ "aAa",
                     "aBa",
@@ -521,3 +533,46 @@ def _BuildLocations( start_line, start_column, end_line, end_column ):
     'line_num'  : end_line,
     'column_num': end_column,
   }
+
+
+def ReplaceChunksList_SortedChunks_test():
+  chunks = [
+    _BuildChunk( 1, 4, 1, 4, '('),
+    _BuildChunk( 1, 11, 1, 11, ')' )
+  ]
+
+  result_buffer = [ "CT<10 >> 2> ct" ]
+  vimsupport.ReplaceChunksList( chunks, result_buffer )
+
+  expected_buffer = [ "CT<(10 >> 2)> ct" ]
+  eq_( expected_buffer, result_buffer )
+
+
+def ReplaceChunksList_UnsortedChunks_test():
+  chunks = [
+    _BuildChunk( 1, 11, 1, 11, ')'),
+    _BuildChunk( 1, 4, 1, 4, '(' )
+  ]
+
+  result_buffer = [ "CT<10 >> 2> ct" ]
+  vimsupport.ReplaceChunksList( chunks, result_buffer )
+
+  expected_buffer = [ "CT<(10 >> 2)> ct" ]
+  eq_( expected_buffer, result_buffer )
+
+
+def _BuildChunk( start_line, start_column, end_line, end_column,
+                 replacement_text ):
+  return {
+    'range': {
+      'start': {
+        'line_num': start_line,
+        'column_num': start_column,
+      },
+      'end': {
+        'line_num': end_line,
+        'column_num': end_column,
+      },
+    },
+    'replacement_text': replacement_text
+  }

python/ycm/vimsupport.py

@@ -459,6 +459,40 @@ def GetIntValue( variable ):
   return int( vim.eval( variable ) )
 
 
+def ReplaceChunksList( chunks, vim_buffer = None ):
+  if vim_buffer is None:
+    vim_buffer = vim.current.buffer
+
+  # We need to track the difference in length, but ensuring we apply fixes
+  # in ascending order of insertion point.
+  chunks.sort( key = lambda chunk: (
+    chunk[ 'range' ][ 'start' ][ 'line_num' ],
+    chunk[ 'range' ][ 'start' ][ 'column_num' ]
+  ) )
+
+  # Remember the line number we're processing. Negative line number means we
+  # haven't processed any lines yet (by nature of being not equal to any
+  # real line number).
+  last_line = -1
+
+  line_delta = 0
+  for chunk in chunks:
+    if chunk[ 'range' ][ 'start' ][ 'line_num' ] != last_line:
+      # If this chunk is on a different line than the previous chunk,
+      # then ignore previous deltas (as offsets won't have changed).
+      last_line = chunk[ 'range' ][ 'end' ][ 'line_num' ]
+      char_delta = 0
+
+    ( new_line_delta, new_char_delta ) = ReplaceChunk(
+      chunk[ 'range' ][ 'start' ],
+      chunk[ 'range' ][ 'end' ],
+      chunk[ 'replacement_text' ],
+      line_delta, char_delta,
+      vim_buffer )
+    line_delta += new_line_delta
+    char_delta += new_char_delta
+
+
 # Replace the chunk of text specified by a contiguous range with the supplied
 # text.
 # * start and end are objects with line_num and column_num properties
@@ -469,10 +503,7 @@ def GetIntValue( variable ):
 # returns the delta (in lines and characters) that any position after the end
 # needs to be adjusted by.
 def ReplaceChunk( start, end, replacement_text, line_delta, char_delta,
-                  vim_buffer = None ):
+                  vim_buffer ):
-  if vim_buffer is None:
-    vim_buffer = vim.current.buffer
-
   # ycmd's results are all 1-based, but vim's/python's are all 0-based
   # (so we do -1 on all of the values)
   start_line = start[ 'line_num' ] - 1 + line_delta