Fixes to code actions (cursor moving, tests, EOL/EOF corner cases)

[liskin]

This PR adds tests for many corner cases (mostly involving newlines and changes near newlines) around applying changes from LSP, fixes these corner cases, and also adds a workaround for LSPs that just replace the entire document instead of sending individual changes.

More details in the individual commits:

---

commit fa83989

Minor cleanup in test/test_code_action.vader

Dropped unnecessary sourcing, which probably originated from
test_codefix.vader which overrides some functions, but
test_code_action.vader does not.

Also, Save &fixeol before changing it, and Restore at the end after
closing buffers, otherwise fixeol isn't fully restored.

commit 354b5b7

code_action: Don't move cursor when change covers entire file

This prevents the cursor jumping to end of file after every
rename/refactor. This happens with e.g. python-language-server which
rewrites the entire buffer regardless of backend:

https://github.com/palantir/python-language-server/blob/3d045b8db9b53d688f433fb2f2e5df27cbc1f788/pyls/plugins/jedi_rename.py#L31-L37
https://github.com/palantir/python-language-server/blob/10cd98c898c89a8c9961b25725a6cd913485e1d0/pyls/plugins/rope_rename.py#L43-L49

commit a4b7069

code_action: Refactor/simplify ApplyChanges

No behaviour change, just simplification that will make further fixes
a bit easier.

commit 53f8856

code_action: Fix EOL at EOF corner cases while performing no changes

Instead of always removing the trailing empty line and hoping it wasn't
supposed to be there, only remove it when we know ale#util#Writefile
will add a trailing newline, and additionally tell ale#util#Writefile to
not add that newline if any change explicitly deleted that newline.

(Unfortunately with a:should_save=0, adding a trailing newline that
wasn't there results in noeol and empty line at end of buffer, because
for some reason setbufvar(l:buffer, '&eol', 1) doesn't work in buffers
that started as noeol. This is likely a bug in vim itself.)

Includes a minor fix that prevents invoking ale#util#SetBufferContents
without a buffer. Not sure whether that might really happen anywhere.

(test_code_action_python.vader depends on two incorrect behaviours
cancelling each other, so it's temporarily adjusted to pass)

commit bae3c8b

code_action: Fix column around EOL corner cases

The original code didn't allow changing the last character in a line
without also replacing the newline, due to an off-by-one error.

Or, from a different point of view, it didn't allow replacing the
newline without also replacing the preceding character. Therefore it
comes as no surprise that it didn't correctly handle whether to replace
or leave the newline at the end of the changed range.

This commit fixes both, and also adjusts two tests that depended on the
incorrect behaviour. Also, the temporary change to
test_code_action_python.vader introduced earlier can now be reverted.

Finally, this adds a _lot_ of tiny unit tests for various corner cases.
These were verified against the reference implementation in
[vscode-languageserver-node][], and a javascript source to run them is
added in a followup commit. The added tests take an additional second or
two. It might be worth reducing them somewhat to only test what was
broken…?

[vscode-languageserver-node]: https://github.com/microsoft/vscode-languageserver-node/

commit b964724

code_action: Handle positions out of bounds

This makes the code more robust, and perhaps even a little easier to
understand: less exceptional cases, more bounds clamping.

commit 12b40b7

code_action: Add instructions for verifying corner case tests against vscode

[liskin]

@jeremija @daliusd You folks authored most of the code I'm touching, so pinging you if you want to have a look. Thanks!

[daliusd]

It is cool to see that you have decided to improve existing code. So far everything looks good. Cheers 🍺

[jeremija]

@liskin Thanks for tagging me. Previously, I did notice there were a few corner cases in which the LSP rename messed up my document, but I never had the time to debug it - thanks for taking the time to do it. I'll be sure to try out your changes over the next few days!

[liskin]

TODOs (add instructions for verifying the tests against vscode reference impl, and rebase) done. I believe this PR is now really ready for review. @daliusd if you'd like to have a look at the commits I added since Wednesday they're ready.

(the right-click menu fix was separated into #3482 as it was somewhat unrelated)

[daliusd]

IMHO, everything is OK.

[tmc]

What's next to merge this?

[daliusd]

@w0rp @hsanson or someone with merge rights should review and approve this.

[dnaaun]

@hsanson , I believe that you're holding off on some PRs because you think they need @w0rp 's attention, but I wasn't sure if you had seen this at all. Thank you for your time!

[hsanson]

@davidatbu thanks for the heads up. I do avoid merging changes that affect core parts of ALE. Those need to be reviewed by @w0rp (in my opinion). For added linters/fixers/lsp tools that I can actually test on my setup I review and merge when time allows.

[dnaaun]

I've been trying out this fork for a couple of days now, and there are sporadic occurrences of ALE completely truncating in an external file file when trying to use ALERename on a variable defined in that file. I'm trying to come up with a small reproducible example, but I can't figure out what causes it even after trimming down my vimrc to very minimal. I was wondering if anyone else experienced this.

FYI, I'm using pyright as an LSP for Python files.

[hsanson]

ALE migrated from Travis to Github Actions (see #3548) so it is necessary to rebase this PR from latest master branch to trigger the required checks.

# Add dense-analysis as remote upstream on your local fork. Not 
# needed if already added.
git remote add upstream https://github.com/dense-analysis/ale.git

# Sync your local master with upstream master
git checkout master
git fetch upstream master
git merge upstream/master

# Rebase the PR branch with master
git checkout my-branch-name
git rebase master   # Fix any conflicts you may have

# Force push the updated PR branch to origin to trigger the checks
git push -f origin my-branch-name

[liskin]

@hsanson A rebase isn't strictly necessary as GH merges PRs with master before running the checks, so I've only forcepushed to re-trigger the tests. (I don't mind rebasing, I just wanted to double check that I'm not incorrect.)

@davidatbu I do have to admit that I haven't done a lot of renaming since creating this PR (to be entirely honest, most of this PR was done for fun rather than solving a specific problem that I had), so if there were some problems, I wouldn't encounter them anyway. Also, I use pyls which always replaces the entire file contents, so it's unlikely to trigger any interesting bugs.

Anyway, the issue you described should definitely be taken care of. Perhaps the best way to debug this is to set up ale#code_action#ApplyChanges to dump the changes array to a file so that when this happens, we can look back and see what might be triggering this. Will you be able to do something like that or shall I try to come up with a patch that enables such logging?

[stale]

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

[w0rp]

It's hard to follow the code, just by the nature of what is being done, but that doesn't matter because the extensive tests you've provided are good. 👍 I'll merge this now, and if anyone has any issues with it, I'll let you know.

[w0rp]

Cheers! 🍻

I really appreciate all of the time and care you've put into this.

[liskin]

Oh, but I'm a bit sad that you squashed the commits and threw all my comments away. I really did try to make the code changes, if not easy, at least possible to follow, and now that's been dumped. If anyone comes and finds a problem, I'll need to look into a backup of my branch to see my individual commits. :-/

And if anyone else wants to debug the problem, they probably won't know to go looking for my original branch, and they'll be wondering in total darkness about the rationale for those changes. This is really stupid. This feature of GitHub should not be used!

[w0rp]

I don't like to have git commit messages that are way, way too long, as it makes it hard to read the git log. Anything that long ought to be written in test comments or documented in the code or documentation.

[liskin]

If you want git log to be easy to read, use --pretty=oneline or tig or something. There really is no good reason to just cut all that info. It's good for reviewers, and it's good for anyone wondering why a certain change was made. Comments are good, but they can only explain why the code is what it is now, not how it became to be, which is often important when debugging.

If you really intend to throw all this away, then I'm afraid I'll need to reconsider my future contributions. :-/