Fixes to code actions (cursor moving, tests, EOL/EOF corner cases) (#3478)
* code_action: Don't move cursor when change covers entire file * code_action: Refactor/simplify ApplyChanges * code_action: Fix EOL at EOF corner cases while performing no changes * code_action: Fix column around EOL corner cases * code_action: Handle positions out of bounds * code_action: Add instructions for verifying corner case tests against vscode
This commit is contained in:
Tomáš Janoušek
GitHub
parent
d340710fcf
commit
2550f5d952
4 changed files with 250 additions and 48 deletions
|
|
@ -71,6 +71,11 @@ function! ale#code_action#ApplyChanges(filename, changes, should_save) abort
|
|||
|
||||
if l:buffer > 0
|
||||
let l:lines = getbufline(l:buffer, 1, '$')
|
||||
|
||||
" Add empty line if there's trailing newline, like readfile() does.
|
||||
if getbufvar(l:buffer, '&eol')
|
||||
let l:lines += ['']
|
||||
endif
|
||||
else
|
||||
let l:lines = readfile(a:filename, 'b')
|
||||
endif
|
||||
|
|
@ -89,62 +94,82 @@ function! ale#code_action#ApplyChanges(filename, changes, should_save) abort
|
|||
let l:end_column = l:code_edit.end.offset
|
||||
let l:text = l:code_edit.newText
|
||||
|
||||
" Adjust the ends according to previous edits.
|
||||
if l:end_line > len(l:lines)
|
||||
let l:end_line_len = 0
|
||||
else
|
||||
let l:end_line_len = len(l:lines[l:end_line - 1])
|
||||
endif
|
||||
|
||||
let l:insertions = split(l:text, '\n', 1)
|
||||
|
||||
if l:line is 1
|
||||
" Same logic as for column below. Vimscript's slice [:-1] will not
|
||||
" be an empty list.
|
||||
let l:start = []
|
||||
else
|
||||
let l:start = l:lines[: l:line - 2]
|
||||
" Fix invalid columns
|
||||
let l:column = l:column > 0 ? l:column : 1
|
||||
let l:end_column = l:end_column > 0 ? l:end_column : 1
|
||||
|
||||
" Clamp start to BOF
|
||||
if l:line < 1
|
||||
let [l:line, l:column] = [1, 1]
|
||||
endif
|
||||
|
||||
" Special case when text must be added after new line
|
||||
if l:column > len(l:lines[l:line - 1])
|
||||
call extend(l:start, [l:lines[l:line - 1]])
|
||||
let l:column = 1
|
||||
" Clamp start to EOF
|
||||
if l:line > len(l:lines) || l:line == len(l:lines) && l:column > len(l:lines[-1]) + 1
|
||||
let [l:line, l:column] = [len(l:lines), len(l:lines[-1]) + 1]
|
||||
" Special case when start is after EOL
|
||||
elseif l:line < len(l:lines) && l:column > len(l:lines[l:line - 1]) + 1
|
||||
let [l:line, l:column] = [l:line + 1, 1]
|
||||
endif
|
||||
|
||||
if l:column is 1
|
||||
" We need to handle column 1 specially, because we can't slice an
|
||||
" empty string ending on index 0.
|
||||
let l:middle = [l:insertions[0]]
|
||||
else
|
||||
let l:middle = [l:lines[l:line - 1][: l:column - 2] . l:insertions[0]]
|
||||
" Adjust end: clamp if invalid and/or adjust if we moved start
|
||||
if l:end_line < l:line || l:end_line == l:line && l:end_column < l:column
|
||||
let [l:end_line, l:end_column] = [l:line, l:column]
|
||||
endif
|
||||
|
||||
call extend(l:middle, l:insertions[1:])
|
||||
|
||||
if l:end_line <= len(l:lines)
|
||||
" Only extend the last line if end_line is within the range of
|
||||
" lines.
|
||||
let l:middle[-1] .= l:lines[l:end_line - 1][l:end_column - 1 :]
|
||||
" Clamp end to EOF
|
||||
if l:end_line > len(l:lines) || l:end_line == len(l:lines) && l:end_column > len(l:lines[-1]) + 1
|
||||
let [l:end_line, l:end_column] = [len(l:lines), len(l:lines[-1]) + 1]
|
||||
" Special case when end is after EOL
|
||||
elseif l:end_line < len(l:lines) && l:end_column > len(l:lines[l:end_line - 1]) + 1
|
||||
let [l:end_line, l:end_column] = [l:end_line + 1, 1]
|
||||
endif
|
||||
|
||||
" Careful, [:-1] is not an empty list
|
||||
let l:start = l:line is 1 ? [] : l:lines[: l:line - 2]
|
||||
let l:middle = l:column is 1 ? [''] : [l:lines[l:line - 1][: l:column - 2]]
|
||||
|
||||
let l:middle[-1] .= l:insertions[0]
|
||||
let l:middle += l:insertions[1:]
|
||||
let l:middle[-1] .= l:lines[l:end_line - 1][l:end_column - 1 :]
|
||||
|
||||
let l:end_line_len = len(l:lines[l:end_line - 1])
|
||||
let l:lines_before_change = len(l:lines)
|
||||
let l:lines = l:start + l:middle + l:lines[l:end_line :]
|
||||
|
||||
let l:current_line_offset = len(l:lines) - l:lines_before_change
|
||||
let l:column_offset = len(l:middle[-1]) - l:end_line_len
|
||||
|
||||
let l:pos = s:UpdateCursor(l:pos,
|
||||
\ [l:line, l:column],
|
||||
\ [l:end_line, l:end_column],
|
||||
\ [l:current_line_offset, l:column_offset])
|
||||
" Keep cursor where it was (if outside of changes) or move it after
|
||||
" the changed text (if inside), but don't touch it when the change
|
||||
" spans the entire buffer, in which case we have no clue and it's
|
||||
" better to not do anything.
|
||||
if l:line isnot 1 || l:column isnot 1
|
||||
\|| l:end_line < l:lines_before_change
|
||||
\|| l:end_line == l:lines_before_change && l:end_column <= l:end_line_len
|
||||
let l:pos = s:UpdateCursor(l:pos,
|
||||
\ [l:line, l:column],
|
||||
\ [l:end_line, l:end_column],
|
||||
\ [l:current_line_offset, l:column_offset])
|
||||
endif
|
||||
endfor
|
||||
|
||||
if l:lines[-1] is# ''
|
||||
if l:buffer > 0
|
||||
" Make sure ale#util#{Writefile,SetBufferContents} add trailing
|
||||
" newline if and only if it should be added.
|
||||
if l:lines[-1] is# '' && getbufvar(l:buffer, '&eol')
|
||||
call remove(l:lines, -1)
|
||||
else
|
||||
call setbufvar(l:buffer, '&eol', 0)
|
||||
endif
|
||||
elseif exists('+fixeol') && &fixeol && l:lines[-1] is# ''
|
||||
" Not in buffer, ale#util#Writefile can't check &eol and always adds
|
||||
" newline if &fixeol: remove to prevent double trailing newline.
|
||||
call remove(l:lines, -1)
|
||||
endif
|
||||
|
||||
if a:should_save
|
||||
if a:should_save || l:buffer < 0
|
||||
call ale#util#Writefile(l:buffer, l:lines, a:filename)
|
||||
else
|
||||
call ale#util#SetBufferContents(l:buffer, l:lines)
|
||||
|
|
|
|||
|
|
@ -1,14 +1,7 @@
|
|||
Before:
|
||||
Save g:ale_enabled
|
||||
|
||||
let g:ale_enabled = 0
|
||||
|
||||
" Enable fix end-of-line as tests below expect that
|
||||
set fixeol
|
||||
|
||||
runtime autoload/ale/code_action.vim
|
||||
runtime autoload/ale/util.vim
|
||||
|
||||
let g:file1 = tempname()
|
||||
let g:file2 = tempname()
|
||||
let g:test = {}
|
||||
|
|
@ -42,8 +35,6 @@ Before:
|
|||
endfunction!
|
||||
|
||||
After:
|
||||
Restore
|
||||
|
||||
" Close the extra buffers if we opened it.
|
||||
if bufnr(g:file1) != -1
|
||||
execute ':bp! | :bd! ' . bufnr(g:file1)
|
||||
|
|
@ -65,8 +56,7 @@ After:
|
|||
unlet! g:changes
|
||||
delfunction WriteFileAndEdit
|
||||
|
||||
runtime autoload/ale/code_action.vim
|
||||
runtime autoload/ale/util.vim
|
||||
Restore
|
||||
|
||||
|
||||
Execute(It should modify and save multiple files):
|
||||
|
|
@ -214,7 +204,6 @@ Execute(End of file can be modified):
|
|||
\)
|
||||
|
||||
AssertEqual g:test.text + [
|
||||
\ '',
|
||||
\ 'type A: string',
|
||||
\ 'type B: number',
|
||||
\ '',
|
||||
|
|
@ -364,6 +353,15 @@ Execute(Cursor will not move when changes happening on lines >= cursor, but afte
|
|||
AssertEqual ' value: number', getline('.')
|
||||
AssertEqual [2, 3], getpos('.')[1:2]
|
||||
|
||||
Execute(Cursor will not move when change covers entire file):
|
||||
call WriteFileAndEdit()
|
||||
call setpos('.', [0, 2, 3, 0])
|
||||
call ale#code_action#HandleCodeAction(
|
||||
\ g:test.create_change(1, 1, len(g:test.text) + 1, 1,
|
||||
\ join(g:test.text + ['x'], "\n")),
|
||||
\ {'should_save': 1})
|
||||
AssertEqual [2, 3], getpos('.')[1:2]
|
||||
|
||||
Execute(It should just modify file when should_save is set to v:false):
|
||||
call WriteFileAndEdit()
|
||||
let g:test.change = g:test.create_change(1, 1, 1, 1, "import { writeFile } from 'fs';\n")
|
||||
|
|
|
|||
179
test/test_code_action_corner_cases.vader
Normal file
179
test/test_code_action_corner_cases.vader
Normal file
|
|
@ -0,0 +1,179 @@
|
|||
" Tests for various corner cases of applying code changes from LSP.
|
||||
"
|
||||
" These can be verified against the reference vscode implementation using the
|
||||
" following javascript program:
|
||||
"
|
||||
" const { TextDocument } = require('vscode-languageserver-textdocument');
|
||||
" const { TextEdit, Position, Range } = require('vscode-languageserver-types');
|
||||
" function MkPos(line, offset) { return Position.create(line - 1, offset - 1); }
|
||||
" function MkInsert(pos, newText) { return TextEdit.insert(pos, newText); }
|
||||
" function MkDelete(start, end) { return TextEdit.del(Range.create(start, end)); }
|
||||
" function TestChanges(s, es) {
|
||||
" return TextDocument.applyEdits(TextDocument.create(null, null, null, s), es);
|
||||
" }
|
||||
"
|
||||
" const fs = require("fs");
|
||||
" const assert = require('assert').strict;
|
||||
" const testRegex = /(?<!vscode skip.*)AssertEqual\s+("[^"]*"),\s*TestChanges\(("[^"]*"),\s*(\[[^\]]*\])/g;
|
||||
" const data = fs.readFileSync(0, "utf-8");
|
||||
" const tests = data.matchAll(testRegex);
|
||||
" for (const test of tests) {
|
||||
" console.log(test[0]);
|
||||
" assert.equal(eval(test[1]), TestChanges(eval(test[2]), eval(test[3])));
|
||||
" }
|
||||
"
|
||||
" Save it to test_code_action_corner_cases.js and invoke it using:
|
||||
"
|
||||
" $ npm install vscode-languageserver-{textdocument,types}
|
||||
" $ node test_code_action_corner_cases.js <test_code_action_corner_cases.vader
|
||||
|
||||
Before:
|
||||
Save &fixeol
|
||||
set nofixeol
|
||||
|
||||
Save &fileformats
|
||||
set fileformats=unix
|
||||
|
||||
" two files, one accessed through a buffer, the other using write/readfile only
|
||||
let g:files = [tempname(), tempname()]
|
||||
|
||||
function! TestChanges(contents, changes, mode) abort
|
||||
let l:file = g:files[a:mode is 'file' ? 0 : 1]
|
||||
call writefile(split(a:contents, '\n', 1), l:file, 'bS')
|
||||
if a:mode isnot 'file'
|
||||
execute 'edit ' . l:file
|
||||
endif
|
||||
call ale#code_action#ApplyChanges(l:file, a:changes, a:mode isnot 'buffer')
|
||||
if a:mode is 'buffer'
|
||||
execute 'write ' . l:file
|
||||
endif
|
||||
return join(readfile(l:file, 'b'), "\n")
|
||||
endfunction!
|
||||
|
||||
function! MkPos(line, offset) abort
|
||||
return {'line': a:line, 'offset': a:offset}
|
||||
endfunction!
|
||||
|
||||
function! MkInsert(pos, newText) abort
|
||||
return {'start': a:pos, 'end': a:pos, 'newText': a:newText}
|
||||
endfunction!
|
||||
|
||||
function! MkDelete(start, end) abort
|
||||
return {'start': a:start, 'end': a:end, 'newText': ''}
|
||||
endfunction!
|
||||
|
||||
After:
|
||||
for g:file in g:files
|
||||
if bufnr(g:file) != -1
|
||||
execute ':bp! | :bd! ' . bufnr(g:file)
|
||||
endif
|
||||
if filereadable(g:file)
|
||||
call delete(g:file)
|
||||
endif
|
||||
endfor
|
||||
unlet! g:files g:file
|
||||
|
||||
unlet! g:mode
|
||||
|
||||
delfunction TestChanges
|
||||
delfunction MkPos
|
||||
delfunction MkInsert
|
||||
delfunction MkDelete
|
||||
|
||||
Restore
|
||||
|
||||
Execute(Preserve (no)eol at eof):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "noeol", TestChanges("noeol", [], g:mode)
|
||||
AssertEqual "eol\n", TestChanges("eol\n", [], g:mode)
|
||||
AssertEqual "eols\n\n", TestChanges("eols\n\n", [], g:mode)
|
||||
endfor
|
||||
|
||||
" there doesn't seem to be a way to tell if a buffer is empty or contains one
|
||||
" empty line :-(
|
||||
AssertEqual "", TestChanges("", [], 'file')
|
||||
|
||||
Execute(Respect fixeol):
|
||||
set fixeol
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
silent echo "vscode skip" | AssertEqual "noeol\n", TestChanges("noeol", [], g:mode)
|
||||
silent echo "vscode skip" | AssertEqual "eol\n", TestChanges("eol\n", [], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(Add/del eol at eof):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "addeol\n", TestChanges("addeol", [MkInsert(MkPos(1, 7), "\n")], g:mode)
|
||||
AssertEqual "deleol", TestChanges("deleol\n", [MkDelete(MkPos(1, 7), MkPos(1, 8))], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(One character insertions to first line):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "xabc\ndef1\nghi\n", TestChanges("abc\ndef1\nghi\n", [MkInsert(MkPos(1, 0), "x")], g:mode)
|
||||
AssertEqual "xabc\ndef2\nghi\n", TestChanges("abc\ndef2\nghi\n", [MkInsert(MkPos(1, 1), "x")], g:mode)
|
||||
AssertEqual "axbc\ndef3\nghi\n", TestChanges("abc\ndef3\nghi\n", [MkInsert(MkPos(1, 2), "x")], g:mode)
|
||||
AssertEqual "abcx\ndef4\nghi\n", TestChanges("abc\ndef4\nghi\n", [MkInsert(MkPos(1, 4), "x")], g:mode)
|
||||
AssertEqual "abc\nxdef5\nghi\n", TestChanges("abc\ndef5\nghi\n", [MkInsert(MkPos(1, 5), "x")], g:mode)
|
||||
AssertEqual "abc\nxdef6\nghi\n", TestChanges("abc\ndef6\nghi\n", [MkInsert(MkPos(1, 6), "x")], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(One character + newline insertions to first line):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "x\nabc\ndef1\nghi\n", TestChanges("abc\ndef1\nghi\n", [MkInsert(MkPos(1, 0), "x\n")], g:mode)
|
||||
AssertEqual "x\nabc\ndef2\nghi\n", TestChanges("abc\ndef2\nghi\n", [MkInsert(MkPos(1, 1), "x\n")], g:mode)
|
||||
AssertEqual "ax\nbc\ndef3\nghi\n", TestChanges("abc\ndef3\nghi\n", [MkInsert(MkPos(1, 2), "x\n")], g:mode)
|
||||
AssertEqual "abcx\n\ndef4\nghi\n", TestChanges("abc\ndef4\nghi\n", [MkInsert(MkPos(1, 4), "x\n")], g:mode)
|
||||
AssertEqual "abc\nx\ndef5\nghi\n", TestChanges("abc\ndef5\nghi\n", [MkInsert(MkPos(1, 5), "x\n")], g:mode)
|
||||
AssertEqual "abc\nx\ndef6\nghi\n", TestChanges("abc\ndef6\nghi\n", [MkInsert(MkPos(1, 6), "x\n")], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(One character insertions near end):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "abc\ndef1\nghxi\n", TestChanges("abc\ndef1\nghi\n", [MkInsert(MkPos(3, 3), "x")], g:mode)
|
||||
AssertEqual "abc\ndef2\nghix\n", TestChanges("abc\ndef2\nghi\n", [MkInsert(MkPos(3, 4), "x")], g:mode)
|
||||
AssertEqual "abc\ndef3\nghi\nx", TestChanges("abc\ndef3\nghi\n", [MkInsert(MkPos(3, 5), "x")], g:mode)
|
||||
AssertEqual "abc\ndef4\nghi\nx", TestChanges("abc\ndef4\nghi\n", [MkInsert(MkPos(3, 6), "x")], g:mode)
|
||||
AssertEqual "abc\ndef5\nghi\nx", TestChanges("abc\ndef5\nghi\n", [MkInsert(MkPos(4, 1), "x")], g:mode)
|
||||
AssertEqual "abc\ndef6\nghi\nx", TestChanges("abc\ndef6\nghi\n", [MkInsert(MkPos(4, 2), "x")], g:mode)
|
||||
AssertEqual "abc\ndef7\nghi\nx", TestChanges("abc\ndef7\nghi\n", [MkInsert(MkPos(5, 1), "x")], g:mode)
|
||||
AssertEqual "abc\ndef8\nghi\nx", TestChanges("abc\ndef8\nghi\n", [MkInsert(MkPos(5, 2), "x")], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(One character + newline insertions near end):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "abc\ndef1\nghx\ni\n", TestChanges("abc\ndef1\nghi\n", [MkInsert(MkPos(3, 3), "x\n")], g:mode)
|
||||
AssertEqual "abc\ndef2\nghix\n\n", TestChanges("abc\ndef2\nghi\n", [MkInsert(MkPos(3, 4), "x\n")], g:mode)
|
||||
AssertEqual "abc\ndef3\nghi\nx\n", TestChanges("abc\ndef3\nghi\n", [MkInsert(MkPos(3, 5), "x\n")], g:mode)
|
||||
AssertEqual "abc\ndef4\nghi\nx\n", TestChanges("abc\ndef4\nghi\n", [MkInsert(MkPos(3, 6), "x\n")], g:mode)
|
||||
AssertEqual "abc\ndef5\nghi\nx\n", TestChanges("abc\ndef5\nghi\n", [MkInsert(MkPos(4, 1), "x\n")], g:mode)
|
||||
AssertEqual "abc\ndef6\nghi\nx\n", TestChanges("abc\ndef6\nghi\n", [MkInsert(MkPos(4, 2), "x\n")], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(Newline insertions near end):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "abc\ndef1\ngh\ni\n", TestChanges("abc\ndef1\nghi\n", [MkInsert(MkPos(3, 3), "\n")], g:mode)
|
||||
AssertEqual "abc\ndef2\nghi\n\n", TestChanges("abc\ndef2\nghi\n", [MkInsert(MkPos(3, 4), "\n")], g:mode)
|
||||
AssertEqual "abc\ndef3\nghi\n\n", TestChanges("abc\ndef3\nghi\n", [MkInsert(MkPos(3, 5), "\n")], g:mode)
|
||||
AssertEqual "abc\ndef4\nghi\n\n", TestChanges("abc\ndef4\nghi\n", [MkInsert(MkPos(3, 6), "\n")], g:mode)
|
||||
AssertEqual "abc\ndef5\nghi\n\n", TestChanges("abc\ndef5\nghi\n", [MkInsert(MkPos(4, 1), "\n")], g:mode)
|
||||
endfor
|
||||
|
||||
Execute(Single char deletions):
|
||||
for g:mode in ['save', 'file', 'buffer']
|
||||
Log g:mode
|
||||
AssertEqual "bc\ndef1\nghi\n", TestChanges("abc\ndef1\nghi\n", [MkDelete(MkPos(1, 1), MkPos(1, 2))], g:mode)
|
||||
AssertEqual "ab\ndef2\nghi\n", TestChanges("abc\ndef2\nghi\n", [MkDelete(MkPos(1, 3), MkPos(1, 4))], g:mode)
|
||||
AssertEqual "abcdef3\nghi\n", TestChanges("abc\ndef3\nghi\n", [MkDelete(MkPos(1, 4), MkPos(1, 5))], g:mode)
|
||||
AssertEqual "abcdef4\nghi\n", TestChanges("abc\ndef4\nghi\n", [MkDelete(MkPos(1, 4), MkPos(1, 6))], g:mode)
|
||||
AssertEqual "abc\ndef5\ngh\n", TestChanges("abc\ndef5\nghi\n", [MkDelete(MkPos(3, 3), MkPos(3, 4))], g:mode)
|
||||
AssertEqual "abc\ndef6\nghi", TestChanges("abc\ndef6\nghi\n", [MkDelete(MkPos(3, 4), MkPos(3, 5))], g:mode)
|
||||
AssertEqual "abc\ndef7\nghi", TestChanges("abc\ndef7\nghi\n", [MkDelete(MkPos(3, 4), MkPos(3, 6))], g:mode)
|
||||
AssertEqual "abc\ndef8\nghi\n", TestChanges("abc\ndef8\nghi\n", [MkDelete(MkPos(4, 1), MkPos(4, 2))], g:mode)
|
||||
endfor
|
||||
|
|
@ -35,7 +35,7 @@ Given python(Second python example):
|
|||
|
||||
Execute():
|
||||
let g:changes = [
|
||||
\ {'end': {'offset': 16, 'line': 2}, 'newText': "\n\ndef func_ivlpdpao(f):\n exif = exifread.process_file(f)\n dt = str(exif['Image DateTime'])\n date = dt[:10].replace(':', '-')\n return date\n", 'start': {'offset': 16, 'line': 2}},
|
||||
\ {'end': {'offset': 16, 'line': 2}, 'newText': "\n\n\ndef func_ivlpdpao(f):\n exif = exifread.process_file(f)\n dt = str(exif['Image DateTime'])\n date = dt[:10].replace(':', '-')\n return date\n", 'start': {'offset': 16, 'line': 2}},
|
||||
\ {'end': {'offset': 32, 'line': 6}, 'newText': 'date = func', 'start': {'offset': 9, 'line': 6}},
|
||||
\ {'end': {'offset': 42, 'line': 8}, 'newText': "ivlpdpao(f)\n", 'start': {'offset': 33, 'line': 6}}
|
||||
\]
|
||||
|
|
|
|||
Reference in a new issue