From 662e63e5ef0d1736b9c4fee8ff22af0fe669e489 Mon Sep 17 00:00:00 2001 From: Jeff Johnston Date: Fri, 10 Jan 2025 19:29:14 -0500 Subject: [PATCH] Fix inline method to attempt to copy over NLS comments when appropriate (#1928) - change SourceProvider.getExpressionRanges() to add in comments of return statement when NLS comments are present - change CallInliner.replaceCall() to try and split the last block at a semi-colon in case it was created by a return statement that had NLS comments - if the split results in 2 segments, try and form a new statement that adds the new expression and NLS comments - add new test to InlineMethodTests - does not handle modifying existing NLS markers at target - fixes #1856 --- .../corext/refactoring/code/CallInliner.java | 79 +++++++++++++------ .../refactoring/code/SourceProvider.java | 29 ++++++- .../TestCases/bugs_in/Test_issue_1856_1.java | 11 +++ .../TestCases/bugs_out/Test_issue_1856_1.java | 11 +++ .../tests/refactoring/InlineMethodTests.java | 7 +- 5 files changed, 112 insertions(+), 25 deletions(-) create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_in/Test_issue_1856_1.java create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_out/Test_issue_1856_1.java diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/CallInliner.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/CallInliner.java index 2eda5f518af..901c6c4f8ea 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/CallInliner.java +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/CallInliner.java @@ -46,6 +46,7 @@ import org.eclipse.ltk.core.refactoring.RefactoringStatus; import org.eclipse.ltk.core.refactoring.RefactoringStatusEntry; +import org.eclipse.jdt.core.IBuffer; import org.eclipse.jdt.core.ICompilationUnit; import org.eclipse.jdt.core.JavaModelException; import org.eclipse.jdt.core.dom.AST; @@ -659,6 +660,7 @@ private void replaceCall(RefactoringStatus status, String[] blocks, TextEditGrou } } else { ASTNode node= null; + boolean needsMethodInvocation= true; for (int i= 0; i < blocks.length - 1; i++) { node= fRewrite.createStringPlaceholder(blocks[i], ASTNode.RETURN_STATEMENT); fListRewrite.insertAt(node, fInsertionIndex++, textEditGroup); @@ -690,33 +692,63 @@ private void replaceCall(RefactoringStatus status, String[] blocks, TextEditGrou status.addWarning(RefactoringCoreMessages.CallInliner_cannot_synchronize_error, JavaStatusContext.create(fCUnit, fInvocation)); } - node= fRewrite.createStringPlaceholder(block, ASTNode.METHOD_INVOCATION); - - // fixes bug #24941 - if (needsExplicitCast(status)) { - AST ast= node.getAST(); - CastExpression castExpression= ast.newCastExpression(); - Type returnType= fImportRewrite.addImport(fSourceProvider.getReturnType(), ast); - castExpression.setType(returnType); - - if (NecessaryParenthesesChecker.needsParentheses(fSourceProvider.getReturnExpressions().get(0), castExpression, CastExpression.EXPRESSION_PROPERTY)) { - ParenthesizedExpression parenthesized= ast.newParenthesizedExpression(); - parenthesized.setExpression((Expression)node); - node= parenthesized; + String[] segments= block.split(";"); //$NON-NLS-1$ + if (segments.length == 2 && !segments[1].trim().isEmpty()) { + Statement targetStatement= ASTNodes.getFirstAncestorOrNull(fTargetNode, Statement.class); + if (targetStatement != null) { + ASTNode root= fTargetNode.getRoot(); + if (root instanceof CompilationUnit cu && cu.getJavaElement() instanceof ICompilationUnit icu) { + int start= targetStatement.getStartPosition(); + int length= targetStatement.getLength(); + String targetStatementString= null; + try { + IBuffer buffer= icu.getBuffer(); + targetStatementString= buffer.getText(start, length); + } catch (JavaModelException e) { + // should never occur + } + if (targetStatementString != null) { + int expOffset= fTargetNode.getStartPosition() - start; + String newTargetStatement= targetStatementString.substring(0, expOffset); + newTargetStatement += segments[0]; + newTargetStatement += targetStatementString.substring(expOffset + fTargetNode.getLength()); + newTargetStatement += " " + segments[1].trim(); //$NON-NLS-1$ + fTargetNode= targetStatement; + node= fRewrite.createStringPlaceholder(newTargetStatement, targetStatement.getNodeType()); + needsMethodInvocation= false; + } + } } + } + if (needsMethodInvocation) { + node= fRewrite.createStringPlaceholder(block, ASTNode.METHOD_INVOCATION); + + // fixes bug #24941 + if (needsExplicitCast(status)) { + AST ast= node.getAST(); + CastExpression castExpression= ast.newCastExpression(); + Type returnType= fImportRewrite.addImport(fSourceProvider.getReturnType(), ast); + castExpression.setType(returnType); + + if (NecessaryParenthesesChecker.needsParentheses(fSourceProvider.getReturnExpressions().get(0), castExpression, CastExpression.EXPRESSION_PROPERTY)) { + ParenthesizedExpression parenthesized= ast.newParenthesizedExpression(); + parenthesized.setExpression((Expression)node); + node= parenthesized; + } - castExpression.setExpression((Expression)node); - node= castExpression; + castExpression.setExpression((Expression)node); + node= castExpression; - if (NecessaryParenthesesChecker.needsParentheses(castExpression, fTargetNode.getParent(), fTargetNode.getLocationInParent())) { - ParenthesizedExpression parenthesized= ast.newParenthesizedExpression(); - parenthesized.setExpression((Expression)node); - node= parenthesized; + if (NecessaryParenthesesChecker.needsParentheses(castExpression, fTargetNode.getParent(), fTargetNode.getLocationInParent())) { + ParenthesizedExpression parenthesized= ast.newParenthesizedExpression(); + parenthesized.setExpression((Expression)node); + node= parenthesized; + } + } else if (fSourceProvider.needsReturnedExpressionParenthesis(fTargetNode.getParent(), fTargetNode.getLocationInParent())) { + ParenthesizedExpression pExp= fTargetNode.getAST().newParenthesizedExpression(); + pExp.setExpression((Expression)node); + node= pExp; } - } else if (fSourceProvider.needsReturnedExpressionParenthesis(fTargetNode.getParent(), fTargetNode.getLocationInParent())) { - ParenthesizedExpression pExp= fTargetNode.getAST().newParenthesizedExpression(); - pExp.setExpression((Expression)node); - node= pExp; } } else if (fContext.callMode == ASTNode.YIELD_STATEMENT) { if (fBlock != null) { @@ -734,6 +766,7 @@ private void replaceCall(RefactoringStatus status, String[] blocks, TextEditGrou } } + // Now replace the target node with the source node if (node != null) { if (fTargetNode == null) { diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceProvider.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceProvider.java index 096618ce468..a75c4b48aeb 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceProvider.java +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceProvider.java @@ -50,6 +50,7 @@ import org.eclipse.ltk.core.refactoring.RefactoringStatus; import org.eclipse.jdt.core.ICompilationUnit; +import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.IJavaProject; import org.eclipse.jdt.core.IPackageDeclaration; import org.eclipse.jdt.core.IType; @@ -77,6 +78,7 @@ import org.eclipse.jdt.core.dom.IVariableBinding; import org.eclipse.jdt.core.dom.IfStatement; import org.eclipse.jdt.core.dom.LabeledStatement; +import org.eclipse.jdt.core.dom.LambdaExpression; import org.eclipse.jdt.core.dom.MethodDeclaration; import org.eclipse.jdt.core.dom.MethodInvocation; import org.eclipse.jdt.core.dom.Modifier; @@ -855,11 +857,36 @@ private List getExpressionRanges() { } if (rs != null) { Expression exp= rs.getExpression(); - result.add(createRange(exp, exp)); + if (!(exp instanceof LambdaExpression) && hasNLS(rs)) { + result.add(createRange(exp, rs)); + } else { + result.add(createRange(exp, exp)); + } } return result; } + private boolean hasNLS(Statement s) { + ASTNode root= s.getRoot(); + if (root instanceof CompilationUnit) { + CompilationUnit unit= (CompilationUnit)root; + int start= unit.getExtendedStartPosition(s); + int length = unit.getExtendedLength(s); + IJavaElement element= unit.getJavaElement(); + if (element != null && element instanceof ICompilationUnit icu) { + try { + String statement= icu.getBuffer().getText(start, length); + if (statement.matches(".*?//\\s?\\$NON-NLS-\\d+\\$.*$")) { //$NON-NLS-1$ + return true; + } + } catch (IndexOutOfBoundsException | JavaModelException e) { + // do nothing + } + } + } + return false; + } + private IRegion createRange(List statements, int end) { ASTNode first= statements.get(0); ASTNode last= statements.get(end); diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_in/Test_issue_1856_1.java b/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_in/Test_issue_1856_1.java new file mode 100644 index 00000000000..2038d13335e --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_in/Test_issue_1856_1.java @@ -0,0 +1,11 @@ +package bugs_in; + +public class Test_issue_1856_1 { + String foo() { + return "abc"; //$NON-NLS-1$ + } + String bar() { + String result = /*]*/foo()/*[*/; + return result; + } +} diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_out/Test_issue_1856_1.java b/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_out/Test_issue_1856_1.java new file mode 100644 index 00000000000..f0590f8e000 --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_out/Test_issue_1856_1.java @@ -0,0 +1,11 @@ +package bugs_in; + +public class Test_issue_1856_1 { + String foo() { + return "abc"; //$NON-NLS-1$ + } + String bar() { + String result = /*]*/"abc"; //$NON-NLS-1$ + return result; + } +} diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/InlineMethodTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/InlineMethodTests.java index 0b859a2a073..bad0fbbf5b8 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/InlineMethodTests.java +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/InlineMethodTests.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2024 IBM Corporation and others. + * Copyright (c) 2000, 2025 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -497,6 +497,11 @@ public void test_issue_1529_2() throws Exception { performInvalidTest(); } + @Test + public void test_issue_1856_1() throws Exception { + performBugTest(); + } + /* *********************** Argument Tests ******************************* */ private void performArgumentTest() throws Exception {