Remove all warnings in lsp4j #932
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pisv
reviewed
Nov 23, 2025
Contributor
pisv
left a comment
pisv
left a comment
There was a problem hiding this comment.
Thanks a lot for taking care of cleaning all the warnings! 👍
I have just a few minor notes...
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/services/NotebookDocumentService.java
Outdated
Show resolved
Hide resolved
...onrpc/src/test/java/org/eclipse/lsp4j/jsonrpc/test/annotations/impl/GenericEndpointTest.java
Show resolved
Hide resolved
...e.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/EitherTypeAdapter.java
Outdated
Show resolved
Hide resolved
Contributor
|
Awesome! |
Because test code and production code have the same warning settings this commit works around some of the warnings in tests. The warnings were in one of these categories: - Javadoc warnings - Declared exception not thrown - Unused imports - Unused objects (these were the result of testing that constructors throw exceptions) - Possible null usage - Missing try-with-resources
This suppresses an info message about missing default case
jonahgraham
force-pushed
the
remove-all-warnings-in-lsp4j
branch
from
87492aa to
2c873b5
Compare
This is a place where we meet the problem of Java vs Typescript. In Typescript, the fields are directly accessible, and matches what the LSP spec says. But in Java we wrap the fields in a get/set pair, meaning the field is not visible and generates a javadoc warning.
There are lots of these warnings and they can be ignored with this setting, or `@param params` can be added to Javadoc block for each of them. As this warning was recently introduced, and is not a common default, I have elected to turn the warning off. To turn this warning on again, ensure that all existing cases are resolved.
The Gson library that LSP4J is heavily dependent on uses the errorprone annotations. If Eclipse supported warning suppression based on the errorprone annotations, the dozens of potential resource leak warnings in the LSP4J code would be automatically suppressed. This happens because Gson classes such as JsonWriter returns `this` in many of its methods, and since JsonWriter is a Closeable, the JDT warns about it. See for example JsonWrite.nullValue. Gson annotates this method as CanIgnoreReturnValue, meaning no potential resource leak. If JDT gets support for error prone annotations, this warning can be re-enabled.
jonahgraham
force-pushed
the
remove-all-warnings-in-lsp4j
branch
from
2c873b5 to
25b3003
Compare
pisv
approved these changes
Nov 24, 2025
Contributor
pisv
left a comment
pisv
left a comment
There was a problem hiding this comment.
Looks good to me, thank you very much for this effort! 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits + commit messages for details of why and how different warnings are handled.
Fixes #923