Merge branch 'master' into 7.0.x

docs/pages/release_notes.md

@@ -26,6 +26,15 @@ for parsing JavaScript code, requires at least Java 8. Therefore we decided to u
 module to Java 8 as well. This means that from now on, a Java 8 or later runtime is required in order
 to analyze JavaScript code. Note that PMD core still only requires Java 7.
 
+#### New rules
+
+*   The new Java rule {% rule "java/bestpractices/JUnit5TestShouldBePackagePrivate" %}
+    enforces the convention that JUnit 5 tests should have minimal visibility.
+    You can try out this rule like so:
+```xml
+    <rule ref="category/java/bestpractices.xml/JUnit5TestShouldBePackagePrivate" />
+```
+
 #### Modified rules
 
 *   The Java rule {% rule "java/errorprone/CompareObjectsWithEquals" %} has now a new property
@@ -35,6 +44,31 @@ to analyze JavaScript code. Note that PMD core still only requires Java 7.
     Additionally comparisons against constants are allowed now. This makes the rule less noisy when two constants
     are compared. Constants are identified by looking for an all-caps identifier.
 
+#### Deprecated rules
+
+*   The java rule {% rule "java/codestyle/DefaultPackage" %} has been deprecated in favor of
+    {% rule "java/codestyle/CommentDefaultAccessModifier" %}.
+
+    The rule "DefaultPackage" assumes that any usage of package-access is accidental,
+    and by doing so, prohibits using a really fundamental and useful feature of the language.
+
+    To satisfy the rule, you have to make the member public even if it doesn't need to, or make it protected,
+    which muddies your intent even more if you don't intend the class to be extended, and may be at odds with
+    other rules like {% rule "java/codestyle/AvoidProtectedFieldInFinalClass" %}.
+
+    The rule {% rule "java/codestyle/CommentDefaultAccessModifier" %} should be used instead.
+    It flags the same thing, but has an escape hatch.
+
+*   The Java rule {% rule "java/errorprone/CloneThrowsCloneNotSupportedException" %} has been deprecated without
+    replacement.
+
+    The rule has no real value as `CloneNotSupportedException` is a
+    checked exception and therefore you need to deal with it while implementing the `clone()` method. You either
+    need to declare the exception or catch it. If you catch it, then subclasses can't throw it themselves explicitly.
+    However, `Object.clone()` will still throw this exception if the `Cloneable` interface is not implemented.
+
+    Note, this rule has also been removed from the Quickstart Ruleset (`rulesets/java/quickstart.xml`).
+
 ### Fixed Issues
 
 *   apex
@@ -56,6 +90,7 @@ to analyze JavaScript code. Note that PMD core still only requires Java 7.
     *   [#3254](https://github.com/pmd/pmd/issues/3254): \[java] AvoidReassigningParameters reports violations on wrong line numbers
 *   java-codestyle
     *   [#2655](https://github.com/pmd/pmd/issues/2655): \[java] UnnecessaryImport false positive for on-demand imports
+    *   [#3206](https://github.com/pmd/pmd/issues/3206): \[java] Deprecate rule DefaultPackage
     *   [#3262](https://github.com/pmd/pmd/pull/3262): \[java] FieldDeclarationsShouldBeAtStartOfClass: false negative with anon classes
     *   [#3265](https://github.com/pmd/pmd/pull/3265): \[java] MethodArgumentCouldBeFinal: false negatives with interfaces and inner classes
     *   [#3266](https://github.com/pmd/pmd/pull/3266): \[java] LocalVariableCouldBeFinal: false negatives with interfaces, anon classes
@@ -65,6 +100,7 @@ to analyze JavaScript code. Note that PMD core still only requires Java 7.
     *   [#2780](https://github.com/pmd/pmd/issues/2780): \[java] DataClass example from documentation results in false-negative
 *   java-errorprone
     *   [#3110](https://github.com/pmd/pmd/issues/3110): \[java] Enhance CompareObjectsWithEquals with list of exceptions
+    *   [#3112](https://github.com/pmd/pmd/issues/3112): \[java] Deprecate rule CloneThrowsCloneNotSupportedException
     *   [#3205](https://github.com/pmd/pmd/issues/3205): \[java] Make CompareObjectWithEquals allow comparing against constants
     *   [#3248](https://github.com/pmd/pmd/issues/3248): \[java] Documentation is wrong for SingletonClassReturningNewInstance rule
     *   [#3249](https://github.com/pmd/pmd/pull/3249): \[java] AvoidFieldNameMatchingTypeName: False negative with interfaces

pmd-java/src/main/resources/category/java/bestpractices.xml

@@ -797,6 +797,71 @@ public class MyTest {
         </example>
     </rule>
 
+    <rule name="JUnit5TestShouldBePackagePrivate"
+          language="java"
+          since="6.35.0"
+          message="JUnit 5 tests should be package-private."
+          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          typeResolution="true"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#junit5testshouldbepackageprivate">
+        <description><![CDATA[
+Reports JUnit 5 test classes and methods that are not package-private.
+Contrary to JUnit 4 tests, which required public visibility to be run by the engine,
+JUnit 5 tests can also be run if they're package-private. Marking them as such
+is a good practice to limit their visibility.
+
+Test methods are identified as those which use `@Test`, `@RepeatedTest`,
+`@TestFactory`, `@TestTemplate` or `@ParameterizedTest`.
+            ]]></description>
+        <priority>3</priority>
+        <properties>
+            <property name="version" value="2.0"/>
+            <property name="xpath">
+                <value>
+<![CDATA[
+//ClassOrInterfaceDeclaration[
+    (: a Junit 5 test class, ie, it has methods with the annotation :)
+    @Interface=false() and
+    ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
+        [Annotation//Name[
+            pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest')
+            or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate')
+            or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest')
+        ]]
+        [MethodDeclaration]
+]/(
+    self::*[@Abstract=false() and (@Public=true() or @Protected=true())]
+|   ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
+        [Annotation//Name[
+            pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest')
+            or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate')
+            or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest')
+        ]]
+        /MethodDeclaration[@Public=true() or @Protected=true()]
+)
+]]>
+                </value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+class MyTest { // not public, that's fine
+    @Test
+    public void testBad() { } // should not have a public modifier
+
+    @Test
+    protected void testAlsoBad() { } // should not have a protected modifier
+
+    @Test
+    private void testNoRun() { } // should not have a private modifier
+
+    @Test
+    void testGood() { } // package private as expected
+}
+]]>
+        </example>
+    </rule>
+
     <rule name="JUnitAssertionsShouldIncludeMessage"
           language="java"
           since="1.04"

pmd-java/src/main/resources/category/java/codestyle.xml

@@ -404,6 +404,7 @@ while (true) {  // preferred approach
     </rule>
 
     <rule name="DefaultPackage"
+          deprecated="true"
           language="java"
           since="3.4"
           message="Use explicit scoping instead of the default package private level"
@@ -412,6 +413,16 @@ while (true) {  // preferred approach
         <description>
 Use explicit scoping instead of accidental usage of default package private level.
 The rule allows methods and fields annotated with Guava's @VisibleForTesting and JUnit 5's annotations.
+
+This rule is deprecated since PMD 6.35.0. It assumes that any usage of package-access is accidental,
+and by doing so, prohibits using a really fundamental and useful feature of the language.
+
+To satisfy the rule, you have to make the member public even if it doesn't need to, or make it protected,
+which muddies your intent even more if you don't intend the class to be extended, and may be at odds with
+other rules like {% rule "java/codestyle/AvoidProtectedFieldInFinalClass" %}.
+
+The rule {% rule "java/codestyle/CommentDefaultAccessModifier" %} should be used instead. This rule flags
+the same thing, but has an escape hatch.
         </description>
         <priority>3</priority>
         <properties>

pmd-java/src/main/resources/category/java/errorprone.xml

@@ -998,6 +998,7 @@ public class Foo implements Cloneable {
     </rule>
 
     <rule name="CloneThrowsCloneNotSupportedException"
+          deprecated="true"
           language="java"
           since="1.9"
           message="clone() method should throw CloneNotSupportedException"
@@ -1005,6 +1006,12 @@ public class Foo implements Cloneable {
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#clonethrowsclonenotsupportedexception">
         <description>
 The method clone() should throw a CloneNotSupportedException.
+
+This rule is deprecated since PMD 6.35.0 without replacement. The rule has no real value as
+`CloneNotSupportedException` is a checked exception and therefore you need to deal with it while
+implementing the `clone()` method. You either need to declare the exception or catch it. If you catch it,
+then subclasses can't throw it themselves explicitly. However, `Object.clone()` will still throw this
+exception if the `Cloneable` interface is not implemented.
         </description>
         <priority>3</priority>
         <properties>

pmd-java/src/main/resources/rulesets/java/quickstart.xml

@@ -27,6 +27,7 @@
     <!-- <rule ref="category/java/bestpractices.xml/JUnit4TestShouldUseAfterAnnotation" /> -->
     <!-- <rule ref="category/java/bestpractices.xml/JUnit4TestShouldUseBeforeAnnotation" /> -->
     <!-- <rule ref="category/java/bestpractices.xml/JUnit4TestShouldUseTestAnnotation" /> -->
+    <!-- <rule ref="category/java/bestpractices.xml/JUnit5TestShouldBePackagePrivate" /> -->
     <!-- <rule ref="category/java/bestpractices.xml/JUnitAssertionsShouldIncludeMessage" /> -->
     <!-- <rule ref="category/java/bestpractices.xml/JUnitTestContainsTooManyAsserts" /> -->
     <!-- <rule ref="category/java/bestpractices.xml/JUnitTestsShouldIncludeAssert" /> -->
@@ -89,7 +90,7 @@
     <!-- <rule ref="category/java/codestyle.xml/CommentDefaultAccessModifier" /> -->
     <!-- <rule ref="category/java/codestyle.xml/ConfusingTernary" /> -->
     <rule ref="category/java/codestyle.xml/ControlStatementBraces"/>
-    <!--<rule ref="category/java/codestyle.xml/DefaultPackage"/>-->
+    <!--<rule ref="category/java/codestyle.xml/DefaultPackage"/> deprecated since 6.35.0 -->
     <!-- <rule ref="category/java/codestyle.xml/EmptyMethodInAbstractClassShouldBeAbstract" /> -->
     <rule ref="category/java/codestyle.xml/ExtendsObject"/>
     <!-- <rule ref="category/java/codestyle.xml/FieldDeclarationsShouldBeAtStartOfClass" /> -->
@@ -203,7 +204,7 @@
     <rule ref="category/java/errorprone.xml/CloneMethodMustBePublic"/>
     <rule ref="category/java/errorprone.xml/CloneMethodMustImplementCloneable"/>
     <rule ref="category/java/errorprone.xml/CloneMethodReturnTypeMustMatchClassName"/>
-    <rule ref="category/java/errorprone.xml/CloneThrowsCloneNotSupportedException"/>
+    <!-- <rule ref="category/java/errorprone.xml/CloneThrowsCloneNotSupportedException"/> deprecated since 6.35.0 -->
     <rule ref="category/java/errorprone.xml/CloseResource"/>
     <rule ref="category/java/errorprone.xml/CompareObjectsWithEquals"/>
     <!-- <rule ref="category/java/errorprone.xml/ConstructorCallsOverridableMethod" /> -->

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/JUnit5TestShouldBePackagePrivateTest.java

@@ -0,0 +1,11 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices;
+
+import net.sourceforge.pmd.testframework.PmdRuleTst;
+
+public class JUnit5TestShouldBePackagePrivateTest extends PmdRuleTst {
+    // no additional unit tests
+}

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml

@@ -0,0 +1,132 @@
+<?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>Public modifier is not necessary on a test method nor on the class</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>3,5</expected-linenumbers>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+public class MyTests {
+    @Test
+    public void testRegular() { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Package private modifiers are what is required</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+class MyTests {
+    @Test
+    void testRegular() { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Private method modifiers should be reported by rule 'JUnit5TestNoPrivateModifier' and ignored by rule 'JUnit5TestShouldBePackagePrivate'</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+class MyTests {
+  @Test
+  private void ignoredTest() {}
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Private modifiers for inner classes should be reported by rule 'JUnit5TestNoPrivateModifier' and ignored by rule 'JUnit5TestShouldBePackagePrivate'</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+class MyTests {
+
+  private static class InnerPrivateClass {
+    @Test
+    void testFoo() {}
+  }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Public modifier is allowed on an abstract test classes</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+public abstract class MyTests {
+    @Test
+    void testRegular() { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Public modifier is allowed on test interfaces</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+
+public interface MyTests {
+    @Test
+    default void testRegular() { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Public and protected modifiers on all JUnit5 test types should be rejected</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>8,10,13,19,23</expected-linenumbers>
+        <code><![CDATA[
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.TestFactory;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+protected class MyTests {
+    @Test
+    public void testRegular() { }
+
+    @RepeatedTest(2)
+    protected void testRepeated() { }
+
+    @TestFactory
+    private void testFactory() { }
+
+    @TestTemplate
+    public void testTemplate() { }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"Hello", "World"})
+    protected void testParameterized(final String value) { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Public JUnit4 tests are not flagged</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import org.junit.Test;
+public class Foo {
+    @Test
+    public void testFoo() { }
+}
+        ]]></code>
+    </test-code>
+</test-data>