Skip to content

Commit

Permalink
Merge pull request #3716 from oowekyala:merge-empty-rules
Browse files Browse the repository at this point in the history
[java] Merge rules that check for empty things #3716
  • Loading branch information
adangel committed May 28, 2022
2 parents 51fa5e4 + 95ad4d3 commit 3b774eb
Show file tree
Hide file tree
Showing 13 changed files with 990 additions and 10 deletions.
47 changes: 47 additions & 0 deletions docs/pages/release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,51 @@ When executing CPD on C# sources, the option `--ignore-annotations` is now suppo
It ignores C# attributes when detecting duplicated code. This option can also be enabled via
the CPD GUI. See [#3974](https://github.com/pmd/pmd/pull/3974) for details.

#### New Rules

This release ships with 2 new Java rules.

* {% rule java/codestyle/EmptyControlStatement %} reports many instances of empty things, e.g. control statements whose
body is empty, as well as empty initializers.

EmptyControlStatement also works for empty `for` and `do` loops, while there were previously
no corresponding rules.

This new rule replaces the rules EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock,
EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, and EmptyWhileStmt.

```xml
<rule ref="category/java/codestyle.xml/EmptyControlStatement"/>
```

The rule is part of the quickstart.xml ruleset.

* {%rule java/codestyle/UnnecessarySemicolon %} reports semicolons that are unnecessary (so called "empty statements"
and "empty declarations").

This new rule replaces the rule EmptyStatementNotInLoop.

```xml
<rule ref="category/java/codestyle.xml/UnnecessarySemicolon"/>
```

The rule is part of the quickstart.xml ruleset.

#### Deprecated Rules

* The following Java rules are deprecated and removed from the quickstart ruleset, as the new rule
{% rule java/codestyle/EmptyControlStatement %} merges their functionality:
* {% rule java/errorprone/EmptyFinallyBlock %}
* {% rule java/errorprone/EmptyIfStmt %}
* {% rule java/errorprone/EmptyInitializer %}
* {% rule java/errorprone/EmptyStatementBlock %}
* {% rule java/errorprone/EmptySwitchStatements %}
* {% rule java/errorprone/EmptySynchronizedBlock %}
* {% rule java/errorprone/EmptyTryBlock %}
* {% rule java/errorprone/EmptyWhileStmt %}
* The Java rule {% rule java/errorprone/EmptyStatementNotInLoop %} is deprecated and removed from the quickstart
ruleset. Use the new rule {% rule java/codestyle/UnnecessarySemicolon %} instead.

### Fixed Issues

* cli
Expand All @@ -52,6 +97,8 @@ the CPD GUI. See [#3974](https://github.com/pmd/pmd/pull/3974) for details.
* [#3954](https://github.com/pmd/pmd/issues/3954): \[java] NPE in UseCollectionIsEmptyRule when .size() is called in a record
* java-design
* [#3874](https://github.com/pmd/pmd/issues/3874): \[java] ImmutableField reports fields annotated with @Autowired (Spring) and @Mock (Mockito)
* java-errorprone
* [#3096](https://github.com/pmd/pmd/issues/3096): \[java] EmptyStatementNotInLoop FP in 6.30.0 with IfStatement
* java-performance
* [#3379](https://github.com/pmd/pmd/issues/3379): \[java] UseArraysAsList must ignore primitive arrays
* [#3965](https://github.com/pmd/pmd/issues/3965): \[java] UseArraysAsList false positive with non-trivial loops
Expand Down
14 changes: 14 additions & 0 deletions pmd-core/src/main/resources/rulesets/releases/6460.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version="1.0"?>

<ruleset name="6460"
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd">
<description>
This ruleset contains links to rules that are new in PMD v6.46.0
</description>

<rule ref="category/java/codestyle.xml/EmptyControlStatement"/>
<rule ref="category/java/codestyle.xml/UnnecessarySemicolon"/>

</ruleset>
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,12 @@ public boolean isStatic() {
public void setStatic() {
isStatic = true;
}

/**
* Returns the body of this initializer.
*/
public ASTBlock getBody() {
return (ASTBlock) getChild(0);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ public Object jjtAccept(JavaParserVisitor visitor, Object data) {
return visitor.visit(this, data);
}

public String getStableName() {
ASTVariableDeclaratorId variableDeclaratorId = getVariableDeclaratorId();
if (variableDeclaratorId != null) {
return variableDeclaratorId.getName();
}
// concise try-with-resources
return getFirstChildOfType(ASTName.class).getImage();
}

// TODO Should we deprecate all methods from ASTFormalParameter?

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,11 @@ public ASTSynchronizedStatement(JavaParser p, int id) {
public Object jjtAccept(JavaParserVisitor visitor, Object data) {
return visitor.visit(this, data);
}

/**
* Returns the body of this statement.
*/
public ASTBlock getBody() {
return (ASTBlock) getChild(1);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/

package net.sourceforge.pmd.lang.java.rule.codestyle;

import net.sourceforge.pmd.lang.java.ast.ASTBlock;
import net.sourceforge.pmd.lang.java.ast.ASTDoStatement;
import net.sourceforge.pmd.lang.java.ast.ASTEmptyStatement;
import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement;
import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTResource;
import net.sourceforge.pmd.lang.java.ast.ASTResourceSpecification;
import net.sourceforge.pmd.lang.java.ast.ASTStatement;
import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement;
import net.sourceforge.pmd.lang.java.ast.ASTSynchronizedStatement;
import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;

public class EmptyControlStatementRule extends AbstractJavaRule {

public EmptyControlStatementRule() {
addRuleChainVisit(ASTFinallyStatement.class);
addRuleChainVisit(ASTSynchronizedStatement.class);
addRuleChainVisit(ASTTryStatement.class);
addRuleChainVisit(ASTDoStatement.class);
addRuleChainVisit(ASTBlock.class);
addRuleChainVisit(ASTForStatement.class);
addRuleChainVisit(ASTWhileStatement.class);
addRuleChainVisit(ASTIfStatement.class);
addRuleChainVisit(ASTSwitchStatement.class);
addRuleChainVisit(ASTInitializer.class);
}

@Override
public Object visit(JavaNode node, Object data) {
throw new UnsupportedOperationException("should not be called");
}

@Override
public Object visit(ASTFinallyStatement node, Object data) {
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty finally clause");
}
return null;
}

@Override
public Object visit(ASTSynchronizedStatement node, Object data) {
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty synchronized statement");
}
return null;
}

@Override
public Object visit(ASTSwitchStatement node, Object data) {
if (node.getNumChildren() == 1) {
asCtx(data).addViolationWithMessage(node, "Empty switch statement");
}
return null;
}

@Override
public Object visit(ASTBlock node, Object data) {
if (isEmpty(node) && node.getNthParent(3) instanceof ASTBlock) {
asCtx(data).addViolationWithMessage(node, "Empty block");
}
return null;
}

@Override
public Object visit(ASTIfStatement node, Object data) {
if (isEmpty(node.getThenBranch().getChild(0))) {
asCtx(data).addViolationWithMessage(node, "Empty if statement");
}
if (node.hasElse() && isEmpty(node.getElseBranch().getChild(0))) {
asCtx(data).addViolationWithMessage(node.getElseBranch(), "Empty else statement");
}
return null;
}

@Override
public Object visit(ASTWhileStatement node, Object data) {
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty while statement");
}
return null;
}

@Override
public Object visit(ASTForStatement node, Object data) {
if (node.isForeach() && JavaRuleUtil.isExplicitUnusedVarName(node.getFirstChildOfType(ASTLocalVariableDeclaration.class)
.getFirstDescendantOfType(ASTVariableDeclaratorId.class).getName())) {
// allow `for (ignored : iterable) {}`
return null;
}
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty for statement");
}
return null;
}

@Override
public Object visit(ASTDoStatement node, Object data) {
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty do..while statement");
}
return null;
}

@Override
public Object visit(ASTInitializer node, Object data) {
if (isEmpty(node.getBody())) {
asCtx(data).addViolationWithMessage(node, "Empty initializer statement");
}
return null;
}

@Override
public Object visit(ASTTryStatement node, Object data) {
if (isEmpty(node.getBody())) {
// all resources must be explicitly ignored
boolean allResourcesIgnored = true;
boolean hasResource = false;
ASTResourceSpecification resources = node.getFirstChildOfType(ASTResourceSpecification.class);
if (resources != null) {
for (ASTResource resource : resources.findDescendantsOfType(ASTResource.class)) {
hasResource = true;
String name = resource.getStableName();
if (!JavaRuleUtil.isExplicitUnusedVarName(name)) {
allResourcesIgnored = false;
break;
}
}
}

if (hasResource && !allResourcesIgnored) {
asCtx(data).addViolationWithMessage(node, "Empty try body - you could rename the resource to ''ignored''");
} else if (!hasResource) {
asCtx(data).addViolationWithMessage(node, "Empty try body");
}
}
return null;
}

private boolean isEmpty(JavaNode node) {
if (node instanceof ASTStatement) {
node = node.getChild(0);
}
return node instanceof ASTBlock && node.getNumChildren() == 0
|| node instanceof ASTEmptyStatement;
}
}
95 changes: 95 additions & 0 deletions pmd-java/src/main/resources/category/java/codestyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,53 @@ public abstract class ShouldBeAbstract {
</example>
</rule>

<rule name="EmptyControlStatement"
language="java"
since="6.46.0"
message="This control statement has an empty branch"
class="net.sourceforge.pmd.lang.java.rule.codestyle.EmptyControlStatementRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#emptycontrolstatement">
<!-- Note about naming: EmptyCodeBlock does not work because the rule flags `if(true);` -->
<!-- EmptyControlStatement is weird because `{}` is not a control statement, and we also flag initializers. -->
<!-- EmptyStatement does not work because `;` is called an "empty statement" and is not flagged by this rule, rather, by UnnecessarySemicolon. -->
<!-- EmptyCodeConstruct would work but sounds a bit odd (right?). -->
<description><![CDATA[
Reports control statements whose body is empty, as well as empty initializers.
The checked code constructs are the following:
- bodies of `try` statements
- `finally` clauses of `try` statements
- `switch` statements
- `synchronized` statements
- `if` statements
- loop statements: `while`, `for`, `do .. while`
- initializers
- blocks used as statements (for scoping)
This rule replaces the rules EmptyFinallyBlock,
EmptyIfStmt, EmptyInitializer, EmptyStatementBlock,
EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, and EmptyWhileStmt.
Notice that {% rule java/errorprone/EmptyCatchBlock %} is still an independent rule.
EmptyStatementNotInLoop is replaced by {% rule java/codestyle/UnnecessarySemicolon %}.
]]></description>
<priority>3</priority>
<example>
<![CDATA[
class Foo {
{
if (true); // empty if statement
if (true) { // empty as well
}
}
{} // empty initializer
}
]]>
</example>
</rule>

<rule name="ExtendsObject"
language="java"
since="5.0"
Expand Down Expand Up @@ -2127,6 +2174,54 @@ public class Foo {
</example>
</rule>

<rule name="UnnecessarySemicolon"
language="java"
since="6.46.0"
message="Unnecessary semicolon"
class="net.sourceforge.pmd.lang.rule.XPathRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#unnecessarysemicolon">
<description>
Reports unnecessary semicolons (so called "empty statements" and "empty declarations").
These can be removed without changing the program. The Java grammar
allows them for historical reasons, but they should be avoided.

This rule will not report empty statements that are syntactically
required, for instance, because they are the body of a control statement.

This rule replaces EmptyStatementNotInLoop.
</description>
<priority>3</priority>
<properties>
<property name="version" value="2.0"/>
<property name="xpath">
<value><![CDATA[
(: empty top-level declarations :)
//EmptyStatement[parent::CompilationUnit]
(: empty body declarations :)
| //ClassOrInterfaceBodyDeclaration[count(*)=0]
(: empty statements :)
| //Block/BlockStatement/Statement/EmptyStatement
(: grammar is irregular in pmd 6 :)
| //ConstructorDeclaration/BlockStatement/Statement/EmptyStatement
]]>
</value>
</property>
</properties>
<example>
<![CDATA[
class Foo {
{
toString();; // one of these semicolons is unnecessary
if (true); // this semicolon is not unnecessary, but it could be an empty block instead (not reported)
}
}; // this semicolon is unnecessary
]]>
</example>
</rule>


<rule name="UseDiamondOperator"
language="java"
since="6.11.0"
Expand Down
Loading

0 comments on commit 3b774eb

Please sign in to comment.