From 6195480da53984c47170c5cf9a3b18f5a9c749b5 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Fri, 13 Dec 2024 12:42:06 -0800 Subject: [PATCH] add values() to EnumOrdinal check This check is in JavaCodeClarity, only runs on changed lines. While testing, discovered that it would flag a lot of instances where `values()[` was being invoked from within the same class. I wasn't sure if that was a reasonable thing to do, seems like it might be a valid internal design choice. See the 'negative_enumValues_internalCall' test case as an example. Or see some of the instances in unknown commit So I added an additional check of the enclosing class to ignore internal usage. That flagged fewer things: unknown commit PiperOrigin-RevId: 705970604 --- .../errorprone/bugpatterns/EnumOrdinal.java | 33 ++++++++++- .../bugpatterns/EnumOrdinalTest.java | 59 +++++++++++++++++++ docs/bugpattern/EnumOrdinal.md | 33 +++++++---- 3 files changed, 113 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EnumOrdinal.java b/core/src/main/java/com/google/errorprone/bugpatterns/EnumOrdinal.java index 8f5a327651b..f783ae78372 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EnumOrdinal.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EnumOrdinal.java @@ -18,24 +18,37 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getSymbol; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ArrayAccessTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ArrayAccessTree; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; /** Discourages the use of {@link Enum#ordinal()} and other ways to access enum values by index. */ @BugPattern( - summary = "You should almost never invoke the Enum.ordinal() method.", + summary = + "You should almost never invoke the Enum.ordinal() method or depend on the enum values by" + + " index.", severity = WARNING) -public final class EnumOrdinal extends BugChecker implements MethodInvocationTreeMatcher { +public final class EnumOrdinal extends BugChecker + implements MethodInvocationTreeMatcher, ArrayAccessTreeMatcher { private static final Matcher ORDINAL = instanceMethod().onDescendantOf("java.lang.Enum").named("ordinal"); + private static final Matcher VALUES = + staticMethod().onDescendantOf("java.lang.Enum").named("values"); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (ORDINAL.matches(tree, state)) { @@ -43,4 +56,20 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } return Description.NO_MATCH; } + + @Override + public Description matchArrayAccess(ArrayAccessTree tree, VisitorState state) { + if (!(tree.getExpression() instanceof MethodInvocationTree mit)) { + return Description.NO_MATCH; + } + if (!VALUES.matches(tree.getExpression(), state)) { + return Description.NO_MATCH; + } + MethodSymbol methodInvocationSymbol = getSymbol(mit); + ClassSymbol enclosingClassSymbol = getSymbol(state.findEnclosing(ClassTree.class)); + if (methodInvocationSymbol.isEnclosedBy(enclosingClassSymbol)) { + return Description.NO_MATCH; + } + return describeMatch(tree); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EnumOrdinalTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EnumOrdinalTest.java index 345b089411f..5eed343189c 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EnumOrdinalTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/EnumOrdinalTest.java @@ -47,4 +47,63 @@ public int callOrdinal() { """) .doTest(); } + + @Test + public void positive_enumValues_externalCall() { + testHelper + .addSourceLines( + "Test.java", + """ + enum TestEnum { + FOO, + BAR, + } + + class Caller { + public TestEnum callValues() { + // BUG: Diagnostic contains: ordinal + return TestEnum.values()[0]; + } + } + """) + .doTest(); + } + + @Test + public void negative_enumValues_internalCall() { + testHelper + .addSourceLines( + "Test.java", + """ + enum TestEnum { + FOO, + BAR; + + private static TestEnum fromValues() { + return values()[0]; + } + } + """) + .doTest(); + } + + @Test + public void negative_enumValues_noIndex() { + testHelper + .addSourceLines( + "Test.java", + """ + enum TestEnum { + FOO, + BAR, + } + + class Caller { + public TestEnum[] callValues() { + return TestEnum.values(); + } + } + """) + .doTest(); + } } diff --git a/docs/bugpattern/EnumOrdinal.md b/docs/bugpattern/EnumOrdinal.md index e934408fd15..814eecc742e 100644 --- a/docs/bugpattern/EnumOrdinal.md +++ b/docs/bugpattern/EnumOrdinal.md @@ -1,7 +1,8 @@ -You should almost never invoke the `Enum.ordinal()` method. The ordinal exists -only to support low-level utilities like `EnumSet`. The ordinal of a given enum -value is not guaranteed to be stable across builds because of the potential for -enum values to be added, removed, or reordered. +You should almost never invoke the `Enum.ordinal()` method, nor depend on some +enum constant being at a particular index of the `values()` array. The ordinal +of a given enum value is not guaranteed to be stable across builds because of +the potential for enum values to be added, removed, or reordered. The ordinal +exists only to support low-level utilities like `EnumSet`. Prefer using enum value directly: @@ -13,7 +14,7 @@ ImmutableMap MAPPING = .buildOrThrow(); ``` -to this: +instead of relying on the ordinal: ```java ImmutableMap MAPPING = @@ -23,8 +24,8 @@ ImmutableMap MAPPING = .buildOrThrow(); ``` -Or if you need a stable number for serialisation, consider defining an explicit -field on the enum instead: +If you need a stable number for serialisation, consider defining an explicit +field on the enum: ```java enum MyStableEnum { @@ -32,9 +33,21 @@ enum MyStableEnum { BAR(2), ; - private final int index; - MyStableEnum(int index) { - this.index = index; + private final int wireCode; + MyStableEnum(int wireCode) { + this.wireCode = wireCode; } } ``` + +rather than relying on the ordinal values: + +```java +enum MyUnstableEnum { + FOO, + BAR, +} +MyUnstableEnum fromWire(int wireCode) { + return MyUnstableEnum.values()[wireCode]; +} +```