Auto merge of #2915 - micbou:improve-diagnostic-sign-placement, r=bstaletic

[READY] Improve diagnostic sign placement

Currently, we store internally the signs that we place in the buffer so that when we receive a new batch of diagnostics, we can efficiently add and remove signs by looking at the line positions of the stored signs. However, there is a flaw in that approach. When lines are added or removed to the buffer, Vim update the lines of the signs accordingly and thus our stored signs may not correspond anymore to the actual signs. Here's an example of the issue:

![sign-diagnostic-issue](https://user-images.githubusercontent.com/10026824/36215183-af078aea-11ab-11e8-9827-0e7ab6e4b7b5.gif)
*Where did my signs go? I want a refund!*

The solution is to not store the signs but instead get them directly from Vim each time we need to update them. This is done by parsing the output of the `:sign place` command (Vim doesn't provide an API to properly do that). The same example as above with the proposed changes:

![sign-diagnostic-fix](https://user-images.githubusercontent.com/10026824/36215595-ce3916ee-11ac-11e8-8625-d44dc6c8bd1c.gif)
*Oh, they were there.*

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

python/ycm/diagnostic_interface.py

@@ -23,7 +23,7 @@ from __future__ import absolute_import
 from builtins import *  # noqa
 
 from future.utils import itervalues, iteritems
-from collections import defaultdict, namedtuple
+from collections import defaultdict
 from ycm import vimsupport
 from ycm.diagnostic_filter import DiagnosticFilter, CompileLevel
 
@@ -36,8 +36,7 @@ class DiagnosticInterface( object ):
     self._diag_filter = DiagnosticFilter.CreateFromOptions( user_options )
     # Line and column numbers are 1-based
     self._line_to_diags = defaultdict( list )
-    self._placed_signs = []
+    self._next_sign_id = vimsupport.SIGN_BUFFER_ID_INITIAL_VALUE
-    self._next_sign_id = 1
     self._previous_diag_line_number = -1
     self._diag_message_needs_clearing = False
 
@@ -162,43 +161,29 @@ class DiagnosticInterface( object ):
 
 
   def _UpdateSigns( self ):
-    new_signs, obsolete_signs = self._GetNewAndObsoleteSigns()
+    signs_to_unplace = vimsupport.GetSignsInBuffer( self._bufnr )
 
-    self._PlaceNewSigns( new_signs )
-
-    self._UnplaceObsoleteSigns( obsolete_signs )
-
-
-  def _GetNewAndObsoleteSigns( self ):
-    new_signs = []
-    obsolete_signs = list( self._placed_signs )
     for line, diags in iteritems( self._line_to_diags ):
       if not diags:
         continue
 
-      # We always go for the first diagnostic on line,
+      # We always go for the first diagnostic on the line because diagnostics
-      # because it is sorted giving priority to the Errors.
+      # are sorted by errors in priority and Vim can only display one sign by
-      diag = diags[ 0 ]
+      # line.
-      sign = _DiagSignPlacement( self._next_sign_id,
+      name = 'YcmError' if _DiagnosticIsError( diags[ 0 ] ) else 'YcmWarning'
-                                 line, _DiagnosticIsError( diag ) )
+      sign = vimsupport.DiagnosticSign( self._next_sign_id,
+                                        line,
+                                        name,
+                                        self._bufnr )
+
       try:
-        obsolete_signs.remove( sign )
+        signs_to_unplace.remove( sign )
       except ValueError:
-        new_signs.append( sign )
+        vimsupport.PlaceSign( sign )
         self._next_sign_id += 1
-    return new_signs, obsolete_signs
 
-
+    for sign in signs_to_unplace:
-  def _PlaceNewSigns( self, new_signs ):
+      vimsupport.UnplaceSign( sign )
-    for sign in new_signs:
-      vimsupport.PlaceSign( sign.id, sign.line, self._bufnr, sign.is_error )
-      self._placed_signs.append( sign )
-
-
-  def _UnplaceObsoleteSigns( self, obsolete_signs ):
-    for sign in obsolete_signs:
-      self._placed_signs.remove( sign )
-      vimsupport.UnplaceSignInBuffer( self._bufnr, sign.id )
 
 
   def _ConvertDiagListToDict( self ):
@@ -229,12 +214,3 @@ def _NormalizeDiagnostic( diag ):
   location[ 'column_num' ] = ClampToOne( location[ 'column_num' ] )
   location[ 'line_num' ] = ClampToOne( location[ 'line_num' ] )
   return diag
-
-
-class _DiagSignPlacement( namedtuple( "_DiagSignPlacement",
-                                      [ 'id', 'line', 'is_error' ] ) ):
-  # We want two signs that have different ids but the same location to compare
-  # equal. ID doesn't matter.
-  def __eq__( self, other ):
-    return ( self.line == other.line and
-             self.is_error == other.is_error )

python/ycm/tests/event_notification_test.py

@@ -25,18 +25,21 @@ from __future__ import absolute_import
 from builtins import *  # noqa
 
 from ycm.tests.test_utils import ( CurrentWorkingDirectory, ExtendedMock,
-                                   MockVimBuffers, MockVimModule, VimBuffer )
+                                   MockVimBuffers, MockVimModule, VimBuffer,
+                                   VimSign )
 MockVimModule()
 
 import contextlib
 import os
 
-from ycm.tests import PathToTestFile, YouCompleteMeInstance, WaitUntilReady
+from ycm.tests import ( PathToTestFile, test_utils, YouCompleteMeInstance,
+                        WaitUntilReady )
+from ycm.vimsupport import SIGN_BUFFER_ID_INITIAL_VALUE
 from ycmd.responses import ( BuildDiagnosticData, Diagnostic, Location, Range,
                              UnknownExtraConf, ServerError )
 
-from hamcrest import ( assert_that, contains, has_entries, has_entry, has_item,
+from hamcrest import ( assert_that, contains, empty, has_entries, has_entry,
-                       has_items, has_key, is_not )
+                       has_item, has_items, has_key, is_not )
 from mock import call, MagicMock, patch
 from nose.tools import eq_, ok_
 
@@ -47,17 +50,6 @@ def PresentDialog_Confirm_Call( message ):
   return call( message, [ 'Ok', 'Cancel' ] )
 
 
-def PlaceSign_Call( sign_id, line_num, buffer_num, is_error ):
-  sign_name = 'YcmError' if is_error else 'YcmWarning'
-  return call( 'sign place {0} name={1} line={2} buffer={3}'
-                  .format( sign_id, sign_name, line_num, buffer_num ) )
-
-
-def UnplaceSign_Call( sign_id, buffer_num ):
-  return call( 'try | exec "sign unplace {0} buffer={1}" |'
-               ' catch /E158/ | endtry'.format( sign_id, buffer_num ) )
-
-
 @contextlib.contextmanager
 def MockArbitraryBuffer( filetype ):
   """Used via the with statement, set up a single buffer with an arbitrary name
@@ -148,16 +140,25 @@ def EventNotification_FileReadyToParse_NonDiagnostic_Error_test(
       ] )
 
 
-@patch( 'vim.command' )
 @YouCompleteMeInstance()
 def EventNotification_FileReadyToParse_NonDiagnostic_Error_NonNative_test(
-    ycm, vim_command ):
+  ycm ):
+
+  test_utils.VIM_MATCHES = []
+  test_utils.VIM_SIGNS = []
 
   with MockArbitraryBuffer( 'javascript' ):
     with MockEventNotification( None, False ):
       ycm.OnFileReadyToParse()
       ycm.HandleFileParseRequest()
-      vim_command.assert_not_called()
+      assert_that(
+        test_utils.VIM_MATCHES,
+        contains()
+      )
+      assert_that(
+        test_utils.VIM_SIGNS,
+        contains()
+      )
 
 
 @patch( 'ycm.client.base_request._LoadExtraConfFile',
@@ -263,13 +264,14 @@ def EventNotification_FileReadyToParse_NonDiagnostic_ConfirmExtraConf_test(
 
 @YouCompleteMeInstance()
 def EventNotification_FileReadyToParse_Diagnostic_Error_Native_test( ycm ):
+  test_utils.VIM_SIGNS = []
+
   _Check_FileReadyToParse_Diagnostic_Error( ycm )
   _Check_FileReadyToParse_Diagnostic_Warning( ycm )
   _Check_FileReadyToParse_Diagnostic_Clean( ycm )
 
 
-@patch( 'vim.command' )
+def _Check_FileReadyToParse_Diagnostic_Error( ycm ):
-def _Check_FileReadyToParse_Diagnostic_Error( ycm, vim_command ):
   # Tests Vim sign placement and error/warning count python API
   # when one error is returned.
   def DiagnosticResponse( *args ):
@@ -284,23 +286,42 @@ def _Check_FileReadyToParse_Diagnostic_Error( ycm, vim_command ):
       ycm.OnFileReadyToParse()
       ok_( ycm.FileParseRequestReady() )
       ycm.HandleFileParseRequest()
-      vim_command.assert_has_calls( [
+      assert_that(
-        PlaceSign_Call( 1, 1, 1, True )
+        test_utils.VIM_SIGNS,
-      ] )
+        contains(
+          VimSign( SIGN_BUFFER_ID_INITIAL_VALUE, 1, 'YcmError', 1 )
+        )
+      )
       eq_( ycm.GetErrorCount(), 1 )
       eq_( ycm.GetWarningCount(), 0 )
 
       # Consequent calls to HandleFileParseRequest shouldn't mess with
       # existing diagnostics, when there is no new parse request.
-      vim_command.reset_mock()
       ycm.HandleFileParseRequest()
-      vim_command.assert_not_called()
+      assert_that(
+        test_utils.VIM_SIGNS,
+        contains(
+          VimSign( SIGN_BUFFER_ID_INITIAL_VALUE, 1, 'YcmError', 1 )
+        )
+      )
+      eq_( ycm.GetErrorCount(), 1 )
+      eq_( ycm.GetWarningCount(), 0 )
+
+      # New identical requests should result in the same diagnostics.
+      ycm.OnFileReadyToParse()
+      ok_( ycm.FileParseRequestReady() )
+      ycm.HandleFileParseRequest()
+      assert_that(
+        test_utils.VIM_SIGNS,
+        contains(
+          VimSign( SIGN_BUFFER_ID_INITIAL_VALUE, 1, 'YcmError', 1 )
+        )
+      )
       eq_( ycm.GetErrorCount(), 1 )
       eq_( ycm.GetWarningCount(), 0 )
 
 
-@patch( 'vim.command' )
+def _Check_FileReadyToParse_Diagnostic_Warning( ycm ):
-def _Check_FileReadyToParse_Diagnostic_Warning( ycm, vim_command ):
   # Tests Vim sign placement/unplacement and error/warning count python API
   # when one warning is returned.
   # Should be called after _Check_FileReadyToParse_Diagnostic_Error
@@ -316,24 +337,29 @@ def _Check_FileReadyToParse_Diagnostic_Warning( ycm, vim_command ):
       ycm.OnFileReadyToParse()
       ok_( ycm.FileParseRequestReady() )
       ycm.HandleFileParseRequest()
-      vim_command.assert_has_calls( [
+      assert_that(
-        PlaceSign_Call( 2, 2, 1, False ),
+        test_utils.VIM_SIGNS,
-        UnplaceSign_Call( 1, 1 )
+        contains(
-      ] )
+          VimSign( SIGN_BUFFER_ID_INITIAL_VALUE + 1, 2, 'YcmWarning', 1 )
+        )
+      )
       eq_( ycm.GetErrorCount(), 0 )
       eq_( ycm.GetWarningCount(), 1 )
 
       # Consequent calls to HandleFileParseRequest shouldn't mess with
       # existing diagnostics, when there is no new parse request.
-      vim_command.reset_mock()
       ycm.HandleFileParseRequest()
-      vim_command.assert_not_called()
+      assert_that(
+        test_utils.VIM_SIGNS,
+        contains(
+          VimSign( SIGN_BUFFER_ID_INITIAL_VALUE + 1, 2, 'YcmWarning', 1 )
+        )
+      )
       eq_( ycm.GetErrorCount(), 0 )
       eq_( ycm.GetWarningCount(), 1 )
 
 
-@patch( 'vim.command' )
+def _Check_FileReadyToParse_Diagnostic_Clean( ycm ):
-def _Check_FileReadyToParse_Diagnostic_Clean( ycm, vim_command ):
   # Tests Vim sign unplacement and error/warning count python API
   # when there are no errors/warnings left.
   # Should be called after _Check_FileReadyToParse_Diagnostic_Warning
@@ -341,9 +367,10 @@ def _Check_FileReadyToParse_Diagnostic_Clean( ycm, vim_command ):
     with MockEventNotification( MagicMock( return_value = [] ) ):
       ycm.OnFileReadyToParse()
       ycm.HandleFileParseRequest()
-      vim_command.assert_has_calls( [
+      assert_that(
-        UnplaceSign_Call( 2, 1 )
+        test_utils.VIM_SIGNS,
-      ] )
+        empty()
+      )
       eq_( ycm.GetErrorCount(), 0 )
       eq_( ycm.GetWarningCount(), 0 )
 

python/ycm/tests/test_utils.py

@@ -23,7 +23,7 @@ from __future__ import absolute_import
 from builtins import *  # noqa
 
 from future.utils import PY2
-from mock import MagicMock, patch
+from mock import DEFAULT, MagicMock, patch
 from hamcrest import assert_that, equal_to
 import contextlib
 import functools
@@ -47,6 +47,15 @@ MATCHDELETE_REGEX = re.compile( '^matchdelete\((?P<id>\d+)\)$' )
 OMNIFUNC_REGEX_FORMAT = (
   '^{omnifunc_name}\((?P<findstart>[01]),[\'"](?P<base>.*)[\'"]\)$' )
 FNAMEESCAPE_REGEX = re.compile( '^fnameescape\(\'(?P<filepath>.+)\'\)$' )
+SIGN_LIST_REGEX = re.compile(
+  "^silent sign place buffer=(?P<bufnr>\d+)$" )
+SIGN_PLACE_REGEX = re.compile(
+  '^sign place (?P<id>\d+) name=(?P<name>\w+) line=(?P<line>\d+) '
+  'buffer=(?P<bufnr>\d+)$' )
+SIGN_UNPLACE_REGEX = re.compile(
+  '^sign unplace (?P<id>\d+) buffer=(?P<bufnr>\d+)$' )
+REDIR_START_REGEX = re.compile( '^redir => (?P<variable>[\w:]+)$' )
+REDIR_END_REGEX = re.compile( '^redir END$' )
 
 # One-and only instance of mocked Vim object. The first 'import vim' that is
 # executed binds the vim module to the instance of MagicMock that is created,
@@ -59,6 +68,13 @@ FNAMEESCAPE_REGEX = re.compile( '^fnameescape\(\'(?P<filepath>.+)\'\)$' )
 VIM_MOCK = MagicMock()
 
 VIM_MATCHES = []
+VIM_SIGNS = []
+
+REDIR = {
+  'status': False,
+  'variable': '',
+  'output': ''
+}
 
 
 @contextlib.contextmanager
@@ -158,6 +174,19 @@ def _MockVimOptionsEval( value ):
   return None
 
 
+def _MockVimFunctionsEval( value ):
+  if value == 'tempname()':
+    return '_TEMP_FILE_'
+
+  if value == 'tagfiles()':
+    return [ 'tags' ]
+
+  if value == 'shiftwidth()':
+    return 2
+
+  return None
+
+
 def _MockVimMatchEval( value ):
   if value == 'getmatches()':
     # Returning a copy, because ClearYcmSyntaxMatches() gets the result of
@@ -196,14 +225,9 @@ def _MockVimEval( value ):
   if value == 'g:ycm_server_python_interpreter':
     return server_python_interpreter
 
-  if value == 'tempname()':
+  result = _MockVimFunctionsEval( value )
-    return '_TEMP_FILE_'
+  if result is not None:
-
+    return result
-  if value == 'tagfiles()':
-    return [ 'tags' ]
-
-  if value == 'shiftwidth()':
-    return 2
 
   result = _MockVimOptionsEval( value )
   if result is not None:
@@ -221,6 +245,9 @@ def _MockVimEval( value ):
   if match:
     return match.group( 'filepath' )
 
+  if value == REDIR[ 'variable' ]:
+    return REDIR[ 'output' ]
+
   raise VimError( 'Unexpected evaluation: {0}'.format( value ) )
 
 
@@ -232,12 +259,65 @@ def _MockWipeoutBuffer( buffer_number ):
       return buffers.pop( index )
 
 
-def MockVimCommand( command ):
+def _MockSignCommand( command ):
+  match = SIGN_LIST_REGEX.search( command )
+  if match and REDIR[ 'status' ]:
+    bufnr = int( match.group( 'bufnr' ) )
+    REDIR[ 'output' ] = ( '--- Signs ---\n'
+                          'Signs for foo:\n' )
+    for sign in VIM_SIGNS:
+      if sign.bufnr == bufnr:
+        REDIR[ 'output' ] += (
+          '    line={0}  id={1}  name={2}'.format( sign.line,
+                                                   sign.id,
+                                                   sign.name ) )
+    return True
+
+  match = SIGN_PLACE_REGEX.search( command )
+  if match:
+    VIM_SIGNS.append( VimSign( int( match.group( 'id' ) ),
+                               int( match.group( 'line' ) ),
+                               match.group( 'name' ),
+                               int( match.group( 'bufnr' ) ) ) )
+    return True
+
+  match = SIGN_UNPLACE_REGEX.search( command )
+  if match:
+    sign_id = int( match.group( 'id' ) )
+    bufnr = int( match.group( 'bufnr' ) )
+    for sign in VIM_SIGNS:
+      if sign.id == sign_id and sign.bufnr == bufnr:
+        VIM_SIGNS.remove( sign )
+        return True
+
+  return False
+
+
+def _MockVimCommand( command ):
   match = BWIPEOUT_REGEX.search( command )
   if match:
     return _MockWipeoutBuffer( int( match.group( 1 ) ) )
 
-  raise RuntimeError( 'Unexpected command: ' + command )
+  match = REDIR_START_REGEX.search( command )
+  if match:
+    REDIR[ 'status' ] = True
+    REDIR[ 'variable' ] = match.group( 'variable' )
+    return
+
+  match = REDIR_END_REGEX.search( command )
+  if match:
+    REDIR[ 'status' ] = False
+    return
+
+  if command == 'unlet ' + REDIR[ 'variable' ]:
+    REDIR[ 'variable' ] = ''
+    return
+
+  result = _MockSignCommand( command )
+  if result:
+    return
+
+  return DEFAULT
 
 
 class VimBuffer( object ):
@@ -318,7 +398,7 @@ class VimBuffers( object ):
 
   def __init__( self, *buffers ):
     """Arguments are VimBuffer objects."""
-    self._buffers = buffers
+    self._buffers = list( buffers )
 
 
   def __getitem__( self, number ):
@@ -334,6 +414,10 @@ class VimBuffers( object ):
     return iter( self._buffers )
 
 
+  def pop( self, index ):
+    return self._buffers.pop( index )
+
+
 class VimMatch( object ):
 
   def __init__( self, group, pattern ):
@@ -358,6 +442,37 @@ class VimMatch( object ):
       return self.id
 
 
+class VimSign( object ):
+
+  def __init__( self, sign_id, line, name, bufnr ):
+    self.id = sign_id
+    self.line = line
+    self.name = name
+    self.bufnr = bufnr
+
+
+  def __eq__( self, other ):
+    return ( self.id == other.id and
+             self.line == other.line and
+             self.name == other.name and
+             self.bufnr == other.bufnr )
+
+
+  def __repr__( self ):
+    return ( "VimSign( id = {0}, line = {1}, "
+                      "name = '{2}', bufnr = {3} )".format( self.id,
+                                                            self.line,
+                                                            self.name,
+                                                            self.bufnr ) )
+
+
+  def __getitem__( self, key ):
+    if key == 'group':
+      return self.group
+    elif key == 'id':
+      return self.id
+
+
 @contextlib.contextmanager
 def MockVimBuffers( buffers, current_buffer, cursor_position = ( 1, 1 ) ):
   """Simulates the Vim buffers list |buffers| where |current_buffer| is the
@@ -398,6 +513,7 @@ def MockVimModule():
   tests."""
 
   VIM_MOCK.buffers = {}
+  VIM_MOCK.command = MagicMock( side_effect = _MockVimCommand )
   VIM_MOCK.eval = MagicMock( side_effect = _MockVimEval )
   VIM_MOCK.error = VimError
   sys.modules[ 'vim' ] = VIM_MOCK

python/ycm/tests/vimsupport_test.py

@@ -26,13 +26,14 @@ from builtins import *  # noqa
 
 from ycm.tests import PathToTestFile
 from ycm.tests.test_utils import ( CurrentWorkingDirectory, ExtendedMock,
-                                   MockVimBuffers, MockVimCommand,
+                                   MockVimBuffers, MockVimModule, VimBuffer,
-                                   MockVimModule, VimBuffer, VimError )
+                                   VimError )
 MockVimModule()
 
 from ycm import vimsupport
 from nose.tools import eq_
-from hamcrest import assert_that, calling, contains, equal_to, has_entry, raises
+from hamcrest import ( assert_that, calling, contains, empty, equal_to,
+                       has_entry, raises )
 from mock import MagicMock, call, patch
 from ycmd.utils import ToBytes
 import os
@@ -1373,22 +1374,16 @@ def BufferIsVisibleForFilename_test():
     eq_( vimsupport.BufferIsVisibleForFilename( 'another_filename' ), False )
 
 
-@patch( 'vim.command',
+def CloseBuffersForFilename_test():
-        side_effect = MockVimCommand,
-        new_callable = ExtendedMock )
-def CloseBuffersForFilename_test( vim_command, *args ):
   vim_buffers = [
     VimBuffer( 'some_filename', number = 2 ),
     VimBuffer( 'some_filename', number = 5 )
   ]
 
-  with patch( 'vim.buffers', vim_buffers ):
+  with MockVimBuffers( vim_buffers, vim_buffers[ 0 ] ) as vim:
     vimsupport.CloseBuffersForFilename( 'some_filename' )
 
-  vim_command.assert_has_exact_calls( [
+  assert_that( vim.buffers, empty() )
-    call( 'silent! bwipeout! 2' ),
-    call( 'silent! bwipeout! 5' )
-  ], any_order = True )
 
 
 @patch( 'vim.command', new_callable = ExtendedMock )

python/ycm/tests/youcompleteme_test.py

@@ -23,7 +23,7 @@ from __future__ import absolute_import
 from builtins import *  # noqa
 
 from ycm.tests.test_utils import ( ExtendedMock, MockVimBuffers, MockVimModule,
-                                   VimBuffer, VimMatch )
+                                   VimBuffer, VimMatch, VimSign )
 MockVimModule()
 
 import os
@@ -33,6 +33,7 @@ from hamcrest import ( assert_that, contains, empty, equal_to, is_in, is_not,
 from mock import call, MagicMock, patch
 
 from ycm.paths import _PathToPythonUsedDuringBuild
+from ycm.vimsupport import SIGN_BUFFER_ID_INITIAL_VALUE
 from ycm.youcompleteme import YouCompleteMe
 from ycm.tests import ( MakeUserOptions, StopServer, test_utils,
                         WaitUntilReady, YouCompleteMeInstance )
@@ -517,9 +518,8 @@ def YouCompleteMe_ShowDiagnostics_DiagnosticsFound_OpenLocationList_test(
 @patch( 'ycm.youcompleteme.YouCompleteMe.FiletypeCompleterExistsForFiletype',
         return_value = True )
 @patch( 'ycm.vimsupport.PostVimMessage', new_callable = ExtendedMock )
-@patch( 'vim.command', new_callable = ExtendedMock )
 def YouCompleteMe_UpdateDiagnosticInterface_PrioritizeErrorsOverWarnings_test(
-  ycm, vim_command, post_vim_message, *args ):
+  ycm, post_vim_message, *args ):
 
   contents = """int main() {
   int x, y;
@@ -592,6 +592,7 @@ def YouCompleteMe_UpdateDiagnosticInterface_PrioritizeErrorsOverWarnings_test(
                               window = 2 )
 
   test_utils.VIM_MATCHES = []
+  test_utils.VIM_SIGNS = []
 
   with MockVimBuffers( [ current_buffer ], current_buffer, ( 3, 1 ) ):
     with patch( 'ycm.client.event_notification.EventNotification.Response',
@@ -615,9 +616,12 @@ def YouCompleteMe_UpdateDiagnosticInterface_PrioritizeErrorsOverWarnings_test(
     )
 
     # Only the error sign is placed.
-    vim_command.assert_has_exact_calls( [
+    assert_that(
-      call( 'sign place 1 name=YcmError line=3 buffer=5' ),
+      test_utils.VIM_SIGNS,
-    ] )
+      contains(
+        VimSign( SIGN_BUFFER_ID_INITIAL_VALUE, 3, 'YcmError', 5 )
+      )
+    )
 
   # The error is not echoed again when moving the cursor along the line.
   with MockVimBuffers( [ current_buffer ], current_buffer, ( 3, 2 ) ):
@@ -639,7 +643,6 @@ def YouCompleteMe_UpdateDiagnosticInterface_PrioritizeErrorsOverWarnings_test(
       "expected ';' after expression (FixIt)",
       truncate = True, warning = False )
 
-    vim_command.reset_mock()
     with patch( 'ycm.client.event_notification.EventNotification.Response',
                 return_value = diagnostics[ 1 : ] ):
       ycm.OnFileReadyToParse()
@@ -653,10 +656,12 @@ def YouCompleteMe_UpdateDiagnosticInterface_PrioritizeErrorsOverWarnings_test(
       )
     )
 
-    vim_command.assert_has_exact_calls( [
+    assert_that(
-      call( 'sign place 2 name=YcmWarning line=3 buffer=5' ),
+      test_utils.VIM_SIGNS,
-      call( 'try | exec "sign unplace 1 buffer=5" | catch /E158/ | endtry' )
+      contains(
-    ] )
+        VimSign( SIGN_BUFFER_ID_INITIAL_VALUE + 1, 3, 'YcmWarning', 5 )
+      )
+    )
 
 
 @YouCompleteMeInstance( { 'echo_current_diagnostic': 1,

python/ycm/vimsupport.py

@@ -27,7 +27,7 @@ import vim
 import os
 import json
 import re
-from collections import defaultdict
+from collections import defaultdict, namedtuple
 from ycmd.utils import ( ByteOffsetToCodepointOffset, GetCurrentDirectory,
                          JoinLinesAsUnicode, ToBytes, ToUnicode )
 from ycmd import user_options_store
@@ -45,6 +45,14 @@ FIXIT_OPENING_BUFFERS_MESSAGE_FORMAT = (
 
 NO_SELECTION_MADE_MSG = "No valid selection was made; aborting."
 
+# This is the starting value assigned to the sign's id of each buffer. This
+# value is then incremented for each new sign. This should prevent conflicts
+# with other plugins using signs.
+SIGN_BUFFER_ID_INITIAL_VALUE = 100000000
+
+SIGN_PLACE_REGEX = re.compile(
+  r"^.*=(?P<line>\d+).*=(?P<id>\d+).*=(?P<name>Ycm\w+)$" )
+
 
 def CurrentLineAndColumn():
   """Returns the 0-based current line and 0-based current column."""
@@ -163,23 +171,41 @@ def GetBufferChangedTick( bufnr ):
   return GetIntValue( 'getbufvar({0}, "changedtick")'.format( bufnr ) )
 
 
-def UnplaceSignInBuffer( buffer_number, sign_id ):
+class DiagnosticSign( namedtuple( 'DiagnosticSign',
-  if buffer_number < 0:
+                                  [ 'id', 'line', 'name', 'buffer_number' ] ) ):
-    return
+  # We want two signs that have different ids but the same location to compare
-  vim.command(
+  # equal. ID doesn't matter.
-    'try | exec "sign unplace {0} buffer={1}" | catch /E158/ | endtry'.format(
+  def __eq__( self, other ):
-        sign_id, buffer_number ) )
+    return ( self.line == other.line and
+             self.name == other.name and
+             self.buffer_number == other.buffer_number )
 
 
-def PlaceSign( sign_id, line_num, buffer_num, is_error = True ):
+def GetSignsInBuffer( buffer_number ):
-  # libclang can give us diagnostics that point "outside" the file; Vim borks
+  vim.command( 'redir => b:ycm_sign' )
-  # on these.
+  vim.command( 'silent sign place buffer={}'.format( buffer_number ) )
-  if line_num < 1:
+  vim.command( 'redir END' )
-    line_num = 1
+  sign_output = vim.eval( 'b:ycm_sign' )
+  vim.command( 'unlet b:ycm_sign' )
+  signs = []
+  for line in sign_output.split( '\n' ):
+    match = SIGN_PLACE_REGEX.search( line )
+    if match:
+      signs.append( DiagnosticSign( int( match.group( 'id' ) ),
+                                    int( match.group( 'line' ) ),
+                                    match.group( 'name' ),
+                                    buffer_number ) )
+  return signs
 
-  sign_name = 'YcmError' if is_error else 'YcmWarning'
+
+def UnplaceSign( sign ):
+  vim.command( 'sign unplace {0} buffer={1}'.format( sign.id,
+                                                     sign.buffer_number ) )
+
+
+def PlaceSign( sign ):
   vim.command( 'sign place {0} name={1} line={2} buffer={3}'.format(
-    sign_id, sign_name, line_num, buffer_num ) )
+    sign.id, sign.name, sign.line, sign.buffer_number ) )
 
 
 def ClearYcmSyntaxMatches():