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] [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 + +