From f9f96b84fe351f5d2fa2bb48d9c62ba95cd7d62f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?=
 <juansotuyo@gmail.com>
Date: Tue, 7 Nov 2017 00:17:38 -0300
Subject: [PATCH] [java] merge UnnecessaryFinalModifier into
 UnnecessaryModifier

 - I take the chance to also flag final resources in try-with-resources
 - This resolves #411
 - This resolves #676
 - The rule is now using the rulechain
---
 .../pmd/RuleSetFactoryCompatibility.java      |   1 +
 .../codestyle/UnnecessaryModifierRule.java    |  74 ++++++--
 .../resources/category/java/codestyle.xml     |  39 -----
 .../resources/rulesets/java/unnecessary.xml   |   1 -
 .../rule/codestyle/CodeStyleRulesTest.java    |   1 -
 .../xml/UnnecessaryFinalModifier.xml          | 161 ------------------
 .../codestyle/xml/UnnecessaryModifier.xml     | 157 +++++++++++++++++
 7 files changed, 222 insertions(+), 212 deletions(-)
 delete mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFinalModifier.xml

diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactoryCompatibility.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactoryCompatibility.java
index f6cfd3dba0..1ae30fd5ff 100644
--- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactoryCompatibility.java
+++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactoryCompatibility.java
@@ -76,6 +76,7 @@ public class RuleSetFactoryCompatibility {
         addFilterRuleMoved("java", "typeresolution", "imports", "UnusedImports");
         addFilterRuleMoved("java", "typeresolution", "strictexception", "SignatureDeclareThrowsException");
         addFilterRuleRenamed("java", "naming", "MisleadingVariableName", "MIsLeadingVariableName");
+        addFilterRuleRenamed("java", "unnecessary", "UnnecessaryFinalModifier", "UnnecessaryModifier");
     }
 
     void addFilterRuleRenamed(String language, String ruleset, String oldName, String newName) {
diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryModifierRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryModifierRule.java
index 068e814a59..98501b7e7e 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryModifierRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryModifierRule.java
@@ -5,16 +5,31 @@
 package net.sourceforge.pmd.lang.java.rule.codestyle;
 
 import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTAnnotation;
 import net.sourceforge.pmd.lang.java.ast.ASTAnnotationMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant;
 import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTMarkerAnnotation;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTResource;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 
 public class UnnecessaryModifierRule extends AbstractJavaRule {
 
+    public UnnecessaryModifierRule() {
+        addRuleChainVisit(ASTEnumDeclaration.class);
+        addRuleChainVisit(ASTAnnotationTypeDeclaration.class);
+        addRuleChainVisit(ASTClassOrInterfaceDeclaration.class);
+        addRuleChainVisit(ASTMethodDeclaration.class);
+        addRuleChainVisit(ASTResource.class);
+        addRuleChainVisit(ASTFieldDeclaration.class);
+        addRuleChainVisit(ASTAnnotationMethodDeclaration.class);
+    }
+    
     @Override
     public Object visit(ASTEnumDeclaration node, Object data) {
         if (node.isStatic()) {
@@ -22,7 +37,7 @@ public class UnnecessaryModifierRule extends AbstractJavaRule {
             addViolation(data, node, getMessage());
         }
 
-        return super.visit(node, data);
+        return data;
     }
 
     public Object visit(ASTAnnotationTypeDeclaration node, Object data) {
@@ -32,7 +47,7 @@ public class UnnecessaryModifierRule extends AbstractJavaRule {
         }
 
         if (!node.isNested()) {
-            return super.visit(node, data);
+            return data;
         }
 
         Node parent = node.jjtGetParent().jjtGetParent().jjtGetParent();
@@ -49,7 +64,7 @@ public class UnnecessaryModifierRule extends AbstractJavaRule {
             addViolation(data, node, getMessage());
         }
 
-        return super.visit(node, data);
+        return data;
     }
 
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
@@ -59,7 +74,7 @@ public class UnnecessaryModifierRule extends AbstractJavaRule {
         }
 
         if (!node.isNested()) {
-            return super.visit(node, data);
+            return data;
         }
 
         Node parent = node.jjtGetParent().jjtGetParent().jjtGetParent();
@@ -81,28 +96,67 @@ public class UnnecessaryModifierRule extends AbstractJavaRule {
             addViolation(data, node, getMessage());
         }
 
-        return super.visit(node, data);
+        return data;
     }
-
-    public Object visit(ASTMethodDeclaration node, Object data) {
+    
+    public Object visit(final ASTMethodDeclaration node, Object data) {
         if (node.isSyntacticallyPublic() || node.isSyntacticallyAbstract()) {
             check(node, data);
         }
-        return super.visit(node, data);
+        
+        if (node.isFinal()) {
+            // If the method is annotated by @SafeVarargs then it's ok
+            if (!isSafeVarargs(node)) {
+                if (node.isPrivate()) {
+                    addViolation(data, node);
+                } else {
+                    final Node n = node.getNthParent(3);
+                    // A final method of an anonymous class / enum constant. Neither can be extended / overridden
+                    if (n instanceof ASTAllocationExpression || n instanceof ASTEnumConstant) {
+                        addViolation(data, node);
+                    } else if (n instanceof ASTClassOrInterfaceDeclaration
+                            && ((ASTClassOrInterfaceDeclaration) n).isFinal()) {
+                        addViolation(data, node);
+                    }
+                }
+            }
+        }
+        
+        return data;
+    }
+    
+    public Object visit(final ASTResource node, final Object data) {
+        if (node.isFinal()) {
+            addViolation(data, node);
+        }
+        
+        return data;
     }
 
     public Object visit(ASTFieldDeclaration node, Object data) {
         if (node.isSyntacticallyPublic() || node.isSyntacticallyStatic() || node.isSyntacticallyFinal()) {
             check(node, data);
         }
-        return super.visit(node, data);
+        return data;
     }
 
     public Object visit(ASTAnnotationMethodDeclaration node, Object data) {
         if (node.isPublic() || node.isAbstract()) {
             check(node, data);
         }
-        return super.visit(node, data);
+        return data;
+    }
+    
+    private boolean isSafeVarargs(final ASTMethodDeclaration node) {
+        for (final ASTAnnotation annotation : node.jjtGetParent().findChildrenOfType(ASTAnnotation.class)) {
+            final Node childAnnotation = annotation.jjtGetChild(0);
+            if (childAnnotation instanceof ASTMarkerAnnotation
+                    && SafeVarargs.class.isAssignableFrom(((ASTMarkerAnnotation) childAnnotation).getType())) {
+                return true;
+            }
+        }
+        
+        return false;
     }
 
     private void check(Node fieldOrMethod, Object data) {
diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml
index ebbe9ad9f3..286bb829d4 100644
--- a/pmd-java/src/main/resources/category/java/codestyle.xml
+++ b/pmd-java/src/main/resources/category/java/codestyle.xml
@@ -1435,45 +1435,6 @@ public class Foo {
         </example>
     </rule>
 
-    <rule name="UnnecessaryFinalModifier"
-          language="java"
-          since="3.0"
-          message="Unnecessary final modifier in final class / private methods"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
-          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#unnecessaryfinalmodifier">
-        <description>
-When a class has the final modifier, all the methods are automatically final and do not need to be
-tagged as such. Similarly, methods that can't be overridden (private methods, methods of anonymous classes,
-methods of enum instance) do not need to be tagged either.
-        </description>
-        <priority>3</priority>
-        <properties>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//ClassOrInterfaceDeclaration[@Final='true' and @Interface='false']
-    /ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
-        [count(./Annotation/MarkerAnnotation/Name[@Image='SafeVarargs' or @Image='java.lang.SafeVarargs']) = 0]
-    /MethodDeclaration[@Final='true']
-| //MethodDeclaration[@Final='true' and @Private='true']
-| //EnumConstant/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration[@Final='true']
-| //AllocationExpression/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration[@Final='true']
-]]>
-                </value>
-            </property>
-        </properties>
-        <example>
-<![CDATA[
-public final class Foo {
-    // This final modifier is not necessary, since the class is final
-    // and thus, all methods are final
-    private final void foo() {
-    }
-}
-]]>
-        </example>
-    </rule>
-
     <rule name="UnnecessaryFullyQualifiedName"
           language="java"
           since="5.0"
diff --git a/pmd-java/src/main/resources/rulesets/java/unnecessary.xml b/pmd-java/src/main/resources/rulesets/java/unnecessary.xml
index 0653312180..dc70be3993 100644
--- a/pmd-java/src/main/resources/rulesets/java/unnecessary.xml
+++ b/pmd-java/src/main/resources/rulesets/java/unnecessary.xml
@@ -13,7 +13,6 @@ The Unnecessary Ruleset contains a collection of rules for unnecessary code.
     <rule ref="category/java/errorprone.xml/UnusedNullCheckInEquals" deprecated="true" />
     <rule ref="category/java/errorprone.xml/UselessOperationOnImmutable" deprecated="true" />
 
-    <rule ref="category/java/codestyle.xml/UnnecessaryFinalModifier" deprecated="true" />
     <rule ref="category/java/codestyle.xml/UnnecessaryModifier" deprecated="true" />
     <rule ref="category/java/codestyle.xml/UnnecessaryReturn" deprecated="true" />
     <rule ref="category/java/codestyle.xml/UselessParentheses" deprecated="true" />
diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/CodeStyleRulesTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/CodeStyleRulesTest.java
index c8f31b24dc..74ec6ec1d1 100644
--- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/CodeStyleRulesTest.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/CodeStyleRulesTest.java
@@ -59,7 +59,6 @@ public class CodeStyleRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "SuspiciousConstantFieldName");
         addRule(RULESET, "TooManyStaticImports");
         addRule(RULESET, "UnnecessaryConstructor");
-        addRule(RULESET, "UnnecessaryFinalModifier");
         addRule(RULESET, "UnnecessaryFullyQualifiedName");
         addRule(RULESET, "UnnecessaryLocalBeforeReturn");
         addRule(RULESET, "UnnecessaryModifier");
diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFinalModifier.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFinalModifier.xml
deleted file mode 100644
index 2f418ce542..0000000000
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFinalModifier.xml
+++ /dev/null
@@ -1,161 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<test-data
-    xmlns="http://pmd.sourceforge.net/rule-tests"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-    xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
-    <test-code>
-        <description><![CDATA[
-TEST1
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Foo {
- public void foo() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-TEST2
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Foo {
- public final void foo() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-TEST3
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public class Foo {
- public final void foo() { }
- public void foo2() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-TEST4
-     ]]></description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public final class Foo {
- public final void foo() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-TEST5
-     ]]></description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public final class Foo {
- public final void foo() { }
- public void foo2() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-TEST6
-     ]]></description>
-        <expected-problems>3</expected-problems>
-        <code><![CDATA[
-public final class Foo {
- public final void fooA() { }
- public final void fooS() { }
- public final void fooD() { }
- public void foo2() { }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-final method in inner class of non-final outer class
-     ]]></description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public final class Foo {
- public static class Bar {
-  public final void buz() {}
- }
-}
-     ]]></code>
-    </test-code>
-    <test-code>
-        <description><![CDATA[
-final method in inner final class 
-     ]]></description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public final class Foo {
- public final class Bar {
-  public final void buz() {}
- }
-}
-     ]]></code>
-    </test-code>
-
-    <test-code>
-        <description>#1464 UnnecessaryFinalModifier false positive on a @SafeVarargs method</description>
-        <expected-problems>0</expected-problems>
-        <code><![CDATA[
-public final class InboxContents<T> {
-    @SafeVarargs
-    public final InboxContents<T> conflateWith(T... values) { // false positive
-        return conflateWith(ImmutableList.copyOf(values));
-    }
-}
-public final class InboxContents2 {
-    @java.lang.SafeVarargs
-    public final InboxContents conflateWith(String... values) {
-        return conflateWith(ImmutableList.copyOf(values));
-    }
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-    <description>Unnecessary final of private method</description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class TestClass {
-	private final int getValue() {
-		return 0;
-	}
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-    <description>Unnecessary final of enum method</description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public enum Foo {
-    BAR {
-        @Override
-        public final void magic() {}
-    };
-
-    public void magic() {}
-}
-        ]]></code>
-    </test-code>
-    <test-code>
-    <description>Unnecessary final of anonymous class method</description>
-        <expected-problems>1</expected-problems>
-        <code><![CDATA[
-public class Foo {
-    public void stuff() {
-        Listener list = new Listener() {
-            @Override
-            public final void onEvent() {}
-        };
-    }
-}
-        ]]></code>
-    </test-code>
-</test-data>
diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryModifier.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryModifier.xml
index e395440c59..78d3292930 100644
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryModifier.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryModifier.xml
@@ -428,6 +428,163 @@ public abstract interface TestInterface {
         <expected-problems>1</expected-problems>
         <code><![CDATA[
 public abstract @interface TestAnnotation {
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+final method on non-final class is ok
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+ public final void foo() { }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+final and non-final methods mixed
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+ public final void foo() { }
+ public void foo2() { }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+final method on a final class
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public final class Foo {
+ public final void foo() { }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+mixed final and non-final methods on final class
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public final class Foo {
+ public final void foo() { }
+ public void foo2() { }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+multiple final methods on final class
+     ]]></description>
+        <expected-problems>3</expected-problems>
+        <code><![CDATA[
+public final class Foo {
+ public final void fooA() { }
+ public final void fooS() { }
+ public final void fooD() { }
+ public void foo2() { }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+final method in inner class of non-final outer class
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public final class Foo {
+ public static class Bar {
+  public final void buz() {}
+ }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+final method in inner final class 
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public final class Foo {
+ public final class Bar {
+  public final void buz() {}
+ }
+}
+     ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1464 UnnecessaryFinalModifier false positive on a @SafeVarargs method</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public final class InboxContents<T> {
+    @SafeVarargs
+    public final InboxContents<T> conflateWith(T... values) { // false positive
+        return conflateWith(ImmutableList.copyOf(values));
+    }
+}
+public final class InboxContents2 {
+    @java.lang.SafeVarargs
+    public final InboxContents conflateWith(String... values) {
+        return conflateWith(ImmutableList.copyOf(values));
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+    <description>Unnecessary final of private method</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class TestClass {
+	private final int getValue() {
+		return 0;
+	}
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+    <description>Unnecessary final of enum method</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public enum Foo {
+    BAR {
+        @Override
+        public final void magic() {}
+    };
+
+    public void magic() {}
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+    <description>Unnecessary final of anonymous class method</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public void stuff() {
+        Listener list = new Listener() {
+            @Override
+            public final void onEvent() {}
+        };
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+    <description>Unnecessary final of try-with-resources resource</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    public void stuff() {
+        try (final FileWriter fw = new FileWriter(new File("something.txt"))) {
+            // do something on fw
+        }
+    }
 }
         ]]></code>
     </test-code>