Adds Error Prone to the `maven-compiler-plugin` #2308

MaicolAntali · 2023-02-01T10:05:39Z

First at all, I have set up the Error Prone in the pom.xml. I had to remove the <failOnWarning>true</failOnWarning> otherwise the compiler would stop at every warning that Error Prone gives us.

After, I have add some @SuppressWarnings(...) to avoid errors from Error Prone (that stop the compiler)

MaicolAntali · 2023-02-01T10:20:38Z

Any idea why CIFuzz is failing?

Is it possible that CIFuzz use Java8 and fail due the <compilerArgs>? Error Prone, for java 8, requires different <compilerArgs> as mentioned here: Link

This reverts commit 61771d0.

eamonnmcmanus · 2023-02-01T18:33:58Z

Thanks, this is great!

Any idea why CIFuzz is failing?

I think the key thing is this:

An exception has occurred in the compiler (15.0.2)

It's running with JDK 15, and I don't think Error Prone supports that. Interestingly I found an old PR that I had lost track of, where I was trying to update Guava fuzzing to use JDK 17 instead of 15. Maybe we can do something like that here too. (It would be more convincing if I had actually made that PR work.)

As a short-term alternative, perhaps you could fiddle with Maven profiles so that Error Prone is not enabled if we're on JDK 15.

MaicolAntali · 2023-02-01T19:17:20Z

I have forked oss-fuzz . I will look into the Dockerfile of the Gson project to see if i can run it under jdk-17

MaicolAntali · 2023-02-02T15:47:16Z

Hello @eamonnmcmanus, the PR google/oss-fuzz#9554 have passed all tests. Can you take a look and tell me if it’s everything good? If yes, I will mark the PR as ready to review so that it will no longer a draft

eamonnmcmanus · 2023-02-02T17:34:27Z

That's fantastic! Thanks for taking the time to do it.

MaicolAntali · 2023-02-02T19:04:00Z

Most welcome, now we have to wait until the PR will be merged

Should fix the error in this [PR](google/gson#2308)

Marcono1234

Is it risky to use Error Prone with the regular compilation run, and should it be executed in a separate compilation run instead? I am a bit concerned that it hooks into compilation of classes which then end up in the final JAR. Though I am not very familiar with Error Prone, so maybe this is not an issue.

otherwise the compiler would stop at every warning that Error Prone gives us

The question might be though whether the other warnings should be fixed, suppressed or whether the severity should be adjusted for them (possibly in a follow-up PR?). Otherwise it might be difficult to notice newly introduced warnings when there are already a bunch of previously existing warnings.

Marcono1234 · 2023-02-04T01:56:52Z

extras/src/test/java/com/google/gson/typeadapters/PostConstructAdapterFactoryTest.java

-    @SuppressWarnings("overrides") // for missing hashCode() override
-    static class Sandwich {
+  @SuppressWarnings({"overrides", "EqualsHashCode"}) // for missing hashCode() override
+  static class Sandwich {


Maybe for consistency the indentation for the complete file should be fixed? This file is not that long and the previous JUnit changes already caused some inconsistent indentation within this file too.

Or Éamonn would you prefer not to have this as part of this PR?

I'm fine either way. If @MaicolAntali prefers to add suppressions in this PR and remove them later by fixing the code in one or more later PRs, that seems like a good approach.

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

pom.xml

Marcono1234 · 2023-02-04T03:02:09Z

pom.xml

            <compilerArgs>
+              <arg>-XDcompilePolicy=simple</arg>
+              <arg>-Xplugin:ErrorProne</arg>


Should probably suppress warnings for classes generated by proto module:

Suggested change

<arg>-Xplugin:ErrorProne</arg>

<arg>-Xplugin:ErrorProne -XepExcludedPaths:.*/generated-test-sources/protobuf/.*</arg>

Error Prone's -XepDisableWarningsInGeneratedCode does not seem to work because it relies on @Generated and the protobuf plugin does not add that yet, see xolstice/protobuf-maven-plugin#29.

Marcono1234 · 2023-02-04T03:10:04Z

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

@@ -35,6 +35,7 @@ public void testEqualsOnEmptyArray() {
  }

  @Test
+  @SuppressWarnings("TruthSelfEquals")


This should not be suppressed, see its description.

assertThat(a).isEqualTo(a); below should be changed to assertThat(a.equals(a)).isTrue() (or MoreAsserts.assertEqualsAndHashCode(a, a)?) since this is intended to explicitly test the equals implementation.

In #2299, Éamonn said we could use com.google.guava:guava-testlib to simplify assert like this.

Link to comment

I never use guava-testlib so I'm not sure how to do that. I have looked around for some documentations/examples, but I don't find nothing.

I assume we could use some like: new EqualsTester().addEqualityGroup(a).testEquals();

Hmm, right. Personally I think for this PR it would be best to omit these suppressions and change the code to a.equals(a) to fix this issue. If desired we could then add guava-testlib in a separate PR.

Yes, sorry, I should have been more explicit. That is exactly what you can write, and alternatively you can have just one test method with all the different kinds of objects in different equality groups. Something like this:

new EqualsTester() .addEqualityGroup(array1, array2) // with equal contents .addEqualityGroup(array3, array4) // equal to each other but not array1 or array2 .addEqualityGroup(object1, object2) ... .testEquals();

EqualsTester tests that each object in an equality group is equal to itself and to each other object in the group, and not equal to any object in any other group. It also checks that equal objects have equal hashcodes. So it removes a lot of drudgery.

I think that would be a fine project for a separate PR.

Marcono1234 · 2023-02-04T03:10:58Z

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

@@ -163,6 +163,7 @@ public void testEqualsOnEmptyObject() {
  }

  @Test
+  @SuppressWarnings("TruthSelfEquals")


This should not be suppressed, see its description.

MaicolAntali · 2023-02-04T08:32:08Z

The question might be though whether the other warnings should be fixed, suppressed or whether the severity should be adjusted for them (possibly in a follow-up PR?). Otherwise it might be difficult to notice newly introduced warnings when there are already a bunch of previously existing warnings.

Every warn could be suppressed or fixed. If you look the build log, Error prone returns to you something like that for each warn:

Warning:  /home/runner/work/gson/gson/gson/src/main/java/com/google/gson/internal/LinkedTreeMap.java:[568,12] [ReferenceEquality] Comparison using reference equality instead of value equality
    (see https://errorprone.info/bugpattern/ReferenceEquality)
  Did you mean 'if (Objects.equals(e, header)) {' or 'if (e.equals(header)) {'?

You have 2 possibility: Fix it or Suppress it. If you follow the link https://errorprone.info/bugpattern/ReferenceEquality (Link) there is an explanation of the problem, a way to fix it and a way to suppress it.

possibly in a follow-up PR?

Exactly, The next PR(s) should fix/suppress the warns

eamonnmcmanus · 2023-02-04T18:43:08Z

I see this in the CIFuzz logs:

Step 2/8 : RUN apt-get update && apt-get install -y make autoconf automake libtool wget

which I think means that your oss-fuzz change isn't yet active. Otherwise we would see openjdk-17-jdk at the end of that line.

We don't really care about running Error Prone in the CIFuzz build. Would it be very hassly to omit it when running on JDK 15? As a workaround until the CIFuzz thing is working properly.

eamonnmcmanus · 2023-02-04T18:44:14Z

extras/src/test/java/com/google/gson/typeadapters/PostConstructAdapterFactoryTest.java

-    @SuppressWarnings("overrides") // for missing hashCode() override
-    static class Sandwich {
+  @SuppressWarnings({"overrides", "EqualsHashCode"}) // for missing hashCode() override
+  static class Sandwich {


I'm fine either way. If @MaicolAntali prefers to add suppressions in this PR and remove them later by fixing the code in one or more later PRs, that seems like a good approach.

eamonnmcmanus · 2023-02-04T18:48:50Z

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

@@ -35,6 +35,7 @@ public void testEqualsOnEmptyArray() {
  }

  @Test
+  @SuppressWarnings("TruthSelfEquals")


Yes, sorry, I should have been more explicit. That is exactly what you can write, and alternatively you can have just one test method with all the different kinds of objects in different equality groups. Something like this:

new EqualsTester() .addEqualityGroup(array1, array2) // with equal contents .addEqualityGroup(array3, array4) // equal to each other but not array1 or array2 .addEqualityGroup(object1, object2) ... .testEquals();

EqualsTester tests that each object in an equality group is equal to itself and to each other object in the group, and not equal to any object in any other group. It also checks that equal objects have equal hashcodes. So it removes a lot of drudgery.

I think that would be a fine project for a separate PR.

pom.xml

eamonnmcmanus · 2023-02-04T18:54:18Z

Is it risky to use Error Prone with the regular compilation run, and should it be executed in a separate compilation run instead? I am a bit concerned that it hooks into compilation of classes which then end up in the final JAR. Though I am not very familiar with Error Prone, so maybe this is not an issue.

I think it's fine. Error Prone doesn't affect the generated code, except that there won't be any if it detects an error. We use it in all Java compilations at Google.

eamonnmcmanus · 2023-02-06T13:56:11Z

This is looking good. Thanks for the CIFuzz workaround! Let me know if this is ready to be merged.

MaicolAntali · 2023-02-06T13:57:29Z

✅ Ready to be merged

Should fix the error in this [PR](google/gson#2308)

* Adds Error Prone to the `pom.xml` * Adds Error Prone annotations to avoid compiling errors * Adds profile to run Error Prone in JDK8 * Revert "Adds profile to run Error Prone in JDK8" This reverts commit 61771d0. * Fix Error Prone warn * Add comment to `pom.xml` * Fix the `@SuppressWarnings("GetClassOnClass")` * Replace the Error Prone link in the `pom.xml` * Disable Error Prone with jdk-15` * Remove a new-line in `pom.xml`

MaicolAntali added 2 commits February 1, 2023 08:53

Adds Error Prone to the pom.xml

a5a5e52

Adds Error Prone annotations to avoid compiling errors

9910d47

MaicolAntali added 2 commits February 1, 2023 11:49

Adds profile to run Error Prone in JDK8

61771d0

Revert "Adds profile to run Error Prone in JDK8"

d14d61f

This reverts commit 61771d0.

MaicolAntali changed the title ~~Add Error Prone to the maven-compiler-plugin~~ Adds Error Prone to the maven-compiler-plugin Feb 1, 2023

MaicolAntali mentioned this pull request Feb 1, 2023

Update the Gson project to use JDK 17 google/oss-fuzz#9554

Merged

DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Feb 2, 2023

Update the Gson project to use JDK 17 (#9554)

6e21448

Should fix the error in this [PR](google/gson#2308)

Fix Error Prone warn

0be7801

Marcono1234 reviewed Feb 4, 2023

View reviewed changes

Add comment to pom.xml

4fa7b35

eamonnmcmanus reviewed Feb 4, 2023

View reviewed changes

MaicolAntali added 4 commits February 6, 2023 08:57

Fix the @SuppressWarnings("GetClassOnClass")

f1206b7

Replace the Error Prone link in the pom.xml

fa641c0

Disable Error Prone with jdk-15`

5faa528

Remove a new-line in pom.xml

89f6e39

eamonnmcmanus merged commit 9f26679 into google:master Feb 6, 2023

MaicolAntali deleted the error-prone branch February 6, 2023 14:17

Marcono1234 mentioned this pull request Feb 14, 2023

Fix error prone warnings #2316

Merged

eamonnmcmanus pushed a commit to eamonnmcmanus/oss-fuzz that referenced this pull request Mar 15, 2023

Update the Gson project to use JDK 17 (google#9554)

14e2a0f

Should fix the error in this [PR](google/gson#2308)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Error Prone to the `maven-compiler-plugin` #2308

Adds Error Prone to the `maven-compiler-plugin` #2308

MaicolAntali commented Feb 1, 2023

MaicolAntali commented Feb 1, 2023 •

edited

Loading

eamonnmcmanus commented Feb 1, 2023

MaicolAntali commented Feb 1, 2023

MaicolAntali commented Feb 2, 2023

eamonnmcmanus commented Feb 2, 2023

MaicolAntali commented Feb 2, 2023

Marcono1234 left a comment

Marcono1234 Feb 4, 2023

eamonnmcmanus Feb 4, 2023

Marcono1234 Feb 4, 2023

Marcono1234 Feb 4, 2023

MaicolAntali Feb 4, 2023 •

edited

Loading

Marcono1234 Feb 4, 2023

eamonnmcmanus Feb 4, 2023

Marcono1234 Feb 4, 2023

MaicolAntali commented Feb 4, 2023 •

edited

Loading

eamonnmcmanus commented Feb 4, 2023

eamonnmcmanus Feb 4, 2023

eamonnmcmanus Feb 4, 2023

eamonnmcmanus commented Feb 4, 2023

eamonnmcmanus commented Feb 6, 2023

MaicolAntali commented Feb 6, 2023 •

edited

Loading

	<arg>-Xplugin:ErrorProne</arg>
	<arg>-Xplugin:ErrorProne -XepExcludedPaths:./generated-test-sources/protobuf/.</arg>

Adds Error Prone to the maven-compiler-plugin #2308

Adds Error Prone to the maven-compiler-plugin #2308

Conversation

MaicolAntali commented Feb 1, 2023

MaicolAntali commented Feb 1, 2023 • edited Loading

eamonnmcmanus commented Feb 1, 2023

MaicolAntali commented Feb 1, 2023

MaicolAntali commented Feb 2, 2023

eamonnmcmanus commented Feb 2, 2023

MaicolAntali commented Feb 2, 2023

Marcono1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaicolAntali Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaicolAntali commented Feb 4, 2023 • edited Loading

eamonnmcmanus commented Feb 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eamonnmcmanus commented Feb 4, 2023

eamonnmcmanus commented Feb 6, 2023

MaicolAntali commented Feb 6, 2023 • edited Loading

Adds Error Prone to the `maven-compiler-plugin` #2308

Adds Error Prone to the `maven-compiler-plugin` #2308

MaicolAntali commented Feb 1, 2023 •

edited

Loading

MaicolAntali Feb 4, 2023 •

edited

Loading

MaicolAntali commented Feb 4, 2023 •

edited

Loading

MaicolAntali commented Feb 6, 2023 •

edited

Loading