[RFC] Handle file operations that are represented by new fixit chunk kinds

[bstaletic]

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

• I have read and understood YCM's CONTRIBUTING document.
• I have read and understood YCM's CODE_OF_CONDUCT document.
• I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
• I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.

Why this change is necessary and useful

This is the client counterpart to ycm-core/ycmd#1766
Again, feedback very much wanted.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 32.25806% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.36%. Comparing base (35d1882) to head (9defff0).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4271      +/-   ##
==========================================
- Coverage   89.95%   89.36%   -0.59%     
==========================================
  Files          37       37              
  Lines        4758     4787      +29     
==========================================
- Hits         4280     4278       -2     
- Misses        478      509      +31     

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

python/ycm/vimsupport.py line 1027 at r1 (raw file):
unfortunately, this now breaches LSP's requirements.

If resource operations are present clients need to execute the operations in the order in which they are provided.

However, I still can't think of a reason why sorting the operations by filename but keeping the order of operations-within-a-file stable could actually cause real problems, so I'm happy to keep it and deal with the fallout.

---

python/ycm/vimsupport.py line 1084 at r1 (raw file):

  # need to fully reverse the list then sort it on the starting position in
  # reverse order.
  chunks.reverse()

hmm this seems kind of hairy now. I guess we're going to sort all file operations after any edits to those files, so "Create foo.txt, add like footer to foo.txt" won't work right?

---

python/ycm/vimsupport.py line 1094 at r1 (raw file):

  replace_chunks = []
  for chunk in chunks:
    if 'range' in chunk:

I think iw Ould be temped to always add a 'kind' to 'chunks' in the API and then rather than inspecting presence of fields, always switch on the kind. In ycmd api we can just always include a kind (such as 'textEdit', 'createFile', 'deleteFile', 'renameFile').

---

python/ycm/vimsupport.py line 1111 at r1 (raw file):

        CreateChunk(
          chunk[ 'file' ],
          vim_buffer,

does this already get created if the file doesn't exist ?

what if the type is 'create' and the file already exists?!

---

python/ycm/vimsupport.py line 1113 at r1 (raw file):

          vim_buffer,
          chunk[ 'kind' ] ) )
    elif chunk[ 'kind' ] == 'delete':

what if the file doesn't exist?

---

python/ycm/vimsupport.py line 1204 at r1 (raw file):

def RenameChunk( old_file, new_file, vim_buffer, kind = 'rename' ):
  OpenFilename( old_file )

normally when processing fixits we don't modify the current buffer in the current window; I think this will do that, no?

---

python/ycm/vimsupport.py line 1206 at r1 (raw file):

  OpenFilename( old_file )
  vim.command( f'silent! saveas { new_file }' )
  vim.command( f'silent! bw! { old_file }' )

this will close out any existing windows that happened to have this buffer in them. We don't really want that; we want to rename the file and associated buffers to be updated.

I wonder if the vim script function rename would do that for us.

I know that I use a Tim Pope plugin for renaming files (eunuch.vim) and it's not completely trivial, but it does use rename()

---

python/ycm/vimsupport.py line 1230 at r1 (raw file):

def DeleteChunk( file, vim_buffer, kind = 'delete' ):
  vim.command( f'silent! bw! { vim_buffer }' )

I feel like honestly if we're going to implement this, we need a better way for the user to confirm the operations. I would be pretty angry if we deleted a file from my project without me having some say over it.

There are a few things:

1. I think there's a "preview" mode for file actions in LSP, perhaps we need it?
2. perhaps we should ask for confirmation for all these file operations. We already have a confirmation for operations that affect multiple files contents let alone baleting entire files.
3. I'm worried about bugs. Say a server doesn't know about the rename operation and sends the action as something like: "delete file A.txt, create file B.txt, write {the old A.txt contents} to B.txt"' we're now a transaction processor, and if we crash after deleting A but before creating and writing B, we just wiped out the user's file with no way to get it back!

So, long story short, I'm nervous about this feature as it stands. it seems we need to add a load of robustness and complexity to avoid badness.

[puremourning]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

python/ycm/vimsupport.py line 1101 at r1 (raw file):
according to the spec:

Note that the names talk about files but the operations are supposed to work on files and folders.

So somehow we need to be able to create and rename and delete folders too.

Imaging renaming a package in java... it would require creating a new folder for the new package name and renaming all the files inside it.

It's not even clear from the LSP spec if an explicit folder-create would be received, but we would certainly require a os.path.mkdirs() or whatever I think.

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

python/ycm/vimsupport.py line 1027 at r1 (raw file):
As discussed in gitter, pathological case is:

1. Do X in FILE1
2. Rename FILE1 to FILE2
3. Do Y in the newly created FILE2

Adding more renames and file operations can make things quite nasty. The LSP specification says:

If resource operations are present clients need to execute the operations in the order in which they are provided.

So we can't do any sort of sorting.
I'll work on dropping the sorting.

---

python/ycm/vimsupport.py line 1094 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think iw Ould be temped to always add a 'kind' to 'chunks' in the API and then rather than inspecting presence of fields, always switch on the kind. In ycmd api we can just always include a kind (such as 'textEdit', 'createFile', 'deleteFile', 'renameFile').

Fair enough. I thought about that and decided to just get this to perform a rename with minimal effort.

---

python/ycm/vimsupport.py line 1101 at r1 (raw file):
Good point.

it would require creating a new folder for the new package name and renaming all the files inside it.

I would expect the server to tell us:

1. Rename directory.
2. Apply $CHANGE_F1 to $FILE_IN_NEW_DIR_1
3. Apply $CHANGE_F2 to $FILE_IN_NEW_DIR_2

or

1. Apply $CHANGE_F1 to $EXISTING_FILE_1
2. Apply $CHANGE_F2 to $EXISTING_FILE_2
3. Rename directory.

Anything else requires YouCompleteMe to know the rules of $LANGUAGE.

---

python/ycm/vimsupport.py line 1111 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

does this already get created if the file doesn't exist ?

what if the type is 'create' and the file already exists?!

Before we enter this function, there's _OpenFileInSplitIfNeeded(), so all files will be open in the current tab page before we reach this line.

I have not tried what you're asking about, as I don't know a way to trigger a create action.

---

python/ycm/vimsupport.py line 1204 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

normally when processing fixits we don't modify the current buffer in the current window; I think this will do that, no?

It will. I don't like it, but I didn't know how else to "rename" an existing buffer then to switch, saveas and bw. Help?

---

python/ycm/vimsupport.py line 1206 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

this will close out any existing windows that happened to have this buffer in them. We don't really want that; we want to rename the file and associated buffers to be updated.

I wonder if the vim script function rename would do that for us.

I know that I use a Tim Pope plugin for renaming files (eunuch.vim) and it's not completely trivial, but it does use rename()

Like I said, I didn't know how to do better. Help needed.

---

python/ycm/vimsupport.py line 1230 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I feel like honestly if we're going to implement this, we need a better way for the user to confirm the operations. I would be pretty angry if we deleted a file from my project without me having some say over it.

There are a few things:

1. I think there's a "preview" mode for file actions in LSP, perhaps we need it?
2. perhaps we should ask for confirmation for all these file operations. We already have a confirmation for operations that affect multiple files contents let alone baleting entire files.
3. I'm worried about bugs. Say a server doesn't know about the rename operation and sends the action as something like: "delete file A.txt, create file B.txt, write {the old A.txt contents} to B.txt"' we're now a transaction processor, and if we crash after deleting A but before creating and writing B, we just wiped out the user's file with no way to get it back!

So, long story short, I'm nervous about this feature as it stands. it seems we need to add a load of robustness and complexity to avoid badness.

Absolutely agreed. One question, though.
If a user says "don't you dare delete $FILE" we can't really proceed with anything else, can we? Later edits might be relying on that operation being executed.

I guess we need to list all file operations for the user to confirm?
That might paint a confusing picture, without the text edits in between... as again there can be pathological cases that are utter insanity.

To be clear, I very much agree we need more guard rails around these operations.

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

python/ycm/vimsupport.py line 1094 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Fair enough. I thought about that and decided to just get this to perform a rename with minimal effort.

Done.

---

python/ycm/vimsupport.py line 1113 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

what if the file doesn't exist?

It will be ignored.