hiifong/pmd
1
0
Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

[java] Fix #4912 - grammar for TWR allows this expression (#4934)

Browse Source 
Merge pull request #4934 from oowekyala:issue4912-java-grammar-fix
This commit is contained in:
Andreas Dangel
2024-05-02 11:39:38 +02:00
parent 42ffb31ca0 b7a39e39e9
commit 435f0dc7b3
12 changed files with 124 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
docs/pages/release_notes.md
View File

@ -20,15 +20,23 @@ This is a {{ site.pmd.release_type }} release.
* [#4978](https://github.com/pmd/pmd/issues/4978): \[core] Referenced Rulesets do not emit details on validation errors
* [#4983](https://github.com/pmd/pmd/pull/4983): \[cpd] Fix CPD crashes about unicode escapes
* java
* [#4912](https://github.com/pmd/pmd/issues/4912): \[java] Unable to parse some Java9+ resource references
* [#4973](https://github.com/pmd/pmd/pull/4973): \[java] Stop parsing Java for CPD
* java-bestpractices
* [#4278](https://github.com/pmd/pmd/issues/4278): \[java] UnusedPrivateMethod FP with Junit 5 @MethodSource and default factory method name
* [#4852](https://github.com/pmd/pmd/issues/4852): \[java] ReplaceVectorWithList false-positive (neither Vector nor List usage)
* [#4975](https://github.com/pmd/pmd/issues/4975): \[java] UnusedPrivateMethod false positive when using @MethodSource on a @Nested test
* [#4985](https://github.com/pmd/pmd/issues/4985): \[java] UnusedPrivateMethod false-positive / method reference in combination with custom object
* java-codestyle
* [#4930](https://github.com/pmd/pmd/issues/4930): \[java] EmptyControlStatement should not allow empty try with concise resources
### 🚨 API Changes
#### Deprecated API
* pmd-java
* {% jdoc !!java::lang.java.ast.ASTResource#getStableName() %} and the corresponding attribute `@StableName`
### ✨ External Contributions
{% endtocmaker %}

4
pmd-java/etc/grammar/Java.jjt
View File

@ -2800,8 +2800,8 @@ void Resource() :
PrimaryExpression()
{
Node top = jjtree.peekNode();
if (!(top instanceof ASTVariableAccess || top instanceof ASTFieldAccess))
throwParseException("Expected a variable access, but was a " + top.getXPathNodeName());
if (!(top instanceof ASTVariableAccess || top instanceof ASTFieldAccess || top instanceof ASTThisExpression))
throwParseException("Expected a variable or field access, but was a " + top.getXPathNodeName());
}
{}
}

22
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java
View File

@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.ast;
import org.checkerframework.checker.nullness.qual.Nullable;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.internal.PrettyPrintingUtil;
/**
* A resource of a {@linkplain ASTTryStatement try-with-resources}. This contains another
@ -48,24 +49,15 @@ public final class ASTResource extends AbstractJavaNode {
* then returns the sequence of names that identifies the expression.
* If this has a local variable declaration, then returns the name
* of the variable.
*
* @deprecated Since 7.1.0. This method is not very useful because the expression
* might be complex and not have a real "name". In this case we return the
* expression pretty printed.
*/
@Deprecated
public String getStableName() {
if (isConciseResource()) {
ASTExpression expr = getInitializer();
StringBuilder builder = new StringBuilder();
while (expr instanceof ASTFieldAccess) {
ASTFieldAccess fa = (ASTFieldAccess) expr;
builder.insert(0, "." + fa.getName());
expr = fa.getQualifier();
}
// the last one may be ambiguous, or a variable reference
// the only common interface we have to get their name is
// unfortunately Node::getImage
if (expr != null) {
builder.insert(0, expr.getImage());
}
return builder.toString();
return PrettyPrintingUtil.prettyPrint(getInitializer()).toString();
} else {
return asLocalVariableDeclaration().iterator().next().getName();
}

9
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java
View File

@ -37,6 +37,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
import net.sourceforge.pmd.lang.java.ast.ASTLambdaParameterList;
import net.sourceforge.pmd.lang.java.ast.ASTList;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTMethodReference;
@ -186,7 +187,12 @@ public final class PrettyPrintingUtil {
} else if (node instanceof ASTFieldDeclaration) {
return ((ASTFieldDeclaration) node).getVarIds().firstOrThrow().getName();
} else if (node instanceof ASTResource) {
return ((ASTResource) node).getStableName();
ASTLocalVariableDeclaration var = ((ASTResource) node).asLocalVariableDeclaration();
if (var != null) {
return var.getVarIds().firstOrThrow().getName();
} else {
return PrettyPrintingUtil.prettyPrint(((ASTResource) node).getInitializer()).toString();
}
} else if (node instanceof ASTTypeDeclaration) {
return ((ASTTypeDeclaration) node).getSimpleName();
} else if (node instanceof ASTVariableId) {
@ -247,6 +253,7 @@ public final class PrettyPrintingUtil {
}
/** Pretty print an expression or any other kind of node. */
public static CharSequence prettyPrint(JavaNode node) {
StringBuilder sb = new StringBuilder();
node.acceptVisitor(new ExprPrinter(), sb);

47
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/EmptyControlStatementRule.java
View File

@ -7,18 +7,26 @@ 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.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
import net.sourceforge.pmd.lang.java.ast.ASTFinallyClause;
import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
import net.sourceforge.pmd.lang.java.ast.ASTForeachStatement;
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.ASTResourceList;
import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
import net.sourceforge.pmd.lang.java.ast.ASTSwitchStatement;
import net.sourceforge.pmd.lang.java.ast.ASTSynchronizedStatement;
import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.ast.internal.PrettyPrintingUtil;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
import net.sourceforge.pmd.properties.PropertyDescriptor;
@ -133,29 +141,54 @@ public class EmptyControlStatementRule extends AbstractJavaRulechainRule {
public Object visit(ASTTryStatement node, Object data) {
if (isEmpty(node.getBody())) {
// all resources must be explicitly ignored
boolean allResourcesIgnored = true;
boolean hasResource = false;
ASTResourceList resources = node.getResources();
if (resources != null) {
for (ASTResource resource : resources) {
hasResource = true;
String name = resource.getStableName();
ASTLocalVariableDeclaration localVarDecl = resource.asLocalVariableDeclaration();
if (localVarDecl == null) {
// not a concise resource.
ASTExpression init = resource.getInitializer();
if (isSimpleExpression(init)) {
// The expression is simple enough, it should be just written this.close() or var.close(),
// so we report this case
asCtx(data).addViolationWithMessage(node, "Empty try-with-resources statement. Should be written {0}.close()", PrettyPrintingUtil.prettyPrint(init));
return null;
}
// Otherwise the expression is more complex and this is allowed, in order
// to avoid bringing a variable into the enclosing scope
continue;
}
// A named resource. Named should be ignored.
ASTVariableId varId = localVarDecl.getVarIds().firstOrThrow();
if (!varId.getLocalUsages().isEmpty()) {
// this resource is used in other resource declarations or in a finally block
return null;
}
String name = varId.getName();
if (!JavaRuleUtil.isExplicitUnusedVarName(name)) {
allResourcesIgnored = false;
break;
asCtx(data).addViolationWithMessage(node, "Empty try-with-resources statement. Rename the resource to `ignored`, `unused` or `_` (Java 22+).");
return null;
}
}
}
if (hasResource && !allResourcesIgnored) {
asCtx(data).addViolationWithMessage(node, "Empty try body - you could rename the resource to ''ignored''");
} else if (!hasResource) {
if (!hasResource) {
asCtx(data).addViolationWithMessage(node, "Empty try body");
}
}
return null;
}
private static boolean isSimpleExpression(ASTExpression init) {
return init instanceof ASTThisExpression
|| init instanceof ASTSuperExpression
|| init instanceof ASTVariableAccess
|| init instanceof ASTFieldAccess && isSimpleExpression(((ASTFieldAccess) init).getQualifier());
}
private boolean isEmpty(JavaNode node) {
boolean allowCommentedBlocks = getProperty(ALLOW_COMMENTED_BLOCKS);

26
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java
View File

@ -23,6 +23,7 @@ import org.pcollections.PSet;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.ast.impl.GenericNode;
import net.sourceforge.pmd.lang.java.ast.ASTAmbiguousName;
import net.sourceforge.pmd.lang.java.ast.ASTAnonymousClassDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTBlock;
@ -386,14 +387,14 @@ public final class SymbolTableResolver {
/**
* Note: caller is responsible for popping.
*/
private int visitBlockLike(Iterable<? extends ASTStatement> node, @NonNull ReferenceCtx ctx) {
private int visitBlockLike(Iterable<? extends JavaNode> node, @NonNull ReferenceCtx ctx) {
/*
* Process the statements of a block in a sequence. Each local
* var/class declaration is only in scope for the following
* statements (and its own initializer).
*/
int pushed = 0;
for (ASTStatement st : node) {
for (JavaNode st : node) {
if (st instanceof ASTLocalVariableDeclaration) {
pushed += processLocalVarDecl((ASTLocalVariableDeclaration) st, ctx);
// note we don't pop here, all those variables will be popped at the end of the block
@ -403,10 +404,17 @@ public final class SymbolTableResolver {
processTypeHeader(local, ctx);
}
setTopSymbolTable(st);
// those vars are the one produced by pattern bindings/ local var decls
PSet<ASTVariableId> newVars = st.acceptVisitor(this.stmtVisitor, ctx);
pushed += pushOnStack(f.localVarSymTable(top(), enclosing(), newVars));
if (st instanceof ASTStatement) {
setTopSymbolTable(st);
// those vars are the one produced by pattern bindings/ local var decls
PSet<ASTVariableId> newVars = st.acceptVisitor(this.stmtVisitor, ctx);
pushed += pushOnStack(f.localVarSymTable(top(), enclosing(), newVars));
} else {
// concise resource initializer
assert st instanceof ASTExpression && st.getParent() instanceof ASTResource : st;
setTopSymbolTable(st.getParent());
st.acceptVisitor(this, ctx);
}
}
return pushed;
@ -450,7 +458,7 @@ public final class SymbolTableResolver {
ASTResourceList resources = node.getResources();
if (resources != null) {
NodeStream<ASTStatement> union =
NodeStream<JavaNode> union =
NodeStream.union(
stmtsOfResources(resources),
// use the body instead of unwrapping it so
@ -750,8 +758,8 @@ public final class SymbolTableResolver {
// <editor-fold defaultstate="collapsed" desc="Convenience methods">
static NodeStream<ASTLocalVariableDeclaration> stmtsOfResources(ASTResourceList node) {
return node.toStream().map(ASTResource::asLocalVariableDeclaration);
static NodeStream<JavaNode> stmtsOfResources(ASTResourceList node) {
return node.toStream().map(GenericNode::getFirstChild);
}

15
pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt
View File

@ -4,10 +4,10 @@
package net.sourceforge.pmd.lang.java.ast
import net.sourceforge.pmd.lang.test.ast.shouldBe
import net.sourceforge.pmd.lang.java.ast.JavaVersion.Companion.Latest
import net.sourceforge.pmd.lang.java.ast.JavaVersion.J1_7
import net.sourceforge.pmd.lang.java.ast.JavaVersion.J9
import net.sourceforge.pmd.lang.test.ast.shouldBe
/**
* @author Clément Fournier
@ -23,7 +23,6 @@ class ASTTryStatementTest : ParserTestSpec({
child<ASTResourceList> {
child<ASTResource> {
it::isConciseResource shouldBe false
it::getStableName shouldBe "a"
it::getInitializer shouldBe fromChild<ASTLocalVariableDeclaration, ASTExpression> {
it::getModifiers shouldBe localVarModifiers {
@ -50,7 +49,6 @@ class ASTTryStatementTest : ParserTestSpec({
child<ASTResourceList> {
child<ASTResource> {
it::isConciseResource shouldBe false
it::getStableName shouldBe "a"
it::getInitializer shouldBe fromChild<ASTLocalVariableDeclaration, ASTExpression> {
it::getModifiers shouldBe localVarModifiers {
@ -83,7 +81,6 @@ class ASTTryStatementTest : ParserTestSpec({
child<ASTResourceList> {
child<ASTResource> {
it::isConciseResource shouldBe true
it::getStableName shouldBe "a"
it::getInitializer shouldBe variableAccess("a")
}
@ -101,7 +98,6 @@ class ASTTryStatementTest : ParserTestSpec({
child<ASTResourceList> {
child<ASTResource> {
it::isConciseResource shouldBe true
it::getStableName shouldBe "a"
it::getInitializer shouldBe variableAccess("a")
}
@ -119,7 +115,6 @@ class ASTTryStatementTest : ParserTestSpec({
child<ASTResourceList> {
child<ASTResource> {
it::isConciseResource shouldBe true
it::getStableName shouldBe "a.b"
it::getInitializer shouldBe fieldAccess("b") {
ambiguousName("a")
@ -132,8 +127,16 @@ class ASTTryStatementTest : ParserTestSpec({
}
}
// the expr must be a field access or var access
"try ( a.foo() ){}" shouldNot parse()
"try (new Foo()){}" shouldNot parse()
"try(arr[0]) {}" shouldNot parse()
"try ( a.foo().b ){}" should parse()
"try (new Foo().x){}" should parse()
// this is also allowed by javac
"try(this) {}" should parse()
"try(Foo.this) {}" should parse()
}
}

12
pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/VarScopingTest.kt
View File

@ -132,14 +132,18 @@ class VarScopingTest : ProcessorTestSpec({
try (Reader f = r; // reader3
BufferedReader r = f.buffered()) { // br2
}
try (Reader f4 = r; // reader4
f4.buffered().field) { // inConciseResource
}
}
}
""".trimIndent())
val (outerField, exception1, reader1, exception2, reader2, bufferedReader, reader3, br2) =
val (outerField, exception1, reader1, exception2, reader2, bufferedReader, reader3, br2, reader4) =
acu.descendants(ASTVariableId::class.java).toList()
val (inCatch1, inTry, inCatch2, inFinally, inResource) =
val (inCatch1, inTry, inCatch2, inFinally, inResource, _, inConciseResource) =
acu.descendants(ASTMethodCall::class.java).toList()
doTest("Inside catch clause: catch param is in scope") {
@ -170,6 +174,10 @@ class VarScopingTest : ProcessorTestSpec({
br2.initializer!! shouldResolveToLocal reader3
}
doTest("Inside concise resource declaration") {
inConciseResource.qualifier!! shouldResolveToLocal reader4
}
doTest("Resources are in scope, even if the try body is empty") {
val emptyBlock = br2.ancestors(ASTTryStatement::class.java).firstOrThrow().body
emptyBlock.toList() should beEmpty()

6
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt
View File

@ -403,7 +403,7 @@
| | +- StringLiteral[@CompileTimeConstant = true, @ConstValue = "/foo", @Empty = false, @Image = "\"/foo\"", @Length = 4, @LiteralText = "\"/foo\"", @ParenthesisDepth = 0, @Parenthesized = false, @TextBlock = false]
| +- TryStatement[@TryWithResources = true]
| | +- ResourceList[@Empty = false, @Size = 1, @TrailingSemiColon = false]
| | | +- Resource[@ConciseResource = false, @StableName = "br"]
| | | +- Resource[@ConciseResource = false]
| | | +- LocalVariableDeclaration[@EffectiveVisibility = Visibility.V_LOCAL, @Final = true, @TypeInferred = false, @Visibility = Visibility.V_LOCAL]
| | | +- ModifierList[@EffectiveModifiers = "{final}", @ExplicitModifiers = "{}"]
| | | +- ClassType[@FullyQualified = false, @SimpleName = "BufferedReader"]
@ -459,7 +459,7 @@
| | +- VariableAccess[@AccessType = AccessType.READ, @CompileTimeConstant = false, @Image = "outputFileName", @Name = "outputFileName", @ParenthesisDepth = 0, @Parenthesized = false]
| +- TryStatement[@TryWithResources = true]
| +- ResourceList[@Empty = false, @Size = 2, @TrailingSemiColon = false]
| | +- Resource[@ConciseResource = false, @StableName = "zf"]
| | +- Resource[@ConciseResource = false]
| | | +- LocalVariableDeclaration[@EffectiveVisibility = Visibility.V_LOCAL, @Final = true, @TypeInferred = false, @Visibility = Visibility.V_LOCAL]
| | | +- ModifierList[@EffectiveModifiers = "{final}", @ExplicitModifiers = "{}"]
| | | +- ClassType[@FullyQualified = true, @SimpleName = "ZipFile"]
@ -469,7 +469,7 @@
| | | +- ClassType[@FullyQualified = true, @SimpleName = "ZipFile"]
| | | +- ArgumentList[@Empty = false, @Size = 1]
| | | +- VariableAccess[@AccessType = AccessType.READ, @CompileTimeConstant = false, @Image = "zipFileName", @Name = "zipFileName", @ParenthesisDepth = 0, @Parenthesized = false]
| | +- Resource[@ConciseResource = false, @StableName = "writer"]
| | +- Resource[@ConciseResource = false]
| | +- LocalVariableDeclaration[@EffectiveVisibility = Visibility.V_LOCAL, @Final = true, @TypeInferred = false, @Visibility = Visibility.V_LOCAL]
| | +- ModifierList[@EffectiveModifiers = "{final}", @ExplicitModifiers = "{}"]
| | +- ClassType[@FullyQualified = true, @SimpleName = "BufferedWriter"]

2
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java21p/Jep443_UnnamedPatternsAndVariables.txt
View File

@ -519,7 +519,7 @@
+- Block[@Empty = false, @Size = 4, @containsComment = false]
+- TryStatement[@TryWithResources = true]
| +- ResourceList[@Empty = false, @Size = 1, @TrailingSemiColon = false]
| | +- Resource[@ConciseResource = false, @StableName = "_"]
| | +- Resource[@ConciseResource = false]
| | +- LocalVariableDeclaration[@EffectiveVisibility = Visibility.V_LOCAL, @Final = true, @TypeInferred = true, @Visibility = Visibility.V_LOCAL]
| | +- ModifierList[@EffectiveModifiers = "{final}", @ExplicitModifiers = "{}"]
| | +- VariableDeclarator[@Initializer = true, @Name = "_"]

2
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java22/Jep456_UnnamedPatternsAndVariables.txt
View File

@ -519,7 +519,7 @@
+- Block[@Empty = false, @Size = 4, @containsComment = false]
+- TryStatement[@TryWithResources = true]
| +- ResourceList[@Empty = false, @Size = 1, @TrailingSemiColon = false]
| | +- Resource[@ConciseResource = false, @StableName = "_"]
| | +- Resource[@ConciseResource = false]
| | +- LocalVariableDeclaration[@EffectiveVisibility = Visibility.V_LOCAL, @Final = true, @TypeInferred = true, @Visibility = Visibility.V_LOCAL]
| | +- ModifierList[@EffectiveModifiers = "{final}", @ExplicitModifiers = "{}"]
| | +- VariableDeclarator[@Initializer = true, @Name = "_"]

20
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/EmptyControlStatement.xml
View File

@ -96,7 +96,7 @@
<description>#432 empty try-with-resource - not ok</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>Empty try body - you could rename the resource to 'ignored'</message>
<message>Empty try-with-resources statement. Rename the resource to `ignored`, `unused` or `_` (Java 22+).</message>
</expected-messages>
<code><![CDATA[
class X {
@ -130,7 +130,7 @@
<expected-problems>1</expected-problems>
<expected-linenumbers>4</expected-linenumbers>
<expected-messages>
<message>Empty try body - you could rename the resource to 'ignored'</message>
<message>Empty try-with-resources statement. Should be written in.close()</message>
</expected-messages>
<code><![CDATA[
import java.io.InputStream;
@ -142,6 +142,22 @@
}
]]></code>
</test-code>
<test-code>
<description>Try with resources with several vars, ok</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.io.InputStream;
class X {
void method(InputStream in) {
try (final var something = executeSomething(in);
something.field) {
// this is ok because writing the corresponding sequence of var decls
// and close statements would be noisy
}
}
}
]]></code>
</test-code>
<test-code>
<description>pos, empty synchronized stmt</description>