Skip to content

Commit

Permalink
Fix remove unnecessary array cleanup to handle non-NLS markers (#1214)
Browse files Browse the repository at this point in the history 
* Fix remove unnecessary array cleanup to handle non-NLS markers

- fixes #1213
- add logic to UnwrapNewArrayOperation to look for non-NLS tags in
  which case just remove the array specifier and its braces to
  preserve non-NLS markers
- add new test to CleanUpTest1d5
  • Loading branch information
@jjohnstn
jjohnstn authored Mar 7, 2024
1 parent 58a530c commit 33a8fa9
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 20 deletions.
94 changes: 75 additions & 19 deletions ...ipulation/core extension/org/eclipse/jdt/internal/corext/fix/UnwrapNewArrayOperation.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2019, 2020 Red Hat Inc. and others.
* Copyright (c) 2019, 2024 Red Hat Inc. and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -13,14 +13,18 @@
*******************************************************************************/
package org.eclipse.jdt.internal.corext.fix;

import java.util.Collections;
import java.util.List;

import org.eclipse.core.runtime.CoreException;

import org.eclipse.text.edits.TextEditGroup;

import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.ArrayCreation;
import org.eclipse.jdt.core.dom.ClassInstanceCreation;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.Expression;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.dom.SuperMethodInvocation;
Expand All @@ -29,6 +33,9 @@

import org.eclipse.jdt.internal.corext.dom.ASTNodes;
import org.eclipse.jdt.internal.corext.fix.CompilationUnitRewriteOperationsFixCore.CompilationUnitRewriteOperation;
import org.eclipse.jdt.internal.corext.refactoring.nls.NLSElement;
import org.eclipse.jdt.internal.corext.refactoring.nls.NLSLine;
import org.eclipse.jdt.internal.corext.refactoring.nls.NLSUtil;
import org.eclipse.jdt.internal.corext.refactoring.structure.CompilationUnitRewrite;

/**
Expand All @@ -46,34 +53,83 @@ public UnwrapNewArrayOperation(ArrayCreation node, Expression call) {
@Override
public void rewriteAST(CompilationUnitRewrite cuRewrite, LinkedProposalModelCore linkedModel) throws CoreException {
ASTRewrite rewrite= cuRewrite.getASTRewrite();
List<Expression> expressionsInArray= node != null && node.getInitializer() != null && node.getInitializer().expressions() != null ?
node.getInitializer().expressions() : Collections.EMPTY_LIST;
boolean isTagged[]= new boolean[expressionsInArray.size()];
ICompilationUnit cu= cuRewrite.getCu();

ListRewrite listRewrite;
if (call instanceof ClassInstanceCreation) {
listRewrite= rewrite.getListRewrite(call, ClassInstanceCreation.ARGUMENTS_PROPERTY);
} else if (call instanceof MethodInvocation) {
listRewrite= rewrite.getListRewrite(call, MethodInvocation.ARGUMENTS_PROPERTY);
} else if (call instanceof SuperMethodInvocation) {
listRewrite= rewrite.getListRewrite(call, SuperMethodInvocation.ARGUMENTS_PROPERTY);
} else {
return;
boolean tagged= false;
for (int i= 0; i < expressionsInArray.size(); ++i) {
Expression operand= expressionsInArray.get(i);
NLSLine nlsLine= NLSUtil.scanCurrentLine(cu, operand.getStartPosition());
if (nlsLine != null) {
for (NLSElement element : nlsLine.getElements()) {
if (element.getPosition().getOffset() == operand.getStartPosition()) {
if (element.hasTag()) {
tagged= true;
isTagged[i]= true;
}
}
}
}
}

TextEditGroup group= createTextEditGroup(FixMessages.UnusedCodeFix_RemoveUnnecessaryArrayCreation_description, cuRewrite);

if (node.getInitializer() != null && node.getInitializer().expressions() != null) {
List<Expression> expressionsInArray= node.getInitializer().expressions();

for (int i= 1; i < expressionsInArray.size(); i++) {
listRewrite.insertLast(ASTNodes.createMoveTarget(rewrite, expressionsInArray.get(i)), group);
if (!tagged) {
ListRewrite listRewrite;
if (call instanceof ClassInstanceCreation) {
listRewrite= rewrite.getListRewrite(call, ClassInstanceCreation.ARGUMENTS_PROPERTY);
} else if (call instanceof MethodInvocation) {
listRewrite= rewrite.getListRewrite(call, MethodInvocation.ARGUMENTS_PROPERTY);
} else if (call instanceof SuperMethodInvocation) {
listRewrite= rewrite.getListRewrite(call, SuperMethodInvocation.ARGUMENTS_PROPERTY);
} else {
return;
}

if (expressionsInArray.isEmpty()) {
listRewrite.remove(node, group);
if (node.getInitializer() != null && node.getInitializer().expressions() != null) {
for (int i= 1; i < expressionsInArray.size(); i++) {
listRewrite.insertLast(ASTNodes.createMoveTarget(rewrite, expressionsInArray.get(i)), group);
}

if (expressionsInArray.isEmpty()) {
listRewrite.remove(node, group);
} else {
listRewrite.replace(node, ASTNodes.createMoveTarget(rewrite, expressionsInArray.get(0)), group);
}
} else {
listRewrite.replace(node, ASTNodes.createMoveTarget(rewrite, expressionsInArray.get(0)), group);
listRewrite.remove(node, group);
}
} else {
listRewrite.remove(node, group);
// to avoid replacing/calculating non-NLS markers, simply remove the array declaration and it's braces
// from the original call
StringBuilder buf= new StringBuilder();
CompilationUnit comp= (CompilationUnit)call.getRoot();
int nodeStart= comp.getExtendedStartPosition(call);
int nodeEnd= comp.getExtendedStartPosition(call) + comp.getExtendedLength(call);
int arrayStart= node.getStartPosition();
List<Expression> expressionList= node.getInitializer().expressions();
int arrayExpressionStart= expressionList.get(0).getStartPosition();
Expression lastExpression= expressionList.get(expressionList.size() - 1);
int arrayExpressionEnd= lastExpression.getStartPosition() + lastExpression.getLength();
int arrayInitializerEnd= node.getInitializer().getStartPosition() + node.getInitializer().getLength();
buf.append(cu.getBuffer().getText(nodeStart, arrayStart - nodeStart));
buf.append(cu.getBuffer().getText(arrayExpressionStart, arrayExpressionEnd - arrayExpressionStart));
buf.append(cu.getBuffer().getText(arrayInitializerEnd, nodeEnd - arrayInitializerEnd));

ASTNode replacementNode= null;
if (call instanceof ClassInstanceCreation) {
replacementNode= rewrite.createStringPlaceholder(buf.toString(), ASTNode.CLASS_INSTANCE_CREATION);
} else if (call instanceof MethodInvocation) {
replacementNode= rewrite.createStringPlaceholder(buf.toString(), ASTNode.METHOD_INVOCATION);
} else if (call instanceof SuperMethodInvocation) {
replacementNode= rewrite.createStringPlaceholder(buf.toString(), ASTNode.SUPER_METHOD_INVOCATION);
} else {
return;
}

rewrite.replace(call, replacementNode, group);
}
}
}
Expand Down
48 changes: 47 additions & 1 deletion org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest1d5.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2019, 2023 IBM Corporation and others.
* Copyright (c) 2019, 2024 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 @@ -3107,6 +3107,52 @@ public void testUnnecessaryArray_default_package() throws Exception {
}
}

@Test
public void testUnnecessaryArrayNLS() throws Exception { // https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1213
try {
// Given
IPackageFragment pack1= fSourceFolder.createPackageFragment("", false, null);
String given= "" //
+ "import java.util.Arrays;\n" //
+ "import java.util.List;\n" //
+ "\n" //
+ "public class NLS {\n" //
+ " private static final List<String> WHITELISTED_IDS = Arrays\n" //
+ " .asList(new String[] { \"org.eclipse.search.text.FileSearchResultPage\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.JavaSearchResultPage\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.CodeGeneration\", \n" //
+ " \"org.eclipse.jdt.ui.ISharedImages\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.IWorkingCopyManager\" }); //$NON-NLS-1$\n" //
+ "}\n"; //
String expected= "" //
+ "import java.util.Arrays;\n" //
+ "import java.util.List;\n" //
+ "\n" //
+ "public class NLS {\n" //
+ " private static final List<String> WHITELISTED_IDS = Arrays\n" //
+ " .asList(\"org.eclipse.search.text.FileSearchResultPage\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.JavaSearchResultPage\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.CodeGeneration\", \n" //
+ " \"org.eclipse.jdt.ui.ISharedImages\", //$NON-NLS-1$\n" //
+ " \"org.eclipse.jdt.ui.IWorkingCopyManager\"); //$NON-NLS-1$\n" //
+ "}\n"; //

// When
ICompilationUnit cu= pack1.createCompilationUnit("A.java", given, false, null);
enable(CleanUpConstants.REMOVE_UNNECESSARY_ARRAY_CREATION);

// Then
assertNotEquals("The class must be changed", given, expected);
assertRefactoringResultAsExpected(new ICompilationUnit[] { cu }, new String[] { expected },
new HashSet<>(Arrays.asList(FixMessages.UnusedCodeFix_RemoveUnnecessaryArrayCreation_description)));
} catch (Exception e) {
StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
e.printStackTrace(pw);
fail(sw.toString());
}
}

@Test
public void testUnnecessaryEmptyArray() throws Exception {
IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null);
Expand Down

0 comments on commit 33a8fa9

Please sign in to comment.