From e63edf358e737f8f07a7ef2f7b142f73ce18d8c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Thu, 21 Nov 2024 15:26:54 +0100 Subject: [PATCH 1/4] [java] Fix #5263 - UnnecessaryFullyQualifiedName FP with forward reference. --- .../lang/java/ast/internal/JavaAstUtils.java | 12 +++- .../UnnecessaryFullyQualifiedNameRule.java | 46 +++++++++++- .../xml/UnnecessaryFullyQualifiedName.xml | 72 +++++++++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java index 3c32710efe..9de709b90f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java @@ -32,6 +32,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType; import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression; import net.sourceforge.pmd.lang.java.ast.ASTBlock; +import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTBooleanLiteral; import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement; import net.sourceforge.pmd.lang.java.ast.ASTCastExpression; @@ -40,11 +41,13 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassType; import net.sourceforge.pmd.lang.java.ast.ASTCompactConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant; import net.sourceforge.pmd.lang.java.ast.ASTExecutableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTExplicitConstructorInvocation; import net.sourceforge.pmd.lang.java.ast.ASTExpression; import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement; import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess; +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTForStatement; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters; @@ -806,10 +809,17 @@ public final class JavaAstUtils { return false; } + /** + * Return true if the node is in static context relative to the deepest enclosing class. + */ public static boolean isInStaticCtx(JavaNode node) { - return node.ancestors() + return node.ancestorsOrSelf() + .map(NodeStream.asInstanceOf(ASTBodyDeclaration.class, ASTExplicitConstructorInvocation.class)) + .take(1) .any(it -> it instanceof ASTExecutableDeclaration && ((ASTExecutableDeclaration) it).isStatic() + || it instanceof ASTFieldDeclaration && ((ASTFieldDeclaration) it).isStatic() || it instanceof ASTInitializer && ((ASTInitializer) it).isStatic() + || it instanceof ASTEnumConstant || it instanceof ASTExplicitConstructorInvocation ); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java index 4c2b79b4bc..172e5cfc9a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java @@ -13,11 +13,15 @@ import java.util.function.Function; import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; +import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassType; import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess; import net.sourceforge.pmd.lang.java.ast.ASTMethodCall; +import net.sourceforge.pmd.lang.java.ast.ASTTypeBody; import net.sourceforge.pmd.lang.java.ast.ASTTypeExpression; +import net.sourceforge.pmd.lang.java.ast.ASTVariableId; import net.sourceforge.pmd.lang.java.ast.JavaNode; +import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; import net.sourceforge.pmd.lang.java.symbols.JAccessibleElementSymbol; import net.sourceforge.pmd.lang.java.symbols.JClassSymbol; @@ -94,7 +98,7 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRulechainRule } else if (getProperty(REPORT_FIELDS) && opa instanceof ASTFieldAccess) { ASTFieldAccess fieldAccess = (ASTFieldAccess) opa; ScopeInfo reasonForFieldInScope = fieldMeansSame(fieldAccess); - if (reasonForFieldInScope != null) { + if (reasonForFieldInScope != null && !isForwardReference(fieldAccess)) { String simpleName = formatMemberName(next, fieldAccess.getReferencedSym()); String reasonToString = unnecessaryReasonWrapper(reasonForFieldInScope); String unnecessary = produceQualifier(deepest, next, true); @@ -252,4 +256,44 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRulechainRule throw AssertionUtil.shouldNotReachHere("unknown constant ScopeInfo: " + scopeInfo); } } + + /** + * Return true if removing the qualification from this field access + * would produce an "Illegal forward reference" compiler error. This + * would happen if the referenced field is defined after the reference, + * in the same class. Note that the java compiler uses definite assignment + * to find forward references. Here we over-approximate this, to avoid + * depending on the dataflow pass. We could fix this later though. + * + * @param fieldAccess A field access + */ + private static boolean isForwardReference(ASTFieldAccess fieldAccess) { + JFieldSymbol referencedSym = fieldAccess.getReferencedSym(); + if (referencedSym == null || referencedSym.isUnresolved()) { + return false; + } + // The field must be declared in the same compilation unit + // to be a forward reference. + ASTVariableId fieldDecl = referencedSym.tryGetNode(); + if (fieldDecl == null) { + return false; + } + ASTBodyDeclaration enclosing = fieldAccess.ancestors(ASTBodyDeclaration.class) + .first(); + if (enclosing != null + && enclosing.getParent().getParent() == fieldDecl.getEnclosingType()) { + // the access is made in the same class + + if (JavaAstUtils.isInStaticCtx(fieldDecl) + && !JavaAstUtils.isInStaticCtx(fieldAccess)) { + // field is static but access is non-static: no problem + return false; + } + // else compare position: if access is before definition, we have a problem + int declIndex = fieldDecl.ancestors().filter(it -> it.getParent() instanceof ASTTypeBody).firstOrThrow().getIndexInParent(); + int accessIndex = enclosing.getIndexInParent(); + return accessIndex <= declIndex; + } + return false; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml index 056c77453e..083d644566 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml @@ -1125,4 +1125,76 @@ public class Foo { } ]]> + + #5263 FP when forward reference is illegal + 0 + + + + #5263 FP when forward reference is illegal (class) + 0 + + + + #5263 not forward references + 4 + + From 918684c154ab2ced66f451a086afc1dfeddc5e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Thu, 21 Nov 2024 16:51:05 +0100 Subject: [PATCH 2/4] Fix static methods being whitelisted --- .../UnnecessaryFullyQualifiedNameRule.java | 13 +++++++++++-- .../codestyle/xml/UnnecessaryFullyQualifiedName.xml | 6 +++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java index 172e5cfc9a..20e072efed 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java @@ -15,7 +15,10 @@ import org.checkerframework.checker.nullness.qual.Nullable; import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassType; +import net.sourceforge.pmd.lang.java.ast.ASTEnumConstant; import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess; +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTInitializer; import net.sourceforge.pmd.lang.java.ast.ASTMethodCall; import net.sourceforge.pmd.lang.java.ast.ASTTypeBody; import net.sourceforge.pmd.lang.java.ast.ASTTypeExpression; @@ -257,6 +260,12 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRulechainRule } } + private static boolean isPartOfStaticInitialization(ASTBodyDeclaration decl) { + return decl instanceof ASTFieldDeclaration && ((ASTFieldDeclaration) decl).isStatic() + || decl instanceof ASTInitializer && ((ASTInitializer) decl).isStatic() + || decl instanceof ASTEnumConstant; + } + /** * Return true if removing the qualification from this field access * would produce an "Illegal forward reference" compiler error. This @@ -275,12 +284,12 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRulechainRule // The field must be declared in the same compilation unit // to be a forward reference. ASTVariableId fieldDecl = referencedSym.tryGetNode(); - if (fieldDecl == null) { + if (fieldDecl == null || !fieldDecl.isStatic()) { return false; } ASTBodyDeclaration enclosing = fieldAccess.ancestors(ASTBodyDeclaration.class) .first(); - if (enclosing != null + if (isPartOfStaticInitialization(enclosing) && enclosing.getParent().getParent() == fieldDecl.getEnclosingType()) { // the access is made in the same class diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml index 083d644566..eae7f46f93 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml @@ -1173,7 +1173,7 @@ public class Foo { #5263 not forward references - 4 + 5 From 3e9e128aa7712fdd7c013d21909de74307e28794 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 22 Nov 2024 09:46:35 +0100 Subject: [PATCH 3/4] [java] UnnecessaryFullyQualifiedName - improve test case --- .../xml/UnnecessaryFullyQualifiedName.xml | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml index eae7f46f93..25a69a3bc1 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml @@ -1129,9 +1129,7 @@ public class Foo { #5263 FP when forward reference is illegal 0 #5263 FP when forward reference is illegal (class) 0 #5263 not forward references 5 + 3,6,9,13,17 Date: Fri, 22 Nov 2024 09:47:53 +0100 Subject: [PATCH 4/4] [doc] Update release notes (#5263, #5353) --- 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 14dc38def2..aecf590b88 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -37,6 +37,8 @@ This is a {{ site.pmd.release_type }} release. * [#5083](https://github.com/pmd/pmd/issues/5083): \[java] UnusedPrivateMethod false positive when method reference has no target type * [#5097](https://github.com/pmd/pmd/issues/5097): \[java] UnusedPrivateMethod FP with raw type missing from the classpath * [#5318](https://github.com/pmd/pmd/issues/5318): \[java] PreserveStackTraceRule: false-positive on Pattern Matching with instanceof +* java-codestyle + * [#5263](https://github.com/pmd/pmd/issues/5263): \[java] UnnecessaryFullyQualifiedName: false-positive in an enum that uses its own static variables * java-performance * [#5287](https://github.com/pmd/pmd/issues/5287): \[java] TooFewBranchesForSwitch false-positive with switch using list of case constants * [#5314](https://github.com/pmd/pmd/issues/5314): \[java] InsufficientStringBufferDeclarationRule: Lack of handling for char type parameters