Merge pull request #9 from PRL-PRG/pmd-ruleset

Suppress pmd's `UnusedImport` and `UnnecessaryParens` and allow pushing without PMD's approval

.github/workflows/maven.yml

@@ -15,7 +15,7 @@ on:
     branches: ["main"]
 
 jobs:
-  verify-and-artifact:
+  artifact:
     runs-on: ubuntu-latest
 
     steps:
@@ -30,8 +30,8 @@ jobs:
         uses: r-lib/actions/setup-r@v2
         with:
           r-version: "4.3.2"
-      - name: Run tests and static analyses
-        run: mvn --batch-mode --update-snapshots verify
+      - name: Build artifact
+        run: mvn --batch-mode --update-snapshots package
       - run: mkdir staging && cp target/*.jar staging
       - name: Upload snapshot as artifact
         uses: actions/upload-artifact@v3
@@ -42,6 +42,42 @@ jobs:
       - name: Update dependency graph
         uses: advanced-security/maven-dependency-submission-action@571e99aab1055c2e71a1e2309b9691de18d6b7d6
 
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v3
+      - name: Set up JDK 21
+        uses: actions/setup-java@v3
+        with:
+          java-version: "21"
+          distribution: "temurin"
+          cache: maven
+      - name: Set up GNU-R
+        uses: r-lib/actions/setup-r@v2
+        with:
+          r-version: "4.3.2"
+      - name: Run tests (no static analyses)
+        run: mvn --batch-mode --update-snapshots test
+
+  verify:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v3
+      - name: Set up JDK 21
+        uses: actions/setup-java@v3
+        with:
+          java-version: "21"
+          distribution: "temurin"
+          cache: maven
+      - name: Set up GNU-R
+        uses: r-lib/actions/setup-r@v2
+        with:
+          r-version: "4.3.2"
+      - name: Run tests and static analyses
+        run: mvn --batch-mode --update-snapshots verify
+
   verify-macos:
     runs-on: macos-latest
 

.gitignore

@@ -20,7 +20,7 @@ target/
 .classpath
 .factorypath
 .project
-# We actually want to sync this to ensure NonNullByDefault
+# We actually want to sync this
 # .settings
 .springBeans
 .sts4-cache
@@ -36,7 +36,8 @@ build/
 !**/src/test/**/build/
 
 ### VS Code ###
-.vscode/
+# We actually want to sync this
+# .vscode/
 
 ### Mac OS ###
 .DS_Store

.pmd-rules.xml

@@ -0,0 +1,83 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<ruleset name="Default Maven PMD Plugin Ruleset"
+         xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
+
+    <description>
+        This is the default pmd ruleset with the following changes:
+
+        - Remove UnnecessaryImport, because it marks star imports in static members which aren't unnecessary, and
+          unused imports don't signal bugs.
+        - Remove UselessParenthesis, because we don't care and extra parentheses don't signal bugs.
+
+        See https://pmd.github.io/latest/pmd_userdocs_making_rulesets.html.
+
+        [0] https://raw.githubusercontent.com/apache/maven-pmd-plugin/pmd7/src/main/resources/rulesets/java/maven-pmd-plugin-default.xml
+    </description>
+
+    <rule ref="category/java/bestpractices.xml/AvoidUsingHardCodedIP" />
+    <rule ref="category/java/bestpractices.xml/CheckResultSet" />
+    <rule ref="category/java/bestpractices.xml/PrimitiveWrapperInstantiation" />
+    <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/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" />
+    <rule ref="category/java/codestyle.xml/UnnecessarySemicolon" />
+    <rule ref="category/java/codestyle.xml/UselessQualifiedThis" />
+
+    <rule ref="category/java/design.xml/CollapsibleIfStatements" />
+    <rule ref="category/java/design.xml/SimplifiedTernary" />
+    <rule ref="category/java/design.xml/UselessOverridingMethod" />
+
+    <rule ref="category/java/errorprone.xml/AvoidBranchingStatementAsLastInLoop" />
+    <rule ref="category/java/errorprone.xml/AvoidDecimalLiteralsInBigDecimalConstructor" />
+    <rule ref="category/java/errorprone.xml/AvoidMultipleUnaryOperators" />
+    <rule ref="category/java/errorprone.xml/AvoidUsingOctalValues" />
+    <rule ref="category/java/errorprone.xml/BrokenNullCheck" />
+    <rule ref="category/java/errorprone.xml/CheckSkipResult" />
+    <rule ref="category/java/errorprone.xml/ClassCastExceptionWithToArray" />
+    <rule ref="category/java/errorprone.xml/DontUseFloatTypeForLoopIndices" />
+    <rule ref="category/java/errorprone.xml/EmptyCatchBlock" />
+    <rule ref="category/java/errorprone.xml/JumbledIncrementer" />
+    <rule ref="category/java/errorprone.xml/MisplacedNullCheck" />
+    <rule ref="category/java/errorprone.xml/OverrideBothEqualsAndHashcode" />
+    <rule ref="category/java/errorprone.xml/ReturnFromFinallyBlock" />
+    <rule ref="category/java/errorprone.xml/UnconditionalIfStatement" />
+    <rule ref="category/java/errorprone.xml/UnnecessaryConversionTemporary" />
+    <rule ref="category/java/errorprone.xml/UnusedNullCheckInEquals" />
+    <rule ref="category/java/errorprone.xml/UselessOperationOnImmutable" />
+
+    <rule ref="category/java/multithreading.xml/AvoidThreadGroup" />
+    <rule ref="category/java/multithreading.xml/DontCallThreadRun" />
+    <rule ref="category/java/multithreading.xml/DoubleCheckedLocking" />
+
+    <rule ref="category/java/performance.xml/BigIntegerInstantiation" />
+
+</ruleset>

.settings/org.eclipse.core.resources.prefs

@@ -0,0 +1,5 @@
+eclipse.preferences.version=1
+encoding//src/main/java=UTF-8
+encoding//src/test/java=UTF-8
+encoding//src/test/resources=UTF-8
+encoding/<project>=UTF-8

.settings/org.eclipse.jdt.apt.core.prefs

@@ -0,0 +1,2 @@
+eclipse.preferences.version=1
+org.eclipse.jdt.apt.aptEnabled=false

.settings/org.eclipse.jdt.core.prefs

@@ -1,11 +1,19 @@
 eclipse.preferences.version=1
-org.eclipse.jdt.core.compiler.annotation.missingNonNullByDefaultAnnotation=disabled
-org.eclipse.jdt.core.compiler.annotation.nonnull=javax.annotation.Nonnull
+org.eclipse.jdt.core.compiler.annotation.missingNonNullByDefaultAnnotation=ignore
+org.eclipse.jdt.core.compiler.annotation.nonnull=org.eclipse.jdt.annotation.NonNull
 org.eclipse.jdt.core.compiler.annotation.nonnullbydefault=org.eclipse.jdt.annotation.NonNullByDefault
-org.eclipse.jdt.core.compiler.annotation.nullable=javax.annotation.Nullable
-org.eclipse.jdt.core.compiler.annotation.nullanalysis=enabled
-org.eclipse.jdt.core.compiler.problem.nullAnnotationInferenceConflict=warning
+org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jdt.annotation.Nullable
+org.eclipse.jdt.core.compiler.annotation.nullanalysis=disabled
+org.eclipse.jdt.core.compiler.codegen.targetPlatform=21
+org.eclipse.jdt.core.compiler.compliance=21
+org.eclipse.jdt.core.compiler.problem.enablePreviewFeatures=disabled
+org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
+org.eclipse.jdt.core.compiler.problem.nullAnnotationInferenceConflict=error
 org.eclipse.jdt.core.compiler.problem.nullReference=warning
-org.eclipse.jdt.core.compiler.problem.nullSpecViolation=warning
-org.eclipse.jdt.core.compiler.problem.potentialNullReference=warning
-org.eclipse.jdt.core.compiler.problem.syntacticNullAnalysisForFields=enabled
+org.eclipse.jdt.core.compiler.problem.nullSpecViolation=error
+org.eclipse.jdt.core.compiler.problem.potentialNullReference=ignore
+org.eclipse.jdt.core.compiler.problem.reportPreviewFeatures=ignore
+org.eclipse.jdt.core.compiler.problem.syntacticNullAnalysisForFields=disabled
+org.eclipse.jdt.core.compiler.processAnnotations=disabled
+org.eclipse.jdt.core.compiler.release=disabled
+org.eclipse.jdt.core.compiler.source=21

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+    "java.compile.nullAnalysis.mode": "disabled"
+}

Makefile

@@ -21,4 +21,3 @@ clean:
 # Install pre-commit and pre-push hooks
 setup:
 	cp -f .githooks/pre-commit.sh .git/hooks/pre-commit
-	cp -f .githooks/pre-push.sh .git/hooks/pre-push

pom.xml

@@ -77,7 +77,6 @@
         <configuration>
           <installHooks>
             <pre-commit>.githooks/pre-commit.sh</pre-commit>
-            <pre-push>.githooks/pre-push.sh</pre-push>
           </installHooks>
         </configuration>
         <executions>
@@ -130,9 +129,6 @@
             <excludes>
               <exclude>target/**/*</exclude>
             </excludes>
-            <cleanthat>
-              <version>2.18</version>
-            </cleanthat>
             <googleJavaFormat>
               <version>1.19.2</version>
               <style>GOOGLE</style>
@@ -218,12 +214,15 @@
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-pmd-plugin</artifactId>
         <version>3.21.2</version>
-        <!-- This prevents the warning "Unable to locate Source XRef to link to - DISABLED" -->
-        <!-- If we want source-links in the `site` pmd report we need to add jxr instead, -->
-        <!-- but IDEs display their own pmd report which already has source-links -->
         <configuration>
+          <!-- This prevents the warning "Unable to locate Source XRef to link to - DISABLED" -->
+          <!-- If we want source-links in the `site` pmd report we need to add jxr instead, -->
+          <!-- but IDEs display their own pmd report which already has source-links -->
           <linkXRef>false</linkXRef>
-          <failOnViolation>false</failOnViolation>
+          <!-- Use a custom ruleset -->
+          <rulesets>
+            <ruleset>.pmd-rules.xml</ruleset>
+          </rulesets>
         </configuration>
         <!-- Use pmd 7 which supports Java 21, not whatever is provided by maven-pmd-plugin -->
         <dependencies>

src/main/java/org/prlprg/bc/Compiler.java

@@ -12,6 +12,7 @@
 import org.prlprg.RSession;
 import org.prlprg.bc.BcInstr.*;
 import org.prlprg.sexp.*;
+import org.prlprg.util.NotImplementedError;
 
 // FIXME: use null instead of Optional (except for return types)
 // FIXME: update the SEXP API based on the experience with this code
@@ -22,7 +23,6 @@
 // TODO: 13 Assignments expressions
 // TODO: 16 Improved subset and sub-assignment handling
 // TODO: simple interpreter for the constantFoldCode
-@SuppressWarnings("PMD.UnnecessaryImport")
 public class Compiler {
   private static final Set<String> MAYBE_NSE_SYMBOLS = Set.of("bquote");
   private static final Set<String> ALLOWED_INLINES =
@@ -850,10 +850,10 @@ private boolean inlineLogicalAndOr(LangSXP call, boolean isAnd) {
 
   private boolean inlineRepeat(LangSXP call) {
     var body = call.arg(0).value();
-    return inlineSimpleLoop(call, body, this::compileRepeatBody);
+    return inlineSimpleLoop(body, this::compileRepeatBody);
   }
 
-  private boolean inlineSimpleLoop(LangSXP call, SEXP body, Consumer<SEXP> cmpBody) {
+  private boolean inlineSimpleLoop(SEXP body, Consumer<SEXP> cmpBody) {
     if (canSkipLoopContext(body, true)) {
       cmpBody.accept(body);
     } else {
@@ -907,7 +907,7 @@ private boolean inlineWhile(LangSXP call) {
     var test = call.arg(0).value();
     var body = call.arg(1).value();
 
-    return inlineSimpleLoop(call, body, (b) -> compileWhileBody(call, test, b));
+    return inlineSimpleLoop(body, (b) -> compileWhileBody(call, test, b));
   }
 
   private void compileWhileBody(LangSXP call, SEXP test, SEXP body) {
@@ -1208,13 +1208,13 @@ private Optional<SEXP> constantFoldSym(RegSymSXP sym) {
   }
 
   private Optional<SEXP> constantFoldCall(LangSXP call) {
-    //        if (!(call.fun() instanceof RegSymSXP funSym && isFoldableFun(funSym))) {
-    //            return Optional.empty();
-    //        }
+    if (!(call.fun() instanceof RegSymSXP funSym && isFoldableFun(funSym))) {
+      return Optional.empty();
+    }
 
     // fold args -- check consts
     // do.call <- need a basic interpreter
-    return Optional.empty();
+    throw new NotImplementedError();
   }
 
   private boolean isFoldableFun(RegSymSXP sym) {