Elasticsearch: Remove disable Xlint warning from gradle files.

Created on 22 Mar 2019 · 8Comments · Source: elastic/elasticsearch

There are a number of build.gradle files that specifically disable -Xlint warnings for both production and test code. This should be avoided, because that can mask real bugs.

The following build.gradle files should be checked:

[x] ./benchmarks/build.gradle
[x] ./client/client-benchmark-noop-api-plugin/build.gradle
[x] ./modules/ingest-common/build.gradle #40505
[x] ./modules/ingest-geoip/build.gradle #40629
[x] ./modules/percolator/build.gradle #40372
[x] ./modules/transport-netty4/build.gradle
[x] ./plugins/ingest-attachment/build.gradle #40721
[x] ./plugins/mapper-size/build.gradle #40721
[x] ./plugins/transport-nio/build.gradle #40721
[ ] ./server/build.gradle
[ ] ./test/framework/build.gradle
[x] ./x-pack/plugin/ccr/build.gradle #40833
[ ] ./x-pack/plugin/core/build.gradle
[x] ./x-pack/plugin/data-frame/build.gradle #40548
[x] ./x-pack/plugin/ml/build.gradle #44285
[x] ./x-pack/plugin/monitoring/build.gradle
[x] ./x-pack/plugin/rollup/build.gradle
[ ] ./x-pack/plugin/security/build.gradle
[ ] ./x-pack/plugin/watcher/build.gradle
[x] ./x-pack/qa/rolling-upgrade-basic/build.gradle
[x] ./x-pack/qa/rolling-upgrade/build.gradle
[x] ./x-pack/qa/third-party/active-directory/build.gradle
[ ] (I may be missing a few files)

If possible both compileJava.options.compilerArgs and compileTestJava.options.compilerArgs should be removed. Removing compileTestJava.options.compilerArgs can be difficult, for example because of many tests having raw types or unchecked checks. So at least we should try to remove compileJava.options.compilerArgs in each of the above mentioned files.

By default the build disables the following warning: -path, -serial, -options ,-deprecation. So if one of those warnings are also mentioned in a build.gradle file then that can be removed.

If you like to fix one of the above gradle files then post a comment which file you like to fix.

:DeliverBuild Meta Delivery good first issue help wanted

Source

martijnvg

Most helpful comment

I agree that try-with-resource makes code much more readable. I favour a global warning instead of using @SuppressWarnings("try") each time the warning needs to be suppressed. The reason is that we will need to suppress this warning many times and using this annotation makes suppressing this warning very verbose.

martijnvg on 3 Apr 2019

👍2

All 8 comments

There are many snippets in the code base that look like this:

try (AutoCloseable ignore = open/acquire/stash(...)) {
   // ignore variable ends up not being used
}

If -try gets removed then the compiler fails with: warning: [try] auto-closeable resource ignore is never referenced in body of corresponding try statement

I personally think that this coding style is not bad and that we should allow this. So should we add -try to the list of default exclusions?

Alternatively we can rewrite these try-with-resources statements to try-finally statements where the resource gets closed explicitly.

martijnvg on 3 Apr 2019

I think that using try-with-resource tops anything that involved using an explicit close in this situation as it's much readable.
We could allow for these warnings globally. One alternative is to use @SuppressWarnings("try") each time. I'm not advocating for it, just putting it out here.
Makes it a bit more obvious when reading the code that the var is not used, but probably generates too much boilerplate to be wroth it.

alpar-t on 3 Apr 2019

martijnvg on 3 Apr 2019

👍2

I would like to help by fixing some of these. I think I'm going to start with modules/transport-netty4. Should I open a PR and link it to this issue?

mariaral on 17 Feb 2020

@mariaral that would be great, thank you! You can assign me as a PR reviewer.

pugnascotia on 18 Feb 2020

@pugnascotia I just created another PR removing the Xlint exclusions from the following files:

x-pack/plugin/rollup/build.gradle
x-pack/plugin/monitoring/build.gradle
x-pack/qa/rolling-upgrade-basic/build.gradle

Can you take a look? It seems that I cannot assign you as a PR reviewer. Also, can you update the current issue by ticking the files from where the exclusions were removed in my previous PR? They are the following: