Enabled error-prone on Java builds

[Todd Lipcon <to...@apache.org>]

Hey folks,

Adar suggested I give a heads up to a wider audience in case people missed
the code reviews this past week. So... heads up:

Earlier this week we committed c55ddd9bc85094c5f780427c3fc82b2219015d24
which switches the Java compiler as used by Gradle from the normal javac to
'error-prone'. Error-prone is a sort of plugin for javac which enhances the
set of warnings and errors provided at compile time. Aside from generating
a wider variety of warnings and errors, it otherwise behaves just like
javac. You can read more about error-prone at http://errorprone.info

Enabling it has already detected several bugs, eg:
6a32001c25527586ba5ba0bf71abdf6b14743f32 - misplaced reference equality
check which could result in wrong predicate pruning
4969989fd0cc02c26993c6a897bd274833800a5c - small bug in the Parquet
importer job
7bd582ab78afebc1f338900b5e2ccf3e97fb3c66 - another decimal-related bug with
an accidental case fallthrough

It also generated a bunch of warnings about bad practice that weren't
functional issues but helped clean up bits and pieces of the code and avoid
future issues.

Note that I only enabled error-prone via gradle and not via maven. Also
note that your IDE won't be aware of errors and warnings generated by
error-prone unless you take some extra steps. It appears that there is a
plugin for IntelliJ: http://errorprone.info/docs/installation

Currently I've set up error-prone to only generate warnings and not fail
the build even for errors it considers to be serious. So, even if you don't
configure your IDE specially, you shouldn't see any new failures on gerrit,
etc, due to these warnings. But, please let's all try to keep it "clean".

Many of the warnings and errors from error-prone overlap with the warnings
and errors from SpotBugs. Grant has separately been working on enabling
SpotBugs and fixing the errors that it finds. I'll let him follow up if
there's anything devs need to know about that.

Thanks
-Todd

-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Enabled error-prone on Java builds

[Adar Lieber-Dembo <ad...@cloudera.com>]

> I think a nice half-way point would be for the "tidy" precommit build to
> run Gradle (and spotbugs and any other static analysis) and propagate the
> warnings as comments on the gerrit review. This is a bit more easy to
> handle than an outright build failure for the developer but should still
> suffice to avoid warnings creep.

+1. We can reevaluate later, if/when IDEs start using error-prone, or
if we find that too many significant errors are creeping in despite
the use of gerrit comments.

Re: Enabled error-prone on Java builds

[Todd Lipcon <to...@cloudera.com>]

On Fri, Apr 27, 2018 at 11:50 AM, Adar Lieber-Dembo <ad...@cloudera.com>
wrote:

> > Currently I've set up error-prone to only generate warnings and not fail
> > the build even for errors it considers to be serious. So, even if you
> don't
> > configure your IDE specially, you shouldn't see any new failures on
> gerrit,
> > etc, due to these warnings. But, please let's all try to keep it "clean".
>
> Do you think we should fail the build on errors generated by
> error-prone in the future? If so, what conditions would you like to
> see satisfied first? If not, why not?
>

I could go either way. The upside of failing is that it prevents warnings
from creeping up over time. The big downside is that a lot of people do
Java development using their IDE to build and run tests, and won't ever
invoke the gradle build from the CLI. Then they'll put up a patch and see a
build failure only after posting the review. That could be a little
frustrating for someone trying to contribute.

I think a nice half-way point would be for the "tidy" precommit build to
run Gradle (and spotbugs and any other static analysis) and propagate the
warnings as comments on the gerrit review. This is a bit more easy to
handle than an outright build failure for the developer but should still
suffice to avoid warnings creep.

Personally I'd be fine either way. The #1 reason I set the "errors as
warnings" flag initially was that we weren't 100% clean and I wanted to be
able to split up all the fixes into separate patches.

-Todd
-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Enabled error-prone on Java builds

[Adar Lieber-Dembo <ad...@cloudera.com>]

> Currently I've set up error-prone to only generate warnings and not fail
> the build even for errors it considers to be serious. So, even if you don't
> configure your IDE specially, you shouldn't see any new failures on gerrit,
> etc, due to these warnings. But, please let's all try to keep it "clean".

Do you think we should fail the build on errors generated by
error-prone in the future? If so, what conditions would you like to
see satisfied first? If not, why not?