From 03b8d1acef9cae64db4fc1e0458f59756516af3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 5 Apr 2024 10:19:58 +0200 Subject: [PATCH 1/6] Fix #4912 - grammar for TWR allows this expression --- pmd-java/etc/grammar/Java.jjt | 4 ++-- .../pmd/lang/java/ast/ASTResource.java | 23 +++++++------------ .../java/ast/internal/PrettyPrintingUtil.java | 1 + .../pmd/lang/java/ast/ASTTryStatementTest.kt | 8 +++++++ 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index c29ac79910..66277aea7d 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/pmd-java/etc/grammar/Java.jjt @@ -2867,8 +2867,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()); } {} } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java index 008753c050..539f49d527 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java @@ -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,16 @@ 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. + * + *

Note that this might be null if the expression is too complex. + * + * @deprecated Since 7.1.0. This method is not very useful because the expression + * might be complex and not have a real "name". */ + @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(); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java index 8f686942c5..2db8d737eb 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java @@ -241,6 +241,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); diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt index ad2c6a3682..89b98ce250 100644 --- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt +++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt @@ -132,8 +132,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() } } From e636c206f063014ffaf0601d9da9e2412bbc70dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 5 Apr 2024 16:35:09 +0200 Subject: [PATCH 2/6] Doc --- .../java/net/sourceforge/pmd/lang/java/ast/ASTResource.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java index 539f49d527..577c28d17d 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTResource.java @@ -50,10 +50,9 @@ public final class ASTResource extends AbstractJavaNode { * If this has a local variable declaration, then returns the name * of the variable. * - *

Note that this might be null if the expression is too complex. - * * @deprecated Since 7.1.0. This method is not very useful because the expression - * might be complex and not have a real "name". + * might be complex and not have a real "name". In this case we return the + * expression pretty printed. */ @Deprecated public String getStableName() { From eccd99bfd624203ab801f612fda383d16ae745f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 5 Apr 2024 16:36:48 +0200 Subject: [PATCH 3/6] release notes --- docs/pages/release_notes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index cdc04217b8..e53ba6d38c 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -28,6 +28,8 @@ This is a {{ site.pmd.release_type }} release. * [#4913](https://github.com/pmd/pmd/issues/4913): \[cli] cpd-gui closes immediately * apex-errorprone * [#3953](https://github.com/pmd/pmd/issues/3953): \[apex] EmptyCatchBlock false positive with formal (doc) comments +* java + * [#4912](https://github.com/pmd/pmd/issues/4912): \[java] Unable to parse some Java9+ resource references * java-bestpractices * [#1084](https://github.com/pmd/pmd/issues/1084): \[java] Allow JUnitTestsShouldIncludeAssert to configure verification methods * [#4435](https://github.com/pmd/pmd/issues/4435): \[java] \[7.0-rc1] UnusedAssignment for used field From 6c0e73600174eec8cb16e08082c219ae60bab0f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 30 Apr 2024 14:45:11 +0200 Subject: [PATCH 4/6] Update reference files --- .../sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt | 6 +++--- .../java21p/Jep443_UnnamedPatternsAndVariables.txt | 2 +- .../java22/Jep456_UnnamedPatternsAndVariables.txt | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt index da23efe42b..1d588cc7f3 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/ParserCornerCases17.txt @@ -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"] diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java21p/Jep443_UnnamedPatternsAndVariables.txt b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java21p/Jep443_UnnamedPatternsAndVariables.txt index 6429a5a730..93734fdee0 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java21p/Jep443_UnnamedPatternsAndVariables.txt +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java21p/Jep443_UnnamedPatternsAndVariables.txt @@ -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 = "_"] diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java22/Jep456_UnnamedPatternsAndVariables.txt b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java22/Jep456_UnnamedPatternsAndVariables.txt index 3de46269b6..c9f4bbc107 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java22/Jep456_UnnamedPatternsAndVariables.txt +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java22/Jep456_UnnamedPatternsAndVariables.txt @@ -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 = "_"] From bca5c0ec09b78579f380fbfb9944d419310c2823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 30 Apr 2024 15:26:10 +0200 Subject: [PATCH 5/6] Remove usages of getStableName Fix #4930 Fix bug with symbol table in concise resources --- docs/pages/release_notes.md | 4 ++ .../java/ast/internal/PrettyPrintingUtil.java | 8 +++- .../codestyle/EmptyControlStatementRule.java | 47 ++++++++++++++++--- .../table/internal/SymbolTableResolver.java | 26 ++++++---- .../pmd/lang/java/ast/ASTTryStatementTest.kt | 7 +-- .../symbols/table/internal/VarScopingTest.kt | 12 ++++- .../codestyle/xml/EmptyControlStatement.xml | 20 +++++++- 7 files changed, 97 insertions(+), 27 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index cc881c5ef5..bd67ed6cf0 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -25,6 +25,10 @@ This is a {{ site.pmd.release_type }} release. ### 🚨 API Changes +#### Deprecated API + +* {% jdoc java::lang.java.ast.ASTResource#getStableName() %} and the corresponding attribute `@StableName` + ### ✨ External Contributions {% endtocmaker %} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java index 8b2ffb60f9..d78b500215 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java @@ -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) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/EmptyControlStatementRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/EmptyControlStatementRule.java index 00b626373b..6c10f58259 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/EmptyControlStatementRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/EmptyControlStatementRule.java @@ -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); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java index 94e2568ff4..f7679a49e3 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java @@ -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 node, @NonNull ReferenceCtx ctx) { + private int visitBlockLike(Iterable 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 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 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 union = + NodeStream union = NodeStream.union( stmtsOfResources(resources), // use the body instead of unwrapping it so @@ -750,8 +758,8 @@ public final class SymbolTableResolver { // - static NodeStream stmtsOfResources(ASTResourceList node) { - return node.toStream().map(ASTResource::asLocalVariableDeclaration); + static NodeStream stmtsOfResources(ASTResourceList node) { + return node.toStream().map(GenericNode::getFirstChild); } diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt index 89b98ce250..cd2a361d2c 100644 --- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt +++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTryStatementTest.kt @@ -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 { child { it::isConciseResource shouldBe false - it::getStableName shouldBe "a" it::getInitializer shouldBe fromChild { it::getModifiers shouldBe localVarModifiers { @@ -50,7 +49,6 @@ class ASTTryStatementTest : ParserTestSpec({ child { child { it::isConciseResource shouldBe false - it::getStableName shouldBe "a" it::getInitializer shouldBe fromChild { it::getModifiers shouldBe localVarModifiers { @@ -83,7 +81,6 @@ class ASTTryStatementTest : ParserTestSpec({ child { child { it::isConciseResource shouldBe true - it::getStableName shouldBe "a" it::getInitializer shouldBe variableAccess("a") } @@ -101,7 +98,6 @@ class ASTTryStatementTest : ParserTestSpec({ child { child { it::isConciseResource shouldBe true - it::getStableName shouldBe "a" it::getInitializer shouldBe variableAccess("a") } @@ -119,7 +115,6 @@ class ASTTryStatementTest : ParserTestSpec({ child { child { it::isConciseResource shouldBe true - it::getStableName shouldBe "a.b" it::getInitializer shouldBe fieldAccess("b") { ambiguousName("a") diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/VarScopingTest.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/VarScopingTest.kt index 33869571c3..6f4eef0e86 100644 --- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/VarScopingTest.kt +++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/VarScopingTest.kt @@ -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() diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/EmptyControlStatement.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/EmptyControlStatement.xml index 8d5dcdce10..42fc1039a5 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/EmptyControlStatement.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/EmptyControlStatement.xml @@ -96,7 +96,7 @@ #432 empty try-with-resource - not ok 1 - Empty try body - you could rename the resource to 'ignored' + Empty try-with-resources statement. Rename the resource to `ignored`, `unused` or `_` (Java 22+). 1 4 - Empty try body - you could rename the resource to 'ignored' + Empty try-with-resources statement. Should be written in.close() + + Try with resources with several vars, ok + 0 + + pos, empty synchronized stmt From b7a39e39e987f312cfe401d839ff5a83f4b4a5ba Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 2 May 2024 11:34:43 +0200 Subject: [PATCH 6/6] Apply suggestions from code review --- docs/pages/release_notes.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index bd67ed6cf0..c6c73219d7 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -22,12 +22,15 @@ This is a {{ site.pmd.release_type }} release. * [#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 +* java-codestyle + * [#4930](https://github.com/pmd/pmd/issues/4930): \[java] EmptyControlStatement should not allow empty try with concise resources ### 🚨 API Changes #### Deprecated API -* {% jdoc java::lang.java.ast.ASTResource#getStableName() %} and the corresponding attribute `@StableName` +* pmd-java + * {% jdoc !!java::lang.java.ast.ASTResource#getStableName() %} and the corresponding attribute `@StableName` ### ✨ External Contributions