Auto merge of #1876 - puremourning:fix-goto-references, r=micbou

[READY] Fix GoTo lists arriving at the wrong column.

## Problem

When a `GoTo` command returns a list of destinations, such as declarations and definitions in Python and references in JavaScript, we pop the Vim quick fix list with a list of destinations. however, we always jump to the character before the actual declaration/definition.

## Resolution

Vim's QuickFix lists require 1-based columns, which is what is returned from ycmd's commands, so don't subtract 1 from the column number.

As noted in the comments, the Vim documentation for `setqflist` is somewhat vague about this "byte offset", but it is confirmed to mean "1-based column number" both in testing and in `:help getqflist`.

## Manual testing

Tested this with the Tern completer (`:YcmCompleter GoToReferences`) and Jedi completer (`:YcmCompleter GoToDeclaration`). For the Jedi completer, use something like:

```python
x = 1
if False:
  x = 2
else:
  x = 3

print( str(x) )
```

Then `:YcmCompleter GoToDeclaration` on `x`. This returns a list, which pops up the quick fix window.

*Previous behaviour*

Previously, the jump would go to the character before the actual declaration (i.e. the `<space>` before `x` in each line)

## Automated testing

Just some simple execution of the changed code, proving that we don't take 1 from the column.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/1876)
<!-- Reviewable:end -->

python/ycm/client/command_request.py

@@ -135,5 +135,13 @@ def _BuildQfListItem( goto_data_item ):
   if 'line_num' in goto_data_item:
     qf_item[ 'lnum' ] = goto_data_item[ 'line_num' ]
   if 'column_num' in goto_data_item:
-    qf_item[ 'col' ] = goto_data_item[ 'column_num' ] - 1
+    # ycmd returns columns 1-based, and QuickFix lists require "byte offsets".
+    # See :help getqflist and equivalent comment in
+    # vimsupport.ConvertDiagnosticsToQfList.
+    #
+    # When the Vim help says "byte index", it really means "1-based column
+    # number" (which is somewhat confusing). :help getqflist states "first
+    # column is 1".
+    qf_item[ 'col' ] = goto_data_item[ 'column_num' ]
+
   return qf_item

python/ycm/client/tests/command_request_test.py

@@ -0,0 +1,91 @@
+#
+# Copyright (C) 2016 YouCompleteMe Contributors
+#
+# This file is part of YouCompleteMe.
+#
+# YouCompleteMe is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# YouCompleteMe is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
+
+from ycm.test_utils import MockVimModule
+MockVimModule()
+
+from mock import patch, call
+from ycm.client.command_request import CommandRequest
+
+
+class GoToResponse_QuickFix_test:
+  """This class tests the generation of QuickFix lists for GoTo responses which
+  return multiple locations, such as the Python completer and JavaScript
+  completer. It mostly proves that we use 1-based indexing for the column
+  number."""
+
+  def setUp( self ):
+    self._request = CommandRequest( [ 'GoToTest' ] )
+
+
+  def tearDown( self ):
+    self._request = None
+
+
+  def GoTo_EmptyList_test( self ):
+    self._CheckGoToList( [], [] )
+
+
+  def GoTo_SingleItem_List_test( self ):
+    self._CheckGoToList( [ {
+      'filepath':     'dummy_file',
+      'line_num':     10,
+      'column_num':   1,
+      'description': 'this is some text',
+    } ], [ {
+      'filename':    'dummy_file',
+      'text':        'this is some text',
+      'lnum':        10,
+      'col':         1
+    } ] )
+
+
+  def GoTo_MultiItem_List_test( self ):
+    self._CheckGoToList( [ {
+      'filepath':     'dummy_file',
+      'line_num':     10,
+      'column_num':   1,
+      'description': 'this is some other text',
+    }, {
+      'filepath':     'dummy_file2',
+      'line_num':     1,
+      'column_num':   21,
+      'description': 'this is some text',
+    } ], [ {
+      'filename':    'dummy_file',
+      'text':        'this is some other text',
+      'lnum':        10,
+      'col':         1
+    }, {
+      'filename':    'dummy_file2',
+      'text':        'this is some text',
+      'lnum':        1,
+      'col':         21
+    } ] )
+
+
+  @patch( 'vim.eval' )
+  def _CheckGoToList( self, completer_response, expected_qf_list, vim_eval ):
+    self._request._response = completer_response
+
+    self._request.RunPostCommandActionsIfNeeded()
+
+    vim_eval.assert_has_calls( [
+      call( 'setqflist( {0} )'.format( repr( expected_qf_list ) ) ),
+      call( 'youcompleteme#OpenGoToList()' ),
+    ] )

python/ycm/vimsupport.py

@@ -240,10 +240,12 @@ def SetLocationList( diagnostics ):
 
 def ConvertDiagnosticsToQfList( diagnostics ):
   def ConvertDiagnosticToQfFormat( diagnostic ):
-    # see :h getqflist for a description of the dictionary fields
+    # See :h getqflist for a description of the dictionary fields.
     # Note that, as usual, Vim is completely inconsistent about whether
     # line/column numbers are 1 or 0 based in its various APIs. Here, it wants
-    # them to be 1-based.
+    # them to be 1-based. The documentation states quite clearly that it
+    # expects a byte offset, by which it means "1-based column number" as
+    # described in :h getqflist ("the first column is 1").
     location = diagnostic[ 'location' ]
     line_num = location[ 'line_num' ]