apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.6k stars 1.01k forks source link

Integerate Error Prone ( Static Analysis Tool ) during compilation [LUCENE-9497] #10536

Closed asfimport closed 3 years ago

asfimport commented 4 years ago

Integrate https://github.com/google/error-prone during compilation of our source code to catch mistakes


Migrated from LUCENE-9497 by Varun Thacker (@vthacker), resolved Feb 21 2021 Linked issues:

Pull requests: https://github.com/apache/lucene-solr/pull/1816, https://github.com/apache/lucene-solr/pull/1828

asfimport commented 4 years ago

Varun Thacker (@vthacker) (migrated from JIRA)

Here is the first warning withing lucene core that this tool found

 

/Users/vthacker/apache-work/lucene-solr/lucene/core/src/java/org/apache/lucene/search/DocIdSetIterator.java:130: warning: [IntLongMath] Expression of type int may overflow before being assigned to a long
        return maxDoc - minDoc;
                      ^
    (see https://errorprone.info/bugpattern/IntLongMath)
  Did you mean 'return (long) maxDoc - minDoc;'?
error: warnings found and -Werror specified
1 error
1 warning

EDIT: If I suppress that warning ( since it looks like safe usage  ) here is the next warning

 

 

> Task :lucene:core:compileJava FAILED
/Users/vthacker/apache-work/lucene-solr/lucene/core/src/java/org/apache/lucene/search/Sort.java:180: warning: [ReferenceEquality] Comparison using reference equality instead of value equality
      if (fields[i] != rewrittenSortFields[i]) {
                    ^
    (see https://errorprone.info/bugpattern/ReferenceEquality)
  Did you mean 'if (!Objects.equals(fields[i], rewrittenSortFields[i])) {' or 'if (!fields[i].equals(rewrittenSortFields[i])) {'?
/Users/vthacker/apache-work/lucene-solr/lucene/core/src/java/org/apache/lucene/search/Sort.java:185: warning: [UnnecessaryParentheses] These grouping parentheses are unnecessary; it is unlikely the code will be misinterpreted without them
    return (changed) ? new Sort(rewrittenSortFields) : this;
           ^
    (see https://errorprone.info/bugpattern/UnnecessaryParentheses)
  Did you mean 'return changed ? new Sort(rewrittenSortFields) : this;'?
error: warnings found and -Werror specified
1 error
2 warnings

 

 

 

asfimport commented 4 years ago

Varun Thacker (@vthacker) (migrated from JIRA)

Here is the first error ( options.errorprone.disableAllWarnings = true )

 

/Users/vthacker/apache-work/lucene-solr/lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java:712: error: [ArrayToString] Calling toString on an array does not provide useful information
          assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
                                                                                     ^
    (see https://errorprone.info/bugpattern/ArrayToString)
  Did you mean 'assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + Arrays.toString(term.termBytes) + " prefix=" + prefix;'?
/Users/vthacker/apache-work/lucene-solr/lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java:744: error: [ArrayToString] Calling toString on an array does not provide useful information
            assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
                                                                                       ^
    (see https://errorprone.info/bugpattern/ArrayToString)
  Did you mean 'assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + Arrays.toString(term.termBytes) + " prefix=" + prefix;'?
2 errors

 

 

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

The compilation problem was caused by the fact that we exclude error prone from published/ runtime dependencies (transitive dependency from guava) but at the same time if it's missing, error prone's annotation processor can complain about missing annotation types. Not to mention each and every of these dependencies uses a different version of error_prone_annotations... this is confusing like hell, sigh.

I think I fixed most of these issues here: https://github.com/apache/lucene-solr/pull/1828

I also looked at how individual pieces of code can be marked as valid - error prone uses regular SuppressWarnings annotation with custom names for each category. This is fine, I think.

asfimport commented 4 years ago

Varun Thacker (@vthacker) (migrated from JIRA)

> error prone uses regular SuppressWarnings annotation with custom names for each category. 

yep! We'll use this when we start enabling warnings to suppress legit uses

asfimport commented 4 years ago

ASF subversion and git services (migrated from JIRA)

Commit 121b262389691a10607e22ff025504e6ee3a9134 in lucene-solr's branch refs/heads/master from Varun Thacker https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=121b262

LUCENE-9497: Integerate Error Prone ( Static Analysis Tool ) during compilation (#1816)

LUCENE-9497: Integrate Error Prone, a static analysis tool during compilation

asfimport commented 4 years ago

ASF subversion and git services (migrated from JIRA)

Commit 121b262389691a10607e22ff025504e6ee3a9134 in lucene-solr's branch refs/heads/master from Varun Thacker https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=121b262

LUCENE-9497: Integerate Error Prone ( Static Analysis Tool ) during compilation (#1816)

LUCENE-9497: Integrate Error Prone, a static analysis tool during compilation

asfimport commented 4 years ago

ASF subversion and git services (migrated from JIRA)

Commit f7cbde2ad85b656b7c7ac16acaafb05481fa75ba in lucene-solr's branch refs/heads/master from Varun Thacker https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=f7cbde2

Update CHANGES.txt

LUCENE-9497 is only for master ( since it's a gradle plugin )

asfimport commented 2 years ago

Adrien Grand (@jpountz) (migrated from JIRA)

Closing after the 9.0.0 release