Attachment #94702 for bug #178441

This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

View | Details | Raw Unified | Return to bug 178441
Collapse All | Expand All





FIX_WrongStringComparison_NullCheck=Use equals() with null check
FIX_WrongStringComparison_TernaryNullCheck=Use equals() with null check (ternary)
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'




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.Kind> TREE_KINDS = 
            EnumSet.<Tree.Kind>of( Tree.Kind.EQUAL_TO, Tree.Kind.NOT_EQUAL_TO );

            FileObject file = info.getFileObject();
            TreePathHandle tph = TreePathHandle.create(treePath, info);
            ArrayList<Fix> fixes = new ArrayList<Fix>();
            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.<ErrorDescription>singletonList(
                ErrorDescriptionFactory.createErrorDescription(
                    getSeverity().toEditorSeverity(), 

        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);

        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<WorkingCopy> {

        protected FileObject file;
        protected TreePathHandle tph;
        protected Kind kind;

        public WrongStringComparisonFix(FileObject file, TreePathHandle tph, Kind kind) {
            this.file = file;
            this.tph = tph;
            this.kind = kind;
        }

        public String getText() {
            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
                default:
                    return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NullCheck"); // NOI18N
                }
            }
        }


            return "[WrongStringComparisonFix:" + getText() + "]";
        }



        public void run(WorkingCopy copy) throws Exception {
            copy.toPhase(JavaSource.Phase.PARSED);
            TreePath path = tph.resolve(copy);

                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.<ExpressionTree>emptyList(), leftEquals, Collections.singletonList(right));
                if (oldTree.getKind() == Tree.Kind.NOT_EQUAL_TO) {
                    leftEqualsRight = make.Unary(Tree.Kind.LOGICAL_COMPLEMENT, leftEqualsRight);
                }
                ExpressionTree newTree;
                if (kind == Kind.REVERSE_OPERANDS) {
                    // "str2".equals(str1)
                    ExpressionTree rightEquals = make.MemberSelect(right, "equals"); // NOI18N
                    ExpressionTree rightEqualsLeft = make.MethodInvocation(Collections.<ExpressionTree>emptyList(), rightEquals, Collections.singletonList(left));
                    rightEqualsLeft = matchSign(make, oldTree, rightEqualsLeft);
                    newTree = rightEqualsLeft;
                } else {
                    ExpressionTree leftEquals = make.MemberSelect(left, "equals"); // NOI18N
                    ExpressionTree leftEqualsRight = make.MethodInvocation(Collections.<ExpressionTree>emptyList(), leftEquals, Collections.singletonList(right));
                    leftEqualsRight = matchSign(make, oldTree, leftEqualsRight);
                    if (kind == Kind.NO_NULL_CHECK) {
                        // str1.equals(str2)
                        newTree = leftEqualsRight;
                    } 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 (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;
            }

        }

    }

}

Lines 1-6 Link Here

(-)a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.form (-4 / +30 lines)
1	<?xml version="1.0" encoding="UTF-8" ?>	1	<?xml version="1.0" encoding="UTF-8" ?>
2		2
3	<Form version="1.2" maxVersion="1.2" type="org.netbeans.modules.form.forminfo.JPanelFormInfo">	3	<Form version="1.2" maxVersion="1.2" type="org.netbeans.modules.form.forminfo.JPanelFormInfo">
		4	<Properties>
		5	<Property name="preferredSize" type="java.awt.Dimension" editor="org.netbeans.beaninfo.editors.DimensionEditor">
		6	<Dimension value="[346, 60]"/>
		7	</Property>
		8	</Properties>
4	<AuxValues>	9	<AuxValues>
5	<AuxValue name="FormSettings_autoResourcing" type="java.lang.Integer" value="0"/>	10	<AuxValue name="FormSettings_autoResourcing" type="java.lang.Integer" value="0"/>
6	<AuxValue name="FormSettings_autoSetComponentName" type="java.lang.Boolean" value="false"/>	11	<AuxValue name="FormSettings_autoSetComponentName" type="java.lang.Boolean" value="false"/>
Lines 16-24 Link Here
16	<Layout>	21	<Layout>
17	<DimensionLayout dim="0">	22	<DimensionLayout dim="0">
18	<Group type="103" groupAlignment="0" attributes="0">	23	<Group type="103" groupAlignment="0" attributes="0">
19	<Group type="102" alignment="1" attributes="0">	24	<Group type="102" attributes="0">
20	<EmptySpace max="-2" attributes="0"/>	25	<EmptySpace max="-2" attributes="0"/>
21	<Component id="ternaryNullCheck" pref="346" max="32767" attributes="0"/>	26	<Group type="103" groupAlignment="0" attributes="0">
		27	<Component id="ternaryNullCheck" alignment="0" min="-2" pref="346" max="-2" attributes="0"/>
		28	<Component id="stringLiteralsFirst" alignment="0" min="-2" pref="346" max="-2" attributes="0"/>
		29	</Group>
		30	<EmptySpace max="32767" attributes="0"/>
22	</Group>	31	</Group>
23	</Group>	32	</Group>
24	</DimensionLayout>	33	</DimensionLayout>
Lines 26-33 Link Here
26	<Group type="103" groupAlignment="0" attributes="0">	35	<Group type="103" groupAlignment="0" attributes="0">
27	<Group type="102" alignment="0" attributes="0">	36	<Group type="102" alignment="0" attributes="0">
28	<EmptySpace max="-2" attributes="0"/>	37	<EmptySpace max="-2" attributes="0"/>
29	<Component id="ternaryNullCheck" min="-2" max="-2" attributes="0"/>	38	<Component id="ternaryNullCheck" min="-2" pref="30" max="-2" attributes="0"/>
30	<EmptySpace pref="161" max="32767" attributes="0"/>	39	<EmptySpace type="unrelated" max="-2" attributes="0"/>
		40	<Component id="stringLiteralsFirst" min="-2" pref="30" max="-2" attributes="0"/>
		41	<EmptySpace max="32767" attributes="0"/>
31	</Group>	42	</Group>
32	</Group>	43	</Group>
33	</DimensionLayout>	44	</DimensionLayout>
Lines 48-52 Link Here
48	<EventHandler event="actionPerformed" listener="java.awt.event.ActionListener" parameters="java.awt.event.ActionEvent" handler="ternaryNullCheckActionPerformed"/>	59	<EventHandler event="actionPerformed" listener="java.awt.event.ActionListener" parameters="java.awt.event.ActionEvent" handler="ternaryNullCheckActionPerformed"/>
49	</Events>	60	</Events>
50	</Component>	61	</Component>
		62	<Component class="javax.swing.JCheckBox" name="stringLiteralsFirst">
		63	<Properties>
		64	<Property name="text" type="java.lang.String" editor="org.netbeans.modules.i18n.form.FormI18nStringEditor">
		65	<ResourceString bundle="org/netbeans/modules/java/hints/Bundle.properties" key="WrongStringComparisonCustomizer.stringLiteralFirst.text" replaceFormat="org.openide.util.NbBundle.getMessage({sourceFileName}.class, "{key}")"/>
		66	</Property>
		67	</Properties>
		68	<AccessibilityProperties>
		69	<Property name="AccessibleContext.accessibleDescription" type="java.lang.String" editor="org.netbeans.modules.i18n.form.FormI18nStringEditor">
		70	<ResourceString bundle="org/netbeans/modules/java/hints/Bundle.properties" key="ACSD_String_Literals_First" replaceFormat="org.openide.util.NbBundle.getMessage({sourceFileName}.class, "{key}")"/>
		71	</Property>
		72	</AccessibilityProperties>
		73	<Events>
		74	<EventHandler event="actionPerformed" listener="java.awt.event.ActionListener" parameters="java.awt.event.ActionEvent" handler="stringLiteralsFirstActionPerformed"/>
		75	</Events>
		76	</Component>
51	</SubComponents>	77	</SubComponents>
52	</Form>	78	</Form>

Lines 56-61 Link Here

(-)a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparisonCustomizer.java (-4 / +26 lines)
56	initComponents();	56	initComponents();
57	this.p = p;	57	this.p = p;
58	ternaryNullCheck.setSelected(WrongStringComparison.getTernaryNullCheck(p));	58	ternaryNullCheck.setSelected(WrongStringComparison.getTernaryNullCheck(p));
		59	stringLiteralsFirst.setSelected(WrongStringComparison.getStringLiteralsFirst(p));
59	}	60	}
60		61
61	/** This method is called from within the constructor to	62	/** This method is called from within the constructor to
Lines 67-72 Link Here
67	private void initComponents() {	68	private void initComponents() {
68		69
69	ternaryNullCheck = new javax.swing.JCheckBox();	70	ternaryNullCheck = new javax.swing.JCheckBox();
		71	stringLiteralsFirst = new javax.swing.JCheckBox();
		72
		73	setPreferredSize(new java.awt.Dimension(346, 60));
70		74
71	ternaryNullCheck.setText(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "WrongStringComparisonCustomizer.ternaryNullCheck.text")); // NOI18N	75	ternaryNullCheck.setText(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "WrongStringComparisonCustomizer.ternaryNullCheck.text")); // NOI18N
72	ternaryNullCheck.addActionListener(new java.awt.event.ActionListener() {	76	ternaryNullCheck.addActionListener(new java.awt.event.ActionListener() {
Lines 75-105 Link Here
75	}	79	}
76	});	80	});
77		81
		82	stringLiteralsFirst.setText(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "WrongStringComparisonCustomizer.stringLiteralFirst.text")); // NOI18N
		83	stringLiteralsFirst.addActionListener(new java.awt.event.ActionListener() {
		84	public void actionPerformed(java.awt.event.ActionEvent evt) {
		85	stringLiteralsFirstActionPerformed(evt);
		86	}
		87	});
		88
78	javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this);	89	javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this);
79	this.setLayout(layout);	90	this.setLayout(layout);
80	layout.setHorizontalGroup(	91	layout.setHorizontalGroup(
81	layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)	92	layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
82	.addGroup(javax.swing.GroupLayout.Alignment.TRAILING, layout.createSequentialGroup()	93	.addGroup(layout.createSequentialGroup()
83	.addContainerGap()	94	.addContainerGap()
84	.addComponent(ternaryNullCheck, javax.swing.GroupLayout.DEFAULT_SIZE, 346, Short.MAX_VALUE))	95	.addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
		96	.addComponent(ternaryNullCheck, javax.swing.GroupLayout.PREFERRED_SIZE, 346, javax.swing.GroupLayout.PREFERRED_SIZE)
		97	.addComponent(stringLiteralsFirst, javax.swing.GroupLayout.PREFERRED_SIZE, 346, javax.swing.GroupLayout.PREFERRED_SIZE))
		98	.addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE))
85	);	99	);
86	layout.setVerticalGroup(	100	layout.setVerticalGroup(
87	layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)	101	layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
88	.addGroup(layout.createSequentialGroup()	102	.addGroup(layout.createSequentialGroup()
89	.addContainerGap()	103	.addContainerGap()
90	.addComponent(ternaryNullCheck)	104	.addComponent(ternaryNullCheck, javax.swing.GroupLayout.PREFERRED_SIZE, 30, javax.swing.GroupLayout.PREFERRED_SIZE)
91	.addContainerGap(161, Short.MAX_VALUE))	105	.addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED)
		106	.addComponent(stringLiteralsFirst, javax.swing.GroupLayout.PREFERRED_SIZE, 30, javax.swing.GroupLayout.PREFERRED_SIZE)
		107	.addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE))
92	);	108	);
93		109
94	ternaryNullCheck.getAccessibleContext().setAccessibleDescription(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "ACSD_Ternary_Null_Check")); // NOI18N	110	ternaryNullCheck.getAccessibleContext().setAccessibleDescription(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "ACSD_Ternary_Null_Check")); // NOI18N
		111	stringLiteralsFirst.getAccessibleContext().setAccessibleDescription(org.openide.util.NbBundle.getMessage(WrongStringComparisonCustomizer.class, "ACSD_String_Literals_First")); // NOI18N
95	}// </editor-fold>//GEN-END:initComponents	112	}// </editor-fold>//GEN-END:initComponents
96		113
97	private void ternaryNullCheckActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_ternaryNullCheckActionPerformed	114	private void ternaryNullCheckActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_ternaryNullCheckActionPerformed
98	WrongStringComparison.setTernaryNullCheck(p, ternaryNullCheck.isSelected());	115	WrongStringComparison.setTernaryNullCheck(p, ternaryNullCheck.isSelected());
99	}//GEN-LAST:event_ternaryNullCheckActionPerformed	116	}//GEN-LAST:event_ternaryNullCheckActionPerformed
100		117
		118	private void stringLiteralsFirstActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_stringLiteralsFirstActionPerformed
		119	WrongStringComparison.setStringLiteralsFirst(p, stringLiteralsFirst.isSelected());
		120	}//GEN-LAST:event_stringLiteralsFirstActionPerformed
		121
101		122
102	// Variables declaration - do not modify//GEN-BEGIN:variables	123	// Variables declaration - do not modify//GEN-BEGIN:variables
		124	private javax.swing.JCheckBox stringLiteralsFirst;
103	private javax.swing.JCheckBox ternaryNullCheck;	125	private javax.swing.JCheckBox ternaryNullCheck;
104	// End of variables declaration//GEN-END:variables	126	// End of variables declaration//GEN-END:variables
105		127





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;

                            "    }" +
                            "}",
                            "0:114-0:120:verifier:Comparing Strings using == or !=",
                            "[WrongStringComparisonFix:Use equals()]",
                            "package test;public class Test { private String s; private void test() { String t = null; if (s.equals(t)); }}");
    }


                            "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<ErrorDescription> computeErrors(CompilationInfo info, TreePath path) {
        return wsc.run(info, path);

Return to bug 178441

Lines 159-168 Link Here

(-)a/java.hints/src/org/netbeans/modules/java/hints/Bundle.properties (-1 / +4 lines)
159		159
160	FIX_WrongStringComparison_NullCheck=Use equals() with null check	160	FIX_WrongStringComparison_NullCheck=Use equals() with null check
161	FIX_WrongStringComparison_TernaryNullCheck=Use equals() with null check (ternary)	161	FIX_WrongStringComparison_TernaryNullCheck=Use equals() with null check (ternary)
162	FIX_WrongStringComparison_NoNullCheck=Use equals() without null check	162	FIX_WrongStringComparison_NoNullCheck=Use equals()
		163	FIX_WrongStringComparison_ReverseOperands=Use equals() and reverse operands
163		164
164	WrongStringComparisonCustomizer.ternaryNullCheck.text=Check for null using ternary conditional operator	165	WrongStringComparisonCustomizer.ternaryNullCheck.text=Check for null using ternary conditional operator
		166	WrongStringComparisonCustomizer.stringLiteralFirst.text=Put String literals first when possible
165	ACSD_Ternary_Null_Check=Controls whether the fix uses a ternary conditional null check.	167	ACSD_Ternary_Null_Check=Controls whether the fix uses a ternary conditional null check.
		168	ACSD_String_Literals_First=Generates less code by avoiding a null check if one of the operands is a String literal.
166		169
167	#Empty statements	170	#Empty statements
168	LBL_Empty_FOR_LOOP=Empty statement after 'for'	171	LBL_Empty_FOR_LOOP=Empty statement after 'for'

Lines 66-71 Link Here

(-)a/java.hints/src/org/netbeans/modules/java/hints/WrongStringComparison.java (-38 / +96 lines)
66	public class WrongStringComparison extends AbstractHint {	66	public class WrongStringComparison extends AbstractHint {
67		67
68	private static final String TERNARY_NULL_CHECK = "ternary-null-check"; // NOI18N	68	private static final String TERNARY_NULL_CHECK = "ternary-null-check"; // NOI18N
		69	private static final String STRING_LITERALS_FIRST = "string-literals-first"; //NOI18N
69	private static final String STRING_TYPE = "java.lang.String"; // NOI18N	70	private static final String STRING_TYPE = "java.lang.String"; // NOI18N
70	private static final Set<Tree.Kind> TREE_KINDS =	71	private static final Set<Tree.Kind> TREE_KINDS =
71	EnumSet.<Tree.Kind>of( Tree.Kind.EQUAL_TO, Tree.Kind.NOT_EQUAL_TO );	72	EnumSet.<Tree.Kind>of( Tree.Kind.EQUAL_TO, Tree.Kind.NOT_EQUAL_TO );
Lines 109-116 Link Here
109	FileObject file = info.getFileObject();	110	FileObject file = info.getFileObject();
110	TreePathHandle tph = TreePathHandle.create(treePath, info);	111	TreePathHandle tph = TreePathHandle.create(treePath, info);
111	ArrayList<Fix> fixes = new ArrayList<Fix>();	112	ArrayList<Fix> fixes = new ArrayList<Fix>();
112	fixes.add(new WrongStringComparisonFix(file, tph, getTernaryNullCheck()));	113	boolean reverseOperands = false;
113	fixes.add(new WrongStringComparisonFix(file, tph, null)); //no null check	114	if (bt.getLeftOperand().getKind() != Tree.Kind.STRING_LITERAL) {
		115	if (bt.getRightOperand().getKind() == Tree.Kind.STRING_LITERAL) {
		116	if (getStringLiteralsFirst()) {
		117	reverseOperands = true;
		118	} else {
		119	fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.NULL_CHECK));
		120	}
		121	} else {
		122	fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.ternaryNullCheck(getTernaryNullCheck())));
		123	}
		124	}
		125	fixes.add(new WrongStringComparisonFix(file, tph, WrongStringComparisonFix.Kind.reverseOperands(reverseOperands)));
114	return Collections.<ErrorDescription>singletonList(	126	return Collections.<ErrorDescription>singletonList(
115	ErrorDescriptionFactory.createErrorDescription(	127	ErrorDescriptionFactory.createErrorDescription(
116	getSeverity().toEditorSeverity(),	128	getSeverity().toEditorSeverity(),
Lines 141-150 Link Here
141	return NbBundle.getMessage(WrongStringComparison.class, "DSC_WrongStringComparison"); // NOI18N	153	return NbBundle.getMessage(WrongStringComparison.class, "DSC_WrongStringComparison"); // NOI18N
142	}	154	}
143		155
144	public boolean getTernaryNullCheck() {
145	return getTernaryNullCheck(getPreferences(null));
146	}
147
148	@Override	156	@Override
149	public JComponent getCustomizer(Preferences node) {	157	public JComponent getCustomizer(Preferences node) {
150	return new WrongStringComparisonCustomizer(node);	158	return new WrongStringComparisonCustomizer(node);
Lines 179-213 Link Here
179	return CopyFinder.isDuplicate(info, originalPath, correctPath, cancel);	187	return CopyFinder.isDuplicate(info, originalPath, correctPath, cancel);
180	}	188	}
181		189
		190	boolean getTernaryNullCheck() {
		191	return getTernaryNullCheck(getPreferences(null));
		192	}
		193
		194	boolean getStringLiteralsFirst() {
		195	return getStringLiteralsFirst(getPreferences(null));
		196	}
		197
182	static boolean getTernaryNullCheck(Preferences p) {	198	static boolean getTernaryNullCheck(Preferences p) {
183	return p.getBoolean(TERNARY_NULL_CHECK, true);	199	return p.getBoolean(TERNARY_NULL_CHECK, true);
184	}	200	}
185		201
		202	static boolean getStringLiteralsFirst(Preferences p) {
		203	return p.getBoolean(STRING_LITERALS_FIRST, true);
		204	}
		205
186	static void setTernaryNullCheck(Preferences p, boolean selected) {	206	static void setTernaryNullCheck(Preferences p, boolean selected) {
187	p.putBoolean(TERNARY_NULL_CHECK, selected);	207	p.putBoolean(TERNARY_NULL_CHECK, selected);
188	}	208	}
189		209
		210	static void setStringLiteralsFirst(Preferences p, boolean selected) {
		211	p.putBoolean(STRING_LITERALS_FIRST, selected);
		212	}
		213
190	static class WrongStringComparisonFix implements Fix, Task<WorkingCopy> {	214	static class WrongStringComparisonFix implements Fix, Task<WorkingCopy> {
191		215
192	protected FileObject file;	216	protected FileObject file;
193	protected TreePathHandle tph;	217	protected TreePathHandle tph;
194	protected Boolean nullCheck;	218	protected Kind kind;
195		219
196	public WrongStringComparisonFix(FileObject file, TreePathHandle tph, Boolean nullCheck) {	220	public WrongStringComparisonFix(FileObject file, TreePathHandle tph, Kind kind) {
197	this.file = file;	221	this.file = file;
198	this.tph = tph;	222	this.tph = tph;
199	this.nullCheck = nullCheck;	223	this.kind = kind;
200	}	224	}
201		225
202	public String getText() {	226	public String getText() {
203	if (nullCheck == null) {	227	switch (kind) {
204	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NoNullCheck"); // NOI18N	228	case REVERSE_OPERANDS:
205	} else {	229	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_ReverseOperands"); // NOI18N
206	if (nullCheck) {	230	case NO_NULL_CHECK:
		231	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NoNullCheck"); // NOI18N
		232	case NULL_CHECK_TERNARY:
207	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_TernaryNullCheck"); // NOI18N	233	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_TernaryNullCheck"); // NOI18N
208	} else {	234	default:
209	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NullCheck"); // NOI18N	235	return NbBundle.getMessage(WrongStringComparison.class, "FIX_WrongStringComparison_NullCheck"); // NOI18N
210	}
211	}	236	}
212	}	237	}
213		238
Lines 222-229 Link Here
222	return "[WrongStringComparisonFix:" + getText() + "]";	247	return "[WrongStringComparisonFix:" + getText() + "]";
223	}	248	}
224		249
225
226
227	public void run(WorkingCopy copy) throws Exception {	250	public void run(WorkingCopy copy) throws Exception {
228	copy.toPhase(JavaSource.Phase.PARSED);	251	copy.toPhase(JavaSource.Phase.PARSED);
229	TreePath path = tph.resolve(copy);	252	TreePath path = tph.resolve(copy);
Lines 232-267 Link Here
232	BinaryTree oldTree = (BinaryTree) path.getLeaf();	255	BinaryTree oldTree = (BinaryTree) path.getLeaf();
233	ExpressionTree left = oldTree.getLeftOperand();	256	ExpressionTree left = oldTree.getLeftOperand();
234	ExpressionTree right = oldTree.getRightOperand();	257	ExpressionTree right = oldTree.getRightOperand();
235	ExpressionTree leftEquals = make.MemberSelect(left, "equals"); // NOI18N
236	ExpressionTree leftEqualsRight = make.MethodInvocation(Collections.<ExpressionTree>emptyList(), leftEquals, Collections.singletonList(right));
237	if (oldTree.getKind() == Tree.Kind.NOT_EQUAL_TO) {
238	leftEqualsRight = make.Unary(Tree.Kind.LOGICAL_COMPLEMENT, leftEqualsRight);
239	}
240	ExpressionTree newTree;	258	ExpressionTree newTree;
241	if (nullCheck == null) {	259	if (kind == Kind.REVERSE_OPERANDS) {
242	// str1.equals(str2)	260	// "str2".equals(str1)
243	newTree = leftEqualsRight;	261	ExpressionTree rightEquals = make.MemberSelect(right, "equals"); // NOI18N
		262	ExpressionTree rightEqualsLeft = make.MethodInvocation(Collections.<ExpressionTree>emptyList(), rightEquals, Collections.singletonList(left));
		263	rightEqualsLeft = matchSign(make, oldTree, rightEqualsLeft);
		264	newTree = rightEqualsLeft;
244	} else {	265	} else {
245	ExpressionTree leftEqNull = make.Binary(Tree.Kind.EQUAL_TO, left, make.Identifier("null")); // NOI18N	266	ExpressionTree leftEquals = make.MemberSelect(left, "equals"); // NOI18N
246	ExpressionTree rightEqNull = make.Binary(oldTree.getKind(), right, make.Identifier("null")); // NOI18N	267	ExpressionTree leftEqualsRight = make.MethodInvocation(Collections.<ExpressionTree>emptyList(), leftEquals, Collections.singletonList(right));
247	if (nullCheck) {	268	leftEqualsRight = matchSign(make, oldTree, leftEqualsRight);
248	// str1 == null ? str2 == null : str1.equals(str2)	269	if (kind == Kind.NO_NULL_CHECK) {
249	newTree = make.ConditionalExpression(leftEqNull, rightEqNull, leftEqualsRight);	270	// str1.equals(str2)
		271	newTree = leftEqualsRight;
250	} else {	272	} else {
251	// (str1 == null && str2 == null) \|\| (str1 != null && str1.equals(str2))	273	ExpressionTree leftEqNull = make.Binary(Tree.Kind.EQUAL_TO, left, make.Identifier("null")); // NOI18N
252	ExpressionTree leftNeNull = make.Binary(Tree.Kind.NOT_EQUAL_TO, left, make.Identifier("null")); // NOI18N	274	ExpressionTree rightEqNull = make.Binary(oldTree.getKind(), right, make.Identifier("null")); // NOI18N
253	ExpressionTree orLeft = make.Parenthesized(make.Binary(Tree.Kind.CONDITIONAL_AND, leftEqNull, rightEqNull));	275	if (kind == Kind.NULL_CHECK_TERNARY) {
254	ExpressionTree orRight = make.Parenthesized(make.Binary(Tree.Kind.CONDITIONAL_AND, leftNeNull, leftEqualsRight));	276	// str1 == null ? str2 == null : str1.equals(str2)
255	newTree = make.Binary(Tree.Kind.CONDITIONAL_OR, orLeft, orRight);	277	newTree = make.ConditionalExpression(leftEqNull, rightEqNull, leftEqualsRight);
256	}	278	} else {
257	if (path.getParentPath().getLeaf().getKind() != Tree.Kind.PARENTHESIZED) {	279	ExpressionTree leftNeNull = make.Binary(Tree.Kind.NOT_EQUAL_TO, left, make.Identifier("null")); // NOI18N
258	newTree = make.Parenthesized(newTree);	280	ExpressionTree leftNeNullAndLeftEqualsRight = make.Binary(Tree.Kind.CONDITIONAL_AND, leftNeNull, leftEqualsRight);
		281	if (right.getKind() == Tree.Kind.STRING_LITERAL) {
		282	// str1 != null && str1.equals("str2")
		283	newTree = leftNeNullAndLeftEqualsRight;
		284	} else {
		285	// (str1 == null && str2 == null) \|\| (str1 != null && str1.equals(str2))
		286	ExpressionTree leftEqNullAndRightEqNull = make.Binary(Tree.Kind.CONDITIONAL_AND, leftEqNull, rightEqNull);
		287	newTree = make.Binary(Tree.Kind.CONDITIONAL_OR, make.Parenthesized(leftEqNullAndRightEqNull), make.Parenthesized(leftNeNullAndLeftEqualsRight));
		288	}
		289	}
		290	if (path.getParentPath().getLeaf().getKind() != Tree.Kind.PARENTHESIZED) {
		291	newTree = make.Parenthesized(newTree);
		292	}
259	}	293	}
260	}	294	}
261	copy.rewrite(oldTree, newTree);	295	copy.rewrite(oldTree, newTree);
262	}	296	}
263	}	297	}
264		298
		299	ExpressionTree matchSign(TreeMaker make, BinaryTree oldTree, ExpressionTree et) {
		300	if (oldTree.getKind() == Tree.Kind.NOT_EQUAL_TO) {
		301	return make.Unary(Tree.Kind.LOGICAL_COMPLEMENT, et);
		302	} else {
		303	return et;
		304	}
		305	}
		306
		307	enum Kind {
		308	REVERSE_OPERANDS,
		309	NO_NULL_CHECK,
		310	NULL_CHECK,
		311	NULL_CHECK_TERNARY;
		312
		313	public static Kind ternaryNullCheck(boolean ternary) {
		314	return ternary ? NULL_CHECK_TERNARY : NULL_CHECK;
		315	}
		316
		317	public static Kind reverseOperands(boolean reverseOperands) {
		318	return reverseOperands ? REVERSE_OPERANDS : NO_NULL_CHECK;
		319	}
		320
		321	}
		322
265	}	323	}
266		324
267	}	325	}

Lines 41-47 Link Here

(-)a/java.hints/test/unit/src/org/netbeans/modules/java/hints/WrongStringComparisonTest.java (-2 / +72 lines)
41		41
42	import com.sun.source.util.TreePath;	42	import com.sun.source.util.TreePath;
43	import java.util.List;	43	import java.util.List;
44	import java.util.prefs.Preferences;
45	import org.netbeans.api.java.source.CompilationInfo;	44	import org.netbeans.api.java.source.CompilationInfo;
46	import org.netbeans.modules.java.hints.infrastructure.TreeRuleTestBase;	45	import org.netbeans.modules.java.hints.infrastructure.TreeRuleTestBase;
47	import org.netbeans.modules.java.hints.options.HintsSettings;	46	import org.netbeans.modules.java.hints.options.HintsSettings;
Lines 114-120 Link Here
114	" }" +	113	" }" +
115	"}",	114	"}",
116	"0:114-0:120:verifier:Comparing Strings using == or !=",	115	"0:114-0:120:verifier:Comparing Strings using == or !=",
117	"[WrongStringComparisonFix:Use equals() without null check]",	116	"[WrongStringComparisonFix:Use equals()]",
118	"package test;public class Test { private String s; private void test() { String t = null; if (s.equals(t)); }}");	117	"package test;public class Test { private String s; private void test() { String t = null; if (s.equals(t)); }}");
119	}	118	}
120		119
Lines 134-139 Link Here
134	"package test;public class Test { private String s; private void test() { String t = null; if ((s == null && t == null) \|\| (s != null && s.equals(t))); }}");	133	"package test;public class Test { private String s; private void test() { String t = null; if ((s == null && t == null) \|\| (s != null && s.equals(t))); }}");
135	}	134	}
136		135
		136	public void testFixWithStringLiteralFirst() throws Exception {
		137	performFixTest("test/Test.java",
		138	"package test;" +
		139	"public class Test {" +
		140	" private String s;" +
		141	" private void test() {" +
		142	" if (\"\" =\|= s);" +
		143	" }" +
		144	"}",
		145	"0:90-0:97:verifier:Comparing Strings using == or !=",
		146	"[WrongStringComparisonFix:Use equals()]",
		147	"package test;public class Test { private String s; private void test() { if (\"\".equals(s)); }}");
		148	}
		149
		150	public void testFixWithStringLiteralSecondReverseOperands() throws Exception {
		151	performFixTest("test/Test.java",
		152	"package test;" +
		153	"public class Test {" +
		154	" private String s;" +
		155	" private void test() {" +
		156	" if (s =\|= \"\");" +
		157	" }" +
		158	"}",
		159	"0:90-0:97:verifier:Comparing Strings using == or !=",
		160	"[WrongStringComparisonFix:Use equals() and reverse operands]",
		161	"package test;public class Test { private String s; private void test() { if (\"\".equals(s)); }}");
		162	}
		163
		164	public void testFixWithStringLiteralSecondNullCheck() throws Exception {
		165	WrongStringComparison.setStringLiteralsFirst(wsc.getPreferences(HintsSettings.getCurrentProfileId()), false);
		166	performFixTest("test/Test.java",
		167	"package test;" +
		168	"public class Test {" +
		169	" private String s;" +
		170	" private void test() {" +
		171	" if (s =\|= \"\");" +
		172	" }" +
		173	"}",
		174	"0:90-0:97:verifier:Comparing Strings using == or !=",
		175	"[WrongStringComparisonFix:Use equals() with null check]",
		176	"package test;public class Test { private String s; private void test() { if (s != null && s.equals(\"\")); }}");
		177	}
		178
		179	public void testFixWithStringLiteralSecondNoNullCheck() throws Exception {
		180	WrongStringComparison.setStringLiteralsFirst(wsc.getPreferences(HintsSettings.getCurrentProfileId()), false);
		181	performFixTest("test/Test.java",
		182	"package test;" +
		183	"public class Test {" +
		184	" private String s;" +
		185	" private void test() {" +
		186	" if (s =\|= \"\");" +
		187	" }" +
		188	"}",
		189	"0:90-0:97:verifier:Comparing Strings using == or !=",
		190	"[WrongStringComparisonFix:Use equals()]",
		191	"package test;public class Test { private String s; private void test() { if (s.equals(\"\")); }}");
		192	}
		193
		194	public void testFixWithTwoStringLiterals() throws Exception {
		195	performFixTest("test/Test.java",
		196	"package test;" +
		197	"public class Test {" +
		198	" private void test() {" +
		199	" if (\"\" =\|= \"\");" +
200	" }" +
201	"}",
202	"0:69-0:77:verifier:Comparing Strings using == or !=",
203	"[WrongStringComparisonFix:Use equals()]",
204	"package test;public class Test { private void test() { if (\"\".equals(\"\")); }}");
205	}
206
137	@Override	207	@Override
138	protected List<ErrorDescription> computeErrors(CompilationInfo info, TreePath path) {	208	protected List<ErrorDescription> computeErrors(CompilationInfo info, TreePath path) {
139	return wsc.run(info, path);	209	return wsc.run(info, path);