diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000000..d674a5bc29 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,13 @@ +version: 2 +updates: + - package-ecosystem: "maven" + directory: "/" + schedule: + interval: "weekly" + target-branch: "master" + open-pull-requests-limit: 0 + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + target-branch: "master" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 index 0000000000..c4c6346b0a --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,29 @@ +name: Java CI + +on: [push, pull_request] + +jobs: + build: + runs-on: ${{ matrix.os }} + continue-on-error: ${{ matrix.experimental }} + strategy: + matrix: + os: [ ubuntu-latest , windows-latest , macos-latest ] + java: [ 11 ] + experimental: [ false ] + + steps: + - uses: actions/checkout@v2.3.2 + - uses: actions/cache@v2 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + ${{ runner.os }}-maven- + - name: Set up JDK ${{ matrix.java }} + uses: actions/setup-java@v1.4.0 + with: + java-version: ${{ matrix.java }} + - name: Build with mvnw + run: | + ./mvnw clean install diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index a0c5c59808..30c35733f5 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -22,14 +22,26 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues * pmd-java - * [#2708](https://github.com/pmd/pmd/pull/2708): \[java] False positive FinalFieldCouldBeStatic when using lombok Builder.Default + * [#2708](https://github.com/pmd/pmd/issues/2708): \[java] False positive FinalFieldCouldBeStatic when using lombok Builder.Default + * [#2738](https://github.com/pmd/pmd/issues/2738): \[java] Custom rule with @ExhaustiveEnumSwitch throws NPE + * [#2756](https://github.com/pmd/pmd/issues/2756): \[java] TypeTestUtil fails with NPE for anonymous class + * [#2759](https://github.com/pmd/pmd/issues/2759): \[java] False positive in UnusedAssignment + * [#2767](https://github.com/pmd/pmd/issues/2767): \[java] IndexOutOfBoundsException when parsing an initializer BlockStatement ### API Changes +#### Deprecated API + +##### For removal + +* {% jdoc !!core::RuleViolationComparator %}. Use {% jdoc !!core::RuleViolation#DEFAULT_COMPARATOR %} instead. + ### External Contributions +* [#2735](https://github.com/pmd/pmd/pull/2735): \[ci] Add github actions for a fast view of pr succeed/not - [XenoAmess](https://github.com/XenoAmess) * [#2747](https://github.com/pmd/pmd/pull/2747): \[java] Don't trigger FinalFieldCouldBeStatic when field is annotated with lombok @Builder.Default - [Ollie Abbey](https://github.com/ollieabbey) +* [#2773](https://github.com/pmd/pmd/pull/2773): \[java] issue-2738: Adding null check to avoid npe when switch case is default - [Nimit Patel](https://github.com/nimit-patel) {% endtocmaker %} diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/Report.java b/pmd-core/src/main/java/net/sourceforge/pmd/Report.java index 15508e1c0a..c24a3d642b 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/Report.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/Report.java @@ -180,7 +180,7 @@ public class Report { * @param violation the violation to add */ public void addRuleViolation(RuleViolation violation) { - int index = Collections.binarySearch(violations, violation, RuleViolationComparator.INSTANCE); + int index = Collections.binarySearch(violations, violation, RuleViolation.DEFAULT_COMPARATOR); violations.add(index < 0 ? -index - 1 : index, violation); for (ThreadSafeReportListener listener : listeners) { listener.ruleViolationAdded(violation); @@ -228,7 +228,7 @@ public class Report { suppressedRuleViolations.addAll(r.suppressedRuleViolations); for (RuleViolation violation : r.getViolations()) { - int index = Collections.binarySearch(violations, violation, RuleViolationComparator.INSTANCE); + int index = Collections.binarySearch(violations, violation, RuleViolation.DEFAULT_COMPARATOR); violations.add(index < 0 ? -index - 1 : index, violation); } } @@ -245,7 +245,7 @@ public class Report { * Returns an unmodifiable list of violations that have been * recorded until now. None of those violations were suppressed. * - *

The violations list is sorted with {@link RuleViolationComparator#INSTANCE}. + *

The violations list is sorted with {@link RuleViolation#DEFAULT_COMPARATOR}. */ public final List getViolations() { return Collections.unmodifiableList(violations); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolation.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolation.java index dc36455ba5..b832c7d8a3 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolation.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolation.java @@ -4,6 +4,8 @@ package net.sourceforge.pmd; +import java.util.Comparator; + /** * A RuleViolation is created by a Rule when it identifies a violation of the * Rule constraints. RuleViolations are simple data holders that are collected @@ -16,6 +18,14 @@ package net.sourceforge.pmd; */ public interface RuleViolation { + /** + * A comparator for rule violations. This compares all exposed attributes + * of a violation, filename first. The remaining parameters are compared + * in an unspecified order. + */ + // TODO in java 8 this can be a chained Comparator.comparing call + Comparator DEFAULT_COMPARATOR = RuleViolationComparator.INSTANCE; + /** * Get the Rule which identified this violation. * diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolationComparator.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolationComparator.java index bbfa48ed07..bc1b1adf39 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolationComparator.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleViolationComparator.java @@ -17,7 +17,19 @@ import java.util.Comparator; *

  • End column
    • *
  • Rule name
    • * + * + * TODO why is begin line/begin column split?? would make more sense to use + * - filename + * - begin line + * - begin column + * - description + * - rule name + * - end line + * - end column + * + * @deprecated Use {@link RuleViolation#DEFAULT_COMPARATOR} */ +@Deprecated public final class RuleViolationComparator implements Comparator { public static final RuleViolationComparator INSTANCE = new RuleViolationComparator(); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationComparatorTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationComparatorTest.java index 45d3571c5c..bc7eb32fe2 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationComparatorTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationComparatorTest.java @@ -58,7 +58,7 @@ public class RuleViolationComparatorTest { Collections.shuffle(ruleViolations, random); // Sort - Collections.sort(ruleViolations, RuleViolationComparator.INSTANCE); + Collections.sort(ruleViolations, RuleViolation.DEFAULT_COMPARATOR); // Check int count = 0; diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationTest.java index d9717e3e4c..c7c448823c 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleViolationTest.java @@ -8,6 +8,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import java.io.File; +import java.util.Comparator; import org.junit.Ignore; import org.junit.Test; @@ -51,7 +52,7 @@ public class RuleViolationTest { @Test public void testComparatorWithDifferentFilenames() { Rule rule = new MockRule("name", "desc", "msg", "rulesetname"); - RuleViolationComparator comp = RuleViolationComparator.INSTANCE; + Comparator comp = RuleViolation.DEFAULT_COMPARATOR; RuleContext ctx = new RuleContext(); ctx.setSourceCodeFile(new File("filename1")); DummyNode s = new DummyNode(); @@ -68,7 +69,7 @@ public class RuleViolationTest { @Test public void testComparatorWithSameFileDifferentLines() { Rule rule = new MockRule("name", "desc", "msg", "rulesetname"); - RuleViolationComparator comp = RuleViolationComparator.INSTANCE; + Comparator comp = RuleViolation.DEFAULT_COMPARATOR; RuleContext ctx = new RuleContext(); ctx.setSourceCodeFile(new File("filename")); DummyNode s = new DummyNode(); @@ -85,7 +86,7 @@ public class RuleViolationTest { @Test public void testComparatorWithSameFileSameLines() { Rule rule = new MockRule("name", "desc", "msg", "rulesetname"); - RuleViolationComparator comp = RuleViolationComparator.INSTANCE; + Comparator comp = RuleViolation.DEFAULT_COMPARATOR; RuleContext ctx = new RuleContext(); ctx.setSourceCodeFile(new File("filename")); DummyNode s = new DummyNode(); diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index 88f2f9d6b3..438cb80ce2 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/pmd-java/etc/grammar/Java.jjt @@ -1731,8 +1731,9 @@ void BlockStatement(): | { List annotations = new ArrayList(); - while (jjtree.peekNode() instanceof ASTAnnotation) { - annotations.add((ASTAnnotation) jjtree.popNode()); + if (jjtree.nodeArity() > 0) { // peekNode would throw if the stack is empty + while (jjtree.peekNode() instanceof ASTAnnotation) { + annotations.add((ASTAnnotation) jjtree.popNode());} } } LocalVariableDeclaration() diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java index f0827016db..324775ae9f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatement.java @@ -79,7 +79,10 @@ public class ASTSwitchStatement extends AbstractJavaNode implements Iterable 0 && child.getChild(0) instanceof ASTAllocationExpression || child instanceof ASTPrimaryPrefix && child.getNumChildren() > 0 && child.getChild(0) instanceof ASTAllocationExpression) { - state.abruptCompletionByThrow(true); + state.abruptCompletionByThrow(true); // this is a noop if we're outside a try block that has catch/finally } } } @@ -1380,11 +1375,6 @@ public class UnusedAssignmentRule extends AbstractJavaRule { return this; } - private boolean isInTryBlock() { - // ignore resources, once they're initialized everything's fine in the block - return !myCatches.isEmpty() || myFinally != null; - } - SpanInfo absorb(SpanInfo other) { // Merge reaching defs of the other scope into this // This is used to join paths after the control flow has forked diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java index 873697e422..a2d187c631 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java @@ -106,7 +106,11 @@ public final class TypeTestUtil { final Class clazz = loadClassWithNodeClassloader(node, canonicalName); + if (clazz != null) { + if (clazz.getCanonicalName() == null) { + return false; // no canonical name, give up: we shouldn't be able to access them + } return clazz.isAssignableFrom(nodeType); } else { return fallbackIsA(node, canonicalName, true); @@ -174,8 +178,16 @@ public final class TypeTestUtil { } - return node.getType() == null ? fallbackIsA(node, canonicalName, false) - : node.getType().getCanonicalName().equals(canonicalName); + if (node.getType() == null) { + return fallbackIsA(node, canonicalName, false); + } + + String canoname = node.getType().getCanonicalName(); + if (canoname == null) { + // anonymous/local class, or class nested within one of those + return false; + } + return canoname.equals(canonicalName); } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatementTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatementTest.java new file mode 100644 index 0000000000..5915a67c3d --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTSwitchStatementTest.java @@ -0,0 +1,21 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.ast; + +import org.junit.Assert; +import org.junit.Test; + +public class ASTSwitchStatementTest extends BaseParserTest { + + @Test + public void exhaustiveEnumSwitchWithDefault() { + ASTSwitchStatement switchStatement = getNodes(ASTSwitchStatement.class, + "import java.nio.file.AccessMode; class Foo { void bar(AccessMode m) {" + + "switch (m) { case READ: break; default: break; } } }") + .get(0); + Assert.assertFalse(switchStatement.isExhaustiveEnumSwitch()); // this should not throw a NPE... + Assert.assertTrue(switchStatement.hasDefaultCase()); + } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ParserCornersTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ParserCornersTest.java index 30799b2550..2ee0d72368 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ParserCornersTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ParserCornersTest.java @@ -172,6 +172,16 @@ public class ParserCornersTest { java8.parseResource("GitHubBug207.java"); } + @Test + public void testGitHubBug2767() { + // PMD fails to parse an initializer block. + // PMD 6.26.0 parses this code just fine. + java.withDefaultVersion("15-preview") + .parse("class Foo {\n" + + " {final int I;}\n" + + "}\n"); + } + @Test public void testBug206() { java8.parse("public @interface Foo {" + "\n" diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java index a04149a88c..d672afc702 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/TypeTestUtilTest.java @@ -6,6 +6,7 @@ package net.sourceforge.pmd.lang.java.types; import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.concurrent.Callable; import org.junit.Assert; import org.junit.Rule; @@ -13,6 +14,7 @@ import org.junit.Test; import org.junit.function.ThrowingRunnable; import org.junit.rules.ExpectedException; +import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; @@ -20,6 +22,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTMarkerAnnotation; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.BaseNonParserTest; +import net.sourceforge.pmd.lang.java.types.testdata.SomeClassWithAnon; public class TypeTestUtilTest extends BaseNonParserTest { @@ -74,6 +77,31 @@ public class TypeTestUtilTest extends BaseNonParserTest { assertIsA(klass, Object.class); } + @Test + public void testAnonClassTypeNPE() { + // #2756 + + ASTAllocationExpression anon = + java.parseClass(SomeClassWithAnon.class) + .getFirstDescendantOfType(ASTAllocationExpression.class); + + + Assert.assertNotNull("Type should be resolved", anon.getType()); + Assert.assertTrue("Anon class", anon.isAnonymousClass()); + Assert.assertTrue("Anon class", anon.getType().isAnonymousClass()); + Assert.assertTrue("Should be a Runnable", TypeTestUtil.isA(Runnable.class, anon)); + + // This is not a canonical name, so we give up early + Assert.assertFalse(TypeTestUtil.isA(SomeClassWithAnon.class.getName() + "$1", anon)); + Assert.assertFalse(TypeTestUtil.isExactlyA(SomeClassWithAnon.class.getName() + "$1", anon)); + + // this is the failure case: if the binary name doesn't match, we test the canoname, which was null + Assert.assertFalse(TypeTestUtil.isA(Callable.class, anon)); + Assert.assertFalse(TypeTestUtil.isA(Callable.class.getCanonicalName(), anon)); + Assert.assertFalse(TypeTestUtil.isExactlyA(Callable.class, anon)); + Assert.assertFalse(TypeTestUtil.isExactlyA(Callable.class.getCanonicalName(), anon)); + } + /** * If we don't have the annotation on the classpath, * we should resolve the full name via the import, if possible diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/testdata/SomeClassWithAnon.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/testdata/SomeClassWithAnon.java new file mode 100644 index 0000000000..09d2467f25 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/testdata/SomeClassWithAnon.java @@ -0,0 +1,25 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.types.testdata; + +/** + * #2756 + */ +public class SomeClassWithAnon { + + { + new Runnable() { + + @Override + public void run() { + + } + }; + + + } + + +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedAssignment.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedAssignment.xml index 1f0d55cebc..0dd7179c5e 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedAssignment.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedAssignment.xml @@ -3159,4 +3159,29 @@ public class UnusedAssignmentNative { ]]> + + False positive with method that may throw in forked state (the if state) #2759 + 0 + 0) { + a--; + try { + if (dummy) { + return somethingThatCanThrowRandomly(1); + } else { + return somethingThatCanThrowRandomly(2); + } + } catch (RuntimeException e) { + // retry + } + } + return 0; + } + } + ]]> + +