Skip to content

Commit bf34c50

Browse files
jjohnstnjjohnstn
authored
Fix add blocks when then statement is on same line as if and nls comment (eclipse-jdt#2064)
- modify ControlStatementsFix to look for NLS comments when we have then statement on same line as if because they will belong to then statement and not if and should not be blindly moved - add new test to CleanUpTest - fixes eclipse-jdt#2059
1 parent e8a094b commit bf34c50

File tree

2 files changed

+136
-18
lines changed

2 files changed

+136
-18
lines changed

‎org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/fix/ControlStatementsFix.java‎

Lines changed: 101 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,32 @@
1616

1717
import java.util.ArrayList;
1818
import java.util.List;
19+
import java.util.regex.Matcher;
20+
import java.util.regex.Pattern;
1921

2022
import org.eclipse.core.runtime.CoreException;
2123

2224
import org.eclipse.text.edits.TextEditGroup;
2325

2426
import org.eclipse.jdt.core.IBuffer;
27+
import org.eclipse.jdt.core.ICompilationUnit;
28+
import org.eclipse.jdt.core.IJavaElement;
2529
import org.eclipse.jdt.core.JavaCore;
30+
import org.eclipse.jdt.core.JavaModelException;
2631
import org.eclipse.jdt.core.dom.ASTNode;
32+
import org.eclipse.jdt.core.dom.ASTVisitor;
2733
import org.eclipse.jdt.core.dom.Block;
2834
import org.eclipse.jdt.core.dom.ChildPropertyDescriptor;
2935
import org.eclipse.jdt.core.dom.Comment;
3036
import org.eclipse.jdt.core.dom.CompilationUnit;
3137
import org.eclipse.jdt.core.dom.DoStatement;
3238
import org.eclipse.jdt.core.dom.EnhancedForStatement;
39+
import org.eclipse.jdt.core.dom.Expression;
3340
import org.eclipse.jdt.core.dom.ForStatement;
3441
import org.eclipse.jdt.core.dom.IfStatement;
3542
import org.eclipse.jdt.core.dom.ReturnStatement;
3643
import org.eclipse.jdt.core.dom.Statement;
44+
import org.eclipse.jdt.core.dom.StringLiteral;
3745
import org.eclipse.jdt.core.dom.ThrowStatement;
3846
import org.eclipse.jdt.core.dom.WhileStatement;
3947
import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
@@ -186,9 +194,10 @@ public AddBlockOperation(ChildPropertyDescriptor bodyProperty, Statement body, S
186194
public void rewriteASTInternal(CompilationUnitRewrite cuRewrite, LinkedProposalModelCore model) throws CoreException {
187195
ASTRewrite rewrite= cuRewrite.getASTRewrite();
188196
String label;
189-
ASTNode expression= null;
197+
Expression expression= null;
190198
int statementType= -1;
191199
int defaultStartPosition= fControlStatement.getStartPosition();
200+
CompilationUnit cuRoot= cuRewrite.getRoot();
192201
if (fBodyProperty == IfStatement.THEN_STATEMENT_PROPERTY) {
193202
label = FixMessages.CodeStyleFix_ChangeIfToBlock_desription;
194203
expression= ((IfStatement)fControlStatement).getExpression();
@@ -198,7 +207,7 @@ public void rewriteASTInternal(CompilationUnitRewrite cuRewrite, LinkedProposalM
198207
} else if (fBodyProperty == IfStatement.ELSE_STATEMENT_PROPERTY) {
199208
label = FixMessages.CodeStyleFix_ChangeElseToBlock_description;
200209
Statement thenStatement= ((IfStatement)fControlStatement).getThenStatement();
201-
defaultStartPosition= thenStatement.getStartPosition() + thenStatement.getLength();
210+
defaultStartPosition= cuRoot.getExtendedStartPosition(thenStatement) + cuRoot.getExtendedLength(thenStatement);
202211
} else {
203212
label = FixMessages.CodeStyleFix_ChangeControlToBlock_description;
204213
if (fBodyProperty == WhileStatement.BODY_PROPERTY) {
@@ -215,9 +224,14 @@ public void rewriteASTInternal(CompilationUnitRewrite cuRewrite, LinkedProposalM
215224

216225
TextEditGroup group= createTextEditGroup(label, cuRewrite);
217226
List<Comment> commentsToPreserve= new ArrayList<>();
218-
CompilationUnit cuRoot= cuRewrite.getRoot();
219227
int controlStatementLine= cuRoot.getLineNumber(fControlStatement.getStartPosition());
220228
int bodyLine= cuRoot.getLineNumber(fBody.getStartPosition());
229+
IBuffer cuBuffer= cuRewrite.getCu().getBuffer();
230+
// Get extended body text and convert multiple indent tabs to be just one tab as they will be relative to control statement
231+
String bodyString= cuBuffer.getText(cuRoot.getExtendedStartPosition(fBody), cuRoot.getExtendedLength(fBody))
232+
.replaceAll("\\r\\n|\\r|\\n(\\t|\\s)*", System.lineSeparator() + "\t"); //$NON-NLS-1$ //$NON-NLS-2$
233+
String commentString= ""; //$NON-NLS-1$
234+
boolean needToManuallyAddNLS= false;
221235
// If single body statement is on next line, we need to preserve any comments pertaining to the
222236
// control statement (e.g. NLS comment for if expression)
223237
if (controlStatementLine != bodyLine) {
@@ -228,31 +242,75 @@ public void rewriteASTInternal(CompilationUnitRewrite cuRewrite, LinkedProposalM
228242
if (commentLine == controlStatementLine && comment.getStartPosition() > startPosition &&
229243
comment.getStartPosition() < fBody.getStartPosition()) {
230244
commentsToPreserve.add(comment);
245+
String commentText= cuBuffer.getText(comment.getStartPosition(), comment.getLength());
246+
commentString += " " + commentText; //$NON-NLS-1$
247+
}
248+
}
249+
} else {
250+
// single body statement is on same line...we might have to keep an NLS comment on same line
251+
if (expression != null) {
252+
List<String> nlsList= fBodyProperty == IfStatement.ELSE_STATEMENT_PROPERTY ?
253+
getLineNLSComments(defaultStartPosition, expression) :
254+
getLineNLSComments(expression.getStartPosition() + expression.getLength(), expression);
255+
if (!nlsList.isEmpty()) {
256+
needToManuallyAddNLS= true;
257+
class Counter {
258+
public int count= 0;
259+
}
260+
final Counter counter= new Counter();
261+
ASTVisitor literalVisitor= new ASTVisitor() {
262+
@Override
263+
public boolean visit(StringLiteral node) {
264+
counter.count++;
265+
return false;
266+
}
267+
};
268+
expression.accept(literalVisitor);
269+
bodyString= cuBuffer.getText(cuRoot.getExtendedStartPosition(fBody),
270+
fBody.getStartPosition() - cuRoot.getExtendedStartPosition(fBody) + fBody.getLength());
271+
int lastNLSIndex= 0;
272+
for (String nlsString : nlsList) {
273+
int start= nlsString.indexOf("$NON-NLS-") + 9; //$NON-NLS-1$
274+
int index= start;
275+
while (Character.isDigit(nlsString.charAt(index))) {
276+
++index;
277+
}
278+
int nlsIndex= Integer.parseInt(nlsString.substring(start, index));
279+
if (nlsIndex <= counter.count) {
280+
commentString += " " + nlsString; //$NON-NLS-1$
281+
lastNLSIndex= nlsIndex;
282+
} else {
283+
bodyString += " " + "//$NON-NLS-" + (nlsIndex - lastNLSIndex) + "$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
284+
}
285+
}
231286
}
232287
}
233288
}
234-
String blockString= "{"; //$NON-NLS-1$
235-
IBuffer cuBuffer= cuRewrite.getCu().getBuffer();
236289
Block replacingBody= null;
237290
String blockPosition= JavaCore.getOption(DefaultCodeFormatterConstants.FORMATTER_BRACE_POSITION_FOR_BLOCK);
238291
boolean blockEndOfLine= blockPosition.equals(DefaultCodeFormatterConstants.END_OF_LINE);
239-
if (!commentsToPreserve.isEmpty()) {
240-
// Get extended body text and convert multiple indent tabs to be just one tab as they will be relative to control statement
241-
String bodyString= cuBuffer.getText(cuRoot.getExtendedStartPosition(fBody), cuRoot.getExtendedLength(fBody))
242-
.replaceAll("\\r\\n|\\r|\\n(\\t|\\s)*", System.lineSeparator() + "\t"); //$NON-NLS-1$ //$NON-NLS-2$
292+
String blockString= "{"; //$NON-NLS-1$
293+
if (!commentsToPreserve.isEmpty() || needToManuallyAddNLS) {
243294
if (blockEndOfLine || statementType == -1) {
244-
// To ensure the comments to preserve end up on same line as the control statement, we need
245-
// to build the block manually as a string and then create a Block placeholder from it
246-
for (Comment comment : commentsToPreserve) {
247-
String commentString= cuBuffer.getText(comment.getStartPosition(), comment.getLength());
248-
blockString += " " + commentString; //$NON-NLS-1$
295+
if (statementType == -1 && !blockEndOfLine) {
296+
if (commentString.contains("$NON-NLS-")) { //$NON-NLS-1$
297+
return; // we cannot handle non-block-end-of-line and NLS comment on if
298+
}
299+
blockString= commentString.trim() + System.lineSeparator() + blockString + System.lineSeparator() + "\t" + bodyString + System.lineSeparator() + "}"; //$NON-NLS-1$ //$NON-NLS-2$
300+
} else {
301+
blockString += commentString + System.lineSeparator() + "\t" + bodyString + System.lineSeparator() + "}"; //$NON-NLS-1$ //$NON-NLS-2$
249302
}
250-
blockString += System.lineSeparator() + "\t" + bodyString + System.lineSeparator() + "}"; //$NON-NLS-1$ //$NON-NLS-2$
251303
replacingBody= (Block)rewrite.createStringPlaceholder(blockString, ASTNode.BLOCK);
252304
} else {
253-
Comment lastComment= commentsToPreserve.get(commentsToPreserve.size()-1);
254-
String newControlStatement= cuBuffer.getText(cuRoot.getExtendedStartPosition(fControlStatement),
255-
lastComment.getStartPosition() + lastComment.getLength() - cuRoot.getExtendedStartPosition(fControlStatement));
305+
String newControlStatement= ""; //$NON-NLS-1$
306+
if (needToManuallyAddNLS) {
307+
newControlStatement= cuBuffer.getText(cuRoot.getExtendedStartPosition(fControlStatement),
308+
fBody.getStartPosition() - cuRoot.getExtendedStartPosition(fControlStatement)) + commentString;
309+
} else {
310+
Comment lastComment= commentsToPreserve.get(commentsToPreserve.size()-1);
311+
newControlStatement= cuBuffer.getText(cuRoot.getExtendedStartPosition(fControlStatement),
312+
lastComment.getStartPosition() + lastComment.getLength() - cuRoot.getExtendedStartPosition(fControlStatement));
313+
}
256314
newControlStatement += System.lineSeparator() + "{" + System.lineSeparator(); //$NON-NLS-1$
257315
newControlStatement += "\t" + bodyString + System.lineSeparator() + "}"; //$NON-NLS-1$ //$NON-NLS-2$
258316
Statement newStatement= (Statement)rewrite.createStringPlaceholder(newControlStatement, statementType);
@@ -267,6 +325,31 @@ public void rewriteASTInternal(CompilationUnitRewrite cuRewrite, LinkedProposalM
267325
rewrite.set(fControlStatement, fBodyProperty, replacingBody, group);
268326
}
269327

328+
private static Pattern nlsPattern= Pattern.compile("//\\s*\\$NON-NLS-(\\d+)\\$"); //$NON-NLS-1$
329+
private List<String> getLineNLSComments(int start, ASTNode node) {
330+
ASTNode root= node.getRoot();
331+
List<String> nlsList= new ArrayList<>();
332+
if (root instanceof CompilationUnit) {
333+
CompilationUnit unit= (CompilationUnit)root;
334+
int line= unit.getLineNumber(start);
335+
int endPosition= unit.getPosition(line + 1, 0);
336+
int length= endPosition - start;
337+
IJavaElement element= unit.getJavaElement();
338+
if (element != null && element instanceof ICompilationUnit icu) {
339+
try {
340+
String statement= icu.getBuffer().getText(start, length);
341+
Matcher m= nlsPattern.matcher(statement);
342+
while (m.find()) {
343+
nlsList.add(m.group());
344+
}
345+
} catch (IndexOutOfBoundsException | JavaModelException e) {
346+
// do nothing
347+
}
348+
}
349+
}
350+
return nlsList;
351+
}
352+
270353
}
271354

272355
static class RemoveBlockOperation extends CompilationUnitRewriteOperationWithSourceRange {

‎org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20616,6 +20616,41 @@ public void foo(String x) {
2061620616
assertRefactoringResultAsExpected(new ICompilationUnit[] {cu1}, new String[] {expected1}, null);
2061720617
}
2061820618

20619+
@Test
20620+
public void testAddParenthesesIssue2059() throws Exception {
20621+
20622+
IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null);
20623+
String sample= """
20624+
package test1;
20625+
public class E {
20626+
public void foo(String x) {
20627+
if (x.equals("abc")) System.out.println("here"); //$NON-NLS-1$ //$NON-NLS-2$
20628+
else System.out.println("def"); //$NON-NLS-1$
20629+
}
20630+
}
20631+
""";
20632+
ICompilationUnit cu1= pack1.createCompilationUnit("E.java", sample, false, null);
20633+
20634+
enable(CleanUpConstants.CONTROL_STATEMENTS_USE_BLOCKS);
20635+
enable(CleanUpConstants.CONTROL_STATEMENTS_USE_BLOCKS_ALWAYS);
20636+
20637+
sample= """
20638+
package test1;
20639+
public class E {
20640+
public void foo(String x) {
20641+
if (x.equals("abc")) { //$NON-NLS-1$
20642+
System.out.println("here"); //$NON-NLS-1$
20643+
} else {
20644+
System.out.println("def"); //$NON-NLS-1$
20645+
}
20646+
}
20647+
}
20648+
""";
20649+
String expected1= sample;
20650+
20651+
assertRefactoringResultAsExpected(new ICompilationUnit[] {cu1}, new String[] {expected1}, null);
20652+
}
20653+
2061920654
@Test
2062020655
public void testRemoveParentheses01() throws Exception {
2062120656

0 commit comments

Comments
 (0)