diff --git a/java.hints/src/org/netbeans/modules/java/hints/Bundle.properties b/java.hints/src/org/netbeans/modules/java/hints/Bundle.properties --- a/java.hints/src/org/netbeans/modules/java/hints/Bundle.properties +++ b/java.hints/src/org/netbeans/modules/java/hints/Bundle.properties @@ -159,10 +159,13 @@ FIX_WrongStringComparison_NullCheck=Use equals() with null check FIX_WrongStringComparison_TernaryNullCheck=Use equals() with null check (ternary) -FIX_WrongStringComparison_NoNullCheck=Use equals() without null check +FIX_WrongStringComparison_NoNullCheck=Use equals() +FIX_WrongStringComparison_ReverseOperands=Use equals() and reverse operands WrongStringComparisonCustomizer.ternaryNullCheck.text=Check for null using ternary conditional operator +WrongStringComparisonCustomizer.stringLiteralFirst.text=Put String literals first when possible ACSD_Ternary_Null_Check=Controls whether the fix uses a ternary conditional null check. +ACSD_String_Literals_First=Generates less code by avoiding a null check if one of the operands is a String literal. #Empty statements LBL_Empty_FOR_LOOP=Empty statement after 'for' diff --git a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparison.java b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparison.java --- a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparison.java +++ b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparison.java @@ -66,6 +66,7 @@ public class WrongStringComparison extends AbstractHint { private static final String TERNARY_NULL_CHECK = "ternary-null-check"; // NOI18N + private static final String STRING_LITERALS_FIRST = "string-literals-first"; //NOI18N private static final String STRING_TYPE = "java.lang.String"; // NOI18N private static final Set TREE_KINDS = EnumSet.of( Tree.Kind.EQUAL_TO, Tree.Kind.NOT_EQUAL_TO ); @@ -109,8 +110,19 @@ FileObject file = info.getFileObject(); TreePathHandle tph = TreePathHandle.create(treePath, info); ArrayList fixes = new ArrayList(); - fixes.add(new WrongStringComparisonFix(file, tph, getTernaryNullCheck())); - fixes.add(new WrongStringComparisonFix(file, tph, null)); //no null check + boolean reverseOperands = false; + if (bt.getLeftOperand().getKind() != Tree.Kind.STRING_LITERAL) { + if (bt.getRightOperand().getKind() == Tree.Kind.STRING_LITERAL) { + if (getStringLiteralsFirst()) { + reverseOperands = true; + } else { + fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.NULL_CHECK)); + } + } else { + fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.ternaryNullCheck(getTernaryNullCheck()))); + } + } + fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.reverseOperands(reverseOperands))); return Collections.singletonList( ErrorDescriptionFactory.createErrorDescription( getSeverity().toEditorSeverity(), @@ -141,10 +153,6 @@ return NbBundle.getMessage(WrongStringComparison.class, "DSC_WrongStringComparison"); // NOI18N } - public boolean getTernaryNullCheck() { - return getTernaryNullCheck(getPreferences(null)); - } - @Override public JComponent getCustomizer(Preferences node) { return new WrongStringComparisonCustomizer(node); @@ -179,35 +187,52 @@ return CopyFinder.isDuplicate(info, originalPath, correctPath, cancel); } + boolean getTernaryNullCheck() { + return getTernaryNullCheck(getPreferences(null)); + } + + boolean getStringLiteralsFirst() { + return getStringLiteralsFirst(getPreferences(null)); + } + static boolean getTernaryNullCheck(Preferences p) { return p.getBoolean(TERNARY_NULL_CHECK, true); } + static boolean getStringLiteralsFirst(Preferences p) { + return p.getBoolean(STRING_LITERALS_FIRST, true); + } + static void setTernaryNullCheck(Preferences p, boolean selected) { p.putBoolean(TERNARY_NULL_CHECK, selected); } + static void setStringLiteralsFirst(Preferences p, boolean selected) { + p.putBoolean(STRING_LITERALS_FIRST, selected); + } + static class WrongStringComparisonFix implements Fix, Task { protected FileObject file; protected TreePathHandle tph; - protected Boolean nullCheck; + protected Kind kind; - public WrongStringComparisonFix(FileObject file, TreePathHandle tph, Boolean nullCheck) { + public WrongStringComparisonFix(FileObject file, TreePathHandle tph, Kind kind) { this.file = file; this.tph = tph; - this.nullCheck = nullCheck; + this.kind = kind; } public String getText() { - if (nullCheck == null) { - return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NoNullCheck"); // NOI18N - } else { - if (nullCheck) { + switch (kind) { + case REVERSE_OPERANDS: + return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_ReverseOperands"); // NOI18N + case NO_NULL_CHECK: + return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NoNullCheck"); // NOI18N + case NULL_CHECK_TERNARY: return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_TernaryNullCheck"); // NOI18N - } else { + default: return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NullCheck"); // NOI18N - } } } @@ -222,8 +247,6 @@ return "[WrongStringComparisonFix:" + getText() + "]"; } - - public void run(WorkingCopy copy) throws Exception { copy.toPhase(JavaSource.Phase.PARSED); TreePath path = tph.resolve(copy); @@ -232,36 +255,71 @@ BinaryTree oldTree = (BinaryTree) path.getLeaf(); ExpressionTree left = oldTree.getLeftOperand(); ExpressionTree right = oldTree.getRightOperand(); - ExpressionTree leftEquals = make.MemberSelect(left, "equals"); // NOI18N - ExpressionTree leftEqualsRight = make.MethodInvocation(Collections.emptyList(), leftEquals, Collections.singletonList(right)); - if (oldTree.getKind() == Tree.Kind.NOT_EQUAL_TO) { - leftEqualsRight = make.Unary(Tree.Kind.LOGICAL_COMPLEMENT, leftEqualsRight); - } ExpressionTree newTree; - if (nullCheck == null) { - // str1.equals(str2) - newTree = leftEqualsRight; + if (kind == Kind.REVERSE_OPERANDS) { + // "str2".equals(str1) + ExpressionTree rightEquals = make.MemberSelect(right, "equals"); // NOI18N + ExpressionTree rightEqualsLeft = make.MethodInvocation(Collections.emptyList(), rightEquals, Collections.singletonList(left)); + rightEqualsLeft = matchSign(make, oldTree, rightEqualsLeft); + newTree = rightEqualsLeft; } else { - ExpressionTree leftEqNull = make.Binary(Tree.Kind.EQUAL_TO, left, make.Identifier("null")); // NOI18N - ExpressionTree rightEqNull = make.Binary(oldTree.getKind(), right, make.Identifier("null")); // NOI18N - if (nullCheck) { - // str1 == null ? str2 == null : str1.equals(str2) - newTree = make.ConditionalExpression(leftEqNull, rightEqNull, leftEqualsRight); + ExpressionTree leftEquals = make.MemberSelect(left, "equals"); // NOI18N + ExpressionTree leftEqualsRight = make.MethodInvocation(Collections.emptyList(), leftEquals, Collections.singletonList(right)); + leftEqualsRight = matchSign(make, oldTree, leftEqualsRight); + if (kind == Kind.NO_NULL_CHECK) { + // str1.equals(str2) + newTree = leftEqualsRight; } else { - // (str1 == null && str2 == null) || (str1 != null && str1.equals(str2)) - ExpressionTree leftNeNull = make.Binary(Tree.Kind.NOT_EQUAL_TO, left, make.Identifier("null")); // NOI18N - ExpressionTree orLeft = make.Parenthesized(make.Binary(Tree.Kind.CONDITIONAL_AND, leftEqNull, rightEqNull)); - ExpressionTree orRight = make.Parenthesized(make.Binary(Tree.Kind.CONDITIONAL_AND, leftNeNull, leftEqualsRight)); - newTree = make.Binary(Tree.Kind.CONDITIONAL_OR, orLeft, orRight); - } - if (path.getParentPath().getLeaf().getKind() != Tree.Kind.PARENTHESIZED) { - newTree = make.Parenthesized(newTree); + ExpressionTree leftEqNull = make.Binary(Tree.Kind.EQUAL_TO, left, make.Identifier("null")); // NOI18N + ExpressionTree rightEqNull = make.Binary(oldTree.getKind(), right, make.Identifier("null")); // NOI18N + if (kind == Kind.NULL_CHECK_TERNARY) { + // str1 == null ? str2 == null : str1.equals(str2) + newTree = make.ConditionalExpression(leftEqNull, rightEqNull, leftEqualsRight); + } else { + ExpressionTree leftNeNull = make.Binary(Tree.Kind.NOT_EQUAL_TO, left, make.Identifier("null")); // NOI18N + ExpressionTree leftNeNullAndLeftEqualsRight = make.Binary(Tree.Kind.CONDITIONAL_AND, leftNeNull, leftEqualsRight); + if (right.getKind() == Tree.Kind.STRING_LITERAL) { + // str1 != null && str1.equals("str2") + newTree = leftNeNullAndLeftEqualsRight; + } else { + // (str1 == null && str2 == null) || (str1 != null && str1.equals(str2)) + ExpressionTree leftEqNullAndRightEqNull = make.Binary(Tree.Kind.CONDITIONAL_AND, leftEqNull, rightEqNull); + newTree = make.Binary(Tree.Kind.CONDITIONAL_OR, make.Parenthesized(leftEqNullAndRightEqNull), make.Parenthesized(leftNeNullAndLeftEqualsRight)); + } + } + if (path.getParentPath().getLeaf().getKind() != Tree.Kind.PARENTHESIZED) { + newTree = make.Parenthesized(newTree); + } } } copy.rewrite(oldTree, newTree); } } + ExpressionTree matchSign(TreeMaker make, BinaryTree oldTree, ExpressionTree et) { + if (oldTree.getKind() == Tree.Kind.NOT_EQUAL_TO) { + return make.Unary(Tree.Kind.LOGICAL_COMPLEMENT, et); + } else { + return et; + } + } + + enum Kind { + REVERSE_OPERANDS, + NO_NULL_CHECK, + NULL_CHECK, + NULL_CHECK_TERNARY; + + public static Kind ternaryNullCheck(boolean ternary) { + return ternary ? NULL_CHECK_TERNARY : NULL_CHECK; + } + + public static Kind reverseOperands(boolean reverseOperands) { + return reverseOperands ? REVERSE_OPERANDS : NO_NULL_CHECK; + } + + } + } } diff --git a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.form b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.form --- a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.form +++ b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.form @@ -1,6 +1,11 @@
+ + + + + @@ -16,9 +21,13 @@ - + - + + + + + @@ -26,8 +35,10 @@ - - + + + + @@ -48,5 +59,20 @@ + + + + + + + + + + + + + + + diff --git a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.java b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.java --- a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.java +++ b/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.java @@ -56,6 +56,7 @@ initComponents(); this.p = p; ternaryNullCheck.setSelected(WrongStringComparison.getTernaryNullCheck(p)); + stringLiteralsFirst.setSelected(WrongStringComparison.getStringLiteralsFirst(p)); } /** This method is called from within the constructor to @@ -67,6 +68,9 @@ private void initComponents() { ternaryNullCheck = new javax.swing.JCheckBox(); + stringLiteralsFirst = new javax.swing.JCheckBox(); + + setPreferredSize(new java.awt.Dimension(346, 60)); ternaryNullCheck.setText(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "WrongStringComparisonCustomizer.ternaryNullCheck.text")); // NOI18N ternaryNullCheck.addActionListener(new java.awt.event.ActionListener() { @@ -75,31 +79,49 @@ } }); + stringLiteralsFirst.setText(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "WrongStringComparisonCustomizer.stringLiteralFirst.text")); // NOI18N + stringLiteralsFirst.addActionListener(new java.awt.event.ActionListener() { + public void actionPerformed(java.awt.event.ActionEvent evt) { + stringLiteralsFirstActionPerformed(evt); + } + }); + javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this); this.setLayout(layout); layout.setHorizontalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) - .addGroup(javax.swing.GroupLayout.Alignment.TRAILING, layout.createSequentialGroup() + .addGroup(layout.createSequentialGroup() .addContainerGap() - .addComponent(ternaryNullCheck, javax.swing.GroupLayout.DEFAULT_SIZE, 346, Short.MAX_VALUE)) + .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) + .addComponent(ternaryNullCheck, javax.swing.GroupLayout.PREFERRED_SIZE, 346, javax.swing.GroupLayout.PREFERRED_SIZE) + .addComponent(stringLiteralsFirst, javax.swing.GroupLayout.PREFERRED_SIZE, 346, javax.swing.GroupLayout.PREFERRED_SIZE)) + .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)) ); layout.setVerticalGroup( layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING) .addGroup(layout.createSequentialGroup() .addContainerGap() - .addComponent(ternaryNullCheck) - .addContainerGap(161, Short.MAX_VALUE)) + .addComponent(ternaryNullCheck, javax.swing.GroupLayout.PREFERRED_SIZE, 30, javax.swing.GroupLayout.PREFERRED_SIZE) + .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED) + .addComponent(stringLiteralsFirst, javax.swing.GroupLayout.PREFERRED_SIZE, 30, javax.swing.GroupLayout.PREFERRED_SIZE) + .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)) ); ternaryNullCheck.getAccessibleContext().setAccessibleDescription(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "ACSD_Ternary_Null_Check")); // NOI18N + stringLiteralsFirst.getAccessibleContext().setAccessibleDescription(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "ACSD_String_Literals_First")); // NOI18N }// //GEN-END:initComponents private void ternaryNullCheckActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_ternaryNullCheckActionPerformed WrongStringComparison.setTernaryNullCheck(p, ternaryNullCheck.isSelected()); }//GEN-LAST:event_ternaryNullCheckActionPerformed + private void stringLiteralsFirstActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_stringLiteralsFirstActionPerformed + WrongStringComparison.setStringLiteralsFirst(p, stringLiteralsFirst.isSelected()); + }//GEN-LAST:event_stringLiteralsFirstActionPerformed + // Variables declaration - do not modify//GEN-BEGIN:variables + private javax.swing.JCheckBox stringLiteralsFirst; private javax.swing.JCheckBox ternaryNullCheck; // End of variables declaration//GEN-END:variables diff --git a/java.hints/test/unit/src/org/netbeans/modules/java/hints/WrongStringComparisonTest.java b/java.hints/test/unit/src/org/netbeans/modules/java/hints/WrongStringComparisonTest.java --- a/java.hints/test/unit/src/org/netbeans/modules/java/hints/WrongStringComparisonTest.java +++ b/java.hints/test/unit/src/org/netbeans/modules/java/hints/WrongStringComparisonTest.java @@ -41,7 +41,6 @@ import com.sun.source.util.TreePath; import java.util.List; -import java.util.prefs.Preferences; import org.netbeans.api.java.source.CompilationInfo; import org.netbeans.modules.java.hints.infrastructure.TreeRuleTestBase; import org.netbeans.modules.java.hints.options.HintsSettings; @@ -114,7 +113,7 @@ " }" + "}", "0:114-0:120:verifier:Comparing Strings using == or !=", - "[WrongStringComparisonFix:Use equals() without null check]", + "[WrongStringComparisonFix:Use equals()]", "package test;public class Test { private String s; private void test() { String t = null; if (s.equals(t)); }}"); } @@ -134,6 +133,77 @@ "package test;public class Test { private String s; private void test() { String t = null; if ((s == null && t == null) || (s != null && s.equals(t))); }}"); } + public void testFixWithStringLiteralFirst() throws Exception { + performFixTest("test/Test.java", + "package test;" + + "public class Test {" + + " private String s;" + + " private void test() {" + + " if (\"\" =|= s);" + + " }" + + "}", + "0:90-0:97:verifier:Comparing Strings using == or !=", + "[WrongStringComparisonFix:Use equals()]", + "package test;public class Test { private String s; private void test() { if (\"\".equals(s)); }}"); + } + + public void testFixWithStringLiteralSecondReverseOperands() throws Exception { + performFixTest("test/Test.java", + "package test;" + + "public class Test {" + + " private String s;" + + " private void test() {" + + " if (s =|= \"\");" + + " }" + + "}", + "0:90-0:97:verifier:Comparing Strings using == or !=", + "[WrongStringComparisonFix:Use equals() and reverse operands]", + "package test;public class Test { private String s; private void test() { if (\"\".equals(s)); }}"); + } + + public void testFixWithStringLiteralSecondNullCheck() throws Exception { + WrongStringComparison.setStringLiteralsFirst(wsc.getPreferences(HintsSettings.getCurrentProfileId()), false); + performFixTest("test/Test.java", + "package test;" + + "public class Test {" + + " private String s;" + + " private void test() {" + + " if (s =|= \"\");" + + " }" + + "}", + "0:90-0:97:verifier:Comparing Strings using == or !=", + "[WrongStringComparisonFix:Use equals() with null check]", + "package test;public class Test { private String s; private void test() { if (s != null && s.equals(\"\")); }}"); + } + + public void testFixWithStringLiteralSecondNoNullCheck() throws Exception { + WrongStringComparison.setStringLiteralsFirst(wsc.getPreferences(HintsSettings.getCurrentProfileId()), false); + performFixTest("test/Test.java", + "package test;" + + "public class Test {" + + " private String s;" + + " private void test() {" + + " if (s =|= \"\");" + + " }" + + "}", + "0:90-0:97:verifier:Comparing Strings using == or !=", + "[WrongStringComparisonFix:Use equals()]", + "package test;public class Test { private String s; private void test() { if (s.equals(\"\")); }}"); + } + + public void testFixWithTwoStringLiterals() throws Exception { + performFixTest("test/Test.java", + "package test;" + + "public class Test {" + + " private void test() {" + + " if (\"\" =|= \"\");" + + " }" + + "}", + "0:69-0:77:verifier:Comparing Strings using == or !=", + "[WrongStringComparisonFix:Use equals()]", + "package test;public class Test { private void test() { if (\"\".equals(\"\")); }}"); + } + @Override protected List computeErrors(CompilationInfo info, TreePath path) { return wsc.run(info, path);