[java] Fix #5263 - UnnecessaryFullyQualifiedName FP with forward references (#5353)

Merge pull request #5353 from oowekyala:issue5263-ufqn-forward-ref

Gemfile.lock

@@ -75,7 +75,7 @@ GEM
     racc (1.8.1)
     rchardet (1.8.0)
     rexml (3.3.9)
-    rouge (4.5.0)
+    rouge (4.5.1)
     rufus-scheduler (3.9.2)
       fugit (~> 1.1, >= 1.11.1)
     safe_yaml (1.0.5)

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

pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/impl/AbstractNodeTest.java

@@ -48,7 +48,7 @@ class AbstractNodeTest {
         return childIndexes;
     }
 
-    static Object childrenAndGrandChildrenIndexes() {
+    static Object[] childrenAndGrandChildrenIndexes() {
         final Integer[] childrenIndexes = childrenIndexes();
         final Integer[] grandChildrenIndexes = grandChildrenIndexes();
         final Object[] indexes = new Object[childrenIndexes.length * grandChildrenIndexes.length];

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
                    );
     }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java

@@ -13,11 +13,18 @@ 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.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;
+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 +101,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 +259,50 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRulechainRule
             throw AssertionUtil.shouldNotReachHere("unknown constant ScopeInfo: " + scopeInfo);
         }
     }
+
+    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
+     * 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 || !fieldDecl.isStatic()) {
+            return false;
+        }
+        ASTBodyDeclaration enclosing = fieldAccess.ancestors(ASTBodyDeclaration.class)
+                                                  .first();
+        if (isPartOfStaticInitialization(enclosing)
+            && 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;
+    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java

@@ -31,6 +31,7 @@ import net.sourceforge.pmd.lang.java.types.JTypeVar;
 import net.sourceforge.pmd.lang.java.types.LexicalScope;
 import net.sourceforge.pmd.lang.java.types.Substitution;
 import net.sourceforge.pmd.lang.java.types.TypeOps;
+import net.sourceforge.pmd.util.AssertionUtil;
 import net.sourceforge.pmd.util.CollectionUtil;
 
 abstract class GenericSigBase<T extends JTypeParameterOwnerSymbol & AsmStub> {
@@ -51,8 +52,18 @@ abstract class GenericSigBase<T extends JTypeParameterOwnerSymbol & AsmStub> {
         this.lock = new ParseLock(parseLockName) {
             @Override
             protected boolean doParse() {
-                GenericSigBase.this.doParse();
-                return true;
+                try {
+                    GenericSigBase.this.doParse();
+                    return true;
+                } catch (RuntimeException e) {
+                    throw AssertionUtil.contexted(e)
+                                       .addContextValue("signature", GenericSigBase.this)
+                                       // Here we don't use the toString of the ctx directly because
+                                       // it could be using the signature indirectly, which would fail
+                                       .addContextValue("owner class", ctx.getEnclosingClass())
+                                       .addContextValue("owner name", ctx.getSimpleName())
+                                       .addContextValue("owner package", ctx.getPackageName());
+                }
             }
 
             @Override

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java

@@ -50,7 +50,7 @@ abstract class ParseLock {
                         finishParse(!success);
                     } catch (Throwable t) {
                         status = ParseStatus.FAILED;
-                        LOG.error("Parsing failed in ParseLock#doParse()", t);
+                        LOG.error("Parsing failed in ParseLock#doParse() of {}", name, t);
                         finishParse(true);
                     }
 

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml

@@ -1125,4 +1125,73 @@ public class Foo {
 }
 ]]></code>
     </test-code>
+    <test-code>
+        <description>#5263 FP when forward reference is illegal</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            public enum EnumX {
+                VALUE_X(EnumX.X); // MUST qualify the name or the file won't compile
+
+                static final String X = "X";
+
+                private final String v;
+
+                EnumX(final String v) {
+                    this.v = v;
+                }
+
+                public String getV() {
+                    return v;
+                }
+            }
+]]></code>
+    </test-code>
+    <test-code>
+        <description>#5263 FP when forward reference is illegal (class)</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            public class ClassX {
+                static final String Y = ClassX.X; // MUST qualify the name or the file won't compile
+                static final String X = "X";
+
+                private final String v;
+
+                ClassX(final String v) {
+                    this.v = v;
+                }
+
+                public String getV() {
+                    return v;
+                }
+            }
+]]></code>
+    </test-code>
+    <test-code>
+        <description>#5263 not forward references</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>3,6,9,13,17</expected-linenumbers>
+        <code><![CDATA[
+            public class ClassX {
+                static final Object Y = new Object() {
+                    String q = ClassX.X; // not FWR
+                };
+
+                private final String v = ClassX.X; // not FWR bc not static
+
+                ClassX() {
+                    this.v = ClassX.X; // not FWR
+                }
+
+                void foo() {
+                    System.out.println(ClassX.X); // not FWR
+                }
+
+                static void bar() {
+                    System.out.println(ClassX.X); // not FWR
+                }
+
+                static final String X = "X";
+            }
+            ]]></code>
+    </test-code>
 </test-data>

pom.xml

@@ -863,7 +863,7 @@
             <dependency>
                 <groupId>org.apache.commons</groupId>
                 <artifactId>commons-lang3</artifactId>
-                <version>3.14.0</version>
+                <version>3.17.0</version>
             </dependency>
             <dependency>
                 <groupId>org.apache.commons</groupId>