Skip to content

Commit

Permalink
Fix inline method to attempt to copy over NLS comments when appropria…
Browse files Browse the repository at this point in the history 
…te (#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
  • Loading branch information
@jjohnstn
jjohnstn authored Jan 11, 2025
1 parent 6f377b8 commit 662e63e
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 25 deletions.
79 changes: 56 additions & 23 deletions ...pulation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/CallInliner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
29 changes: 28 additions & 1 deletion ...ation/core extension/org/eclipse/jdt/internal/corext/refactoring/code/SourceProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -855,11 +857,36 @@ private List<IRegion> 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<Statement> statements, int end) {
ASTNode first= statements.get(0);
ASTNode last= statements.get(end);
Expand Down
11 changes: 11 additions & 0 deletions ...ests.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_in/Test_issue_1856_1.java
View file
Original file line number Diff line number Diff line change
@@ -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;
}
}
11 changes: 11 additions & 0 deletions ...sts.refactoring/resources/InlineMethodWorkspace/TestCases/bugs_out/Test_issue_1856_1.java
View file
Original file line number Diff line number Diff line change
@@ -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;
}
}
7 changes: 6 additions & 1 deletion ....tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/InlineMethodTests.java
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 662e63e

Please sign in to comment.