resolve all pmd violations

- Should pass `mvn verify` and therefore CI. - had to slightly refactor `PirId.GlobalLogical` constructor due to a bug in the parser.
PRL-PRG · Jun 25, 2024 · 77e5c00 · 77e5c00
1 parent 4cc15e6
commit 77e5c00
Show file tree

Hide file tree

Showing 23 changed files with 91 additions and 133 deletions.
diff --git a/.pmd-rules.xml b/.pmd-rules.xml
@@ -40,12 +40,15 @@ under the License.
     <rule ref="category/java/bestpractices.xml/UnusedFormalParameter" />
     <rule ref="category/java/bestpractices.xml/UnusedLocalVariable" />
     <rule ref="category/java/bestpractices.xml/UnusedPrivateField" />
-    <rule ref="category/java/bestpractices.xml/UnusedPrivateMethod" />
+    <rule ref="category/java/bestpractices.xml/UnusedPrivateMethod">
+        <properties>
+            <property name="ignoredAnnotations" value="org.prlprg.parseprint.ParseMethod,org.prlprg.parseprint.PrintMethod" />
+        </properties>
+    </rule>
 
     <rule ref="category/java/codestyle.xml/EmptyControlStatement" />
     <rule ref="category/java/codestyle.xml/ExtendsObject" />
     <rule ref="category/java/codestyle.xml/ForLoopShouldBeWhileLoop" />
-    <rule ref="category/java/codestyle.xml/TooManyStaticImports" />
     <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName" />
     <rule ref="category/java/codestyle.xml/UnnecessaryModifier" />
     <rule ref="category/java/codestyle.xml/UnnecessaryReturn" />

diff --git a/src/main/java/org/prlprg/ir/analysis/DomTree.java b/src/main/java/org/prlprg/ir/analysis/DomTree.java
@@ -473,13 +473,11 @@ public Set<BB> dominees(Set<BB> input) {
             result.add(suc);
           }
           todo.push(suc);
-        } else if (!curState) {
-          if (result.remove(suc)) {
-            // Our parent is _not_ dominated, but we're in the result
-            // set. We were wrong, so remove ourself from the result;
-            // then add ourself to the worklist.
-            todo.push(suc);
-          }
+        } else if (!curState && result.remove(suc)) {
+          // Our parent is _not_ dominated, but we're in the result
+          // set. We were wrong, so remove ourself from the result;
+          // then add ourself to the worklist.
+          todo.push(suc);
         }
       }
     }

diff --git a/src/main/java/org/prlprg/ir/cfg/BB.java b/src/main/java/org/prlprg/ir/cfg/BB.java
@@ -136,8 +136,8 @@ public InstrOrPhi next() {
           i++;
         }
         if (i == 2) {
+          i++;
           if (jump != null) {
-            i++;
             next = jump;
             return next;
           }
@@ -174,6 +174,7 @@ public void remove() {
               cfg()
                   .record(
                       new CFGEdit.RemoveJump(BB.this), CFGEdit.InsertJump.of(BB.this, (Jump) next));
+          case 3 -> throw new NoSuchElementException();
           default -> throw new UnreachableError();
         }
         next = null;
@@ -211,8 +212,8 @@ public Instr next() {
               i++;
             }
             if (i == 1) {
+              i++;
               if (jump != null) {
-                i++;
                 next = jump;
                 return next;
               }
@@ -245,6 +246,7 @@ public void remove() {
                       .record(
                           new CFGEdit.RemoveJump(BB.this),
                           CFGEdit.InsertJump.of(BB.this, (Jump) next));
+              case 2 -> throw new NoSuchElementException();
               default -> throw new UnreachableError();
             }
             next = null;

diff --git a/src/main/java/org/prlprg/ir/cfg/CFG.java b/src/main/java/org/prlprg/ir/cfg/CFG.java
@@ -318,10 +318,8 @@ public void verify() {
           assert arg.cfg() == null || arg.cfg() == this;
           assert !nodes.containsKey(arg.id()) || nodes.get(arg.id()) == arg;
 
-          if (arg.cfg() != null) {
-            if (!nodes.containsKey(arg.id())) {
-              errors.add(new CFGVerifyException.UntrackedArg(bb.id(), instrOrPhi.id(), arg.id()));
-            }
+          if (arg.cfg() != null && !nodes.containsKey(arg.id())) {
+            errors.add(new CFGVerifyException.UntrackedArg(bb.id(), instrOrPhi.id(), arg.id()));
           }
         }
       }

diff --git a/src/main/java/org/prlprg/ir/cfg/CFGCleanup.java b/src/main/java/org/prlprg/ir/cfg/CFGCleanup.java
@@ -57,22 +57,21 @@ default void cleanup() {
             for (var bb : iter()) {
               if (bb.jump() != null
                   && bb.jump().data()
-                      instanceof JumpData.Branch(var _, var _, var ifTrue, var ifFalse)) {
-                if (ifTrue == ifFalse) {
-                  var phiInputs = ifTrue.phis().stream().map(phi -> phi.input(bb)).toList();
-                  Instr.mutateArgs(bb.jump(), new JumpData.Goto(ifTrue));
-                  Streams.forEachPair(
-                      ifTrue.phis().stream(),
-                      phiInputs.stream(),
-                      (phi, input) -> {
-                        // This is safe because the input is the same type as the phi,
-                        // but Java can't enforce because it's an existential.
-                        @SuppressWarnings("unchecked")
-                        var phi1 = (Phi<Node>) phi;
-                        phi1.setInput(bb, input);
-                      });
-                  // Don't need to set `mayImproveOnRepeat` for the first optimization.
-                }
+                      instanceof JumpData.Branch(var _, var _, var ifTrue, var ifFalse)
+                  && ifTrue == ifFalse) {
+                var phiInputs = ifTrue.phis().stream().map(phi -> phi.input(bb)).toList();
+                Instr.mutateArgs(bb.jump(), new JumpData.Goto(ifTrue));
+                Streams.forEachPair(
+                    ifTrue.phis().stream(),
+                    phiInputs.stream(),
+                    (phi, input) -> {
+                      // This is safe because the input is the same type as the phi,
+                      // but Java can't enforce because it's an existential.
+                      @SuppressWarnings("unchecked")
+                      var phi1 = (Phi<Node>) phi;
+                      phi1.setInput(bb, input);
+                    });
+                // Don't need to set `mayImproveOnRepeat` for the first optimization.
               }
             }
 

diff --git a/src/main/java/org/prlprg/ir/cfg/CFGEdit.java b/src/main/java/org/prlprg/ir/cfg/CFGEdit.java
@@ -255,14 +255,18 @@ public RemovePhis apply(CFG cfg) {
 
     // The type argument and cast *are* redundant, but the Java compiler has an annoying bug where
     // it occasionally fails to type-check and must be fully rebuilt without them.
-    @SuppressWarnings("Convert2Diamond")
+    @SuppressWarnings({"Convert2Diamond", "RedundantCast"})
     @Override
     public Iterable<? extends InsertPhi<?>> subEdits() {
       return phis.stream()
           .map(
               a ->
                   new InsertPhi<Node>(
-                      bbId, a.id(), a.nodeClass(), ImmutableSet.copyOf(a.inputIds())))
+                      bbId,
+                      (NodeId<? extends Phi<? extends Node>>) a.id(),
+                      (Class<? extends Node>) a.nodeClass(),
+                      ImmutableSet.copyOf(
+                          (Collection<? extends InputId<? extends Node>>) a.inputIds())))
           .toList();
     }
   }
@@ -676,7 +680,7 @@ record Divider(String label) implements Context<Divider> {
     @Override
     public Divider apply(CFG cfg) {
       cfg.recordDivider(label);
-      return new Divider(CFGEdit.undoLabel(label));
+      return new Divider(undoLabel(label));
     }
   }
 
@@ -689,7 +693,7 @@ public Section apply(CFG cfg) {
       var undoEdits =
           edits.stream().map(e -> e.apply(cfg)).collect(ImmutableList.toImmutableList()).reverse();
       cfg.endSection();
-      return new Section(CFGEdit.undoLabel(label), undoEdits);
+      return new Section(undoLabel(label), undoEdits);
     }
   }
 

diff --git a/src/main/java/org/prlprg/ir/cfg/CFGParsePrint.java b/src/main/java/org/prlprg/ir/cfg/CFGParsePrint.java
@@ -434,7 +434,7 @@ private void printBB(BB bb, Printer p) {
       }
 
       @ParseMethod
-      private Node parseNode(Parser p, Class<? extends Node> nodeClass) {
+      private Node parseNode(Parser p) {
         var id = (NodeId<?>) p.parse(NodeId.class);
 
         // The ID may exist, but in a `Void` instruction that got assigned an "arbitrary" unused ID.

diff --git a/src/main/java/org/prlprg/ir/cfg/CFGPirSerialize.java b/src/main/java/org/prlprg/ir/cfg/CFGPirSerialize.java
@@ -899,24 +899,26 @@ private RType parseRType(Parser p) {
         assert type.promise() != null && type.missing() != null;
         type = RTypes.primVec(type.primVec().notScalar(), type.promise(), type.missing());
       }
+
       if (s.trySkip('#') && type.primVec() != null) {
         assert type.promise() != null && type.missing() != null;
         type = RTypes.primVec(type.primVec().notNAOrNaN(), type.promise(), type.missing());
       }
+
       if (s.trySkip('^')) {
         type = type.notStrict();
       } else if (s.trySkip('~')) {
         type = type.promiseWrapped();
       }
-      if (s.trySkip('-')) {
-        // REACH: Remove attributes
-      } else if (s.trySkip('_')) {
-        // REACH: Add all attributes except notFastVecElt
-      } else if (s.trySkip('+')) {
-        // REACH: Add all attributes except object
-      } else {
-        // REACH: Add unknown attributes
-      }
+
+      // REACH: Remove attributes (`if`)
+      s.trySkip('-');
+      // REACH: Add all attributes except notFastVecElt (`else if`)
+      s.trySkip('_');
+      // REACH: Add all attributes except object (`else if`)
+      s.trySkip('+');
+      // REACH: Add unknown attributes (`else`)
+
       if (s.trySkip(" | miss")) {
         type = type.maybeMissing();
       }
@@ -1729,14 +1731,17 @@ Node getGlobal() {
   private static final class GlobalLogical extends PirId {
     private final Logical value;
 
+    /** How logicals are printed in PIR output. */
+    private static String pirLogicalPrint(Logical value) {
+      return switch (value) {
+        case TRUE -> "true";
+        case FALSE -> "false";
+        case NA -> "na-lgl";
+      };
+    }
+
     GlobalLogical(Logical value) {
-      super(
-          switch (value) {
-            case TRUE -> "true";
-            case FALSE -> "false";
-            case NA -> "na-lgl";
-          },
-          "_" + value);
+      super(pirLogicalPrint(value), "_" + value);
       this.value = value;
     }
 

diff --git a/src/main/java/org/prlprg/ir/cfg/FrameStateStmt.java b/src/main/java/org/prlprg/ir/cfg/FrameStateStmt.java
@@ -3,10 +3,10 @@
 import org.prlprg.ir.cfg.StmtData.FrameState_;
 
 /**
- * {@link Stmt} (IR instruction) which produces a {@linkplain org.prlprg.ir.cfg.FrameState
- * frame-state} (snapshot of the current stack top).
+ * {@link Stmt} (IR instruction) which produces a {@linkplain FrameState frame-state} (snapshot of
+ * the current stack top).
  */
-public interface FrameStateStmt extends Stmt, org.prlprg.ir.cfg.FrameState {
+public interface FrameStateStmt extends Stmt, FrameState {
   @Override
   NodeId<? extends FrameStateStmt> id();
 

diff --git a/src/main/java/org/prlprg/ir/cfg/InstrOrPhiImpl.java b/src/main/java/org/prlprg/ir/cfg/InstrOrPhiImpl.java
@@ -73,7 +73,7 @@ final void setId(NodeId<? extends InstrOrPhi> newId) {
 
   // region serialization and deserialization
   @ParseMethod
-  private Phi<?> parse(Parser p) {
+  private Phi<?> parse(Parser ignored) {
     throw new UnsupportedOperationException("can't parse an instruction or phi outside of a BB");
   }
 

diff --git a/src/main/java/org/prlprg/ir/cfg/InvalidNode.java b/src/main/java/org/prlprg/ir/cfg/InvalidNode.java
@@ -29,10 +29,10 @@ private static int disambiguatorFor(String desc) {
    * {@linkplain BB basic block} gets a new predecessor, and when the phi is added to a block
    * without preset input nodes.
    */
-  public static final InvalidNode UNSET_PHI_INPUT = InvalidNode.create("unsetPhiInput");
+  public static final InvalidNode UNSET_PHI_INPUT = create("unsetPhiInput");
 
   public static InvalidNode todoGlobal() {
-    return InvalidNode.create("todoGlobal");
+    return create("todoGlobal");
   }
 
   private final GlobalNodeId<InvalidNode> id;

diff --git a/src/main/java/org/prlprg/ir/cfg/JumpData.java b/src/main/java/org/prlprg/ir/cfg/JumpData.java
@@ -80,7 +80,7 @@ public Stream<Pair<RValue, org.prlprg.rshruntime.DeoptReason>> streamAssumptionD
     }
 
     @Override
-    public org.prlprg.ir.cfg.Checkpoint make(CFG cfg, NodeId<? extends Instr> id) {
+    public Checkpoint make(CFG cfg, NodeId<? extends Instr> id) {
       return new CheckpointImpl(cfg, id, this);
     }
   }

diff --git a/src/main/java/org/prlprg/ir/cfg/StaticEnv.java b/src/main/java/org/prlprg/ir/cfg/StaticEnv.java
@@ -16,7 +16,7 @@ public sealed interface StaticEnv extends RValue, GlobalNode {
 
   /** All global static environments, associated to their name. */
   ImmutableMap<String, StaticEnv> ALL =
-      Stream.of(StaticEnv.GLOBAL, StaticEnv.BASE, StaticEnv.NOT_CLOSED, StaticEnv.ELIDED)
+      Stream.of(GLOBAL, BASE, NOT_CLOSED, ELIDED)
           .collect(ImmutableMap.toImmutableMap(StaticEnv::name, e -> e));
 
   /** Descriptive, uniquely identifying name of this environment. */

diff --git a/src/main/java/org/prlprg/ir/closure/Closure.java b/src/main/java/org/prlprg/ir/closure/Closure.java
@@ -109,11 +109,6 @@ public Closure(String name, CloSXP origin, @IsEnv RValue env) {
     optimizedVersions = new TreeMap<>();
   }
 
-  /** Name of the closure for debugging, typically the name of the variable it's assigned to. */
-  public String name() {
-    return super.name();
-  }
-
   /**
    * The GNU-R closure this was created from.
    *

diff --git a/src/main/java/org/prlprg/ir/closure/Promise.java b/src/main/java/org/prlprg/ir/closure/Promise.java
@@ -67,16 +67,6 @@ public Promise(String name, Bc bc, CFG body, @IsEnv RValue env, Properties prope
     this.properties = properties;
   }
 
-  /**
-   * A descriptive name for the promise for debugging.
-   *
-   * <p>It may be the empty string.
-   */
-  @Override
-  public String name() {
-    return super.name();
-  }
-
   /** The promise code as GNU-R bytecode. */
   public Bc origin() {
     return bc;
@@ -180,7 +170,7 @@ public record Properties(ImmutableSet<Property> flags) implements Lattice<Proper
 
     /** Whether the properties don't guarantee anything, i.e. equal {@link Properties#EMPTY}. */
     public boolean isEmpty() {
-      return equals(Properties.EMPTY);
+      return equals(EMPTY);
     }
 
     /** Whether the promise performs reflection (no or maybe). */

diff --git a/src/main/java/org/prlprg/ir/type/PromiseRType.java b/src/main/java/org/prlprg/ir/type/PromiseRType.java
@@ -139,14 +139,14 @@ public boolean isSupersetOf(PromiseRType other) {
 
   @Override
   public PromiseRType union(PromiseRType other) {
-    return PromiseRType.of(isPromise().union(other.isPromise()), isLazy().union(other.isLazy()));
+    return of(isPromise().union(other.isPromise()), isLazy().union(other.isLazy()));
   }
 
   @Override
   public @Nullable PromiseRType intersection(PromiseRType other) {
     var isPromise = Troolean.intersection(isPromise(), other.isPromise());
     var isLazy = Troolean.intersection(isLazy(), other.isLazy());
-    return isPromise == null || isLazy == null ? null : PromiseRType.tryOf(isPromise, isLazy);
+    return isPromise == null || isLazy == null ? null : tryOf(isPromise, isLazy);
   }
 
   @Override

diff --git a/src/main/java/org/prlprg/ir/type/RTypes.java b/src/main/java/org/prlprg/ir/type/RTypes.java
@@ -278,7 +278,7 @@ public static RType toForSeq(RType ignored) {
   public static RType builtin(BuiltinId id) {
     // TODO: Specialize
     if (id.name().equals("environment")) {
-      return RTypes.simple(SEXPType.ENV);
+      return simple(SEXPType.ENV);
     }
     return ANY_SYM_FUN;
   }

diff --git a/src/main/java/org/prlprg/ir/type/lattice/Troolean.java b/src/main/java/org/prlprg/ir/type/lattice/Troolean.java
@@ -121,17 +121,17 @@ public boolean isSubsetOf(Troolean other) {
 
   /** Anything is a subset of MAYBE, otherwise (if YES or NO) they must be equal. */
   public boolean isSubsetOf(YesOrMaybe other) {
-    return isSubset(this, Troolean.of(other));
+    return isSubset(this, of(other));
   }
 
   /** Anything is a subset of MAYBE, otherwise (if YES or NO) they must be equal. */
   public boolean isSubsetOf(NoOrMaybe other) {
-    return isSubset(this, Troolean.of(other));
+    return isSubset(this, of(other));
   }
 
   /** Anything is a subset of MAYBE, otherwise (if YES/true or NO/false) they must be equal. */
   public boolean isSubsetOf(boolean other) {
-    return isSubset(this, Troolean.of(other));
+    return isSubset(this, of(other));
   }
 
   /** MAYBE is a superset of anything, otherwise (if YES or NO) they must be equal. */
@@ -142,17 +142,17 @@ public boolean isSupersetOf(Troolean other) {
 
   /** MAYBE is a superset of anything, otherwise (if YES or NO) they must be equal. */
   public boolean isSupersetOf(YesOrMaybe other) {
-    return isSuperset(this, Troolean.of(other));
+    return isSuperset(this, of(other));
   }
 
   /** MAYBE is a superset of anything, otherwise (if YES or NO) they must be equal. */
   public boolean isSupersetOf(NoOrMaybe other) {
-    return isSuperset(this, Troolean.of(other));
+    return isSuperset(this, of(other));
   }
 
   /** MAYBE is a superset of anything, otherwise (if YES/true or NO/false) they must be equal. */
   public boolean isSupersetOf(boolean other) {
-    return isSuperset(this, Troolean.of(other));
+    return isSuperset(this, of(other));
   }
 
   /** If args are YES and NO, or either is MAYBE, returns MAYBE. */