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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Feb 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a5a5e52
Adds Error Prone to the `pom.xml`
MaicolAntali Feb 1, 2023
9910d47
Adds Error Prone annotations to avoid compiling errors
MaicolAntali Feb 1, 2023
61771d0
Adds profile to run Error Prone in JDK8
MaicolAntali Feb 1, 2023
d14d61f
Revert "Adds profile to run Error Prone in JDK8"
MaicolAntali Feb 1, 2023
0be7801
Fix Error Prone warn
MaicolAntali Feb 3, 2023
4fa7b35
Add comment to `pom.xml`
MaicolAntali Feb 4, 2023
f1206b7
Fix the `@SuppressWarnings("GetClassOnClass")`
MaicolAntali Feb 6, 2023
fa641c0
Replace the Error Prone link in the `pom.xml`
MaicolAntali Feb 6, 2023
5faa528
Disable Error Prone with jdk-15`
MaicolAntali Feb 6, 2023
89f6e39
Remove a new-line in `pom.xml`
MaicolAntali Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions extras/src/test/java/com/google/gson/typeadapters/PostConstructAdapterFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ public void testList() {
assertEquals(sandwiches, sandwichesFromJson);
}

@SuppressWarnings("overrides") // for missing hashCode() override
static class Sandwich {
@SuppressWarnings({"overrides", "EqualsHashCode"}) // for missing hashCode() override
static class Sandwich {
Comment on lines -60 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public String bread;
public String cheese;

Expand Down Expand Up @@ -92,7 +92,7 @@ public boolean equals(Object o) {
}
}

@SuppressWarnings("overrides") // for missing hashCode() override
@SuppressWarnings({"overrides", "EqualsHashCode"}) // for missing hashCode() override
static class MultipleSandwiches {
public List<Sandwich> sandwiches;

Expand Down
1 change: 1 addition & 0 deletions gson/src/test/java/com/google/gson/JsonArrayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public void testEqualsOnEmptyArray() {
}

@Test
@SuppressWarnings("TruthSelfEquals")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MaicolAntali MaicolAntali Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public void testEqualsNonEmptyArray() {
JsonArray a = new JsonArray();
JsonArray b = new JsonArray();
Expand Down
1 change: 1 addition & 0 deletions gson/src/test/java/com/google/gson/JsonObjectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ public void testEqualsOnEmptyObject() {
}

@Test
@SuppressWarnings("TruthSelfEquals")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be suppressed, see its description.

public void testEqualsNonEmptyObject() {
JsonObject a = new JsonObject();
JsonObject b = new JsonObject();
Expand Down
3 changes: 2 additions & 1 deletion gson/src/test/java/com/google/gson/common/TestTypes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,8 @@ public String getExpectedJson() {
}
}

@SuppressWarnings("overrides") // for missing hashCode() override
// for missing hashCode() override
@SuppressWarnings({"overrides", "EqualsHashCode"})
public static class ClassWithNoFields {
// Nothing here..
@Override
Expand Down
2 changes: 1 addition & 1 deletion gson/src/test/java/com/google/gson/functional/DefaultTypeAdaptersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public void testClassSerialization() {
@Test
public void testClassDeserialization() {
try {
gson.fromJson("String.class", String.class.getClass());
gson.fromJson("String.class", Class.class);
fail();
} catch (UnsupportedOperationException expected) {
}
Expand Down
4 changes: 2 additions & 2 deletions gson/src/test/java/com/google/gson/functional/InstanceCreatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,11 @@ class SubTreeSet<T> extends TreeSet<T> {}

Type sortedSetType = new TypeToken<SortedSet<String>>() {}.getType();
SortedSet<String> set = gson.fromJson("[\"a\"]", sortedSetType);
assertThat("a").isEqualTo(set.first());
assertThat(set.first()).isEqualTo("a");
assertThat(set.getClass()).isEqualTo(SubTreeSet.class);

set = gson.fromJson("[\"b\"]", SortedSet.class);
assertThat("b").isEqualTo(set.first());
assertThat(set.first()).isEqualTo("b");
assertThat(set.getClass()).isEqualTo(SubTreeSet.class);
}
}
2 changes: 1 addition & 1 deletion gson/src/test/java/com/google/gson/functional/ReflectionAccessFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeNotNull;;
import static org.junit.Assume.assumeNotNull;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
Expand Down
5 changes: 3 additions & 2 deletions gson/src/test/java/com/google/gson/functional/TypeVariableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ public void testBasicTypeVariables() {
assertThat(blue2).isEqualTo(blue1);
}

@SuppressWarnings("overrides") // for missing hashCode() override
// for missing hashCode() override
@SuppressWarnings({"overrides", "EqualsHashCode"})
public static class Blue extends Red<Boolean> {
public Blue() {
super(false);
Expand Down Expand Up @@ -103,7 +104,7 @@ public Red(S redField) {
}
}

@SuppressWarnings("overrides") // for missing hashCode() override
@SuppressWarnings({"overrides", "EqualsHashCode"}) // for missing hashCode() override
public static class Foo<S, T> extends Red<Boolean> {
private S someSField;
private T someTField;
Expand Down
3 changes: 2 additions & 1 deletion gson/src/test/java/com/google/gson/stream/JsonReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,7 +1698,8 @@ public void testDeeplyNestedObjects() throws IOException {
reader.beginObject();
assertThat(reader.nextName()).isEqualTo("a");
}
assertThat(reader.getPath());
assertThat(reader.getPath()).isEqualTo("$.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a"
+ ".a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a");
assertThat(reader.nextBoolean()).isTrue();
for (int i = 0; i < 40; i++) {
reader.endObject();
Expand Down
42 changes: 41 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,32 @@
<configuration>
<showWarnings>true</showWarnings>
<showDeprecation>true</showDeprecation>
<failOnWarning>true</failOnWarning>
<fork>true</fork>
<compilerArgs>
<!-- Args related to Error Prone, see: https://errorprone.info/docs/installation#maven -->
<arg>-XDcompilePolicy=simple</arg>
MaicolAntali marked this conversation as resolved.
Show resolved Hide resolved
<arg>-Xplugin:ErrorProne</arg>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
<!-- Enable all warnings, except for ones which cause issues when building with newer JDKs, see also
https://docs.oracle.com/en/java/javase/11/tools/javac.html -->
<compilerArg>-Xlint:all,-options</compilerArg>
</compilerArgs>
<annotationProcessorPaths>
<path>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>2.18.0</version>
</path>
</annotationProcessorPaths>
<jdkToolchain>
<version>[11,)</version>
</jdkToolchain>
Expand Down Expand Up @@ -271,6 +291,26 @@
</build>

<profiles>
<!-- Disable Error Prone in Java 15 -->
<profile>
<id>jdk15</id>
<activation>
<jdk>15</jdk>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs combine.self="override">
<compilerArg>-Xlint:all,-options</compilerArg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>
</profile>
<!-- Profile defining additional plugins to be executed for release -->
<profile>
<id>release</id>
Expand Down