From 670c4f4ac0ae2ddba4a58cf89fdd906676300a97 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E6=9B=BE=E5=80=99?= <zenghou.fw@alibaba-inc.com>
Date: Thu, 5 Dec 2019 10:28:43 +0800
Subject: [PATCH] fix https://github.com/alibaba/p3c/issues/209

---
 .../rule/oop/WrapperTypeEqualityRule.java     |  15 +-
 .../rule/oop/xml/WrapperTypeEqualityRule.xml  | 320 ++++++++++--------
 2 files changed, 185 insertions(+), 150 deletions(-)

diff --git a/p3c-pmd/src/main/java/com/alibaba/p3c/pmd/lang/java/rule/oop/WrapperTypeEqualityRule.java b/p3c-pmd/src/main/java/com/alibaba/p3c/pmd/lang/java/rule/oop/WrapperTypeEqualityRule.java
index 6a262d7..1936940 100644
--- a/p3c-pmd/src/main/java/com/alibaba/p3c/pmd/lang/java/rule/oop/WrapperTypeEqualityRule.java
+++ b/p3c-pmd/src/main/java/com/alibaba/p3c/pmd/lang/java/rule/oop/WrapperTypeEqualityRule.java
@@ -45,8 +45,14 @@ public class WrapperTypeEqualityRule extends AbstractAliRule {
         // possible elements around "==" are PrimaryExpression or UnaryExpression(e.g. a == -2)
         List<ASTPrimaryExpression> expressions = node.findChildrenOfType(ASTPrimaryExpression.class);
         if (expressions.size() == NumberConstants.INTEGER_SIZE_OR_LENGTH_2) {
-            if (NodeUtils.isWrapperType(expressions.get(0)) &&
-                    NodeUtils.isWrapperType(expressions.get(1))) {
+            // PMD can not resolve array length type, but only the
+            ASTPrimaryExpression left = expressions.get(0);
+            ASTPrimaryExpression right = expressions.get(1);
+
+            boolean bothArrayLength = isArrayLength(left) && isArrayLength(right);
+            boolean bothWrapperType = NodeUtils.isWrapperType(left) && NodeUtils.isWrapperType(right);
+
+            if (!bothArrayLength && bothWrapperType) {
                 addViolationWithMessage(data, node, "java.oop.WrapperTypeEqualityRule.violation.msg");
             }
         }
@@ -54,4 +60,9 @@ public class WrapperTypeEqualityRule extends AbstractAliRule {
         return super.visit(node, data);
     }
 
+    private boolean isArrayLength(ASTPrimaryExpression expression) {
+        // assume expression like "x.length" is the length of array, field with name "length" may result in misrecognition
+        return "length".equals(expression.jjtGetLastToken().getImage())
+            && ".".equals(expression.jjtGetFirstToken().getNext().getImage());
+    }
 }
diff --git a/p3c-pmd/src/test/resources/com/alibaba/p3c/pmd/lang/java/rule/oop/xml/WrapperTypeEqualityRule.xml b/p3c-pmd/src/test/resources/com/alibaba/p3c/pmd/lang/java/rule/oop/xml/WrapperTypeEqualityRule.xml
index 5c1a2bc..08d65da 100644
--- a/p3c-pmd/src/test/resources/com/alibaba/p3c/pmd/lang/java/rule/oop/xml/WrapperTypeEqualityRule.xml
+++ b/p3c-pmd/src/test/resources/com/alibaba/p3c/pmd/lang/java/rule/oop/xml/WrapperTypeEqualityRule.xml
@@ -3,151 +3,175 @@
            xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests https://pmd.sourceforge.io/rule-tests_1_0_0.xsd">
 
-	<code-fragment id="wrap-type-not-use-equal">
-		<![CDATA[
-	public class Foo {
-		private static final int x = 3;
-	    public void bar() {
-	    	Integer a = 2;
-	    	int b = 2;
-	    	Integer c = 3;
-
-	    	if (a == b) {	// OK
-	    		// do nothing
-	    	}
-	    	if (a == c) {	// BAD
-	    		// do nothing
-	    	}
-	    	if (a != null) {	// OK
-	    		// do nothing
-	    	}
-	    	if (a == -6) {	// OK
-	    		// do nothing
-	    	}
-	    	if (a == x) {	// OK
-	    		// do nothing
-	    	}
-	    	if (a == Integer.MAX_VALUE) {	// BAD
-	    		// do nothing
-	    	}
-	    	// PMD can not resolve type of Inner.FLAG
-	    	if (a == Inner.FLAG) {
-	    		// do nothing
-	    	}
-	    	// After upgrade pmd version, can resolve
-	    	if (a == Integer.valueOf("2")) { //BAD
-	    		// do nothing
-	    	}
-	    	if (a == new Integer("2")) {	// BAD
-	    		// do nothing
-	    	}
-	    }
-
-	    private static class Inner {
-	    	public static final Integer FLAG = 3;
-	    }
-	}
-		]]>
-	</code-fragment>
-	<test-code>
-		<description>compare wrapper type objects without equals</description>
-		<expected-problems>4</expected-problems>
-		<expected-linenumbers>11,23,31,34</expected-linenumbers>
-		<code-ref id="wrap-type-not-use-equal" />
-	</test-code>
-
-	<!-- ====================================================================== -->
-
-	<code-fragment id="wrong-result-fix">
-		<![CDATA[
-	package com.alibaba.p3c.pmd.lang.java.rule.concurrent;
-
-	import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
-	import net.sourceforge.pmd.lang.java.ast.ASTName;
-	import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
-	import net.sourceforge.pmd.lang.java.ast.Token;
-	import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
-
-	import java.util.HashSet;
-	import java.util.Set;
-	import java.util.concurrent.Executors;
-
-	public class ThreadPoolCreationRule extends AbstractJavaRule {
-
-		private static final String DOT = ".";
-		private static final String COLON = ";";
-		private static final String NEW = "new";
-		private static final String EXECUTORS_NEW = Executors.class.getSimpleName() + DOT + NEW;
-		private static final String FULL_EXECUTORS_NEW = Executors.class.getName() + DOT + NEW;
-		private static final String BRACKETS = "()";
-		private boolean executorsUsed;
-		private Set<String> importedExecutorsMethods = new HashSet<>();
-
-		@Override
-		public Object visit(ASTPrimaryExpression node, Object data) {
-			if (!executorsUsed && importedExecutorsMethods.isEmpty()) {
-				return super.visit(node, data);
-			}
-
-			Token initToken = (Token) node.jjtGetFirstToken();
-			if (!checkInitStatement(initToken)) {
-				addViolation(data, node);
-			}
-			return super.visit(node, data);
-		}
-
-		private boolean checkInitStatement(Token token) {
-			String fullAssignStatement = getFullAssignStatement(token);
-			if (fullAssignStatement.startsWith(EXECUTORS_NEW)) {
-				return false;
-			}
-			if (!fullAssignStatement.startsWith(NEW) && !fullAssignStatement.startsWith(FULL_EXECUTORS_NEW)) {
-				return true;
-			}
-			// in case of lambda
-			int index = fullAssignStatement.indexOf(BRACKETS);
-			if (index == -1) {
-				return true;
-			}
-			fullAssignStatement = fullAssignStatement.substring(0, index);
-
-			// avoid java.util.concurrent.Executors.newxxxx
-			if (importedExecutorsMethods.contains(fullAssignStatement)) {
-				return false;
-			}
-			// static import
-			return !importedExecutorsMethods.contains(Executors.class.getName() + DOT + fullAssignStatement);
-		}
-
-		private String getFullAssignStatement(final Token token) {
-			if (token == null) {
-				return "";
-			}
-			StringBuilder sb = new StringBuilder(48);
-			Token next = token;
-			while (next.next != null && !COLON.equals(next.image)) {
-				sb.append(next.image);
-				next = next.next;
-			}
-			return sb.toString();
-		}
-
-		@Override
-		public Object visit(ASTImportDeclaration node, Object data) {
-			ASTName name = node.getFirstChildOfType(ASTName.class);
-			// in case of static import
-			executorsUsed = executorsUsed || name.getType() == Executors.class;
-			if (name.getImage().startsWith(Executors.class.getName() + DOT)) {
-				importedExecutorsMethods.add(name.getImage());
-			}
-			return super.visit(node, data);
-		}
-	}
-		]]>
-	</code-fragment>
-	<test-code>
-		<description>bugfix</description>
-		<expected-problems>0</expected-problems>
-		<code-ref id="wrong-result-fix" />
-	</test-code>
-</test-data>
+    <code-fragment id="wrap-type-not-use-equal">
+        <![CDATA[
+    public class Foo {
+        private static final int x = 3;
+        public void bar() {
+            Integer a = 2;
+            int b = 2;
+            Integer c = 3;
+
+            if (a == b) {    // OK
+                // do nothing
+            }
+            if (a == c) {    // BAD
+                // do nothing
+            }
+            if (a != null) {    // OK
+                // do nothing
+            }
+            if (a == -6) {    // OK
+                // do nothing
+            }
+            if (a == x) {    // OK
+                // do nothing
+            }
+            if (a == Integer.MAX_VALUE) {    // BAD
+                // do nothing
+            }
+            // PMD can not resolve type of Inner.FLAG
+            if (a == Inner.FLAG) {
+                // do nothing
+            }
+            // After upgrade pmd version, can resolve
+            if (a == Integer.valueOf("2")) { //BAD
+                // do nothing
+            }
+            if (a == new Integer("2")) {    // BAD
+                // do nothing
+            }
+        }
+
+        private static class Inner {
+            public static final Integer FLAG = 3;
+        }
+    }
+        ]]>
+    </code-fragment>
+    <test-code>
+        <description>compare wrapper type objects without equals</description>
+        <expected-problems>4</expected-problems>
+        <expected-linenumbers>11,23,31,34</expected-linenumbers>
+        <code-ref id="wrap-type-not-use-equal" />
+    </test-code>
+
+    <!-- ====================================================================== -->
+
+    <code-fragment id="wrong-result-fix">
+        <![CDATA[
+    package com.alibaba.p3c.pmd.lang.java.rule.concurrent;
+
+    import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
+    import net.sourceforge.pmd.lang.java.ast.ASTName;
+    import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
+    import net.sourceforge.pmd.lang.java.ast.Token;
+    import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+    import java.util.HashSet;
+    import java.util.Set;
+    import java.util.concurrent.Executors;
+
+    public class ThreadPoolCreationRule extends AbstractJavaRule {
+
+        private static final String DOT = ".";
+        private static final String COLON = ";";
+        private static final String NEW = "new";
+        private static final String EXECUTORS_NEW = Executors.class.getSimpleName() + DOT + NEW;
+        private static final String FULL_EXECUTORS_NEW = Executors.class.getName() + DOT + NEW;
+        private static final String BRACKETS = "()";
+        private boolean executorsUsed;
+        private Set<String> importedExecutorsMethods = new HashSet<>();
+
+        @Override
+        public Object visit(ASTPrimaryExpression node, Object data) {
+            if (!executorsUsed && importedExecutorsMethods.isEmpty()) {
+                return super.visit(node, data);
+            }
+
+            Token initToken = (Token) node.jjtGetFirstToken();
+            if (!checkInitStatement(initToken)) {
+                addViolation(data, node);
+            }
+            return super.visit(node, data);
+        }
+
+        private boolean checkInitStatement(Token token) {
+            String fullAssignStatement = getFullAssignStatement(token);
+            if (fullAssignStatement.startsWith(EXECUTORS_NEW)) {
+                return false;
+            }
+            if (!fullAssignStatement.startsWith(NEW) && !fullAssignStatement.startsWith(FULL_EXECUTORS_NEW)) {
+                return true;
+            }
+            // in case of lambda
+            int index = fullAssignStatement.indexOf(BRACKETS);
+            if (index == -1) {
+                return true;
+            }
+            fullAssignStatement = fullAssignStatement.substring(0, index);
+
+            // avoid java.util.concurrent.Executors.newxxxx
+            if (importedExecutorsMethods.contains(fullAssignStatement)) {
+                return false;
+            }
+            // static import
+            return !importedExecutorsMethods.contains(Executors.class.getName() + DOT + fullAssignStatement);
+        }
+
+        private String getFullAssignStatement(final Token token) {
+            if (token == null) {
+                return "";
+            }
+            StringBuilder sb = new StringBuilder(48);
+            Token next = token;
+            while (next.next != null && !COLON.equals(next.image)) {
+                sb.append(next.image);
+                next = next.next;
+            }
+            return sb.toString();
+        }
+
+        @Override
+        public Object visit(ASTImportDeclaration node, Object data) {
+            ASTName name = node.getFirstChildOfType(ASTName.class);
+            // in case of static import
+            executorsUsed = executorsUsed || name.getType() == Executors.class;
+            if (name.getImage().startsWith(Executors.class.getName() + DOT)) {
+                importedExecutorsMethods.add(name.getImage());
+            }
+            return super.visit(node, data);
+        }
+    }
+        ]]>
+    </code-fragment>
+    <test-code>
+        <description>bugfix</description>
+        <expected-problems>0</expected-problems>
+        <code-ref id="wrong-result-fix" />
+    </test-code>
+
+    <!-- ====================================================================== -->
+
+    <code-fragment id="array-length-equals">
+        <![CDATA[
+    public class Test {
+        public void foo(){
+            Integer[] a;
+            Integer[] b;
+            if (a.length == b.length) {
+                return;
+            };
+        }
+    }
+        ]]>
+    </code-fragment>
+    <test-code>
+        <description>array length equals</description>
+        <expected-problems>0</expected-problems>
+        <code-ref id="array-length-equals"/>
+    </test-code>
+
+    <!-- ====================================================================== -->
+
+</test-data>
\ No newline at end of file
-- 
GitLab