Support refactoring when renaming or moving files #1445

testforstephen · 2020-05-15T09:44:33Z

Signed-off-by: Jinbo Wang [email protected]

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java

fbricon · 2020-05-15T22:32:46Z

org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandlerTest.java

@@ -163,7 +163,7 @@ public void testMoveFile() throws JavaModelException, BadLocationException {
 		assertNotNull(refactorEdit);
 		assertNotNull(refactorEdit.edit);
 		List<Either<TextDocumentEdit, ResourceOperation>> changes = refactorEdit.edit.getDocumentChanges();
-		assertEquals(4, changes.size());
+		assertEquals(3, changes.size());


why would this change?

The move refactoring code generates some empty textEdit operation, for example, insert empty string at the same location. I refined the ChangeUtils utility to filter out such empty operations. That's why you see the changes list is shorter.

testforstephen · 2020-05-18T14:32:36Z

test this please

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java

testforstephen · 2021-02-24T06:09:14Z

test this please

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java

rgrunber · 2021-03-03T21:31:23Z

Just a few things I've noticed from trying this out :

Performing a file rename, choosing to preview the change, and then discarding the proposed refactorings doesn't undo the rename itself. Not sure if this is intended behaviour in this case though.
The Refactor Preview entries seem to display more than just the actual changes when the change occurs in an import
Also, even after accepting the refactoring with 'OK', the changes are left unsaved in the files, so an additional save (all) needs to be done manually

I also see the following error in the output :

[Info  - 4:22:49 PM] Mar 3, 2021, 4:22:49 PM >> document/documentSymbol
[Trace - 4:22:49 PM] Received response 'textDocument/documentSymbol - (1070)' in 5ms. Request failed: The request (id: 1070, method: 'textDocument/documentSymbol') has been cancelled (-32800).
...
...
[Error - 4:22:49 PM] Mar 3, 2021, 4:22:49 PM Problem getting outline forUtil.java
File not found: /tmp/foo/src/org/foo/bar/biz/Util.java.
Java Model Exception: Core Exception [code 368] File not found: /tmp/foo/src/org/foo/bar/biz/Util.java.
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1214)
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1186)
	at org.eclipse.jdt.internal.core.CompilationUnit.openBuffer(CompilationUnit.java:1218)
	at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:114)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:266)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.ls.core.internal.handlers.DocumentSymbolHandler.getHierarchicalOutline(DocumentSymbolHandler.java:133)
	at org.eclipse.jdt.ls.core.internal.handlers.DocumentSymbolHandler.documentSymbol(DocumentSymbolHandler.java:78)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$13(JDTLanguageServer.java:633)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:75)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Caused by: org.eclipse.core.internal.resources.ResourceException: File not found: /tmp/foo/src/org/foo/bar/biz/Util.java.
	at org.eclipse.core.internal.localstore.FileSystemResourceManager.read(FileSystemResourceManager.java:835)
	at org.eclipse.core.internal.resources.File.getContents(File.java:275)
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1212)
	... 19 more
Caused by: java.lang.Exception: File not found: /tmp/foo/src/org/foo/bar/biz/Util.java.
	at org.eclipse.core.internal.resources.ResourceException.provideStackTrace(ResourceException.java:42)
	at org.eclipse.core.internal.resources.ResourceException.<init>(ResourceException.java:38)
	... 22 more
Caused by: org.eclipse.core.internal.resources.ResourceException(/foo_caeb84bf/_/src/org/foo/bar/biz/Util.java)[368]: java.lang.Exception: File not found: /tmp/foo/src/org/foo/bar/biz/Util.java.
	at org.eclipse.core.internal.resources.ResourceException.provideStackTrace(ResourceException.java:42)
	at org.eclipse.core.internal.resources.ResourceException.<init>(ResourceException.java:38)
	at org.eclipse.core.internal.localstore.FileSystemResourceManager.read(FileSystemResourceManager.java:835)
	at org.eclipse.core.internal.resources.File.getContents(File.java:275)
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1212)
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1186)
	at org.eclipse.jdt.internal.core.CompilationUnit.openBuffer(CompilationUnit.java:1218)
	at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:114)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:266)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.ls.core.internal.handlers.DocumentSymbolHandler.getHierarchicalOutline(DocumentSymbolHandler.java:133)
	at org.eclipse.jdt.ls.core.internal.handlers.DocumentSymbolHandler.documentSymbol(DocumentSymbolHandler.java:78)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$13(JDTLanguageServer.java:633)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:75)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

testforstephen · 2021-03-04T14:04:49Z

Performing a file rename, choosing to preview the change, and then discarding the proposed refactorings doesn't undo the rename itself. Not sure if this is intended behaviour in this case though.

This is by design. Clicking discard button just mean skipping the refactoring changes. It won't stop the file move operation.

The Refactor Preview entries seem to display more than just the actual changes when the change occurs in an import

The preview just shows what we give to it. Do you mean the refactoring job returns some unnecessary changes in the workspace edit? Or just about the displayed diff label for the change?

Also, even after accepting the refactoring with 'OK', the changes are left unsaved in the files, so an additional save (all) needs to be done manually.

We just use the client's file API FileWillRenameEvent.waitUntil to return a workspace edit. The subsequent actions are controlled by the client itself. So the save behavior is a decision from the client implementation. So far, VS Code won't save the edit, If there is more people think it's necessary to auto save the refactoring changes, we can open a feature request in VS Code for saving the rename edit.

Last one about the documentSymbol exception, looks like the client sends an extra document symbol request while the file is moving. There is race condition, where the disk file has been moved when dealing with the document symbol for the old file uri. Since the documentSymbol for old uri is no meaning during move, this failure won't affect any UI experience. There could three possible solutions to fix this:

Ask the client not to send documentSymbol for old uri when moving files.
JDT language server checks whether the file exists to guard the exception.
Keep the exception because it reflects the actual LSP status.

rgrunber · 2021-03-04T16:05:51Z

Ok, then the majority of the points are fine as long as it's expected, or not harmful. It looks like needing to save all the files is the same even prior to this change so should be ok for now.

The Refactor Preview entries seem to display more than just the actual changes when the change occurs in an import

The preview just shows what we give to it. Do you mean the refactoring job returns some unnecessary changes in the workspace edit? Or just about the displayed diff label for the change?

After trying a current release, it seems the behaviour of the diff label is the same so maybe this can be addressed separately. But yes, the labels seem long. I thought they would have just show the text being changed, but I seem to also be getting 'public class Main', in the demo above, though it remains unchanged.

testforstephen · 2021-03-05T04:39:39Z

After trying a current release, it seems the behaviour of the diff label is the same so maybe this can be addressed separately. But yes, the labels seem long. I thought they would have just show the text being changed, but I seem to also be getting 'public class Main', in the demo above, though it remains unchanged.

Look at your sample again, actually it updates two references in Main.java. One is the import reference for Util, another is the class reference from getTest() body. It seems we merge these two changes into one long text change in the final WorkspaceEdit. That's why you see a long diff label "Helper;\n\npublic class Main {\n \n public String getTest() {\n return Helper".

After debugging jdt.ls code, looks like the original JDT refactoring utilities return a MultiTextEdit to update the references in Main.java, which contains two child replace edits for import update and getText() body update. But when we use the TextEditConverter helper to convert the eclipse TextEdit to lsp WorkspaceEdit, that MultiTextEdit is converted to one change finally.

Since this is the converter quality issue of the helper method TextEditConverter, i'd suggest to improve it with a separate issue.

testforstephen · 2021-03-12T08:32:09Z

With the latest commit, the "File Not Found" exception above is suppressed.

@rgrunber Could you take a look again to see whether there is any more improvement to fix before we merge it?

Signed-off-by: Jinbo Wang <[email protected]>

testforstephen · 2021-03-22T07:26:56Z

The latest commit has adopted the lsp4j interfaces to handle willRenameFiles request. The last mile is to register FileOperationsServerCapabilities to notify the client we support willRenameFiles file operation.

But there is an issue eclipse-lsp4j/lsp4j#522 in lsp4j to block the adoption of FileOperationsServerCapabilities. What i want is to only listen to "**/*.java" files and "**" folders in file watcher. Currently there is no way to specify a glob pattern for files only. I already pushed a fix to lsp4j, not sure how soon a new lsp4j release will come.

My thought is that we keep using client-side API workspace.onWillRenameFiles to trigger workspace/willRenameFiles request and get this feature out first. Once lsp4j fix is ready, we can add back FileOperationsServerCapabilities and retire the client-side listener.

fbricon requested changes May 15, 2020

View reviewed changes

testforstephen marked this pull request as ready for review May 18, 2020 14:32

testforstephen mentioned this pull request May 18, 2020

Enable refactor when moving files redhat-developer/vscode-java#1454

Closed

kuafuwang reviewed Jul 1, 2020

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java Outdated Show resolved Hide resolved

testforstephen force-pushed the jinbo_move branch from e4252d7 to faa7b9d Compare October 21, 2020 08:44

testforstephen force-pushed the jinbo_move branch from faa7b9d to 6a42fb3 Compare February 24, 2021 05:37

testforstephen changed the title ~~Support move files~~ Support refactoring when renaming or moving files Feb 24, 2021

testforstephen mentioned this pull request Feb 24, 2021

Rework rename/move refactoring support redhat-developer/vscode-java#1815

Merged

testforstephen requested review from snjeza, rgrunber and jdneo February 24, 2021 06:50

jdneo reviewed Mar 1, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java Outdated Show resolved Hide resolved

jdneo reviewed Mar 2, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandler.java Outdated Show resolved Hide resolved

testforstephen force-pushed the jinbo_move branch from 6a42fb3 to 3e8c692 Compare March 2, 2021 07:42

jdneo approved these changes Mar 16, 2021

View reviewed changes

testforstephen added this to the End March 2021 milestone Mar 17, 2021

testforstephen added 2 commits March 22, 2021 09:43

Support refactoring when renaming or moving files

53720c6

Signed-off-by: Jinbo Wang <[email protected]>

Adopt lsp4j interfaces to handle willRenameFiles event

be0fd0a

Signed-off-by: Jinbo Wang <[email protected]>

testforstephen force-pushed the jinbo_move branch from d5d79f6 to be0fd0a Compare March 22, 2021 06:55

testforstephen merged commit 30583b9 into eclipse-jdtls:master Mar 24, 2021

testforstephen deleted the jinbo_move branch March 24, 2021 02:52

testforstephen self-assigned this Apr 6, 2021

testforstephen mentioned this pull request Apr 15, 2021

TextEditConverter should preserve the granularity of text edit when converting textEdit to workspaceEdit #1726

Open

This was referenced Nov 13, 2021

Regression - support for files that aren't on disk #1939

Closed

jdt ls should not assume that the file exists on the disk #1815

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support refactoring when renaming or moving files #1445

Support refactoring when renaming or moving files #1445

testforstephen commented May 15, 2020

fbricon May 15, 2020

testforstephen May 16, 2020

testforstephen commented May 18, 2020

testforstephen commented Feb 24, 2021

rgrunber commented Mar 3, 2021 •

edited

Loading

testforstephen commented Mar 4, 2021

rgrunber commented Mar 4, 2021

testforstephen commented Mar 5, 2021 •

edited

Loading

testforstephen commented Mar 12, 2021

testforstephen commented Mar 22, 2021

Support refactoring when renaming or moving files #1445

Support refactoring when renaming or moving files #1445

Conversation

testforstephen commented May 15, 2020

fbricon May 15, 2020

Choose a reason for hiding this comment

testforstephen May 16, 2020

Choose a reason for hiding this comment

testforstephen commented May 18, 2020

testforstephen commented Feb 24, 2021

rgrunber commented Mar 3, 2021 • edited Loading

testforstephen commented Mar 4, 2021

rgrunber commented Mar 4, 2021

testforstephen commented Mar 5, 2021 • edited Loading

testforstephen commented Mar 12, 2021

testforstephen commented Mar 22, 2021

rgrunber commented Mar 3, 2021 •

edited

Loading

testforstephen commented Mar 5, 2021 •

edited

Loading