-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix error prone warns #2320
Fix error prone warns #2320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The reason for the build failure seems to be that Javadoc is generated with Java 11 as target (to document the module descriptor), but the module-info.java
file does not declare the dependency on the error_prone_annotations module.
You have to add the following to the module-info.java
file:
requires com.google.errorprone.annotations;
(ideally somewhere between the exports
and the requires static
lines, to keep them ordered)
Most likely that issue would also have occurred if we compiled against Java >= 9, but we are not doing that at the moment.
This PR should fix the latest warnings given by ErrorProne. (If I haven't missed someone
Do you want to try adding <failOnWarning>true</failOnWarning>
again now?
Also, is this pull request done, or are you planning to extend it? Could you please refine the exclusion for the proto
module, as mentioned in #2316 (comment)? It looks like the non-generated code of the proto
module only has one or two warnings.
In general, maybe it would also be easier to have one pull request (this one here for example now) where you address the error-prone warnings, instead of multiple. You could for example also mark it as draft to indicate that it is not complete yet. Also no hurry, take your time.
metrics/src/main/java/com/google/gson/metrics/ParseBenchmark.java
Outdated
Show resolved
Hide resolved
You can also add the following here to the <link>https://errorprone.info/api/latest/</link> This will then from Gson's Javadoc add links for the |
Removed from: - ParseBenchmark
Thanks! Yesterday I was struggling to understand why the build was failing.
Yep, I have just tried. I had to push another commit, but now, yes, everything work fine also with the
Yes, I want to go further and fix every warns given by ErrorProne
I appreciate your advice. I'm new in the open-source world and I want learn as much as possible so thanks! 😃 |
Replaces `.*proto.*` with `.*/generated-test-sources/protobuf/.*` in such way will be excluded just the generated code and not the whole `proto` directory
Removes the `descriptor` variable because is unused.
Removes the `gson/src/test.*` path from the `-XepExcludedPaths` parameter of the ErrorProne plugin
This commit fix all `UnusedVariable` warns given by ErrorProne in the `gson/src/test.*` path. Some field is annotated with `@Keep` that means is used by reflection. In some other case the record is annotated with `@SuppressWarnings("unused")`
This commit fix all `JavaUtilDate` warns given by ErrorProne in the `gson/src/test.*` path. Classes/Methods are annotated with `@SuppressWarnings("JavaUtilDate")` because it's not possible use differente Date API.
This commit fix all `EqualsGetClass` warns given by ErrorProne in the `gson/src/test.*` path. I have rewrite the `equals()` methods to use `instanceof`
Thanks for addressing the comments!
You are welcome 🙂! I think it is often the case that one discovers features of GitHub (or maybe in general of any tool), when they see someone else using or mentioning it. Also to avoid any misunderstanding, personally I think it can be useful in some cases to split large pull requests to make reviewing and testing their changes easier. But here in this case I think the changes are small enough to keep them all in one pull request. |
return false; | ||
} else if (!stringValue.equals(other.stringValue)) | ||
} | ||
if (!(o instanceof BagOfPrimitives)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did Error Prone suggest changing the getClass()
check into this instanceof
? I believe it is correct, but I had to study the code a bit to make sure. There is actually a subclass of BagOfPrimitives
called SubTypeOfBagOfPrimitives
in JsonTreeTest
which adds an extra field. But it doesn't override equals
and the test doesn't rely on equals
, so I think it is OK that an instance of BagOfPrimitives
can now equal an instance of SubTypeOfBagOfPrimitives
where it couldn't before.
@@ -233,7 +226,7 @@ public static class ClassWithNoFields { | |||
// Nothing here.. | |||
@Override | |||
public boolean equals(Object other) { | |||
return other.getClass() == ClassWithNoFields.class; | |||
return other instanceof ClassWithNoFields; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this has subclasses (anonymous ones) in ObjectTest
, but I don't think the change in equals
semantics matters.
gson/src/test/java/com/google/gson/functional/ParameterizedTypesTest.java
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/ParameterizedTypesTest.java
Show resolved
Hide resolved
metrics/src/main/java/com/google/gson/metrics/BagOfPrimitives.java
Outdated
Show resolved
Hide resolved
proto/src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsTest.java
Outdated
Show resolved
Hide resolved
This commit fix all `JdkObsolete` warns given by ErrorProne in the `gson/src/test.*` path. In some cases I have replaced the obsolete JDK classes with the newest alternatives. In other cases, I have added the `@SuppressWarnings("JdkObsolete")`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really shows that integration of Error Prone was worth it! It discovered multiple flaws in the existing tests already.
gson/src/test/java/com/google/gson/functional/CustomDeserializerTest.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/ToNumberPolicyFunctionalTest.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/internal/UnsafeAllocatorInstantiationTest.java
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/functional/ToNumberPolicyFunctionalTest.java
Outdated
Show resolved
Hide resolved
gson/src/test/java/com/google/gson/internal/UnsafeAllocatorInstantiationTest.java
Outdated
Show resolved
Hide resolved
I tried to fix this but lambdas are not supported in JDK 7 so the build fails. |
Are you up for modifying Truth be told, I think we could probably target Java 8 now. As far as I know recent Android tooling works quite well with that. But that's a bigger step that would require deeper investigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to go? Maybe you could just Resolve the remaining open conversations.
I think that pretty much everything is fixed. |
This is great. Thanks again! |
Since all warnings are fixed or suppressed now, should we consider setting |
+1 |
Turns out the optional dependency on Error Prone Annotations is not ideal; due to JDK-6365854 the compiler will emit warnings when someone uses
While this probably won't cause any issues (as mentioned in the JDK bug report above), it will still be confusing since it is not obvious how to solve this problem. So should we either remove the dependency on Error Prone Annotations (or make it a test-scoped dependency only and remove its usage from The reason why these warnings are not shown during the Gson Maven build despite gson-extras and other modules depending on gson, without themselves declaring a dependency on Error Prone Annotations is that due to Error Prone usage compilation is run in fork mode. |
Huh. Making the dependency non-optional would not be the end of the world, though it would be the first non-optional dependency that Gson has. Another possibility, suggested by my team-mates, would be to add Gson-specific copies of the annotations. Currently we use only |
Digression:
I think that https://errorprone.info/docs/installation#maven no longer requires forking now that Error Prone has dropped support for Java 8. But maybe there are cases in which it's still needed. But mainly, thanks for pointing out the issue with plexus-compiler's hidden warnings. I had seen that before, but I hadn't connected it to the issue of missing annotation dependencies. |
Also, @cpovirk pointed out that we have similar issues with Guava, where we did decide just to include the annotation dependencies. |
The Error Prone setup here currently specifies the Edit: Pull request #2339 changes it to use |
* Adds `@SuppressWarnings("NarrowingCompoundAssignment")` * Adds `@SuppressWarnings("TypeParameterUnusedInFormals")` * Adds `@SuppressWarnings("JavaUtilDate")` * Adds a limit to `String.split()` * Add `error_prone_annotations` to the `pom.xml` * Adds `@InlineMe(...)` to deprecated methods * Adds `@SuppressWarnings("ImmutableEnumChecker")` * Adds `@SuppressWarnings("ModifiedButNotUsed")` * Adds `@SuppressWarnings("MixedMutabilityReturnType")` * Removes an unused import * Adds `requires` to `module-info.java` * Adds ErrorProne `link` into `pom.xml` * Remove unused imports Removed from: - ParseBenchmark * Adds `@SuppressWarnings("EqualsGetClass")` * Excludes from `proto` just the generated code. Replaces `.*proto.*` with `.*/generated-test-sources/protobuf/.*` in such way will be excluded just the generated code and not the whole `proto` directory * Removes an unused variable Removes the `descriptor` variable because is unused. * Fixes the `BadImport` warn into `ProtosWithAnnotationsTest` * Fixes the `BadImport` warns into `ProtosWithAnnotationsTest` * Enables ErrorProne in `gson/src/test.*` Removes the `gson/src/test.*` path from the `-XepExcludedPaths` parameter of the ErrorProne plugin * Fixes `UnusedVariable` warns This commit fix all `UnusedVariable` warns given by ErrorProne in the `gson/src/test.*` path. Some field is annotated with `@Keep` that means is used by reflection. In some other case the record is annotated with `@SuppressWarnings("unused")` * Fixes `JavaUtilDate` warns This commit fix all `JavaUtilDate` warns given by ErrorProne in the `gson/src/test.*` path. Classes/Methods are annotated with `@SuppressWarnings("JavaUtilDate")` because it's not possible use differente Date API. * Fixes `EqualsGetClass` warns This commit fix all `EqualsGetClass` warns given by ErrorProne in the `gson/src/test.*` path. I have rewrite the `equals()` methods to use `instanceof` * Replaces pattern matching for instanceof with casts * Fixes `JdkObsolete` warns This commit fix all `JdkObsolete` warns given by ErrorProne in the `gson/src/test.*` path. In some cases I have replaced the obsolete JDK classes with the newest alternatives. In other cases, I have added the `@SuppressWarnings("JdkObsolete")` * Fixes `ClassCanBeStatic` warns This commit fix all `ClassCanBeStatic` warns given by ErrorProne in the `gson/src/test.*` path. I have added the `static` keyword, or I have added `@SuppressWarnings("ClassCanBeStatic")` * Fixes `UndefinedEquals` warns This commit fix all `UndefinedEquals` warns given by ErrorProne in the `gson/src/test.*` path. I have added `@SuppressWarnings("UndefinedEquals")` or fixed the asserts Note: In this commit I have also renamed a test from `testRawCollectionDeserializationNotAlllowed` to `testRawCollectionDeserializationNotAllowed` * Fixes `GetClassOnEnum` warns This commit fix all `GetClassOnEnum` warns given by ErrorProne in the `gson/src/test.*` path. I have replaced the `.getClass()` with `.getDeclaringClass()` * Fixes `ImmutableEnumChecker` warns This commit fix all `ImmutableEnumChecker` warns given by ErrorProne in the `gson/src/test.*` path. I have added the `final` keyword, or I have added the `@SuppressWarnings("ImmutableEnumChecker")` annotation * Fixes `StaticAssignmentOfThrowable` warns This commit fix all `StaticAssignmentOfThrowable` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `AssertionFailureIgnored` warns This commit fix all `AssertionFailureIgnored` warns given by ErrorProne in the `gson/src/test.*` path. I have added the `@SuppressWarnings("AssertionFailureIgnored")` annotation * Fixes `ModifiedButNotUsed` warns This commit fix all `ModifiedButNotUsed` warns given by ErrorProne in the `gson/src/test.*` path. I have added the `@SuppressWarnings("ModifiedButNotUsed")` annotation * Fixes `MissingSummary` warns This commit fix all `MissingSummary` warns given by ErrorProne in the `gson/src/test.*` path. I have remove the Javadoc `@author` * Fixes `FloatingPointLiteralPrecision` warns This commit fix all `FloatingPointLiteralPrecision` warns given by ErrorProne in the `gson/src/test.*` path. I have added the `@SuppressWarnings("FloatingPointLiteralPrecision")` annotation * Fixes `StringSplitter` warns This commit fix all `StringSplitter` warns given by ErrorProne in the `gson/src/test.*` path. I have replaced the `String.split(...)` with `Splitter` * Fixes `EmptyCatch` warns This commit fix all `EmptyCatch` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `UnicodeEscape` warns This commit fix all `UnicodeEscape` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `EmptyBlockTag` warns This commit fix all `EmptyBlockTag` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `LongFloatConversion` warns This commit fix all `LongFloatConversion` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `LongDoubleConversion` warns This commit fix all `LongDoubleConversion` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `TruthAssertExpected` warns This commit fix all `TruthAssertExpected` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `UnusedMethod` warns This commit fix all `UnusedMethod` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `UnusedTypeParameter` warns This commit fix all `UnusedTypeParameter` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `CatchFail` warns This commit fix all `CatchFail` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `MathAbsoluteNegative` warns This commit fix all `MathAbsoluteNegative` warns given by ErrorProne in the `gson/src/test.*` path. * Fixes `LoopOverCharArray` warns This commit fix all `LoopOverCharArray` warns given by ErrorProne in the `gson/src/test.*` path. `toCharArray` allocates a new array, using `charAt` is more efficient * Fixes `HidingField` warns This commit fix all `HidingField` warns given by ErrorProne in the `gson/src/test.*` path. * Implements code review feedback * Implements code review feedback This commit implements some other code review feedback * Enable ErrorProne in `extra` Thi commit removes the `.*extras/src/test.*` path from the `-XepExcludedPaths` parameter of ErrorProne. * Fix the `JavaUtilDate` warns This commit fix all `JavaUtilDate` warns given by ErrorProne in the `extras/src/test.*` path. * Implements code review feedback * Removes redundant new-line * Implements code review feedback * Adds `JDK11` to run test with `--release 11` * Revert "Adds `JDK11` to run test with `--release 11`" This reverts commit a7cca38.
This PR fix all warns given by ErrorProne.