[GR-17457] Add jt spotbugs and mx ruby_spotbugs to run more SpotBugs checks

PullRequest: truffleruby/1982

Author: eregon

mx.truffleruby/mx_truffleruby.py

@@ -13,6 +13,7 @@
 import mx
 import mx_sdk
 import mx_subst
+import mx_spotbugs
 
 if 'RUBY_BENCHMARKS' in os.environ:
     import mx_truffleruby_benchmark  # pylint: disable=unused-import
@@ -160,6 +161,14 @@ def ruby_testdownstream_sulong(args):
     jt('test', 'mri', '--all-sulong')
     jt('test', 'cexts')
 
+def ruby_spotbugs(args):
+    """Run SpotBugs with custom options to detect more issues"""
+    filters = join(root, 'mx.truffleruby', 'spotbugs-filters.xml')
+    spotbugsArgs = ['-textui', '-low', '-longBugCodes', '-include', filters]
+    if mx.is_interactive():
+        spotbugsArgs.append('-progress')
+    mx_spotbugs.spotbugs(args, spotbugsArgs)
+
 def verify_ci(args):
     """Verify CI configuration"""
     mx.verify_ci(args, mx.suite('truffle'), _suite, 'common.json')
@@ -247,5 +256,6 @@ def verify_ci(args):
     'ruby_testdownstream_aot': [ruby_testdownstream_aot, 'aot_bin'],
     'ruby_testdownstream_hello': [ruby_testdownstream_hello, ''],
     'ruby_testdownstream_sulong': [ruby_testdownstream_sulong, ''],
+    'ruby_spotbugs': [ruby_spotbugs, ''],
     'verify-ci' : [verify_ci, '[options]'],
 })

mx.truffleruby/spotbugs-filters.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<FindBugsFilter>
+  <Match>
+    <Package name="~org\.truffleruby.*" />
+    <Not>
+      <Or>
+        <!-- These are not worth fixing -->
+        <Bug pattern="MS_PKGPROTECT" />
+        <Bug pattern="MS_MUTABLE_ARRAY" />
+        <Bug pattern="MS_MUTABLE_COLLECTION" />
+        <Bug pattern="SF_SWITCH_NO_DEFAULT" />
+        <Bug pattern="RI_REDUNDANT_INTERFACES" />
+        <Bug pattern="EQ_COMPARETO_USE_OBJECT_EQUALS" />
+        <Bug pattern="SE_BAD_FIELD" />
+        <!-- Intentional -->
+        <Bug pattern="BC_UNCONFIRMED_CAST" />
+        <Bug pattern="BC_UNCONFIRMED_CAST_OF_RETURN_VALUE" />
+        <Bug pattern="DM_EXIT" />
+        <Bug pattern="DM_GC" />
+        <Bug pattern="EI_EXPOSE_REP" />
+        <Bug pattern="EI_EXPOSE_REP2" />
+        <Bug pattern="FE_FLOATING_POINT_EQUALITY" />
+        <Bug pattern="NM_CLASS_NOT_EXCEPTION" />
+        <Bug pattern="RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN" />
+        <Bug pattern="BX_UNBOXING_IMMEDIATELY_REBOXED" /> <!-- used as a way to assert the type -->
+        <Bug pattern="PZLA_PREFER_ZERO_LENGTH_ARRAYS" />
+      </Or>
+    </Not>
+  </Match>
+</FindBugsFilter>

spec/truffle/interop/special_forms_spec.rb

@@ -189,8 +189,10 @@
     l.log.should include(["instantiate"])
   end
 
+  # Always include in the documentation, even when run on native
+  desc = description['.class', :readMember, ['"class"'], 'when `foreign_object` is a `java.lang.Class`']
   guard -> { !TruffleRuby.native? } do
-    it description['.class', :readMember, ['"class"'], 'when `foreign_object` is a `java.lang.Class`'] do
+    it desc do
       Java.type('java.math.BigInteger').class.getName.should == 'java.math.BigInteger'
     end
   end

src/annotations/java/org/truffleruby/SuppressFBWarnings.java

@@ -12,10 +12,10 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 
-/** Used to suppress <a href="http://findbugs.sourceforge.net">FindBugs</a> warnings. */
+/** Used to suppress <a href="http://findbugs.sourceforge.net">SpotBugs</a> warnings. */
 @Retention(RetentionPolicy.CLASS)
 public @interface SuppressFBWarnings {
-    /** The set of FindBugs <a href="http://findbugs.sourceforge.net/bugDescriptions.html">warnings</a> that are to be
-     * suppressed in annotated element. The value can be a bug category, kind or pattern. */
+    /** The set of SpotBugs <a href="https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html">warnings</a> that
+     * are suppressed for the annotated element. The value can be a bug category, kind or pattern. */
     String[] value();
 }

src/annotations/java/org/truffleruby/language/Visibility.java

@@ -31,26 +31,4 @@ public enum Visibility {
     PROTECTED,
     PRIVATE,
     MODULE_FUNCTION;
-
-    private static final Visibility[] VALUES = values();
-
-    public boolean isPublic() {
-        return this == PUBLIC;
-    }
-
-    public boolean isProtected() {
-        return this == PROTECTED;
-    }
-
-    public boolean isPrivate() {
-        return this == PRIVATE;
-    }
-
-    public boolean isModuleFunction() {
-        return this == MODULE_FUNCTION;
-    }
-
-    public static Visibility[] getValues() {
-        return VALUES;
-    }
 }

src/main/java/org/truffleruby/RubyContext.java

@@ -144,11 +144,16 @@ public class RubyContext {
 
     private static boolean preInitializeContexts = TruffleRuby.PRE_INITIALIZE_CONTEXTS;
 
+    private static boolean isPreInitializingContext() {
+        boolean isPreInitializingContext = preInitializeContexts;
+        preInitializeContexts = false; // Only the first context is pre-initialized
+        return isPreInitializingContext;
+    }
+
     public RubyContext(RubyLanguage language, TruffleLanguage.Env env) {
         Metrics.printTime("before-context-constructor");
 
-        this.preInitializing = preInitializeContexts;
-        RubyContext.preInitializeContexts = false; // Only the first context is pre-initialized
+        this.preInitializing = isPreInitializingContext();
         this.preInitialized = preInitializing;
 
         preInitializationManager = preInitializing ? new PreInitializationManager(this) : null;

src/main/java/org/truffleruby/RubyFileTypeDetector.java

@@ -14,7 +14,6 @@
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.Locale;
-import java.util.function.BiConsumer;
 import java.util.regex.Pattern;
 
 import org.jcodings.Encoding;
@@ -80,14 +79,11 @@ public Charset findEncoding(TruffleFile file) throws IOException {
                 if (encodingCommentLine != null) {
                     Rope encodingCommentRope = StringOperations.encodeRope(encodingCommentLine, UTF8Encoding.INSTANCE);
                     Charset[] encodingHolder = new Charset[1];
-                    RubyLexer.parseMagicComment(encodingCommentRope, new BiConsumer<String, Rope>() {
-                        @Override
-                        public void accept(String name, Rope value) {
-                            if (RubyLexer.isMagicEncodingComment(name)) {
-                                Encoding encoding = EncodingManager.getEncoding(value);
-                                if (encoding != null) {
-                                    encodingHolder[0] = encoding.getCharset();
-                                }
+                    RubyLexer.parseMagicComment(encodingCommentRope, (name, value) -> {
+                        if (RubyLexer.isMagicEncodingComment(name)) {
+                            Encoding encoding = EncodingManager.getEncoding(value);
+                            if (encoding != null) {
+                                encodingHolder[0] = encoding.getCharset();
                             }
                         }
                     });

src/main/java/org/truffleruby/aot/TruffleRubySupport.java

@@ -26,6 +26,7 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import com.oracle.truffle.api.CompilerDirectives;
 import org.jcodings.Encoding;
 import org.jcodings.EncodingDB;
 import org.jcodings.util.ArrayReader;
@@ -73,12 +74,15 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
             try (InputStream is = ArrayReader.class.getResourceAsStream(entry)) {
                 if (is != null) {
                     byte[] buf = new byte[is.available()];
-                    new DataInputStream(is).readFully(buf);
+                    try (DataInputStream dataInputStream = new DataInputStream(is)) {
+                        dataInputStream.readFully(buf);
+                    }
                     jcodingsTables.put(name, buf);
                 } else {
-                    throw new Error("Unable to load Jcodings table " + name);
+                    throw CompilerDirectives.shouldNotReachHere("Unable to load Jcodings table " + name);
                 }
             } catch (IOException e) {
+                throw CompilerDirectives.shouldNotReachHere(e);
             }
         }
         return jcodingsTables;

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -754,11 +754,11 @@ protected boolean checkCallerVisibility(RubySymbol visibility) {
 
             switch (visibility.getString()) {
                 case "private":
-                    return callerVisibility.isPrivate();
+                    return callerVisibility == Visibility.PRIVATE;
                 case "protected":
-                    return callerVisibility.isProtected();
+                    return callerVisibility == Visibility.PROTECTED;
                 case "module_function":
-                    return callerVisibility.isModuleFunction();
+                    return callerVisibility == Visibility.MODULE_FUNCTION;
                 default:
                     throw CompilerDirectives.shouldNotReachHere();
             }

src/main/java/org/truffleruby/cext/ValueWrapperManager.java

@@ -17,8 +17,10 @@
 
 import org.truffleruby.RubyContext;
 import org.truffleruby.RubyLanguage;
+import org.truffleruby.SuppressFBWarnings;
 import org.truffleruby.cext.ValueWrapperManagerFactory.AllocateHandleNodeGen;
 import org.truffleruby.cext.ValueWrapperManagerFactory.GetHandleBlockHolderNodeGen;
+import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.language.NotProvided;
 import org.truffleruby.language.RubyBaseNode;
 
@@ -32,6 +34,7 @@
 import com.oracle.truffle.api.library.ExportLibrary;
 import com.oracle.truffle.api.library.ExportMessage;
 
+@SuppressFBWarnings("VO")
 public class ValueWrapperManager {
 
     static final long UNSET_HANDLE = -2L;
@@ -232,7 +235,7 @@ public boolean isFull() {
         }
 
         public long setHandleOnWrapper(ValueWrapper wrapper) {
-            long handle = getBase() + count * 8;
+            long handle = getBase() + count * Pointer.SIZE;
             wrapper.setHandle(handle, this);
             wrappers[count] = wrapper;
             count++;