Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove disable Xlint warning from gradle files. #40366

Closed
22 of 23 tasks
martijnvg opened this issue Mar 22, 2019 · 8 comments · Fixed by #76302
Closed
22 of 23 tasks

Remove disable Xlint warning from gradle files. #40366

martijnvg opened this issue Mar 22, 2019 · 8 comments · Fixed by #76302
Assignees
Labels
:Delivery/Build Build or test infrastructure Meta Team:Delivery Meta label for Delivery team

Comments

@martijnvg
Copy link
Member

martijnvg commented Mar 22, 2019

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:

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.

@martijnvg martijnvg added :Delivery/Build Build or test infrastructure Meta help wanted adoptme labels Mar 22, 2019
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Mar 22, 2019
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Mar 27, 2019
The generics in AbstractStringProcessor is not needed
(IngestDocument#setFieldValue(...) accepts Object) and
therefor it was removed. Otherwise the corresponding
factory implementations would need typing too.

Relates to elastic#40366
astefan pushed a commit to astefan/elasticsearch that referenced this issue Mar 29, 2019
Fix the generics in processors extending AbstractStringProcessor and its factory.

Relates to elastic#40366
martijnvg added a commit that referenced this issue Mar 29, 2019
Fix the generics in processors extending AbstractStringProcessor and its factory.

Relates to #40366
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Mar 29, 2019
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Apr 2, 2019
The xlint exclusions of the following plugins were removed:
* ingest-attachment.
* mapper-size.
* transport-nio. Removing the -try exclusion required some work, because
  the NettyAdaptor implements AutoCloseable and NettyAdaptor#close() method
  could throw an InterruptedException (ChannelFuture#await() and a generic
  Exception is re-thrown, which maybe an ChannelFuture). The easiest way
  around this to me seemed that NettyAdaptor should not implement AutoCloseable,
  because it is not directly used in a try-with-resources statement.

Relates to elastic#40366
@martijnvg
Copy link
Member Author

martijnvg commented Apr 3, 2019

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.

@alpar-t
Copy link
Contributor

alpar-t commented Apr 3, 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.

@martijnvg
Copy link
Member Author

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 added a commit that referenced this issue Apr 4, 2019
The xlint exclusions of the following plugins were removed:
* ingest-attachment.
* mapper-size.
* transport-nio. Removing the -try exclusion required some work, because
  the NettyAdaptor implements AutoCloseable and NettyAdaptor#close() method
  could throw an InterruptedException (ChannelFuture#await() and a generic
  Exception is re-thrown, which maybe an ChannelFuture). The easiest way
  around this to me seemed that NettyAdaptor should not implement AutoCloseable,
  because it is not directly used in a try-with-resources statement.

Relates to #40366
martijnvg added a commit that referenced this issue Apr 4, 2019
The xlint exclusions of the following plugins were removed:
* ingest-attachment.
* mapper-size.
* transport-nio. Removing the -try exclusion required some work, because
  the NettyAdaptor implements AutoCloseable and NettyAdaptor#close() method
  could throw an InterruptedException (ChannelFuture#await() and a generic
  Exception is re-thrown, which maybe an ChannelFuture). The easiest way
  around this to me seemed that NettyAdaptor should not implement AutoCloseable,
  because it is not directly used in a try-with-resources statement.

Relates to #40366
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Apr 4, 2019
Many gradle projects specifically use the -try exclude flag, because
there are many cases where auto-closeable resource ignore is never
referenced in body of corresponding try statement. Suppressing this
warning specifically in each case that it happens using
`@SuppressWarnings("try")` would be very verbose.

This change removes `-try` from any gradle project and adds it to the
build plugin. Also this change removes exclude flags from gradle projects
that is already specified in build plugin (for example -deprecation).

Relates to elastic#40366
martijnvg added a commit that referenced this issue Apr 5, 2019
Many gradle projects specifically use the -try exclude flag, because
there are many cases where auto-closeable resource ignore is never
referenced in body of corresponding try statement. Suppressing this
warning specifically in each case that it happens using
`@SuppressWarnings("try")` would be very verbose.

This change removes `-try` from any gradle project and adds it to the
build plugin. Also this change removes exclude flags from gradle projects
that is already specified in build plugin (for example -deprecation).

Relates to #40366
martijnvg added a commit that referenced this issue Apr 5, 2019
Many gradle projects specifically use the -try exclude flag, because
there are many cases where auto-closeable resource ignore is never
referenced in body of corresponding try statement. Suppressing this
warning specifically in each case that it happens using
`@SuppressWarnings("try")` would be very verbose.

This change removes `-try` from any gradle project and adds it to the
build plugin. Also this change removes exclude flags from gradle projects
that is already specified in build plugin (for example -deprecation).

Relates to #40366
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
The xlint exclusions of the following plugins were removed:
* ingest-attachment.
* mapper-size.
* transport-nio. Removing the -try exclusion required some work, because
  the NettyAdaptor implements AutoCloseable and NettyAdaptor#close() method
  could throw an InterruptedException (ChannelFuture#await() and a generic
  Exception is re-thrown, which maybe an ChannelFuture). The easiest way
  around this to me seemed that NettyAdaptor should not implement AutoCloseable,
  because it is not directly used in a try-with-resources statement.

Relates to elastic#40366
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
Many gradle projects specifically use the -try exclude flag, because
there are many cases where auto-closeable resource ignore is never
referenced in body of corresponding try statement. Suppressing this
warning specifically in each case that it happens using
`@SuppressWarnings("try")` would be very verbose.

This change removes `-try` from any gradle project and adds it to the
build plugin. Also this change removes exclude flags from gradle projects
that is already specified in build plugin (for example -deprecation).

Relates to elastic#40366
pugnascotia added a commit that referenced this issue Jul 20, 2021
pugnascotia added a commit that referenced this issue Jul 20, 2021
@pugnascotia pugnascotia self-assigned this Jul 21, 2021
@pugnascotia pugnascotia removed good first issue low hanging fruit help wanted adoptme labels Jul 21, 2021
pugnascotia added a commit that referenced this issue Jul 27, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Jul 27, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
ywangd pushed a commit to ywangd/elasticsearch that referenced this issue Jul 30, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this issue Jul 30, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this issue Jul 30, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this issue Jul 30, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this issue Jul 30, 2021
Part of elastic#40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Aug 2, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Aug 2, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Aug 10, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Aug 10, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
pugnascotia added a commit that referenced this issue Aug 11, 2021
Closes #40366.

Fix the last remaining javac issues when linting is enforced in `server/`.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this issue Aug 11, 2021
Closes elastic#40366.

Fix the last remaining javac issues when linting is enforced in `server/`.
elasticsearchmachine pushed a commit that referenced this issue Aug 16, 2021
* Fix compiler warnings in :server - part 4 (#76302)

Closes #40366.

Fix the last remaining javac issues when linting is enforced in `server/`.

* Further fixes after backport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Meta Team:Delivery Meta label for Delivery team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants