Add rename compilation unit support. #864

yaohaizh · 2018-11-22T15:27:32Z

@fbricon For resource rename support, I'll enable it in two steps: Firstly support compilation unit for most scenarios. Would like to roll out this feature ASAP.

For rename package, it need LSP support from these two issue:

snjeza · 2018-11-22T23:54:34Z

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

-		assertEquals(JDTUtils.toURI(cu), resourceChange.getCurrent());
+		Either<TextDocumentEdit, ResourceOperation> change = resourceChanges.get(2);
+		RenameFile resourceChange = (RenameFile) change.getRight();
+		assertEquals(JDTUtils.toURI(cu), resourceChange.getOldUri());
 		assertEquals(JDTUtils.toURI(cu).replace("E", "Newname"), resourceChange.getNewUri());


assertEquals(JDTUtils.toURI(cu).replace("E.java", "Newname.java"), resourceChange.getNewUri());

If the workspace name contains "E", the test will fail.

@snjeza Add test cases for failure situation.

I agree with @snjeza, the regexp in replace() call needs to be more specific

snjeza · 2018-11-23T17:45:49Z

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

 			create.run(monitor);
+			if (check.getStatus().getSeverity() >= RefactoringStatus.ERROR) {


if (check.getStatus().hasFatalError()) {

otherwise, a rename refactoring operation will fail if there is an invalid rename participant.

That's why the following four tests fail in Eclipse - testRenamePackage, testRenameType, testRenameOverrideMethodSimple and testRenameField.

The testRenameTypeWithResourceChanges test still fails in Eclipse, if the workspace name contains "E".

Could you try to run RenameHandlerTest in Eclipse using the workspace '${workspace_loc}/../junit-workspacE' name ?

snjeza · 2018-11-23T17:46:44Z

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

@@ -262,7 +260,42 @@ public void testRenameTypeWhithResourceChanges() throws JavaModelException, BadL
 		assertEquals(expected, TextEditUtil.apply(builder.toString(), testChanges));
 	}

-	@Test
+	@Test(expected = ResponseErrorException.class)
+	public void testRenameTypeWithrErrors() throws JavaModelException, BadLocationException {


testRenameTypeWithErrors()

Signed-off-by: Yaohai Zheng <[email protected]>

yaohaizh · 2018-11-26T14:26:21Z

@snjeza Updated the PR

fbricon · 2018-11-26T19:26:42Z

This now works much better! One problem I found though: if, from a file, you try to rename a package to something that already exists (effectively moving the type from one package to another), then the server complains the package already exists. I think it should just move the file to the new package if it already exists. Although this might go beyond the scope of the current PR. It's probably ok to handle that separately.

snjeza

The testRenameTypeWithResourceChanges test still fails, if the workspace name contains "E".

Could you try the following:

cd eclipse.jdt.ls
cd ..
mv eclipse.jdt.ls Eclipse.jdt.ls
cd Eclipse.jdt.ls
mvn clean verify

You will get the following:

Failed tests: 
  RenameHandlerTest.testRenameTypeWithResourceChanges:243 expected:<.../home/snjeza/projects/[Newname]clipse.jdt.ls/org.ec...> but was:<.../home/snpe/projects/[E]clipse.jdt.ls/org.ec...>

fbricon · 2018-11-26T19:37:30Z

ok so I was able to rename the package in package example; but not package ex.am.ple (Rename this element is not supported yet)

fbricon · 2018-11-26T20:48:35Z

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

+									context.setASTRoot(ast);
+									ASTNode node = context.getCoveredNode();
+									// Rename package is not fully support yet.
+									if (!(node instanceof PackageDeclaration) && !(node instanceof QualifiedName && node.getParent() instanceof PackageDeclaration)) {


when renaming package example;, example is found to be a SimpleName instead of a QualifiedName, so it goes through this condition

fbricon · 2018-11-26T20:49:25Z

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

+				JavaLanguageServerPlugin.logException("Problem with compute occurrences for" + unit.getElementName() + " in prepareRename", e);
+			}
+		}
+		throw new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidRequest, "Rename this element is not supported yet.", null));


Renaming this element is not supported.

snjeza · 2018-11-27T12:36:37Z

test this please

yaohaizh · 2018-11-27T14:13:13Z

@snjeza @fbricon Updated the PR

fbricon · 2018-11-27T14:28:13Z

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

+				JavaLanguageServerPlugin.logException("Problem with compute occurrences for" + unit.getElementName() + " in prepareRename", e);
+			}
+		}
+		throw new ResponseErrorException(new ResponseError(ResponseErrorCode.InvalidRequest, "Rename this element is not supported.", null));


s/Rename/Renaming

fbricon · 2018-11-27T14:28:43Z

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

+				}
+
+			} catch (CoreException e) {
+				JavaLanguageServerPlugin.logException("Problem with compute occurrences for" + unit.getElementName() + " in prepareRename", e);


Problem computing occurrences for

fbricon · 2018-11-27T14:28:58Z

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

+									InnovationContext context = new InnovationContext(unit, loc.getOffset(), loc.getLength());
+									context.setASTRoot(ast);
+									ASTNode node = context.getCoveredNode();
+									// Rename package is not fully support yet.


Renaming package is not fully supported yet.

yaohaizh · 2018-11-27T15:38:31Z

test this please

yaohaizh · 2018-11-27T15:51:27Z

@fbricon Updated the PR

Signed-off-by: Fred Bricon <[email protected]>

fbricon · 2018-11-27T22:45:39Z

@snjeza can you please check the last changes I pushed to this PR?

fbricon · 2018-11-27T23:37:13Z

if you try to rename Exception in public class MyException extends Exception, the rename query returns an internal error.

[Error - 6:36:21 PM] Request textDocument/rename failed.
  Message: Internal error.
  Code: -32603 
java.util.concurrent.CompletionException: java.lang.NullPointerException
	at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:273)
	at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:280)
	at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:604)
	at java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
	at java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:443)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
Caused by: java.lang.NullPointerException
	at org.eclipse.jdt.internal.corext.refactoring.Checks.checkIfCuBroken(Checks.java:559)
	at org.eclipse.jdt.ls.core.internal.corext.refactoring.rename.RenameTypeProcessor.checkInitialConditions(RenameTypeProcessor.java:475)
	at org.eclipse.ltk.core.refactoring.participants.ProcessorBasedRefactoring.checkInitialConditions(ProcessorBasedRefactoring.java:206)
	at org.eclipse.ltk.core.refactoring.Refactoring.checkAllConditions(Refactoring.java:161)
	at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:82)
	at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:122)
	at org.eclipse.jdt.ls.core.internal.handlers.RenameHandler.rename(RenameHandler.java:89)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$21(JDTLanguageServer.java:666)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$26(JDTLanguageServer.java:793)
	at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
	... 6 more

fbricon · 2018-11-28T00:04:28Z

@snjeza please check the new fix

fbricon · 2018-11-28T00:09:00Z

new fix is incomplete. Renaming java.lang.Exception is still allowed.

Signed-off-by: Fred Bricon <[email protected]>

fbricon · 2018-11-28T01:39:58Z

new fix handles FQCN renames

Add rename compilation unit support.

a3575c8

snjeza self-requested a review November 22, 2018 23:48

snjeza reviewed Nov 22, 2018

View reviewed changes

snjeza reviewed Nov 23, 2018

View reviewed changes

Add error handling for rename operation.

5142e40

Signed-off-by: Yaohai Zheng <[email protected]>

yaohaizh force-pushed the rename_unit branch from d5f386d to 5142e40 Compare November 26, 2018 02:27

Add test cases for prepare rename.

ea7725c

snjeza reviewed Nov 26, 2018

View reviewed changes

fbricon reviewed Nov 26, 2018

View reviewed changes

fbricon reviewed Nov 27, 2018

View reviewed changes

Address review feedback.

2a2712c

yaohaizh force-pushed the rename_unit branch from 4cf9bb3 to 2a2712c Compare November 27, 2018 15:17

Better handle case of package renaming

813d6a6

Signed-off-by: Fred Bricon <[email protected]>

snjeza approved these changes Nov 27, 2018

View reviewed changes

Don't allow renaming classfiles

b0604c9

Signed-off-by: Fred Bricon <[email protected]>

fbricon force-pushed the rename_unit branch from 54212cc to b0604c9 Compare November 28, 2018 01:09

snjeza approved these changes Nov 28, 2018

View reviewed changes

fbricon merged commit c9c5b0a into master Nov 28, 2018

fbricon deleted the rename_unit branch November 28, 2018 06:07

This was referenced Nov 28, 2018

Add rename support #43

Closed

'Force Java compilation' breaks diagnostics in dirty files #834

Closed

Move Refactoring #363

Closed

yyoncho mentioned this pull request Dec 14, 2018

Add rename compilation unit support emacs-lsp/lsp-java#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rename compilation unit support. #864

Add rename compilation unit support. #864

yaohaizh commented Nov 22, 2018 •

edited

Loading

snjeza Nov 22, 2018

yaohaizh Nov 23, 2018

fbricon Nov 26, 2018

snjeza Nov 23, 2018

snjeza Nov 23, 2018

yaohaizh commented Nov 26, 2018

fbricon commented Nov 26, 2018 •

edited

Loading

snjeza left a comment •

edited

Loading

fbricon commented Nov 26, 2018

fbricon Nov 26, 2018 •

edited

Loading

fbricon Nov 26, 2018

snjeza commented Nov 27, 2018

yaohaizh commented Nov 27, 2018

fbricon Nov 27, 2018

fbricon Nov 27, 2018

fbricon Nov 27, 2018 •

edited

Loading

yaohaizh commented Nov 27, 2018

yaohaizh commented Nov 27, 2018

fbricon commented Nov 27, 2018

fbricon commented Nov 27, 2018

fbricon commented Nov 28, 2018

fbricon commented Nov 28, 2018

fbricon commented Nov 28, 2018

		create.run(monitor);
		if (check.getStatus().getSeverity() >= RefactoringStatus.ERROR) {

Add rename compilation unit support. #864

Add rename compilation unit support. #864

Conversation

yaohaizh commented Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaohaizh commented Nov 26, 2018

fbricon commented Nov 26, 2018 • edited Loading

snjeza left a comment • edited Loading

Choose a reason for hiding this comment

fbricon commented Nov 26, 2018

fbricon Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Nov 27, 2018

yaohaizh commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

yaohaizh commented Nov 27, 2018

yaohaizh commented Nov 27, 2018

fbricon commented Nov 27, 2018

fbricon commented Nov 27, 2018

fbricon commented Nov 28, 2018

fbricon commented Nov 28, 2018

fbricon commented Nov 28, 2018

yaohaizh commented Nov 22, 2018 •

edited

Loading

fbricon commented Nov 26, 2018 •

edited

Loading

snjeza left a comment •

edited

Loading

fbricon Nov 26, 2018 •

edited

Loading

fbricon Nov 27, 2018 •

edited

Loading