Prioritize errors over warnings on the same line

python/ycm/diagnostic_interface.py

@@ -108,8 +108,9 @@ class DiagnosticInterface( object ):
         self._diag_message_needs_clearing = False
       return
 
-    text = diags[ 0 ][ 'text' ]
+    first_diag = diags[ 0 ]
-    if diags[ 0 ].get( 'fixit_available', False ):
+    text = first_diag[ 'text' ]
+    if first_diag.get( 'fixit_available', False ):
       text += ' (FixIt)'
 
     vimsupport.PostVimMessage( text, warning = False, truncate = True )
@@ -132,7 +133,8 @@ def _UpdateSquiggles( buffer_number_to_line_to_diags ):
   line_to_diags = buffer_number_to_line_to_diags[ vim.current.buffer.number ]
 
   for diags in itervalues( line_to_diags ):
-    for diag in diags:
+    # Insert squiggles in reverse order so that errors overlap warnings.
+    for diag in reversed( diags ):
       location_extent = diag[ 'location_extent' ]
       is_error = _DiagnosticIsError( diag )
 
@@ -201,23 +203,23 @@ def _GetKeptAndNewSigns( placed_signs, buffer_number_to_line_to_diags,
       continue
 
     for line, diags in iteritems( line_to_diags ):
-      for diag in diags:
+      # Only one sign is visible by line.
-        sign = _DiagSignPlacement( next_sign_id,
+      first_diag = diags[ 0 ]
-                                   line,
+      sign = _DiagSignPlacement( next_sign_id,
-                                   buffer_number,
+                                 line,
-                                   _DiagnosticIsError( diag ) )
+                                 buffer_number,
-        if sign not in placed_signs:
+                                 _DiagnosticIsError( first_diag ) )
-          new_signs += [ sign ]
+      if sign not in placed_signs:
-          next_sign_id += 1
+        new_signs.append( sign )
-        else:
+        next_sign_id += 1
-          # We use .index here because `sign` contains a new id, but
+      else:
-          # we need the sign with the old id to unplace it later on.
+        # We use .index here because `sign` contains a new id, but
-          # We won't be placing the new sign.
+        # we need the sign with the old id to unplace it later on.
-          kept_signs += [ placed_signs[ placed_signs.index( sign ) ] ]
+        # We won't be placing the new sign.
+        kept_signs.append( placed_signs[ placed_signs.index( sign ) ] )
   return new_signs, kept_signs, next_sign_id
 
 
-
 def _PlaceNewSigns( kept_signs, new_signs ):
   placed_signs = kept_signs[:]
   for sign in new_signs:
@@ -226,7 +228,7 @@ def _PlaceNewSigns( kept_signs, new_signs ):
     if sign in placed_signs:
       continue
     vimsupport.PlaceSign( sign.id, sign.line, sign.buffer, sign.is_error )
-    placed_signs.append(sign)
+    placed_signs.append( sign )
   return placed_signs
 
 
@@ -247,10 +249,10 @@ def _ConvertDiagListToDict( diag_list ):
 
   for line_to_diags in itervalues( buffer_to_line_to_diags ):
     for diags in itervalues( line_to_diags ):
-      # We also want errors to be listed before warnings so that errors aren't
+      # We want errors to be listed before warnings so that errors aren't hidden
-      # hidden by the warnings; Vim won't place a sign oven an existing one.
+      # by the warnings.
-      diags.sort( key = lambda diag: ( diag[ 'location' ][ 'column_num' ],
+      diags.sort( key = lambda diag: ( diag[ 'kind' ],
-                                       diag[ 'kind' ] ) )
+                                       diag[ 'location' ][ 'column_num' ] ) )
   return buffer_to_line_to_diags
 
 

python/ycm/tests/event_notification_test.py

@@ -48,8 +48,8 @@ def PresentDialog_Confirm_Call( message ):
 
 def PlaceSign_Call( sign_id, line_num, buffer_num, is_error ):
   sign_name = 'YcmError' if is_error else 'YcmWarning'
-  return call( 'sign place {0} line={1} name={2} buffer={3}'
+  return call( 'sign place {0} name={1} line={2} buffer={3}'
-                  .format( sign_id, line_num, sign_name, buffer_num ) )
+                  .format( sign_id, sign_name, line_num, buffer_num ) )
 
 
 def UnplaceSign_Call( sign_id, buffer_num ):

python/ycm/tests/test_utils.py

@@ -41,6 +41,9 @@ BWIPEOUT_REGEX = re.compile(
   '^(?:silent! )bwipeout!? (?P<buffer_number>[0-9]+)$' )
 GETBUFVAR_REGEX = re.compile(
   '^getbufvar\((?P<buffer_number>[0-9]+), "&(?P<option>.+)"\)$' )
+MATCHADD_REGEX = re.compile(
+  '^matchadd\(\'(?P<group>.+)\', \'(?P<pattern>.+)\'\)$' )
+MATCHDELETE_REGEX = re.compile( '^matchdelete\((?P<id>)\)$' )
 
 # 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,
@@ -52,6 +55,8 @@ GETBUFVAR_REGEX = re.compile(
 # https://github.com/Valloric/YouCompleteMe/pull/1694
 VIM_MOCK = MagicMock()
 
+VIM_MATCHES = []
+
 
 @contextlib.contextmanager
 def CurrentWorkingDirectory( path ):
@@ -130,6 +135,30 @@ def _MockVimOptionsEval( value ):
   return None
 
 
+def _MockVimMatchEval( value ):
+  if value == 'getmatches()':
+    return VIM_MATCHES
+
+  match = MATCHADD_REGEX.search( value )
+  if match:
+    group = match.group( 'group' )
+    option = match.group( 'pattern' )
+    vim_match = VimMatch( group, option )
+    VIM_MATCHES.append( vim_match )
+    return vim_match.id
+
+  match = MATCHDELETE_REGEX.search( value )
+  if match:
+    identity = match.group( 'id' )
+    for index, vim_match in enumerate( VIM_MATCHES ):
+      if vim_match.id == identity:
+        VIM_MATCHES.pop( index )
+        return -1
+    return 0
+
+  return None
+
+
 def _MockVimEval( value ):
   if value == 'g:ycm_min_num_of_chars_for_completion':
     return 0
@@ -154,6 +183,10 @@ def _MockVimEval( value ):
   if result is not None:
     return result
 
+  result = _MockVimMatchEval( value )
+  if result is not None:
+    return result
+
   raise ValueError( 'Unexpected evaluation: {0}'.format( value ) )
 
 
@@ -217,6 +250,23 @@ class VimBuffer( object ):
     return [ ToUnicode( x ) for x in self.contents ]
 
 
+class VimMatch( object ):
+
+  def __init__( self, group, pattern ):
+    self.id = len( VIM_MATCHES )
+    self.group = group
+    self.pattern = pattern
+
+
+  def __eq__( self, other ):
+    return self.group == other.group and self.pattern == other.pattern
+
+
+  def __repr__( self ):
+    return "VimMatch( group = '{0}', pattern = '{1}' )".format( self.group,
+                                                                self.pattern )
+
+
 @contextlib.contextmanager
 def MockVimBuffers( buffers, current_buffer, cursor_position = ( 1, 1 ) ):
   """Simulates the Vim buffers list |buffers| where |current_buffer| is the

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 )
+                                   VimBuffer, VimMatch )
 MockVimModule()
 
 import os
@@ -32,7 +32,7 @@ from hamcrest import ( assert_that, contains, empty, is_in, is_not, has_length,
                        matches_regexp )
 from mock import call, MagicMock, patch
 
-from ycm.tests import StopServer, YouCompleteMeInstance
+from ycm.tests import StopServer, test_utils, YouCompleteMeInstance
 from ycmd.responses import ServerError
 
 
@@ -348,8 +348,8 @@ def YouCompleteMe_ShowDiagnostics_DiagnosticsFound_DoNotOpenLocationList_test(
     'text': 'error text',
     'location': {
       'filepath': 'buffer',
-      'column_num': 2,
+      'line_num': 19,
-      'line_num': 19
+      'column_num': 2
     }
   }
 
@@ -388,8 +388,8 @@ def YouCompleteMe_ShowDiagnostics_DiagnosticsFound_OpenLocationList_test(
     'text': 'error text',
     'location': {
       'filepath': 'buffer',
-      'column_num': 2,
+      'line_num': 19,
-      'line_num': 19
+      'column_num': 2
     }
   }
 
@@ -413,3 +413,120 @@ def YouCompleteMe_ShowDiagnostics_DiagnosticsFound_OpenLocationList_test(
       'valid': 1
   } ] )
   open_location_list.assert_called_once_with( focus = True )
+
+
+@YouCompleteMeInstance( { 'echo_current_diagnostic': 1,
+                          'enable_diagnostic_signs': 1,
+                          'enable_diagnostic_highlighting': 1 } )
+@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 ):
+
+  contents = """int main() {
+  int x, y;
+  x == y
+}"""
+
+  # List of diagnostics returned by ycmd for the above code.
+  diagnostics = [ {
+    'kind': 'ERROR',
+    'text': "expected ';' after expression",
+    'location': {
+      'filepath': 'buffer',
+      'line_num': 3,
+      'column_num': 9
+    },
+    # Looks strange but this is really what ycmd is returning.
+    'location_extent': {
+      'start': {
+        'filepath': '',
+        'line_num': 0,
+        'column_num': 0,
+      },
+      'end': {
+        'filepath': '',
+        'line_num': 0,
+        'column_num': 0,
+      }
+    },
+    'ranges': [],
+    'fixit_available': True
+  }, {
+    'kind': 'WARNING',
+    'text': 'equality comparison result unused',
+    'location': {
+      'filepath': 'buffer',
+      'line_num': 3,
+      'column_num': 7,
+    },
+    'location_extent': {
+      'start': {
+        'filepath': 'buffer',
+        'line_num': 3,
+        'column_num': 5,
+      },
+      'end': {
+        'filepath': 'buffer',
+        'line_num': 3,
+        'column_num': 7,
+      }
+    },
+    'ranges': [ {
+      'start': {
+        'filepath': 'buffer',
+        'line_num': 3,
+        'column_num': 3,
+      },
+      'end': {
+        'filepath': 'buffer',
+        'line_num': 3,
+        'column_num': 9,
+      }
+    } ],
+    'fixit_available': True
+  } ]
+
+  current_buffer = VimBuffer( 'buffer',
+                              filetype = 'c',
+                              contents = contents.splitlines(),
+                              number = 5,
+                              window = 2 )
+
+  test_utils.VIM_MATCHES = []
+
+  with MockVimBuffers( [ current_buffer ], current_buffer, ( 3, 1 ) ):
+    with patch( 'ycm.client.event_notification.EventNotification.Response',
+                return_value = diagnostics ):
+      ycm.OnFileReadyToParse()
+      ycm.HandleFileParseRequest( block = True )
+
+    # Error match is added after warning matches.
+    assert_that(
+      test_utils.VIM_MATCHES,
+      contains(
+        VimMatch( 'YcmWarningSection', '\%3l\%5c\_.\{-}\%3l\%7c' ),
+        VimMatch( 'YcmWarningSection', '\%3l\%3c\_.\{-}\%3l\%9c' ),
+        # FIXME: match should be inserted at the end of line 3 (missing ";").
+        VimMatch( 'YcmErrorSection', '\%0l\%0c' )
+      )
+    )
+
+    # Only the error sign is placed.
+    vim_command.assert_has_exact_calls( [
+      call( 'sign define ycm_dummy_sign' ),
+      call( 'sign place 3 name=ycm_dummy_sign line=3 buffer=5' ),
+      call( 'sign place 1 name=YcmError line=3 buffer=5' ),
+      call( 'sign undefine ycm_dummy_sign' ),
+      call( 'sign unplace 3 buffer=5' )
+    ] )
+
+    # When moving the cursor on the diagnostics, the error is displayed to the
+    # user, not the warning.
+    ycm.OnCursorMoved()
+    post_vim_message.assert_has_exact_calls( [
+      call( "expected ';' after expression (FixIt)",
+            truncate = True, warning = False )
+    ] )

python/ycm/vimsupport.py

@@ -175,8 +175,8 @@ def PlaceSign( sign_id, line_num, buffer_num, is_error = True ):
     line_num = 1
 
   sign_name = 'YcmError' if is_error else 'YcmWarning'
-  vim.command( 'sign place {0} line={1} name={2} buffer={3}'.format(
+  vim.command( 'sign place {0} name={1} line={2} buffer={3}'.format(
-    sign_id, line_num, sign_name, buffer_num ) )
+    sign_id, sign_name, line_num, buffer_num ) )
 
 
 def PlaceDummySign( sign_id, buffer_num, line_num ):