#333 Update line numbers for loclist items when current sign_id values are set

5 files changed, 172 insertions, 55 deletions

diff --git a/autoload/ale/engine.vim b/autoload/ale/engine.vim
index 63362785..31591ded 100644
--- a/autoload/ale/engine.vim
+++ b/autoload/ale/engine.vim
@@ -254,14 +254,15 @@ function! s:HandleExit(job) abort
 endfunction
 
 function! ale#engine#SetResults(buffer, loclist) abort
-    if g:ale_set_quickfix || g:ale_set_loclist
-        call ale#list#SetLists(a:buffer, a:loclist)
-    endif
-
+    " Set signs first. This could potentially fix some line numbers.
     if g:ale_set_signs
         call ale#sign#SetSigns(a:buffer, a:loclist)
     endif
 
+    if g:ale_set_quickfix || g:ale_set_loclist
+        call ale#list#SetLists(a:buffer, a:loclist)
+    endif
+
     if exists('*ale#statusline#Update')
         " Don't load/run if not already loaded.
         call ale#statusline#Update(a:buffer, a:loclist)
diff --git a/autoload/ale/sign.vim b/autoload/ale/sign.vim
index fc12e158..0c4488b5 100644
--- a/autoload/ale/sign.vim
+++ b/autoload/ale/sign.vim
@@ -37,14 +37,17 @@ function! ale#sign#ParseSigns(line_list) abort
     " línea=12 id=1000001 nombre=ALEWarningSign
     " riga=1 id=1000001, nome=ALEWarningSign
     let l:pattern = '^.*=\(\d\+\).*=\(\d\+\).*=ALE\(Error\|Warning\|Dummy\)Sign'
-
     let l:result = []
 
     for l:line in a:line_list
         let l:match = matchlist(l:line, l:pattern)
 
         if len(l:match) > 0
-            call add(l:result, [str2nr(l:match[1]), str2nr(l:match[2])])
+            call add(l:result, [
+            \   str2nr(l:match[1]),
+            \   str2nr(l:match[2]),
+            \   'ALE' . l:match[3] . 'Sign',
+            \])
         endif
     endfor
 
@@ -58,25 +61,25 @@ function! ale#sign#FindCurrentSigns(buffer) abort
 endfunction
 
 " Given a loclist, group the List into with one List per line.
-function! s:GroupSigns(loclist) abort
-    let l:signlist = []
+function! s:GroupLoclistItems(loclist) abort
+    let l:grouped_items = []
     let l:last_lnum = -1
 
     for l:obj in a:loclist
         " Create a new sub-List when we hit a new line.
         if l:obj.lnum != l:last_lnum
-            call add(l:signlist, [])
+            call add(l:grouped_items, [])
         endif
 
-        call add(l:signlist[-1], l:obj)
+        call add(l:grouped_items[-1], l:obj)
         let l:last_lnum = l:obj.lnum
     endfor
 
-    return l:signlist
+    return l:grouped_items
 endfunction
 
 function! s:IsDummySignSet(current_id_list) abort
-    for [l:line, l:id] in a:current_id_list
+    for [l:line, l:id, l:name] in a:current_id_list
         if l:id == g:ale_sign_offset
             return 1
         endif
@@ -89,37 +92,27 @@ function! s:IsDummySignSet(current_id_list) abort
     return 0
 endfunction
 
-" This function will set the signs which show up on the left.
-function! ale#sign#SetSigns(buffer, loclist) abort
-    let l:signlist = s:GroupSigns(a:loclist)
-
-    " Find the current markers
-    let l:current_id_list = ale#sign#FindCurrentSigns(a:buffer)
-    " Check if we set the dummy sign already.
-    let l:dummy_sign_set = s:IsDummySignSet(l:current_id_list)
+function! s:SetDummySignIfNeeded(buffer, current_sign_list, new_signs) abort
+    let l:is_dummy_sign_set = s:IsDummySignSet(a:current_sign_list)
 
     " If we haven't already set a dummy sign, and we have some previous signs
     " or always want a dummy sign, then set one, to keep the sign column open.
-    if !l:dummy_sign_set && (len(l:signlist) > 0 || g:ale_sign_column_always)
+    if !l:is_dummy_sign_set && (a:new_signs || g:ale_sign_column_always)
         execute 'sign place ' .  g:ale_sign_offset
         \   . ' line=1 name=ALEDummySign buffer='
         \   . a:buffer
 
-        let l:dummy_sign_set = 1
+        let l:is_dummy_sign_set = 1
     endif
 
-    " Now remove the previous signs. The dummy will hold the column open
-    " while we add the new signs, if we had signs before.
-    for [l:line, l:current_id] in l:current_id_list
-        if l:current_id != g:ale_sign_offset
-            exec 'sign unplace ' . l:current_id . ' buffer=' . a:buffer
-        endif
-    endfor
+    return l:is_dummy_sign_set
+endfunction
 
+function! s:PlaceNewSigns(buffer, grouped_items) abort
     " Add the new signs,
-    for l:index in range(0, len(l:signlist) - 1)
+    for l:index in range(0, len(a:grouped_items) - 1)
         let l:sign_id = l:index + g:ale_sign_offset + 1
-        let l:sublist = l:signlist[l:index]
+        let l:sublist = a:grouped_items[l:index]
         let l:type = !empty(filter(copy(l:sublist), 'v:val.type ==# ''E'''))
         \   ? 'ALEErrorSign'
         \   : 'ALEWarningSign'
@@ -130,18 +123,81 @@ function! ale#sign#SetSigns(buffer, loclist) abort
             let l:obj.sign_id = l:sign_id
         endfor
 
-        let l:sign_line = 'sign place ' . l:sign_id
-            \. ' line=' . l:sublist[0].lnum
-            \. ' name=' . l:type
-            \. ' buffer=' . a:buffer
+        execute 'sign place ' . l:sign_id
+        \   . ' line=' . l:sublist[0].lnum
+        \   . ' name=' . l:type
+        \   . ' buffer=' . a:buffer
+    endfor
+endfunction
+
+" Get items grouped by any current sign IDs they might have.
+function! s:GetItemsWithSignIDs(loclist) abort
+    let l:items_by_sign_id = {}
+
+    for l:item in a:loclist
+        if has_key(l:item, 'sign_id')
+            if !has_key(l:items_by_sign_id, l:item.sign_id)
+                let l:items_by_sign_id[l:item.sign_id] = []
+            endif
+
+            call add(l:items_by_sign_id[l:item.sign_id], l:item)
+        endif
+    endfor
+
+    return l:items_by_sign_id
+endfunction
+
+" Given some current signs and a loclist, look for items with sign IDs,
+" and change the line numbers for loclist items to match the signs.
+function! s:UpdateLineNumbers(current_sign_list, loclist) abort
+    let l:items_by_sign_id = s:GetItemsWithSignIDs(a:loclist)
+
+    " Do nothing if there's nothing to work with.
+    if empty(l:items_by_sign_id)
+        return
+    endif
+
+    for [l:line, l:sign_id, l:name] in a:current_sign_list
+        for l:obj in get(l:items_by_sign_id, l:sign_id, [])
+            let l:obj.lnum = l:line
+        endfor
+    endfor
+
+    " Sort items again.
+    call sort(a:loclist, 'ale#util#LocItemCompare')
+endfunction
 
-        exec l:sign_line
+" This function will set the signs which show up on the left.
+function! ale#sign#SetSigns(buffer, loclist) abort
+    " Find the current markers
+    let l:current_sign_list = ale#sign#FindCurrentSigns(a:buffer)
+
+    call s:UpdateLineNumbers(l:current_sign_list, a:loclist)
+
+    let l:grouped_items = s:GroupLoclistItems(a:loclist)
+
+    " Set the dummy sign if we need to.
+    " This keeps the sign gutter open while we remove things, etc.
+    let l:is_dummy_sign_set = s:SetDummySignIfNeeded(
+    \   a:buffer,
+    \   l:current_sign_list,
+    \   !empty(l:grouped_items),
+    \)
+
+    " Now remove the previous signs. The dummy will hold the column open
+    " while we add the new signs, if we had signs before.
+    for [l:line, l:sign_id, l:name] in l:current_sign_list
+        if l:sign_id != g:ale_sign_offset
+            exec 'sign unplace ' . l:sign_id . ' buffer=' . a:buffer
+        endif
     endfor
 
+    call s:PlaceNewSigns(a:buffer, l:grouped_items)
+
     " Remove the dummy sign now we've updated the signs, unless we want
     " to keep it, which will keep the sign column open even when there are
     " no warnings or errors.
-    if l:dummy_sign_set && !g:ale_sign_column_always
+    if l:is_dummy_sign_set && !g:ale_sign_column_always
         execute 'sign unplace ' . g:ale_sign_offset . ' buffer=' . a:buffer
     endif
 endfunction
diff --git a/test/sign/test_sign_parsing.vader b/test/sign/test_sign_parsing.vader
index 916fb570..ee112a1b 100644
--- a/test/sign/test_sign_parsing.vader
+++ b/test/sign/test_sign_parsing.vader
@@ -1,14 +1,27 @@
 Execute (Parsing English signs should work):
-  AssertEqual [[9, 1000001]], ale#sign#ParseSigns(['Signs for app.js:', '    line=9  id=1000001  name=ALEWarningSign'])
+  AssertEqual
+  \ [[9, 1000001, 'ALEWarningSign']],
+  \ ale#sign#ParseSigns([
+  \   'Signs for app.js:',
+  \   '    line=9  id=1000001  name=ALEWarningSign',
+  \ ])
 
 Execute (Parsing Russian signs should work):
-  AssertEqual [[1, 1000001]], ale#sign#ParseSigns(['    строка=1  id=1000001  имя=ALEErrorSign'])
+  AssertEqual
+  \ [[1, 1000001, 'ALEErrorSign']],
+  \ ale#sign#ParseSigns(['    строка=1  id=1000001  имя=ALEErrorSign'])
 
 Execute (Parsing Japanese signs should work):
-  AssertEqual [[1, 1000001]], ale#sign#ParseSigns(['    行=1  識別子=1000001  名前=ALEWarningSign'])
+  AssertEqual
+  \ [[1, 1000001, 'ALEWarningSign']],
+  \ ale#sign#ParseSigns(['    行=1  識別子=1000001  名前=ALEWarningSign'])
 
 Execute (Parsing Spanish signs should work):
-  AssertEqual [[12, 1000001]], ale#sign#ParseSigns(['    línea=12 id=1000001 nombre=ALEWarningSign'])
+  AssertEqual
+  \ [[12, 1000001, 'ALEWarningSign']],
+  \ ale#sign#ParseSigns(['    línea=12 id=1000001 nombre=ALEWarningSign'])
 
 Execute (Parsing Italian signs should work):
-  AssertEqual [[1, 1000001]], ale#sign#ParseSigns(['    riga=1 id=1000001, nome=ALEWarningSign'])
+  AssertEqual
+  \ [[1, 1000001, 'ALEWarningSign']],
+  \ ale#sign#ParseSigns(['    riga=1 id=1000001, nome=ALEWarningSign'])
diff --git a/test/sign/test_sign_placement.vader b/test/sign/test_sign_placement.vader
index 6fa72c86..396ef5c4 100644
--- a/test/sign/test_sign_placement.vader
+++ b/test/sign/test_sign_placement.vader
@@ -46,6 +46,17 @@ Before:
     \]
   endfunction
 
+  function! ParseSigns()
+    redir => l:output
+      silent sign place
+    redir END
+
+    return map(
+    \ split(l:output, '\n')[2:],
+    \ 'matchlist(v:val, ''^.*=\(\d\+\).*=\(\d\+\).*=\(.*\)$'')[1:3]',
+    \)
+  endfunction
+
   call ale#linter#Define('testft', {
   \ 'name': 'x',
   \ 'executable': 'echo',
@@ -54,9 +65,11 @@ Before:
   \})
 
 After:
-  call ale#linter#Reset()
+  unlet! g:loclist
   delfunction GenerateResults
-  unlet! g:output
+  delfunction ParseSigns
+  call ale#linter#Reset()
+  sign unplace *
 
 Given testft(A file with warnings/errors):
   foo
@@ -65,14 +78,10 @@ Given testft(A file with warnings/errors):
   fourth line
   fifth line
 
-Execute:
+Execute(The current signs should be set for running a job):
   call ale#Lint()
   call ale#engine#WaitForJobs(2000)
 
-  redir => g:output
-    silent sign place
-  redir END
-
   AssertEqual
   \ [
   \   ['1', '1000001', 'ALEErrorSign'],
@@ -81,7 +90,45 @@ Execute:
   \   ['4', '1000004', 'ALEErrorSign'],
   \   ['5', '1000005', 'ALEErrorSign'],
   \ ],
-  \ map(
-  \   split(g:output, '\n')[2:],
-  \   'matchlist(v:val, "[^=]*=\\(\\d\\+\\)[^=]*=\\(\\d\\+\\).*\\(ALE.*\\)$")[1:3]'
-  \ )
+  \ ParseSigns()
+
+
+Execute(Loclist items with sign_id values should be kept):
+  exec 'sign place 1000347 line=15 name=ALEErrorSign buffer=' . bufnr('%')
+  exec 'sign place 1000348 line=16 name=ALEWarningSign buffer=' . bufnr('%')
+
+  let g:loclist = [
+  \ {'lnum': 1, 'col': 1, 'type': 'E', 'text': 'a', 'sign_id': 1000347},
+  \ {'lnum': 2, 'col': 1, 'type': 'W', 'text': 'b', 'sign_id': 1000348},
+  \ {'lnum': 3, 'col': 1, 'type': 'E', 'text': 'c'},
+  \ {'lnum': 4, 'col': 1, 'type': 'W', 'text': 'd'},
+  \ {'lnum': 15, 'col': 2, 'type': 'W', 'text': 'e'},
+  \ {'lnum': 16, 'col': 2, 'type': 'E', 'text': 'f'},
+  \]
+
+  call ale#sign#SetSigns(bufnr('%'), g:loclist)
+
+  " Line numbers should be changed, sign_id values should be replaced,
+  " and items should be sorted again.
+  AssertEqual
+  \ [
+  \   {'lnum': 3, 'col': 1, 'type': 'E', 'text': 'c', 'sign_id': 1000001},
+  \   {'lnum': 4, 'col': 1, 'type': 'W', 'text': 'd', 'sign_id': 1000002},
+  \   {'lnum': 15, 'col': 1, 'type': 'E', 'text': 'a', 'sign_id': 1000003},
+  \   {'lnum': 15, 'col': 2, 'type': 'W', 'text': 'e', 'sign_id': 1000003},
+  \   {'lnum': 16, 'col': 1, 'type': 'W', 'text': 'b', 'sign_id': 1000004},
+  \   {'lnum': 16, 'col': 2, 'type': 'E', 'text': 'f', 'sign_id': 1000004},
+  \ ],
+  \ g:loclist
+
+  " Items should be grouped again. We should see error signs, where there
+  " were warnings before, and errors where there were errors and where we
+  " now have new warnings.
+  AssertEqual
+  \ [
+  \   ['3', '1000001', 'ALEErrorSign'],
+  \   ['4', '1000002', 'ALEWarningSign'],
+  \   ['15', '1000003', 'ALEErrorSign'],
+  \   ['16', '1000004', 'ALEErrorSign'],
+  \ ],
+  \ ParseSigns()
diff --git a/test/test_ale_toggle.vader b/test/test_ale_toggle.vader
index 90dba401..2cd0110d 100644
--- a/test/test_ale_toggle.vader
+++ b/test/test_ale_toggle.vader
@@ -85,7 +85,7 @@ Execute(ALEToggle should reset everything and then run again):
 
   " First check that everything is there...
   AssertEqual g:expected_loclist, getloclist(0)
-  AssertEqual [[2, 1000001]], ale#sign#FindCurrentSigns(bufnr('%'))
+  AssertEqual [[2, 1000001, 'ALEErrorSign']], ale#sign#FindCurrentSigns(bufnr('%'))
   AssertEqual
   \ [{'group': 'ALEError', 'pos1': [2, 3, 1]}],
   \ map(getmatches(), '{''group'': v:val.group, ''pos1'': v:val.pos1}')
@@ -105,7 +105,7 @@ Execute(ALEToggle should reset everything and then run again):
   call ale#engine#WaitForJobs(2000)
 
   AssertEqual g:expected_loclist, getloclist(0)
-  AssertEqual [[2, 1000001]], ale#sign#FindCurrentSigns(bufnr('%'))
+  AssertEqual [[2, 1000001, 'ALEErrorSign']], ale#sign#FindCurrentSigns(bufnr('%'))
   AssertEqual
   \ [{'group': 'ALEError', 'pos1': [2, 3, 1]}],
   \ map(getmatches(), '{''group'': v:val.group, ''pos1'': v:val.pos1}')