Tune warnings and errors for static compilation checks

kurtisvg commented 4 years ago

We need to tun our warnings and errors thrown by different static complication checks to be more useful. Using this issue to track which are useful/not useful.

Spotbugs

Useful

LSC_LITERAL_STRING_COMPARISON
IMC_IMMATURE_CLASS_WRONG_FIELD_ORDER
DM_CONVERT_CASE
DM_DEFAULT_ENCODING
MDM_STRING_BYTES_ENCODING
LI_LAZY_INIT_STATIC
WEM_WEAK_EXCEPTION_MESSAGING
OPM_OVERLY_PERMISSIVE_METHOD
VA_FORMAT_STRING_USES_NEWLINE
PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS (maybe)
DLS_DEAD_LOCAL_STORE (maybe)

Not useful

How to surpress

One off suppressions with the following:

@SuppressFBWarnings(value = "RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE",
        justification = "Scanning generated code of try-with-resources")

ace-n commented 4 years ago

From the java-docs-samples/functions snippets:

Definitely useful:

LSC_LITERAL_STRING_COMPARISON
IMC_IMMATURE_CLASS_WRONG_FIELD_ORDER
DM_CONVERT_CASE
DM_DEFAULT_ENCODING
MDM_STRING_BYTES_ENCODING
LI_LAZY_INIT_STATIC
WEM_WEAK_EXCEPTION_MESSAGING
OPM_OVERLY_PERMISSIVE_METHOD
VA_FORMAT_STRING_USES_NEWLINE

Might be useful:

PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS
DLS_DEAD_LOCAL_STORE

lesv commented 4 years ago

From GoogleCloudPlatform/java-docs-samples#2559: value = "OBL_UNSATISFIED_OBLIGATION", justification = "https://github.com/spotbugs/spotbugs/issues/293")

lesv commented 4 years ago

I'd rather we continue to use the Exclude filter Happy to add whatever you want.

ace-n commented 4 years ago

+1 to the exclude filter, at least for broadly inapplicable bugs.

Les - we can definitely exclude the following:

IMC_IMMATURE_CLASS_NO_TOSTRING
CRLF_INJECTION_LOGS

We might want to exclude EI_EXPOSE_REP as well - though I'll leave that judgment to y'all.

lesv commented 4 years ago

From Spanner folks:

The following samples currently require this annotation:

@SuppressFBWarnings(
      value = "OBL_UNSATISFIED_OBLIGATION",
      justification = "https://github.com/spotbugs/spotbugs/issues/293")
AbortBatchExample.abortBatch
AutocommitUpdateDataExample.update
BatchDdlExample.batchDdl
BatchDdlUsingSqlStatementsExample.batchDdlUsingSqlStatements
BatchDmlExample.batchDml
BatchDmlUsingSqlStatementsExample.batchDmlUsingSqlStatements
ConnectionWithQueryOptionsExample.connectionWithQueryOptions
CreateConnectionExample.createConnection
CreateConnectionWithCredentialsExample.createConnectionWithCredentials
CreateConnectionWithCustomHostExample.createConnectionWithCustomHost
CreateConnectionWithDataSourceExample.createConnectionWithDataSource
CreateConnectionWithDefaultProjectIdExample.createConnectionWithDefaultProjectId
CreateConnectionWithPropertiesExample.createConnectionWithProperties
CreateConnectionWithUrlPropertiesExample.createConnectionWithUrlProperties
CreateTableExample.createTable
GetCommitTimestampExample.getCommitTimestamp
GetReadTimestampExample.getReadTimestamp
InsertDataExample.insertData
PartitionedDmlExample.partitionedDml
ReadOnlyTransactionExample.readOnlyTransaction
ReadWriteTransactionExample.readWriteTransaction
SetQueryOptionsExample.setQueryOptions
SingleUseReadOnlyExample.singleUseReadOnly
SingleUseReadOnlyTimestampBoundExample.singleUseReadOnlyTimestampBound

ace-n commented 4 years ago

Here's the whitelist I used for the functions samples (copied from this comment):

<?xml version="1.0" encoding="UTF-8"?>
 <FindBugsFilter
              xmlns="https://github.com/spotbugs/filter/3.0.0"
              xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
              xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">

    <Match> <!-- Global excludes -->
        <Or>
            <Bug pattern="IMC_IMMATURE_CLASS_NO_TOSTRING" />
            <Bug pattern="CRLF_INJECTION_LOGS" />
            <Bug pattern="CLI_CONSTANT_LIST_INDEX" />
            <Bug pattern="LI_LAZY_INIT_STATIC" />
            <Bug pattern="PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS" />
            <Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE" />
        </Or>
    </Match>

    <!-- Functions samples -->
    <Match>
        <Or>
            <Class name="com.example.functions.helloworld.GroovyHelloWorld" />
            <Class name="com.example.functions.helloworld.GroovyHelloBackground" />
        </Or>
        <Bug pattern="MS_SHOULD_BE_FINAL" />
    </Match>

    <Match>
        <Or>
            <Class name="com.example.functions.helloworld.HelloError" />
            <Class name="com.example.functions.concepts.RetryPubSub" />
            <Class name="com.example.functions.ocr.OcrProcessImage" />
            <Class name="com.example.functions.ocr.OcrTranslateApiMessage" />
        </Or>
        <Bug pattern="WEM_WEAK_EXCEPTION_MESSAGING" />
    </Match>

    <Match>
        <Class name="com.example.functions.helloworld.KotlinHelloBackground" />
        <Bug pattern="DLS_DEAD_LOCAL_STORE" />
    </Match>

    <Match>
        <Class name="com.example.functions.imagemagick.ImageMagick" />
        <Or>
            <Bug pattern="PATH_TRAVERSAL_IN" />
            <Bug pattern="DMI_HARDCODED_ABSOLUTE_FILENAME" />
            <Bug pattern="COMMAND_INJECTION" />
        </Or>
    </Match>

    <Match>
        <Class name="com.example.functions.concepts.LazyFields" />
        <Bug pattern="UPM_UNCALLED_PRIVATE_METHOD" />
    </Match>

    <Match>
        <Class name="com.example.functions.concepts.FileSystem" />
        <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE" />
    </Match>

    <!-- TODO(ace-n): switch Date objects in eventpojos to java.time ones -->
    <Match>
        <Or>
            <Bug pattern="EI_EXPOSE_REP" />
            <Bug pattern="EI_EXPOSE_REP2" />
        </Or>
    </Match>

</FindBugsFilter>

lesv commented 4 years ago

@ace-n Thanks for the list. I'm not going to include several of them as they are against either the Google Style guide, Effective Java, or general best practices.

lesv commented 4 years ago

@ace-n Are you aware of @SuppressFBWarnings(value="BUG_PATTERN", justification="why you don't care.") ?

lesv commented 4 years ago

@ace-n FYI what I'll be checking in.

<FindBugsFilter
  xmlns="https://github.com/spotbugs/filter/3.0.0"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">
  <Match>
    <!-- TODO: Remove once SpotBugs fully supports Java 11 (JENKINS-53720) -->
    <!-- JDK 11 generates a different binary code than expected by SpotBugs -->
    <!-- https://github.com/spotbugs/spotbugs/issues/756 -->
    <Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
  </Match>
  <!-- From https://github.com/GoogleCloudPlatform/java-repo-tools/issues/123 -->
  <!-- https://github.com/GoogleCloudPlatform/java-docs-samples/pull/2630#issuecomment-612320968 -->
  <!--      <Bug pattern="IMC_IMMATURE_CLASS_NO_TOSTRING" />-->
  <!--      <Bug pattern="CRLF_INJECTION_LOGS" />-->
  <!--      <Bug pattern="LI_LAZY_INIT_STATIC" />-->
  <!--      <Bug pattern="DLS_DEAD_LOCAL_STORE" />-->

  <Match>
    <!-- https://github.com/spotbugs/spotbugs/issues/735 -->
    <Bug pattern="UPM_UNCALLED_PRIVATE_METHOD"/>
  </Match>
  <Match>
    <!-- https://github.com/spotbugs/spotbugs/issues/651 -->
    <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
  </Match>

  <Match>
    <!-- Samples uses this idiom a lot, so I'm turning it off -->
    <Bug pattern="CLI_CONSTANT_LIST_INDEX"/>
  </Match>
  <Match>
    <!-- https://github.com/spotbugs/spotbugs/issues/925 -->
    <Bug pattern="PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS"/>
  </Match>
  <Match>
    <!-- https://github.com/spotbugs/spotbugs/issues/293 -->
    <Bug pattern="OBL_UNSATISFIED_OBLIGATION"/>
  </Match>

GoogleCloudPlatform / java-repo-tools