Fix or ignore remaining SpotBugs issues (#1331)

* Fix remaining issues identified by SpotBugs:
  * Fixes to issues identified by SpotBugs
  * Ignore clone/Cloneable warning
  * MergingSamRecordIterator and SortingCollection are not actually Serializable.
  * Remove unnecessary check (it's known that i3 > -1 at this point)
  * Deprecate IOUtils#VCF_EXTENSIONS since it is a public mutable array.
  * Ignore gc warning
  * Ignore Thread.start() warning in constructor of AbstractAsyncWriter
  * Ignore redundant null check in htsjdk.variant.vcf.AbstractVCFCodec
  * Clarify that inherited (not outer) method hasNext() is being called by TupleIterator
  * Ignore warning in HTTPHelper about writing to a static field from an instance.
  * Ignore test for floating point equality in Histogram and LinearIndex
  * Preserve existing behaviour of TextualBAMIndexWriter

* Adding SpotBugs run to travis matrix to identify new issues introduced in the future.
* Adding SpotBugs exclusion file which ignores certain errors / classes of errors.  We may want to address some of the ignored ones in the future.

.travis.yml

@@ -22,9 +22,13 @@ matrix:
        env: TEST_TYPE=EXTERNAL_APIS
      - jdk: oraclejdk8
        env: TEST_TYPE=FTP
+     - jdk: openjdk8
+       env: SPOT_BUGS=true
 
 script:
-  - if [[ $TEST_TYPE == "FTP" ]]; then
+  - if [[ $SPOT_BUGS == "true" ]]; then
+      ./gradlew spotBugsMain spotBugsTest;
+    elif [[ $TEST_TYPE == "FTP" ]]; then
         ./gradlew testFTP jacocoTestReport;
     elif [[ $TEST_TYPE == "EXTERNAL_APIS" ]]; then
         ./gradlew testExternalApis jacocoTestReport;

build.gradle

@@ -13,6 +13,7 @@ plugins {
     id 'com.palantir.git-version' version '0.11.0'
     id 'com.github.johnrengelman.shadow' version '4.0.4'
     id 'com.github.maiflai.scalatest' version '0.23'
+    id 'com.github.spotbugs' version '1.6.9'
 }
 
 repositories {
@@ -159,6 +160,18 @@ task sourcesJar(type: Jar) {
     archiveClassifier.set('sources')
 }
 
+spotbugs {
+    reportLevel = 'high'
+    excludeFilter = file('gradle/spotbugs-exclude.xml')
+}
+
+tasks.withType(com.github.spotbugs.SpotBugsTask) {
+    reports {
+        xml.enabled = false
+        html.enabled = true
+    }
+}
+
 publishing {
     publications {
         htsjdk(MavenPublication) {

gradle/spotbugs-exclude.xml

@@ -0,0 +1,75 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<FindBugsFilter>
+    <Match>
+        <!-- This class of warnings could be evaluated at some point -->
+        <!-- Dm: Reliance on default encoding -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#dm-reliance-on-default-encoding-dm-default-encoding -->
+        <Bug pattern="DM_DEFAULT_ENCODING" />
+    </Match>
+    <Match>
+        <!-- SAMFileHeader#clone is public but we don't implement Cloneable (since Java's cloning mechanism is discouraged -->
+        <!-- CN: Class defines clone() but doesn’t implement Cloneable   -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#cn-class-defines-clone-but-doesn-t-implement-cloneable-cn-implements-clone-but-not-cloneable -->
+        <Class name="htsjdk.samtools.SAMFileHeader" />
+        <Bug pattern="CN_IMPLEMENTS_CLONE_BUT_NOT_CLONEABLE" />
+    </Match>
+    <Match>
+        <!-- MS: Field is a mutable array -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#ms-field-is-a-mutable-array-ms-mutable-array -->
+        <Class name="htsjdk.samtools.util.IOUtil" />
+        <Bug pattern="MS_MUTABLE_ARRAY" />
+    </Match>
+    <Match>
+        <!-- DM_GC: Explicit garbage collection; extremely dubious except in benchmarking code -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#dm-explicit-garbage-collection-extremely-dubious-except-in-benchmarking-code-dm-gc -->
+        <Or>
+            <Class name="htsjdk.samtools.util.FileAppendStreamLRUCache$Functor" />
+            <Class name="htsjdk.samtools.util.SortingCollection" />
+            <Class name="htsjdk.samtools.util.SortingCollection$MergingIterator" />
+        </Or>
+        <Bug pattern="DM_GC" />
+    </Match>
+    <Match>
+        <!-- SC: Constructor invokes Thread.start() -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#sc-constructor-invokes-thread-start-sc-start-in-ctor -->
+        <Class name="htsjdk.samtools.util.AbstractAsyncWriter" />
+        <Bug pattern="SC_START_IN_CTOR" />
+    </Match>
+    <Match>
+        <!-- ST: Write to static field from instance method  -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#st-write-to-static-field-from-instance-method-st-write-to-static-from-instance-method -->
+        <Class name="htsjdk.tribble.util.HTTPHelper" />
+        <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" />
+    </Match>
+    <Match>
+        <!-- Redundant null check is OK in this case, for clarity  -->
+        <!-- RCN: Redundant nullcheck of value known to be non-null  -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#rcn-redundant-nullcheck-of-value-known-to-be-non-null-rcn-redundant-nullcheck-of-nonnull-value -->
+        <Class name="htsjdk.variant.vcf.AbstractVCFCodec" />
+        <Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE" />
+    </Match>
+    <Match>
+        <!-- FE: Test for floating point equality -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#fe-test-for-floating-point-equality-fe-floating-point-equality -->
+        <Or>
+            <Class name="htsjdk.samtools.util.Histogram" />
+            <Class name="htsjdk.tribble.index.linear.LinearIndex$ChrIndex" />
+        </Or>
+        <Bug pattern="FE_FLOATING_POINT_EQUALITY" />
+    </Match>
+    <Match>
+        <!-- It's OK for test classes to create one-off Random objects -->
+        <!-- DMI: Random object created and used only once -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#dmi-random-object-created-and-used-only-once-dmi-random-used-only-once -->
+        <Class name="~.*\.*Test" />
+        <Bug pattern="DMI_RANDOM_USED_ONLY_ONCE" />
+    </Match>
+    <Match>
+        <!-- See note in BlockCompressedInputStreamTest.CountingInflater -->
+        <!-- ST: Write to static field from instance method  -->
+        <!-- https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#st-write-to-static-field-from-instance-method-st-write-to-static-from-instance-method -->
+        <Class name="htsjdk.samtools.util.BlockCompressedInputStreamTest" />
+        <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" />
+    </Match>
+
+</FindBugsFilter>

src/main/java/htsjdk/samtools/MergingSamRecordIterator.java

@@ -25,7 +25,6 @@
 
 import htsjdk.samtools.util.CloseableIterator;
 
-import java.io.Serializable;
 import java.util.Collection;
 import java.util.Map;
 import java.util.PriorityQueue;
@@ -209,9 +208,7 @@ public SAMFileHeader getMergedHeader() {
      * sequence dictionary.  I hate the fact that this extends SAMRecordCoordinateComparator, but it avoids
      * more copy & paste.
      */
-    private class MergedSequenceDictionaryCoordinateOrderComparator extends SAMRecordCoordinateComparator implements Serializable {
-        private static final long serialVersionUID = 1L;
-
+    private class MergedSequenceDictionaryCoordinateOrderComparator extends SAMRecordCoordinateComparator {
         @Override
         public int fileOrderCompare(final SAMRecord samRecord1, final SAMRecord samRecord2) {
             final int referenceIndex1 = getReferenceIndex(samRecord1);

src/main/java/htsjdk/samtools/TextualBAMIndexWriter.java

@@ -71,14 +71,12 @@ private void writeHeader() {
     @Override
     public void writeReference(final BAMIndexContent content) {
 
-        final int reference = content.getReferenceSequence();
-
         if (content == null) {
-            writeNullContent(reference);
-            count++;
-            return;
+            throw new NullPointerException("BAMIndexContent cannot be null");
         }
 
+        final int reference = content.getReferenceSequence();
+
         if (reference != count){
             throw new SAMException("Reference on content is " + reference + " but expecting reference " + count);
         }
@@ -162,6 +160,11 @@ private void writeChunkMetaData(final int reference, final BAMIndexMetaData meta
         }
 
     }
+
+    private void writeNullContent() {
+        pw.println("Reference <unknown> has n_bin=0");
+        pw.println("Reference <unknown> has n_intv=0");
+    }
 
     private void writeNullContent(final int reference) {
         pw.println("Reference " + reference + " has n_bin=0");

src/main/java/htsjdk/samtools/cram/compression/rans/E14.java

@@ -43,7 +43,7 @@ static int compress(final ByteBuffer in, final RansEncSymbol[][] syms,
         // Deal with the remainder
         l3 = 0xFF & in.get(in_size - 1);
         for (i3 = in_size - 2; i3 > 4 * isz4 - 2 && i3 >= 0; i3--) {
-            final int c3 = 0xFF & in.get(i3 > -1 ? i3 : 0);
+            final int c3 = 0xFF & in.get(i3);
             rans3 = Encoding.RansEncPutSymbol(rans3, ptr, syms[c3][l3]);
             l3 = c3;
         }

src/main/java/htsjdk/samtools/util/IOUtil.java

@@ -70,7 +70,14 @@ public class IOUtil {
     public static final String COMPRESSED_VCF_INDEX_EXTENSION = ".tbi";
 
     /** Possible extensions for VCF files and related formats. */
-    public static final String[] VCF_EXTENSIONS = {VCF_FILE_EXTENSION, COMPRESSED_VCF_FILE_EXTENSION, BCF_FILE_EXTENSION};
+    public static final List<String> VCF_EXTENSIONS_LIST = Collections.unmodifiableList(Arrays.asList(VCF_FILE_EXTENSION, COMPRESSED_VCF_FILE_EXTENSION, BCF_FILE_EXTENSION));
+
+    /**
+     * Possible extensions for VCF files and related formats.
+     * @deprecated Use {@link #VCF_EXTENSIONS_LIST} instead.
+     */
+    @Deprecated
+    public static final String[] VCF_EXTENSIONS = VCF_EXTENSIONS_LIST.toArray(new String[0]);
 
     public static final String INTERVAL_LIST_FILE_EXTENSION = IntervalList.INTERVAL_LIST_FILE_EXTENSION;
 

src/main/java/htsjdk/samtools/util/SortingCollection.java

@@ -29,7 +29,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.io.Serializable;
 import java.lang.reflect.Array;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -645,9 +644,7 @@ class PeekFileRecordIterator extends PeekIterator<T> {
         }
     }
 
-    class PeekFileRecordIteratorComparator implements Comparator<PeekFileRecordIterator>, Serializable {
-        private static final long serialVersionUID = 1L;
-
+    class PeekFileRecordIteratorComparator implements Comparator<PeekFileRecordIterator> {
         @Override
         public int compare(final PeekFileRecordIterator lhs, final PeekFileRecordIterator rhs) {
             final int result = comparator.compare(lhs.peek(), rhs.peek());

src/main/java/htsjdk/tribble/readers/AsciiLineReaderIterator.java

@@ -75,7 +75,7 @@ public String peek() {
     private class TupleIterator extends AbstractIterator<Tuple<String, Long>> implements LocationAware {
 
         public TupleIterator() {
-            hasNext(); // Initialize the iterator, which appears to be a requirement of the parent class.  TODO: Really?
+            super.hasNext(); // Initialize the iterator, which appears to be a requirement of the parent class.  TODO: Really?
         }
 
         @Override