Fix error prone warns #2320

MaicolAntali · 2023-02-18T13:58:09Z

This PR fix all warns given by ErrorProne.

Marcono1234

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

gson/pom.xml

Marcono1234 · 2023-02-19T00:57:19Z

You can also add the following here to the pom.xml

<link>https://errorprone.info/api/latest/</link>

This will then from Gson's Javadoc add links for the @InlineMe annotations of JsonParser.

Removed from: - ParseBenchmark

MaicolAntali · 2023-02-19T14:25:20Z

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.

Thanks! Yesterday I was struggling to understand why the build was failing.

Do you want to try adding <failOnWarning>true</failOnWarning> again now?

Yep, I have just tried. I had to push another commit, but now, yes, everything work fine also with the <failOnWarning>true</failOnWarning>.

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.

Yes, I want to go further and fix every warns given by ErrorProne

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.

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`

Marcono1234 · 2023-02-19T17:44:42Z

Thanks for addressing the comments!

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.

I appreciate your advice. I'm new in the open-source world and I want learn as much as possible so thanks! 😃

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.

gson/src/main/java/com/google/gson/stream/JsonReader.java

gson/src/test/java/com/google/gson/ParameterizedTypeFixtures.java

eamonnmcmanus · 2023-02-19T19:03:05Z

gson/src/test/java/com/google/gson/common/TestTypes.java

-          return false;
-      } else if (!stringValue.equals(other.stringValue))
+      }
+      if (!(o instanceof BagOfPrimitives)) {


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.

eamonnmcmanus · 2023-02-19T19:05:33Z

gson/src/test/java/com/google/gson/common/TestTypes.java

@@ -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;


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

metrics/src/main/java/com/google/gson/metrics/BagOfPrimitives.java

proto/src/test/java/com/google/gson/protobuf/functional/ProtosWithAnnotationsTest.java

gson/pom.xml

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")`

eamonnmcmanus

Not quite done reviewing but here's what I have so far.

gson/src/test/java/com/google/gson/CommentsTest.java

gson/src/test/java/com/google/gson/JsonArrayTest.java

gson/src/test/java/com/google/gson/JsonNullTest.java

gson/src/test/java/com/google/gson/ObjectTypeAdapterTest.java

gson/src/test/java/com/google/gson/ParameterizedTypeFixtures.java

gson/src/test/java/com/google/gson/functional/PrintFormattingTest.java

gson/src/test/java/com/google/gson/internal/LinkedTreeMapTest.java

gson/src/test/java/com/google/gson/internal/UnsafeAllocatorInstantiationTest.java

gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java

gson/src/test/java/com/google/gson/stream/JsonReaderTest.java

Marcono1234

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

gson/src/test/java/com/google/gson/functional/EnumTest.java

gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java

gson/src/test/java/com/google/gson/functional/StringTest.java

gson/src/test/java/com/google/gson/functional/ToNumberPolicyFunctionalTest.java

gson/src/test/java/com/google/gson/internal/UnsafeAllocatorInstantiationTest.java

gson/src/test/java/com/google/gson/stream/JsonReaderTest.java

gson/src/test/java/com/google/gson/functional/EnumTest.java

gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java

gson/src/test/java/com/google/gson/functional/StringTest.java

gson/src/test/java/com/google/gson/functional/ToNumberPolicyFunctionalTest.java

gson/src/test/java/com/google/gson/internal/UnsafeAllocatorInstantiationTest.java

MaicolAntali · 2023-02-24T08:25:36Z

I tried to fix this but lambdas are not supported in JDK 7 so the build fails.

eamonnmcmanus · 2023-02-24T18:11:28Z

I tried to fix this but lambdas are not supported in JDK 7 so the build fails.

Are you up for modifying pom.xml so that tests are run with --release 11 even though the main code is compiled with --release 7? The Continuous Integration runs with JDK 11 and 17 so that should be fine. It would be a separate PR, and don't feel obliged, especially if you are not very well acquainted with Maven.

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.

This reverts commit a7cca38.

eamonnmcmanus

I think this is ready to go? Maybe you could just Resolve the remaining open conversations.

MaicolAntali · 2023-03-01T19:06:16Z

I think that pretty much everything is fixed.

eamonnmcmanus · 2023-03-01T22:23:19Z

This is great. Thanks again!

Marcono1234 · 2023-03-04T10:55:03Z

Since all warnings are fixed or suppressed now, should we consider setting <failOnWarning>true</failOnWarning> again in a follow-up PR?
Otherwise in the future newly introduced warnings might be overlooked.

eamonnmcmanus · 2023-03-05T16:45:43Z

Since all warnings are fixed or suppressed now, should we consider setting <failOnWarning>true</failOnWarning> again in a follow-up PR?

+1

Marcono1234 · 2023-03-05T22:55:28Z

Turns out the optional dependency on Error Prone Annotations is not ideal; due to JDK-6365854 the compiler will emit warnings when someone uses JsonParser in their project, without having declared a dependency on Error Prone Annotations:

\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'replacement()' in type 'InlineMe': class file for com.google.errorprone.annotations.InlineMe not found
\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'imports()' in type 'InlineMe'
\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'replacement()' in type 'InlineMe'
\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'imports()' in type 'InlineMe'
\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'replacement()' in type 'InlineMe'
\gson\target\gson-2.10.2-SNAPSHOT.jar(/com/google/gson/JsonParser.class): warning: Cannot find annotation method 'imports()' in type 'InlineMe'

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 JsonParser), or make it a required dependency?

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.
That means plexus-compiler (used by the Maven Compiler Plugin) parses the process output to determine compiler diagnostics, but apparently it is unable to detect all warnings and errors and in that case produces no messages at all (codehaus-plexus/plexus-compiler#66), and there is currently no way to view the messages unless you run in non-fork mode (which is not possible with Error Prone).

eamonnmcmanus · 2023-03-06T17:28:48Z

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 @Keep and @InlineMe, and in both cases Error Prone recognizes those annotations regardless of what package they are in. So for example we could add com.google.gson.internal.annotations, or we could add a package-private copy of the annotation to every package that needs it. Maybe it's easiest just to make the dependency non-optional, though.

cpovirk · 2023-03-06T17:35:50Z

Digression:

unless you run in non-fork mode (which is not possible with Error Prone)

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.

eamonnmcmanus · 2023-03-06T17:54:46Z

Also, @cpovirk pointed out that we have similar issues with Guava, where we did decide just to include the annotation dependencies.

Marcono1234 · 2023-03-06T20:33:20Z

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.

The Error Prone setup here currently specifies the -J options to open the internal packages using the Maven Compiler Plugin, which requires fork mode for -J options. Though maybe we could indeed use the .mvn/jvm.config approach suggested by the Error Prone documentation, that apparently also improves compilation time (see google/error-prone#2786).

Edit: Pull request #2339 changes it to use .mvn/jvm.config

MaicolAntali added 10 commits February 18, 2023 14:06

Adds @SuppressWarnings("NarrowingCompoundAssignment")

18336d0

Adds @SuppressWarnings("TypeParameterUnusedInFormals")

0c2f8c3

Adds @SuppressWarnings("JavaUtilDate")

7f64bf3

Adds a limit to String.split()

f34c35d

Add error_prone_annotations to the pom.xml

5e6a279

Adds @InlineMe(...) to deprecated methods

682b369

Adds @SuppressWarnings("ImmutableEnumChecker")

41e0c28

Adds @SuppressWarnings("ModifiedButNotUsed")

b6965d1

Adds @SuppressWarnings("MixedMutabilityReturnType")

04feff6

Removes an unused import

5bd180d

Marcono1234 reviewed Feb 19, 2023

View reviewed changes

metrics/src/main/java/com/google/gson/metrics/ParseBenchmark.java Outdated Show resolved Hide resolved

gson/pom.xml Outdated Show resolved Hide resolved

MaicolAntali added 3 commits February 19, 2023 15:01

Adds requires to module-info.java

373525d

Adds ErrorProne link into pom.xml

bd60abf

Remove unused imports

5e1e970

Removed from: - ParseBenchmark

MaicolAntali marked this pull request as draft February 19, 2023 14:25

MaicolAntali added 10 commits February 19, 2023 15:29

Adds @SuppressWarnings("EqualsGetClass")

f3c4763

Excludes from proto just the generated code.

71a75b8

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

a1beaf2

Removes the `descriptor` variable because is unused.

Fixes the BadImport warn into ProtosWithAnnotationsTest

9c5073f

Fixes the BadImport warns into ProtosWithAnnotationsTest

2aad0bc

Enables ErrorProne in gson/src/test.*

ba27715

Removes the `gson/src/test.*` path from the `-XepExcludedPaths` parameter of the ErrorProne plugin

Fixes UnusedVariable warns

e3d65bc

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

d1c0554

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

7769c75

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

a8cff33

eamonnmcmanus reviewed Feb 21, 2023

View reviewed changes

Fixes JdkObsolete warns

eb05e0d

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")`

eamonnmcmanus requested changes Feb 23, 2023

View reviewed changes

MaicolAntali added 2 commits February 23, 2023 11:27

Implements code review feedback

a507512

Removes redundant new-line

bd2916e

Marcono1234 reviewed Feb 23, 2023

View reviewed changes

eamonnmcmanus requested changes Feb 23, 2023

View reviewed changes

Implements code review feedback

13ab165

MaicolAntali added 2 commits February 24, 2023 19:29

Adds JDK11 to run test with --release 11

a7cca38

Revert "Adds JDK11 to run test with --release 11"

5dbbbee

This reverts commit a7cca38.

MaicolAntali mentioned this pull request Feb 27, 2023

[Discussion] Deprecate non-type-safe Gson.fromJson(..., Type) overloads #2328

Open

Merge branch 'master' into fix-error-prone-warns

f89d42a

MaicolAntali marked this pull request as ready for review February 28, 2023 09:25

eamonnmcmanus approved these changes Feb 28, 2023

View reviewed changes

Merge branch 'master' into fix-error-prone-warns

039bf62

eamonnmcmanus merged commit dc20b75 into google:master Mar 1, 2023

MaicolAntali deleted the fix-error-prone-warns branch March 2, 2023 09:31

Marcono1234 mentioned this pull request Mar 5, 2023

Fail Maven build on warnings #2335

Merged

9 tasks

Marcono1234 mentioned this pull request Mar 6, 2023

Specify Error Prone JVM args with .mvn/jvm.config #2339

Merged

9 tasks

Marcono1234 mentioned this pull request Mar 18, 2023

Make dependency on Error Prone Annotations non-optional #2346

Merged

9 tasks

Marcono1234 mentioned this pull request May 20, 2024

[2.11.0] error_prone_annotations dependency #2676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error prone warns #2320

Fix error prone warns #2320

MaicolAntali commented Feb 18, 2023 •

edited

Marcono1234 left a comment

Marcono1234 commented Feb 19, 2023

MaicolAntali commented Feb 19, 2023

Marcono1234 commented Feb 19, 2023

eamonnmcmanus Feb 19, 2023

eamonnmcmanus Feb 19, 2023

eamonnmcmanus left a comment

Marcono1234 left a comment

MaicolAntali commented Feb 24, 2023 •

edited

eamonnmcmanus commented Feb 24, 2023

eamonnmcmanus left a comment

MaicolAntali commented Mar 1, 2023

eamonnmcmanus commented Mar 1, 2023

Marcono1234 commented Mar 4, 2023

eamonnmcmanus commented Mar 5, 2023

Marcono1234 commented Mar 5, 2023 •

edited

eamonnmcmanus commented Mar 6, 2023

cpovirk commented Mar 6, 2023

eamonnmcmanus commented Mar 6, 2023

Marcono1234 commented Mar 6, 2023 •

edited

Fix error prone warns #2320

Fix error prone warns #2320

Conversation

MaicolAntali commented Feb 18, 2023 • edited

Marcono1234 left a comment

Choose a reason for hiding this comment

Marcono1234 commented Feb 19, 2023

MaicolAntali commented Feb 19, 2023

Marcono1234 commented Feb 19, 2023

eamonnmcmanus Feb 19, 2023

Choose a reason for hiding this comment

eamonnmcmanus Feb 19, 2023

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 left a comment

Choose a reason for hiding this comment

MaicolAntali commented Feb 24, 2023 • edited

eamonnmcmanus commented Feb 24, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

MaicolAntali commented Mar 1, 2023

eamonnmcmanus commented Mar 1, 2023

Marcono1234 commented Mar 4, 2023

eamonnmcmanus commented Mar 5, 2023

Marcono1234 commented Mar 5, 2023 • edited

eamonnmcmanus commented Mar 6, 2023

cpovirk commented Mar 6, 2023

eamonnmcmanus commented Mar 6, 2023

Marcono1234 commented Mar 6, 2023 • edited

MaicolAntali commented Feb 18, 2023 •

edited

MaicolAntali commented Feb 24, 2023 •

edited

Marcono1234 commented Mar 5, 2023 •

edited

Marcono1234 commented Mar 6, 2023 •

edited