Support selecting from the list of FixIts when multiple are supplied

README.md

@@ -1359,8 +1359,11 @@ undone, and never saves or writes files to the disk.
 
 #### The `FixIt` subcommand
 
-Where available, attempts to make changes to the buffer to correct the
+Where available, attempts to make changes to the buffer to correct diagnostics
-diagnostic closest to the cursor position.
+on the current line. Where multiple suggestions are available (such as when
+there are multiple ways to resolve a given warning, or where multiple
+diagnostics are reported for the current line), the options are presented
+and one can be selected.
 
 Completers which provide diagnostics may also provide trivial modifications to
 the source in order to correct the diagnostic. Examples include syntax errors

doc/youcompleteme.txt

@@ -1637,8 +1637,11 @@ undone, and never saves or writes files to the disk.
 -------------------------------------------------------------------------------
 The *FixIt* subcommand
 
-Where available, attempts to make changes to the buffer to correct the
+Where available, attempts to make changes to the buffer to correct diagnostics
-diagnostic closest to the cursor position.
+on the current line. Where multiple suggestions are available (such as when
+there are multiple ways to resolve a given warning, or where multiple
+diagnostics are reported for the current line), the options are presented and
+one can be selected.
 
 Completers which provide diagnostics may also provide trivial modifications to
 the source in order to correct the diagnostic. Examples include syntax errors

python/ycm/client/command_request.py

@@ -105,9 +105,19 @@ class CommandRequest( BaseRequest ):
     if not len( self._response[ 'fixits' ] ):
       vimsupport.EchoText( "No fixits found for current line" )
     else:
-      chunks = self._response[ 'fixits' ][ 0 ][ 'chunks' ]
       try:
-        vimsupport.ReplaceChunks( chunks )
+        fixit_index = 0
+
+        # When there are multiple fixit suggestions, present them as a list to
+        # the user hand have her choose which one to apply.
+        if len( self._response[ 'fixits' ] ) > 1:
+          fixit_index = vimsupport.SelectFromList(
+            "Multiple FixIt suggestions are available at this location. "
+            "Which one would you like to apply?",
+            [ fixit[ 'text' ] for fixit in self._response[ 'fixits' ] ] )
+
+        vimsupport.ReplaceChunks(
+          self._response[ 'fixits' ][ fixit_index ][ 'chunks' ] )
       except RuntimeError as e:
         vimsupport.PostMultiLineNotice( str( e ) )
 

python/ycm/client/tests/command_request_test.py

@@ -156,15 +156,17 @@ class Response_Detection_test( object ):
 
   def FixIt_Response_test( self ):
     # Ensures we recognise and handle fixit responses with some dummy chunk data
-    def FixItTest( command, response, chunks ):
+    def FixItTest( command, response, chunks, selection ):
       with patch( 'ycm.vimsupport.ReplaceChunks' ) as replace_chunks:
         with patch( 'ycm.vimsupport.EchoText' ) as echo_text:
-          request = CommandRequest( [ command ] )
+          with patch( 'ycm.vimsupport.SelectFromList',
-          request._response = response
+                      return_value = selection ):
-          request.RunPostCommandActionsIfNeeded()
+            request = CommandRequest( [ command ] )
+            request._response = response
+            request.RunPostCommandActionsIfNeeded()
 
-          replace_chunks.assert_called_with( chunks )
+            replace_chunks.assert_called_with( chunks )
-          echo_text.assert_not_called()
+            echo_text.assert_not_called()
 
     basic_fixit = {
       'fixits': [ {
@@ -177,25 +179,31 @@ class Response_Detection_test( object ):
 
     multi_fixit = {
       'fixits': [ {
+        'text': 'first',
         'chunks': [ {
           'dummy chunk contents': True
         } ]
       }, {
-        'additional fixits are ignored currently': True
+        'text': 'second',
+        'chunks': [ {
+          'dummy chunk contents': False
+        }]
       } ]
     }
     multi_fixit_first_chunks = multi_fixit[ 'fixits' ][ 0 ][ 'chunks' ]
+    multi_fixit_second_chunks = multi_fixit[ 'fixits' ][ 1 ][ 'chunks' ]
 
     tests = [
-      [ 'AnythingYouLike',        basic_fixit, basic_fixit_chunks ],
+      [ 'AnythingYouLike',        basic_fixit, basic_fixit_chunks, 0 ],
-      [ 'GoToEvenWorks',          basic_fixit, basic_fixit_chunks ],
+      [ 'GoToEvenWorks',          basic_fixit, basic_fixit_chunks, 0 ],
-      [ 'FixItWorks',             basic_fixit, basic_fixit_chunks ],
+      [ 'FixItWorks',             basic_fixit, basic_fixit_chunks, 0 ],
-      [ 'and8434fd andy garbag!', basic_fixit, basic_fixit_chunks ],
+      [ 'and8434fd andy garbag!', basic_fixit, basic_fixit_chunks, 0 ],
-      [ 'additional fixits ignored', multi_fixit, multi_fixit_first_chunks ],
+      [ 'select from multiple 1',   multi_fixit, multi_fixit_first_chunks, 0 ],
+      [ 'select from multiple 2',   multi_fixit, multi_fixit_second_chunks, 1 ],
     ]
 
     for test in tests:
-      yield FixItTest, test[ 0 ], test[ 1 ], test[ 2 ]
+      yield FixItTest, test[ 0 ], test[ 1 ], test[ 2 ], test[ 3 ]
 
 
   def Message_Response_test( self ):

python/ycm/tests/event_notification_test.py

@@ -49,7 +49,7 @@ def PostVimMessage_Call( message ):
 def PostMultiLineNotice_Call( message ):
   """Return a mock.call object for a call to vimsupport.PostMultiLineNotice with
   the supplied message"""
-  return call( 'echohl WarningMsg | echo \''
+  return call( 'redraw | echohl WarningMsg | echo \''
                + message +
                '\' | echohl None' )
 

python/ycm/tests/vimsupport_test.py

@@ -1432,3 +1432,52 @@ def VimExpressionToPythonType_ObjectPassthrough_test( *args ):
 def VimExpressionToPythonType_GeneratorPassthrough_test( *args ):
   gen = ( x**2 for x in [ 1, 2, 3 ] )
   eq_( vimsupport.VimExpressionToPythonType( gen ), gen )
+
+
+@patch( 'vim.eval',
+        new_callable = ExtendedMock,
+        side_effect = [ None, 2, None ] )
+def SelectFromList_LastItem_test( vim_eval ):
+  eq_( vimsupport.SelectFromList( 'test', [ 'a', 'b' ] ),
+       1 )
+
+  vim_eval.assert_has_exact_calls( [
+    call( 'inputsave()' ),
+    call( 'inputlist( ["test", "1: a", "2: b"] )' ),
+    call( 'inputrestore()' )
+  ] )
+
+
+@patch( 'vim.eval',
+        new_callable = ExtendedMock,
+        side_effect = [ None, 1, None ] )
+def SelectFromList_FirstItem_test( vim_eval ):
+  eq_( vimsupport.SelectFromList( 'test', [ 'a', 'b' ] ),
+       0 )
+
+  vim_eval.assert_has_exact_calls( [
+    call( 'inputsave()' ),
+    call( 'inputlist( ["test", "1: a", "2: b"] )' ),
+    call( 'inputrestore()' )
+  ] )
+
+
+@patch( 'vim.eval', side_effect = [ None, 3, None ] )
+def SelectFromList_OutOfRange_test( vim_eval ):
+  assert_that( calling( vimsupport.SelectFromList).with_args( 'test',
+                                                              [ 'a', 'b' ] ),
+               raises( RuntimeError, vimsupport.NO_SELECTION_MADE_MSG ) )
+
+
+@patch( 'vim.eval', side_effect = [ None, 0, None ] )
+def SelectFromList_SelectPrompt_test( vim_eval ):
+  assert_that( calling( vimsupport.SelectFromList ).with_args( 'test',
+                                                             [ 'a', 'b' ] ),
+               raises( RuntimeError, vimsupport.NO_SELECTION_MADE_MSG ) )
+
+
+@patch( 'vim.eval', side_effect = [ None, -199, None ] )
+def SelectFromList_Negative_test( vim_eval ):
+  assert_that( calling( vimsupport.SelectFromList ).with_args( 'test',
+                                                               [ 'a', 'b' ] ),
+               raises( RuntimeError, vimsupport.NO_SELECTION_MADE_MSG ) )

python/ycm/vimsupport.py

@@ -44,6 +44,8 @@ FIXIT_OPENING_BUFFERS_MESSAGE_FORMAT = (
     'buffers. The quickfix list can then be used to review the changes. No '
     'files will be written to disk. Do you wish to continue?' )
 
+NO_SELECTION_MADE_MSG = "No valid selection was made; aborting."
+
 
 def CurrentLineAndColumn():
   """Returns the 0-based current line and 0-based current column."""
@@ -441,8 +443,11 @@ def PostVimMessage( message ):
 # Unlike PostVimMesasge, this supports messages with newlines in them because it
 # uses 'echo' instead of 'echomsg'. This also means that the message will NOT
 # appear in Vim's message log.
+# Similarly to PostVimMesasge, we do a redraw first to clear any previous
+# messages, which might lead to this message appearing without a newline and/or
+# requring the "Press ENTER or type command to continue".
 def PostMultiLineNotice( message ):
-  vim.command( "echohl WarningMsg | echo '{0}' | echohl None"
+  vim.command( "redraw | echohl WarningMsg | echo '{0}' | echohl None"
                .format( EscapeForVim( ToUnicode( message ) ) ) )
 
 
@@ -458,6 +463,10 @@ def PresentDialog( message, choices, default_choice_index = 0 ):
   PresentDialog will return a 0-based index into the list
   or -1 if the dialog was dismissed by using <Esc>, Ctrl-C, etc.
 
+  If you are presenting a list of options for the user to choose from, such as
+  a list of imports, or lines to insert (etc.), SelectFromList is a better
+  option.
+
   See also:
     :help confirm() in vim (Note that vim uses 1-based indexes)
 
@@ -478,6 +487,58 @@ def Confirm( message ):
   return bool( PresentDialog( message, [ "Ok", "Cancel" ] ) == 0 )
 
 
+def SelectFromList( prompt, items ):
+  """Ask the user to select an item from the list |items|.
+
+  Presents the user with |prompt| followed by a numbered list of |items|,
+  from which they select one. The user is asked to enter the number of an
+  item or click it.
+
+  |items| should not contain leading ordinals: they are added automatically.
+
+  Returns the 0-based index in the list |items| that the user selected, or a
+  negative number if no valid item was selected.
+
+  See also :help inputlist()."""
+
+  vim_items = [ prompt ]
+  vim_items.extend( [ "{0}: {1}".format( i + 1, item )
+                      for i, item in enumerate( items ) ] )
+
+  # The vim documentation warns not to present lists larger than the number of
+  # lines of display. This is sound advice, but there really isn't any sensible
+  # thing we can do in that scenario. Testing shows that Vim just pages the
+  # message; that behaviour is as good as any, so we don't manipulate the list,
+  # or attempt to page it.
+
+  # For an explanation of the purpose of inputsave() / inputrestore(),
+  # see :help input(). Briefly, it makes inputlist() work as part of a mapping.
+  vim.eval( 'inputsave()' )
+  try:
+    # Vim returns the number the user entered, or the line number the user
+    # clicked. This may be wildly out of range for our list. It might even be
+    # negative.
+    #
+    # The first item is index 0, and this maps to our "prompt", so we subtract 1
+    # from the result and return that, assuming it is within the range of the
+    # supplied list. If not, we return negative.
+    #
+    # See :help input() for explanation of the use of inputsave() and inpput
+    # restore(). It is done in try/finally in case vim.eval ever throws an
+    # exception (such as KeyboardInterrupt)
+    selected = int( vim.eval( "inputlist( "
+                              + json.dumps( vim_items )
+                              + " )" ) ) - 1
+  finally:
+    vim.eval( 'inputrestore()' )
+
+  if selected < 0 or selected >= len( items ):
+    # User selected something outside of the range
+    raise RuntimeError( NO_SELECTION_MADE_MSG )
+
+  return selected
+
+
 def EchoText( text, log_as_message = True ):
   def EchoLine( text ):
     command = 'echom' if log_as_message else 'echo'