Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there any plan to support workspace.workspaceEdit.resourceOperations #4267

Open
5 of 11 tasks
fortime opened this issue Sep 24, 2024 · 9 comments · May be fixed by ycm-core/ycmd#1766
Open
5 of 11 tasks

Is there any plan to support workspace.workspaceEdit.resourceOperations #4267

fortime opened this issue Sep 24, 2024 · 9 comments · May be fixed by ycm-core/ycmd#1766

Comments

@fortime
Copy link

fortime commented Sep 24, 2024

Issue Prelude

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

  • [x ] I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have read and understood YCM's README, especially the
    Frequently Asked Questions section.
  • I have searched YCM's issue tracker to find issues similar to the one I'm
    about to report and couldn't find an answer to my problem. (Example Google
    search.
    )
  • If filing a bug report, I have included the output of vim --version.
  • If filing a bug report, I have included the output of :YcmDebugInfo.
  • If filing a bug report, I have attached the contents of the logfiles using
    the :YcmToggleLogs command.
  • If filing a bug report, I have included which OS (including specific OS
    version) I am using.
  • If filing a bug report, I have included a minimal test case that reproduces
    my issue, using vim -Nu /path/to/YCM/vimrc_ycm_minimal, including what I
    expected to happen and what actually happened.
  • If filing a installation failure report, I have included the entire output
    of instdeleteall.py (or cmake/make/ninja) including its invocation
  • I understand this is an open-source project staffed by volunteers and
    that any help I receive is a selfless, heartfelt gift of their free time. I
    know I am not entitled to anything and will be polite and courteous.
  • I understand my issue may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Thank you for adhering to this process! It ensures your issue is resolved
quickly and that neither your nor our time is needlessly wasted.

Issue Details

  • About workspace.workspaceEdit.resourceOperations

/**

  • The resource operations the client supports. Clients should at least
  • support 'create', 'rename' and 'delete' files and folders.
  • @SInCE 3.13.0
    */
    resourceOperations?: ResourceOperationKind[];
  • Why
    In Java, a rename operation is needed to refactor the name of a Java Class. Currently, since there is no support of rename operation, eclipse.jdt.ls won't return the rename operation. We must rename the file manually after a refactor. I think this problem may exist in other language too.

Is there any plan to support workspace.workspaceEdit.resourceOperations?

@bstaletic
Copy link
Collaborator

Hmm... so far, we have not implemented those for a mix of many reasons:

  • No personal need for such has been encountered.
  • The API by which YCM talks to ycm-core/ycmd does not currently support that either.
  • These actions can be quite intrusive. What if a server decides to delete the only currently open file?
    • Also, what does delete mean? Delete file? Delete buffer?

None of which are a hard "no".

Would you mind sharing a minimal example and steps that would make jdt respond with a rename operation?

@puremourning
Copy link
Member

Can probably just rename a class in Java as the files have to match the class name.

@fortime
Copy link
Author

fortime commented Sep 24, 2024

Here is the steps:

  1. Apply this patch to ycmd, commit id: 8b61f198. The commit id of YouCompleteMe is 63ab13e.
diff --git a/ycmd/completers/java/java_completer.py b/ycmd/completers/java/java_completer.py
index 33803957..03dbc1c4 100644
--- a/ycmd/completers/java/java_completer.py
+++ b/ycmd/completers/java/java_completer.py
@@ -660,3 +660,13 @@ class JavaCompleter( language_server_completer.LanguageServerCompleter ):
           *language_server_completer._LspLocationToLocationAndDescription(
             request_data, item[ 'from' ] ) )
     return result
+
+
+  def ExtraCapabilities( self ):
+    return {
+      'workspace': {
+        'workspaceEdit': {
+          'resourceOperations': [ 'create', 'rename', 'delete' ],
+        }
+      }
+    }
diff --git a/ycmd/completers/language_server/language_server_completer.py b/ycmd/completers/language_server/language_server_completer.py
index 489e15d0..ebc45b11 100644
--- a/ycmd/completers/language_server/language_server_completer.py
+++ b/ycmd/completers/language_server/language_server_completer.py
@@ -3587,6 +3587,8 @@ def WorkspaceEditToFixIt( request_data,
   """Converts a LSP workspace edit to a ycmd FixIt suitable for passing to
   responses.BuildFixItResponse."""

+  LOGGER.info( 'workspace_edit: %s', workspace_edit )
+
   if not workspace_edit:
     return None
  1. Create a maven project. The rename operation won't be returned in a single java source file.
mvn archetype:generate -DgroupId=test -DartifactId=test-resource-op -DarchetypeArtifactId=maven-archetype-quickstart -DarchetypeVersion=1.5 -DinteractiveMode=false
  1. Go into the project. And open a java file with vim.
cd test-resource-op
vim src/main/java/test/App.java
  1. Move the cursor to the class name, and run YcmCompleter RefactorRename App1. The rename operation will appear in the log file.

BTW, there is another issue here. In WorkspaceEditToFixIt, it checks if there is changes in the response. In above case, jdt returns both changes and documentChanges, and documentChanges is being ignored.

@puremourning
Copy link
Member

BTW, there is another issue here. In WorkspaceEditToFixIt

Hardly surprising given that is only used with the capability you just added.

@bstaletic
Copy link
Collaborator

In above case, jdt returns both changes and documentChanges, and documentChanges is being ignored.

Well... that's another case where JDT is violating the protocol...

The edit should either provide changes or documentChanges

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspaceEdit

@bstaletic
Copy link
Collaborator

It's not pretty, but the simplest java rename case works.
Request for comments/feedback!
#4271
ycm-core/ycmd#1766

@fortime
Copy link
Author

fortime commented Oct 14, 2024

It's not pretty, but the simplest java rename case works. Request for comments/feedback! #4271 ycm-core/ycmd#1766

Thank you for your work @bstaletic .

Actually, I open this issue for discussing whether this feature was going to support, and how it should be implemented, and then I can make a PR for it. It will introduce changes both in the client side and the server side. In the current design of Refactor, changes won't be written to the file. But with file operations, files will be changed instantly.

While waiting for a response, I implemented my own version.
fortime@092824f
fortime/ycmd@7694713

In my version, I still keep the sort of chunks. But I split chunks into 3 parts: 1. normal one, 2. chunks related to rename operations, 3. chunks related to delete operations. And I also replace the buffer in the opening window in a rename operation.

I hope these codes may be of some help to you.

@bstaletic
Copy link
Collaborator

It's not pretty, but the simplest java rename case works.

To correct myself, everything works, as long as the chunks received are such that sorting does not matter.

@fortime Thanks for the effort! I'll have to take a closer look at your implementation later, but one thing in your comment sticks out:

In my version, I still keep the sort of chunks.

LSP protocol says we need to apply chunks in order of appearance.
More than that, I can easily imagine some cases where sorting would be wrong.

  1. Apply some change to foo.cpp
  2. Rename foo.cpp to bar.cpp
  3. Apply some change to the new bar.cpp.
  4. Rename bar.cpp to... foo.cpp
  5. Apply more changes to foo.cpp

If you split work into three categories and then sort the chunks by file name, you will lose the point that in between changes to foo.cpp there should have been more actions.
I do think that scenario is pretty insane, but it's completely allowed by the LSP protocol.

And I also replace the buffer in the opening window in a rename operation.

That might be a good idea. Will have to play with it.

@fortime
Copy link
Author

fortime commented Oct 14, 2024

LSP protocol says we need to apply chunks in order of appearance. More than that, I can easily imagine some cases where sorting would be wrong.

  1. Apply some change to foo.cpp
  2. Rename foo.cpp to bar.cpp
  3. Apply some change to the new bar.cpp.
  4. Rename bar.cpp to... foo.cpp
  5. Apply more changes to foo.cpp

If you split work into three categories and then sort the chunks by file name, you will lose the point that in between changes to foo.cpp there should have been more actions. I do think that scenario is pretty insane, but it's completely allowed by the LSP protocol.

I think i has handled this case. The "chunks of rename" is actually a list of filename and chunks tuples. I will keep the chunks of the old file. Sorting will be happened in each tuple accordingly.

  1. Apply some change to foo.cpp

chunks will be saved in the chunks_by_file.

  1. Rename foo.cpp to bar.cpp

a. chunks of bar.cpp will be removed from chunks_by_file and rename_chunks_by_file.
b. chunks of foo.cpp will be popped from chunks_by_file.
c. A [('foo.cpp', chunks),('bar.cpp',[])] will be assigned to rename_chunks_by_file with key foo.cpp.

  1. Apply some change to the new bar.cpp.

chunks will be added to the last tuple of bar.cpp.

  1. Rename bar.cpp to... foo.cpp

a. chunks of foo.cpp will be removed from chunks_by_file and rename_chunks_by_file.
b. chunks of bar.cpp will be popped from rename_chunks_by_file. Assume the value is old_path_chunks.
c. append ('foo.cpp',[]) to old_path_chunks.
d. assign old_path_chunks to foo.cpp in rename_chunks_by_file.

  1. Apply more changes to foo.cpp

chunks will be added to the last tuple of foo.cpp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants