From 0b9e581c36f7d0b6479f1088d08d805cbe4d9b59 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 24 May 2019 10:37:36 -0700 Subject: [PATCH] Error Prone: Enable and fix ImmutableEnumChecker * Mark some enums using @Immutable from Error Prone (and also import that library). * Suppress one usage of an effectively-immutable JGit class which we don't control. * Fixing ReviewerStateInternal to store a String internally rather than a FooterKey. FooterKey is actually not immutable: it has a package- private byte array field. Keep returning FooterKey from the interface for type safety; given that one method was already constructing a new FooterKey dynamically on every invocation, and that this class is primarily used for constructing ChangeUpdates, this shouldn't be a performance issue. Change-Id: Ieb7ee10e9be39184cff7e0d1d18ff6b433016b31 --- Documentation/licenses.txt | 1 + WORKSPACE | 6 ++++++ java/com/google/gerrit/reviewdb/BUILD | 1 + .../converter/AccountIdProtoConverter.java | 2 ++ .../converter/BranchNameKeyProtoConverter.java | 2 ++ .../converter/ChangeIdProtoConverter.java | 2 ++ .../converter/ChangeKeyProtoConverter.java | 2 ++ .../ChangeMessageKeyProtoConverter.java | 2 ++ .../converter/ChangeMessageProtoConverter.java | 2 ++ .../reviewdb/converter/ChangeProtoConverter.java | 2 ++ .../converter/LabelIdProtoConverter.java | 2 ++ .../converter/ObjectIdProtoConverter.java | 2 ++ .../PatchSetApprovalKeyProtoConverter.java | 2 ++ .../PatchSetApprovalProtoConverter.java | 2 ++ .../converter/PatchSetIdProtoConverter.java | 2 ++ .../converter/PatchSetProtoConverter.java | 2 ++ .../converter/ProjectNameKeyProtoConverter.java | 2 ++ .../reviewdb/converter/ProtoConverter.java | 2 ++ .../gerrit/server/change/ArchiveFormat.java | 2 ++ .../server/notedb/ReviewerStateInternal.java | 16 ++++++++-------- lib/errorprone/BUILD | 6 ++++++ tools/BUILD | 2 +- 22 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 lib/errorprone/BUILD diff --git a/Documentation/licenses.txt b/Documentation/licenses.txt index 6e61e29acf..17c118467f 100644 --- a/Documentation/licenses.txt +++ b/Documentation/licenses.txt @@ -52,6 +52,7 @@ Apache2.0 * commons:pool * commons:validator * dropwizard:dropwizard-core +* errorprone:annotations * flogger:api * fonts:robotofonts * guice:guice diff --git a/WORKSPACE b/WORKSPACE index 836c3742d5..7aa09e26d4 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -186,6 +186,12 @@ maven_jar( sha1 = "94ad16d728b374d65bd897625f3fbb3da223a2b6", ) +maven_jar( + name = "error-prone-annotations", + artifact = "com.google.errorprone:error_prone_annotations:2.3.3", + sha1 = "42aa5155a54a87d70af32d4b0d06bf43779de0e2", +) + FLOGGER_VERS = "0.4" maven_jar( diff --git a/java/com/google/gerrit/reviewdb/BUILD b/java/com/google/gerrit/reviewdb/BUILD index d241140236..8c286ced14 100644 --- a/java/com/google/gerrit/reviewdb/BUILD +++ b/java/com/google/gerrit/reviewdb/BUILD @@ -13,6 +13,7 @@ java_library( "//lib:protobuf", "//lib/auto:auto-value", "//lib/auto:auto-value-annotations", + "//lib/errorprone:annotations", "//lib/jgit/org.eclipse.jgit:jgit", "//proto:entities_java_proto", ], diff --git a/java/com/google/gerrit/reviewdb/converter/AccountIdProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/AccountIdProtoConverter.java index 51e98c7fb6..5c7b03b946 100644 --- a/java/com/google/gerrit/reviewdb/converter/AccountIdProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/AccountIdProtoConverter.java @@ -14,10 +14,12 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.protobuf.Parser; +@Immutable public enum AccountIdProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java index f1018fc01d..6fa9353015 100644 --- a/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java @@ -14,11 +14,13 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.BranchNameKey; import com.google.gerrit.reviewdb.client.Project; import com.google.protobuf.Parser; +@Immutable public enum BranchNameKeyProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeIdProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeIdProtoConverter.java index a89434ec57..5e90c873d6 100644 --- a/java/com/google/gerrit/reviewdb/converter/ChangeIdProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeIdProtoConverter.java @@ -14,10 +14,12 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Change; import com.google.protobuf.Parser; +@Immutable public enum ChangeIdProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java index b9a4f4d3e9..4aa900b6b4 100644 --- a/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java @@ -14,10 +14,12 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Change; import com.google.protobuf.Parser; +@Immutable public enum ChangeKeyProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeMessageKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeMessageKeyProtoConverter.java index 7d97e392c5..3d362937d9 100644 --- a/java/com/google/gerrit/reviewdb/converter/ChangeMessageKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeMessageKeyProtoConverter.java @@ -14,11 +14,13 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.protobuf.Parser; +@Immutable public enum ChangeMessageKeyProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeMessageProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeMessageProtoConverter.java index 0895d8d89b..31b0e11faa 100644 --- a/java/com/google/gerrit/reviewdb/converter/ChangeMessageProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeMessageProtoConverter.java @@ -14,6 +14,7 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.ChangeMessage; @@ -22,6 +23,7 @@ import com.google.protobuf.Parser; import java.sql.Timestamp; import java.util.Objects; +@Immutable public enum ChangeMessageProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java index 384dbcab6e..2dfa516bc3 100644 --- a/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java @@ -14,6 +14,7 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.BranchNameKey; @@ -22,6 +23,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.protobuf.Parser; import java.sql.Timestamp; +@Immutable public enum ChangeProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/LabelIdProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/LabelIdProtoConverter.java index 7bc2ab1f27..42049a4689 100644 --- a/java/com/google/gerrit/reviewdb/converter/LabelIdProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/LabelIdProtoConverter.java @@ -14,10 +14,12 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.LabelId; import com.google.protobuf.Parser; +@Immutable public enum LabelIdProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverter.java index 7413af9af8..246207dc8a 100644 --- a/java/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverter.java @@ -14,6 +14,7 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.protobuf.Parser; import org.eclipse.jgit.lib.ObjectId; @@ -30,6 +31,7 @@ import org.eclipse.jgit.lib.ObjectId; *
  • This maintains backwards wire compatibility with a pre-NoteDb implementation. * */ +@Immutable public enum ObjectIdProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java index 43f6295f71..d3136b1136 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java @@ -14,6 +14,7 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.LabelId; @@ -21,6 +22,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.protobuf.Parser; +@Immutable public enum PatchSetApprovalKeyProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverter.java index 3baec9958d..a08d745dda 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverter.java @@ -14,6 +14,7 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSetApproval; @@ -21,6 +22,7 @@ import com.google.protobuf.Parser; import java.sql.Timestamp; import java.util.Objects; +@Immutable public enum PatchSetApprovalProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetIdProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetIdProtoConverter.java index 4101a6b154..154b0bf0f0 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetIdProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetIdProtoConverter.java @@ -14,11 +14,13 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.protobuf.Parser; +@Immutable public enum PatchSetIdProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java index f9ed8efb25..5006906b36 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java @@ -15,6 +15,7 @@ package com.google.gerrit.reviewdb.converter; import com.google.common.collect.ImmutableList; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSet; @@ -23,6 +24,7 @@ import java.sql.Timestamp; import java.util.List; import org.eclipse.jgit.lib.ObjectId; +@Immutable public enum PatchSetProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java index 74849af279..99048a0d85 100644 --- a/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java @@ -14,10 +14,12 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Project; import com.google.protobuf.Parser; +@Immutable public enum ProjectNameKeyProtoConverter implements ProtoConverter { INSTANCE; diff --git a/java/com/google/gerrit/reviewdb/converter/ProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ProtoConverter.java index 568759c3c8..f4f1de061c 100644 --- a/java/com/google/gerrit/reviewdb/converter/ProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/ProtoConverter.java @@ -14,9 +14,11 @@ package com.google.gerrit.reviewdb.converter; +import com.google.errorprone.annotations.Immutable; import com.google.protobuf.MessageLite; import com.google.protobuf.Parser; +@Immutable public interface ProtoConverter

    { P toProto(C valueClass); diff --git a/java/com/google/gerrit/server/change/ArchiveFormat.java b/java/com/google/gerrit/server/change/ArchiveFormat.java index 0316c5f36a..d895a66d48 100644 --- a/java/com/google/gerrit/server/change/ArchiveFormat.java +++ b/java/com/google/gerrit/server/change/ArchiveFormat.java @@ -35,7 +35,9 @@ public enum ArchiveFormat { TXZ("application/x-xz", new TxzFormat()), ZIP("application/x-zip", new ZipFormat()); + @SuppressWarnings("ImmutableEnumChecker") // ArchiveCommand.Format is effectively immutable. private final ArchiveCommand.Format format; + private final String mimeType; ArchiveFormat(String mimeType, ArchiveCommand.Format format) { diff --git a/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java b/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java index fad9832d90..d5a7259417 100644 --- a/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java +++ b/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java @@ -21,13 +21,13 @@ import org.eclipse.jgit.revwalk.FooterKey; /** State of a reviewer on a change. */ public enum ReviewerStateInternal { /** The user has contributed at least one nonzero vote on the change. */ - REVIEWER(new FooterKey("Reviewer"), ReviewerState.REVIEWER), + REVIEWER("Reviewer", ReviewerState.REVIEWER), /** The reviewer was added to the change, but has not voted. */ - CC(new FooterKey("CC"), ReviewerState.CC), + CC("CC", ReviewerState.CC), /** The user was previously a reviewer on the change, but was removed. */ - REMOVED(new FooterKey("Removed"), ReviewerState.REMOVED); + REMOVED("Removed", ReviewerState.REMOVED); public static ReviewerStateInternal fromReviewerState(ReviewerState state) { return ReviewerStateInternal.values()[state.ordinal()]; @@ -50,20 +50,20 @@ public enum ReviewerStateInternal { } } - private final FooterKey footerKey; + private final String footer; private final ReviewerState state; - ReviewerStateInternal(FooterKey footerKey, ReviewerState state) { - this.footerKey = footerKey; + ReviewerStateInternal(String footer, ReviewerState state) { + this.footer = footer; this.state = state; } FooterKey getFooterKey() { - return footerKey; + return new FooterKey(footer); } FooterKey getByEmailFooterKey() { - return new FooterKey(footerKey.getName() + "-email"); + return new FooterKey(footer + "-email"); } public ReviewerState asReviewerState() { diff --git a/lib/errorprone/BUILD b/lib/errorprone/BUILD new file mode 100644 index 0000000000..b5c130bf48 --- /dev/null +++ b/lib/errorprone/BUILD @@ -0,0 +1,6 @@ +java_library( + name = "annotations", + data = ["//lib:LICENSE-Apache2.0"], + visibility = ["//visibility:public"], + exports = ["@error-prone-annotations//jar"], +) diff --git a/tools/BUILD b/tools/BUILD index 8d34156ef8..27af453f63 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -62,7 +62,7 @@ java_package_configuration( "-Xep:FutureReturnValueIgnored:ERROR", "-Xep:GetClassOnEnum:ERROR", "-Xep:ImmutableAnnotationChecker:ERROR", - #"-Xep:ImmutableEnumChecker:ERROR", + "-Xep:ImmutableEnumChecker:ERROR", "-Xep:IncompatibleModifiers:ERROR", "-Xep:InjectOnConstructorOfAbstractClass:ERROR", "-Xep:InputStreamSlowMultibyteRead:ERROR",