Introduce ErrorProneFlags.get{Set,List}OrEmpty, because basically e…

…very caller wants to interpret an empty optional as an empty list/set. There's some uses of getList outside core EP, but after a JB release I should be able to migrate them, and then delete the method. PiperOrigin-RevId: 566931790
google · Sep 21, 2023 · c7662ed · c7662ed
1 parent 1bec842
commit c7662ed
Show file tree

Hide file tree

Showing 10 changed files with 59 additions and 39 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneFlags.java b/check_api/src/main/java/com/google/errorprone/ErrorProneFlags.java
@@ -152,21 +152,47 @@ public Optional<Integer> getInteger(String key) {
    * an {@link Optional}, which is empty if the flag is unset.
    *
    * <p>(note: empty strings included, e.g. {@code "-XepOpt:List=,1,,2," => ["","1","","2",""]})
+   *
+   * @deprecated prefer {@link #getListOrEmpty(String)}
    */
+  @Deprecated
   public Optional<List<String>> getList(String key) {
     return this.get(key).map(v -> ImmutableList.copyOf(Splitter.on(',').split(v)));
   }
 
+  /**
+   * Gets the flag value for the given key as a comma-separated {@link ImmutableList} of Strings, or
+   * an empty list if the flag is unset.
+   *
+   * <p>(note: empty strings included, e.g. {@code "-XepOpt:List=,1,,2," => ["","1","","2",""]})
+   */
+  public ImmutableList<String> getListOrEmpty(String key) {
+    return getList(key).map(ImmutableList::copyOf).orElse(ImmutableList.of());
+  }
+
   /**
    * Gets the flag value for the given key as a comma-separated {@link Set} of Strings, wrapped in
    * an {@link Optional}, which is empty if the flag is unset.
    *
    * <p>(note: empty strings included, e.g. {@code "-XepOpt:Set=,1,,1,2," => ["","1","2"]})
+   *
+   * @deprecated prefer {@link #getSetOrEmpty(String)}
    */
+  @Deprecated
   public Optional<Set<String>> getSet(String key) {
     return this.get(key).map(v -> ImmutableSet.copyOf(Splitter.on(',').split(v)));
   }
 
+  /**
+   * Gets the flag value for the given key as a comma-separated {@link Set} of Strings, or an empty
+   * set if unset.
+   *
+   * <p>(note: empty strings included, e.g. {@code "-XepOpt:Set=,1,,1,2," => ["","1","2"]})
+   */
+  public ImmutableSet<String> getSetOrEmpty(String key) {
+    return getSet(key).map(ImmutableSet::copyOf).orElse(ImmutableSet.of());
+  }
+
   /** Whether this Flags object is empty, i.e. no flags have been set. */
   public boolean isEmpty() {
     return this.flagsMap.isEmpty();

diff --git a/check_api/src/test/java/com/google/errorprone/ErrorProneFlagsTest.java b/check_api/src/test/java/com/google/errorprone/ErrorProneFlagsTest.java
@@ -20,7 +20,6 @@
 import static com.google.common.truth.Truth8.assertThat;
 import static org.junit.Assert.assertThrows;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import org.junit.Test;
@@ -92,12 +91,12 @@ public void parseAndGetList() {
             .parseFlag("-XepOpt:ArgD=7")
             .parseFlag("-XepOpt:ArgE=")
             .build();
-    assertThat(flags.getList("ArgA")).hasValue(ImmutableList.of("1", "2", "3"));
-    assertThat(flags.getList("ArgB")).hasValue(ImmutableList.of("4", ""));
-    assertThat(flags.getList("ArgC")).hasValue(ImmutableList.of("5", "", "", "6"));
-    assertThat(flags.getList("ArgD")).hasValue(ImmutableList.of("7"));
-    assertThat(flags.getList("ArgE")).hasValue(ImmutableList.of(""));
-    assertThat(flags.getList("absent")).isEmpty();
+    assertThat(flags.getListOrEmpty("ArgA")).containsExactly("1", "2", "3").inOrder();
+    assertThat(flags.getListOrEmpty("ArgB")).containsExactly("4", "").inOrder();
+    assertThat(flags.getListOrEmpty("ArgC")).containsExactly("5", "", "", "6").inOrder();
+    assertThat(flags.getListOrEmpty("ArgD")).containsExactly("7");
+    assertThat(flags.getListOrEmpty("ArgE")).containsExactly("");
+    assertThat(flags.getListOrEmpty("absent")).isEmpty();
   }
 
   @Test

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java
@@ -169,9 +169,10 @@ public MethodKind getMethodKind(MethodSymbol method) {
 
                 // This is conceptually lower precedence than the above rules.
                 externalIgnoreList());
-    flags
-        .getList(CRV_PACKAGES)
-        .ifPresent(packagePatterns -> builder.addRule(PackagesRule.fromPatterns(packagePatterns)));
+    var crvPackages = flags.getListOrEmpty(CRV_PACKAGES);
+    if (!crvPackages.isEmpty()) {
+      builder.addRule(PackagesRule.fromPatterns(crvPackages));
+    }
     this.evaluator =
         builder
             .addRule(

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java b/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java
@@ -72,10 +72,7 @@ public class ParameterName extends BugChecker
   @Inject
   ParameterName(ErrorProneFlags errorProneFlags) {
     this.exemptPackages =
-        errorProneFlags
-            .getList("ParameterName:exemptPackagePrefixes")
-            .orElse(ImmutableList.of())
-            .stream()
+        errorProneFlags.getListOrEmpty("ParameterName:exemptPackagePrefixes").stream()
             // add a trailing '.' so that e.g. com.foo matches as a prefix of com.foo.bar, but not
             // com.foobar
             .map(p -> p.endsWith(".") ? p : p + ".")

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java
@@ -167,9 +167,7 @@ public final class UnusedMethod extends BugChecker implements CompilationUnitTre
   UnusedMethod(ErrorProneFlags errorProneFlags) {
     this.exemptingMethodAnnotations =
         union(
-                errorProneFlags
-                    .getSet("UnusedMethod:ExemptingMethodAnnotations")
-                    .orElseGet(ImmutableSet::of),
+                errorProneFlags.getSetOrEmpty("UnusedMethod:ExemptingMethodAnnotations"),
                 EXEMPTING_METHOD_ANNOTATIONS)
             .immutableCopy();
   }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
@@ -178,21 +178,24 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
 
   @Inject
   UnusedVariable(ErrorProneFlags flags) {
-    ImmutableSet.Builder<String> methodAnnotationsExemptingParameters =
-        ImmutableSet.<String>builder().add("org.robolectric.annotation.Implementation");
-    flags
-        .getList("Unused:methodAnnotationsExemptingParameters")
-        .ifPresent(methodAnnotationsExemptingParameters::addAll);
-    this.methodAnnotationsExemptingParameters = methodAnnotationsExemptingParameters.build();
+    this.methodAnnotationsExemptingParameters =
+        ImmutableSet.<String>builder()
+            .add("org.robolectric.annotation.Implementation")
+            .addAll(flags.getListOrEmpty("Unused:methodAnnotationsExemptingParameters"))
+            .build();
     this.reportInjectedFields = flags.getBoolean("Unused:ReportInjectedFields").orElse(false);
 
-    ImmutableSet.Builder<String> exemptNames = ImmutableSet.<String>builder().add("ignored");
-    flags.getList("Unused:exemptNames").ifPresent(exemptNames::addAll);
-    this.exemptNames = exemptNames.build();
-
-    ImmutableSet.Builder<String> exemptPrefixes = ImmutableSet.<String>builder().add("unused");
-    flags.getSet("Unused:exemptPrefixes").ifPresent(exemptPrefixes::addAll);
-    this.exemptPrefixes = exemptPrefixes.build();
+    this.exemptNames =
+        ImmutableSet.<String>builder()
+            .add("ignored")
+            .addAll(flags.getListOrEmpty("Unused:exemptNames"))
+            .build();
+
+    this.exemptPrefixes =
+        ImmutableSet.<String>builder()
+            .add("unused")
+            .addAll(flags.getSetOrEmpty("Unused:exemptPrefixes"))
+            .build();
   }
 
   @Override

diff --git a/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java
@@ -97,9 +97,7 @@ public CanIgnoreReturnValueSuggester(ErrorProneFlags errorProneFlags) {
     this.exemptingMethodAnnotations =
         Sets.union(
                 EXEMPTING_METHOD_ANNOTATIONS,
-                errorProneFlags
-                    .getSet("CanIgnoreReturnValue:ExemptingMethodAnnotations")
-                    .orElse(ImmutableSet.of()))
+                errorProneFlags.getSetOrEmpty("CanIgnoreReturnValue:ExemptingMethodAnnotations"))
             .immutableCopy();
   }
 

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
@@ -91,8 +91,7 @@ public final class Inliner extends BugChecker
 
   @Inject
   Inliner(ErrorProneFlags flags) {
-    this.apiPrefixes =
-        ImmutableSet.copyOf(flags.getSet(PREFIX_FLAG).orElse(ImmutableSet.<String>of()));
+    this.apiPrefixes = flags.getSetOrEmpty(PREFIX_FLAG);
     this.skipCallsitesWithComments = flags.getBoolean(SKIP_COMMENTS_FLAG).orElse(true);
     this.checkFixCompiles = flags.getBoolean(CHECK_FIX_COMPILES).orElse(false);
   }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java
@@ -46,12 +46,12 @@ public final class WellKnownMutability implements ThreadSafety.KnownTypes {
 
   @Inject
   WellKnownMutability(ErrorProneFlags flags) {
-    List<String> immutable = flags.getList("Immutable:KnownImmutable").orElse(ImmutableList.of());
+    ImmutableList<String> immutable = flags.getListOrEmpty("Immutable:KnownImmutable");
     ImmutableList<String> mutable =
         // Please use "KnownMutable", as it's a bit clearer what we mean. "KnownUnsafe" is kept
         // for a while for backwards compatibility.
         Stream.of("Immutable:KnownMutable", "Immutable:KnownUnsafe")
-            .flatMap(f -> flags.getList(f).orElse(ImmutableList.of()).stream())
+            .flatMap(f -> flags.getListOrEmpty(f).stream())
             .collect(toImmutableList());
     this.knownImmutableClasses = buildImmutableClasses(immutable);
     this.knownMutableClasses = buildMutableClasses(mutable);

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java
@@ -27,8 +27,7 @@
 public final class WellKnownThreadSafety implements ThreadSafety.KnownTypes {
   @Inject
   WellKnownThreadSafety(ErrorProneFlags flags, WellKnownMutability wellKnownMutability) {
-    List<String> knownThreadSafe =
-        flags.getList("ThreadSafe:KnownThreadSafe").orElse(ImmutableList.of());
+    ImmutableList<String> knownThreadSafe = flags.getListOrEmpty("ThreadSafe:KnownThreadSafe");
     this.knownThreadSafeClasses = buildThreadSafeClasses(knownThreadSafe, wellKnownMutability);
     this.knownUnsafeClasses = wellKnownMutability.getKnownMutableClasses();
   }