From 6a01c374cfd5d471503b9db76c7ead94db544c3f Mon Sep 17 00:00:00 2001 From: Morgan Chang Date: Fri, 7 Aug 2026 16:20:50 -0400 Subject: [PATCH] Support confirmation of move instance method refactoring errors Signed-off-by: Morgan Chang --- .../handlers/GetRefactorEditHandler.java | 8 ++ .../core/internal/handlers/MoveHandler.java | 48 +++++++--- .../handlers/RefactoringConfirmation.java | 73 +++++++++++++++ .../internal/handlers/MoveHandlerTest.java | 89 +++++++++++++++++++ 4 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/RefactoringConfirmation.java diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/GetRefactorEditHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/GetRefactorEditHandler.java index 081b44db0d..b0dd431c24 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/GetRefactorEditHandler.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/GetRefactorEditHandler.java @@ -309,6 +309,8 @@ public static class RefactorWorkspaceEdit { */ public Command command; public String errorMessage; + public Boolean canContinue; + public String confirmationToken; public RefactorWorkspaceEdit(WorkspaceEdit edit) { this.edit = edit; @@ -322,6 +324,12 @@ public RefactorWorkspaceEdit(WorkspaceEdit edit, Command command) { public RefactorWorkspaceEdit(String errorMessage) { this.errorMessage = errorMessage; } + + public RefactorWorkspaceEdit(String errorMessage, boolean canContinue, String confirmationToken) { + this.errorMessage = errorMessage; + this.canContinue = canContinue; + this.confirmationToken = confirmationToken; + } } public static class GetRefactorEditParams { diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandler.java index eeff31f754..b97ef6ca33 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandler.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandler.java @@ -91,6 +91,8 @@ import org.eclipse.ltk.core.refactoring.CreateChangeOperation; import org.eclipse.ltk.core.refactoring.Refactoring; import org.eclipse.ltk.core.refactoring.RefactoringStatus; +import org.eclipse.ltk.core.refactoring.RefactoringStatusEntry; + import org.eclipse.ltk.core.refactoring.participants.MoveRefactoring; import com.google.gson.Gson; @@ -291,7 +293,7 @@ public static RefactorWorkspaceEdit move(MoveParams moveParams, IProgressMonitor return new RefactorWorkspaceEdit("Invalid destination object: " + moveParams.destination); } - return moveInstanceMethod(moveParams.params, variableBinding, monitor); + return moveInstanceMethod(moveParams.params, variableBinding, moveParams.confirmationToken, monitor); } else if ("moveStaticMember".equalsIgnoreCase(moveParams.moveKind)) { String typeName = resolveTargetTypeName(moveParams.destination); return moveStaticMember(moveParams.params, typeName, monitor); @@ -436,7 +438,7 @@ public boolean confirm(String question, Object[] elements) throws OperationCance return null; } - private static RefactorWorkspaceEdit moveInstanceMethod(CodeActionParams params, LspVariableBinding destination, IProgressMonitor monitor) { + private static RefactorWorkspaceEdit moveInstanceMethod(CodeActionParams params, LspVariableBinding destination, String confirmationToken, IProgressMonitor monitor) { final ICompilationUnit unit = JDTUtils.resolveCompilationUnit(params.getTextDocument().getUri()); if (unit == null) { return new RefactorWorkspaceEdit("Failed to move instance method because cannot find the compilation unit associated with " + params.getTextDocument().getUri()); @@ -459,10 +461,14 @@ private static RefactorWorkspaceEdit moveInstanceMethod(CodeActionParams params, CheckConditionsOperation check = new CheckConditionsOperation(refactoring, CheckConditionsOperation.INITIAL_CONDITONS); try { check.run(subMonitor.split(20)); - if (check.getStatus().getSeverity() >= RefactoringStatus.FATAL) { + RefactoringStatus status = new RefactoringStatus(); + status.merge(check.getStatus()); + if (status.hasFatalError()) { JavaLanguageServerPlugin.logError("Failed to execute the 'move' refactoring."); - JavaLanguageServerPlugin.logError(check.getStatus().toString()); - return new RefactorWorkspaceEdit("Failed to move instance method. Reason: " + check.getStatus().toString()); + JavaLanguageServerPlugin.logError(status.toString()); + + List messages = Stream.of(status.getEntries()).map(RefactoringStatusEntry::getMessage).toList(); + return new RefactorWorkspaceEdit("Failed to move instance method. Reason:\n" + String.join(System.lineSeparator(), messages)); } IVariableBinding[] possibleTargets = processor.getPossibleTargets(); @@ -473,14 +479,26 @@ private static RefactorWorkspaceEdit moveInstanceMethod(CodeActionParams params, processor.setInlineDelegator(true); processor.setRemoveDelegator(true); check = new CheckConditionsOperation(refactoring, CheckConditionsOperation.FINAL_CONDITIONS); - check.run(subMonitor.split(60)); - if (check.getStatus().getSeverity() >= RefactoringStatus.FATAL) { + CreateChangeOperation create = new CreateChangeOperation(check, RefactoringStatus.FATAL); + create.run(subMonitor.split(80)); + status.merge(create.getConditionCheckingStatus()); + Change change = create.getChange(); + if (change == null) { JavaLanguageServerPlugin.logError("Failed to execute the 'move' refactoring."); - JavaLanguageServerPlugin.logError(check.getStatus().toString()); - return new RefactorWorkspaceEdit("Failed to move instance method. Reason: " + check.getStatus().toString()); + JavaLanguageServerPlugin.logError(status.toString()); + return new RefactorWorkspaceEdit("Failed to move instance method. Reason: " + status.toString()); + } + if (status.hasError()) { + String expectedConfirmationToken = RefactoringConfirmation.createToken("moveInstanceMethodConfirmation:v1", status, params, destination, unit.getSource()); + if (confirmationToken == null) { + List messages = Stream.of(status.getEntries()).map(RefactoringStatusEntry::getMessage).toList(); + return new RefactorWorkspaceEdit(String.join(System.lineSeparator(), messages), true, expectedConfirmationToken); + } + if (!Objects.equals(confirmationToken, expectedConfirmationToken)) { + return new RefactorWorkspaceEdit("Failed to move instance method because the source or refactoring conditions changed after the problems were shown. Run the refactoring again to review the current problems."); + } } - Change change = processor.createChange(subMonitor.split(20)); return new RefactorWorkspaceEdit(ChangeUtil.convertToWorkspaceEdit(change)); } else { return new RefactorWorkspaceEdit("Failed to move instance method because cannot find the target " + destination.name); @@ -727,6 +745,11 @@ public static class MoveParams { */ Object destination; boolean updateReferences; + /** + * An opaque token returned by a previous move request after the user reviewed + * its error-level condition-checking problems. + */ + String confirmationToken; public MoveParams(String moveKind, String[] sourceUris) { this(moveKind, sourceUris, null); @@ -745,11 +768,16 @@ public MoveParams(String moveKind, CodeActionParams params, Object destination, } public MoveParams(String moveKind, String[] sourceUris, CodeActionParams params, Object destination, boolean updateReferences) { + this(moveKind, sourceUris, params, destination, updateReferences, null); + } + + public MoveParams(String moveKind, String[] sourceUris, CodeActionParams params, Object destination, boolean updateReferences, String confirmationToken) { this.moveKind = moveKind; this.sourceUris = sourceUris; this.params = params; this.destination = destination; this.updateReferences = updateReferences; + this.confirmationToken = confirmationToken; } } } diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/RefactoringConfirmation.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/RefactoringConfirmation.java new file mode 100644 index 0000000000..a9030c714e --- /dev/null +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/RefactoringConfirmation.java @@ -0,0 +1,73 @@ +/******************************************************************************* + * Copyright (c) 2026 IBM Corporation and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * IBM Corporation - initial API and implementation + *******************************************************************************/ + +package org.eclipse.jdt.ls.core.internal.handlers; + +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.HexFormat; +import java.util.Objects; + +import org.eclipse.ltk.core.refactoring.RefactoringStatus; +import org.eclipse.ltk.core.refactoring.RefactoringStatusEntry; + +import com.google.gson.Gson; + +/** + * Adapts Eclipse LTK refactoring statuses to the language-server protocol and + * creates fingerprints for refactoring confirmations that span multiple client + * requests. + */ +public final class RefactoringConfirmation { + private static final Gson GSON = new Gson(); + + private RefactoringConfirmation() { + } + + /** + * Creates a deterministic fingerprint for the operation, its inputs, and the + * problems the user is being asked to confirm. + * + * @param operationId a versioned identifier for the confirmation protocol + * @param status the refactoring problems being confirmed + * @param confirmationInputs the request and workspace state that must still match + * @return a lowercase hexadecimal SHA-256 fingerprint + */ + public static String createToken(String operationId, RefactoringStatus status, Object... confirmationInputs) { + Objects.requireNonNull(operationId, "operationId"); + try { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + updateDigest(digest, operationId); + for (Object input : confirmationInputs) { + updateDigest(digest, GSON.toJson(input)); + } + if (status != null) { + for (RefactoringStatusEntry entry : status.getEntries()) { + updateDigest(digest, Integer.toString(entry.getSeverity())); + updateDigest(digest, entry.getMessage()); + } + } + return HexFormat.of().formatHex(digest.digest()); + } catch (NoSuchAlgorithmException e) { + throw new IllegalStateException("SHA-256 is not available", e); + } + } + + private static void updateDigest(MessageDigest digest, String value) { + byte[] bytes = Objects.toString(value, "").getBytes(StandardCharsets.UTF_8); + digest.update(ByteBuffer.allocate(Integer.BYTES).putInt(bytes.length).array()); + digest.update(bytes); + } +} diff --git a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandlerTest.java b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandlerTest.java index 60208443be..169a80c960 100644 --- a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandlerTest.java +++ b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/MoveHandlerTest.java @@ -15,6 +15,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -47,6 +48,7 @@ import org.eclipse.lsp4j.TextDocumentEdit; import org.eclipse.lsp4j.TextEdit; import org.eclipse.lsp4j.jsonrpc.messages.Either; +import org.eclipse.ltk.core.refactoring.RefactoringStatus; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -353,6 +355,93 @@ public void testMoveInstanceMethod() throws Exception { assertEquals(expected, TextEditUtil.apply(cuSecond.getSource(), textEdit.getEdits().stream().filter(Either::isLeft).map(Either::getLeft).toList())); } + @Test + public void testMoveInstanceMethodConfirmsTargetNullCheck() throws Exception { + IPackageFragment pack1 = sourceFolder.createPackageFragment("test1", false, null); + //@formatter:off + ICompilationUnit cu = pack1.createCompilationUnit("Demo.java", "package test1;\n" + + "\n" + + "public class Demo {\n" + + " public static void main(String[] args) {\n" + + " A a = new A();\n" + + " System.out.println(a.process());\n" + + " }\n" + + "}\n" + + "\n" + + "class A {\n" + + " C c;\n" + + "\n" + + " int process() {\n" + + " if (c != null) return 2;\n" + + " return 0;\n" + + " }\n" + + "}\n" + + "\n" + + "class B extends A {\n" + + "}\n" + + "\n" + + "class C extends B {\n" + + "}\n", + false, null); + //@formatter:on + CodeActionParams params = CodeActionUtil.constructCodeActionParams(cu, "c != null"); + MoveParams moveParams = new MoveParams("moveInstanceMethod", new String[] { JDTUtils.toURI(cu) }, params); + MoveDestinationsResponse response = MoveHandler.getMoveDestinations(moveParams); + assertNotNull(response); + assertNull(response.errorMessage); + assertNotNull(response.destinations); + assertEquals(1, response.destinations.length); + assertEquals("c", ((LspVariableBinding) response.destinations[0]).name); + + RefactorWorkspaceEdit refactorEdit = MoveHandler.move(new MoveParams("moveInstanceMethod", params, response.destinations[0], true), new NullProgressMonitor()); + assertNotNull(refactorEdit); + assertNull(refactorEdit.edit); + assertNotNull(refactorEdit.errorMessage); + assertTrue(refactorEdit.errorMessage.contains("compared to null"), refactorEdit.errorMessage); + assertEquals(Boolean.TRUE, refactorEdit.canContinue); + assertNotNull(refactorEdit.confirmationToken); + + String originalSource = cu.getSource(); + cu.getBuffer().setContents(originalSource + "\n// source changed\n"); + cu.save(null, true); + MoveParams staleConfirmation = new MoveParams("moveInstanceMethod", null, params, response.destinations[0], true, refactorEdit.confirmationToken); + RefactorWorkspaceEdit staleEdit = MoveHandler.move(staleConfirmation, new NullProgressMonitor()); + assertNotNull(staleEdit); + assertNull(staleEdit.edit); + assertNull(staleEdit.canContinue); + assertNull(staleEdit.confirmationToken); + assertTrue(staleEdit.errorMessage.contains("conditions changed"), staleEdit.errorMessage); + + cu.getBuffer().setContents(originalSource); + cu.save(null, true); + MoveParams confirmedMove = new MoveParams("moveInstanceMethod", null, params, response.destinations[0], true, refactorEdit.confirmationToken); + RefactorWorkspaceEdit confirmedEdit = MoveHandler.move(confirmedMove, new NullProgressMonitor()); + assertNotNull(confirmedEdit); + assertNotNull(confirmedEdit.edit); + assertNull(confirmedEdit.errorMessage); + List> changes = confirmedEdit.edit.getDocumentChanges(); + assertEquals(1, changes.size()); + TextDocumentEdit textEdit = changes.get(0).getLeft(); + assertNotNull(textEdit); + String movedSource = TextEditUtil.apply(originalSource, textEdit.getEdits().stream().filter(Either::isLeft).map(Either::getLeft).toList()); + assertTrue(movedSource.contains("return c.process();"), movedSource); + assertTrue(movedSource.contains("if (this != null) return 2;"), movedSource); + } + + @Test + public void testConfirmationTokenFingerprintsInputsAndProblems() { + RefactoringStatus status = new RefactoringStatus(); + status.addError("problem"); + + String token = RefactoringConfirmation.createToken("operation:v1", status, "request", "source"); + assertEquals(64, token.length()); + assertEquals(token, RefactoringConfirmation.createToken("operation:v1", status, "request", "source")); + assertNotEquals(token, RefactoringConfirmation.createToken("operation:v1", status, "request", "changed source")); + + status.addWarning("another problem"); + assertNotEquals(token, RefactoringConfirmation.createToken("operation:v1", status, "request", "source")); + } + @Test public void testMoveStaticMethod() throws Exception { IPackageFragment pack1 = sourceFolder.createPackageFragment("test1", false, null);