From 1d2bc93bfab99cc08f96e9c4c534a829ece8da2b Mon Sep 17 00:00:00 2001 From: ghm Date: Thu, 21 Sep 2023 04:49:35 -0700 Subject: [PATCH] Introduce `ErrorProneFlags.get{Set,List}OrEmpty`, because basically every 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: 567270684 --- .../google/errorprone/ErrorProneFlags.java | 26 +++++++++++++++++ .../errorprone/ErrorProneFlagsTest.java | 13 ++++----- .../bugpatterns/CheckReturnValue.java | 7 +++-- .../errorprone/bugpatterns/ParameterName.java | 5 +--- .../errorprone/bugpatterns/UnusedMethod.java | 4 +-- .../bugpatterns/UnusedVariable.java | 29 ++++++++++--------- .../CanIgnoreReturnValueSuggester.java | 4 +-- .../bugpatterns/inlineme/Inliner.java | 3 +- .../threadsafety/WellKnownThreadSafety.java | 3 +- 9 files changed, 57 insertions(+), 37 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 index f053cdaa6b7..8cb8e9f8c76 100644 --- 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 getInteger(String key) { * an {@link Optional}, which is empty if the flag is unset. * *

(note: empty strings included, e.g. {@code "-XepOpt:List=,1,,2," => ["","1","","2",""]}) + * + * @deprecated prefer {@link #getListOrEmpty(String)} */ + @Deprecated public Optional> 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. + * + *

(note: empty strings included, e.g. {@code "-XepOpt:List=,1,,2," => ["","1","","2",""]}) + */ + public ImmutableList 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. * *

(note: empty strings included, e.g. {@code "-XepOpt:Set=,1,,1,2," => ["","1","2"]}) + * + * @deprecated prefer {@link #getSetOrEmpty(String)} */ + @Deprecated public Optional> 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. + * + *

(note: empty strings included, e.g. {@code "-XepOpt:Set=,1,,1,2," => ["","1","2"]}) + */ + public ImmutableSet 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 index b4bb00e8757..89655c03eae 100644 --- 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 index 63235b42a92..e242bd2bdd9 100644 --- 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 index 4eec04fe978..baf7fd384eb 100644 --- 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 index b1b7b3f61b7..9193319d5c2 100644 --- 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 index b80f872414d..7d5728bcee3 100644 --- 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 methodAnnotationsExemptingParameters = - ImmutableSet.builder().add("org.robolectric.annotation.Implementation"); - flags - .getList("Unused:methodAnnotationsExemptingParameters") - .ifPresent(methodAnnotationsExemptingParameters::addAll); - this.methodAnnotationsExemptingParameters = methodAnnotationsExemptingParameters.build(); + this.methodAnnotationsExemptingParameters = + ImmutableSet.builder() + .add("org.robolectric.annotation.Implementation") + .addAll(flags.getListOrEmpty("Unused:methodAnnotationsExemptingParameters")) + .build(); this.reportInjectedFields = flags.getBoolean("Unused:ReportInjectedFields").orElse(false); - ImmutableSet.Builder exemptNames = ImmutableSet.builder().add("ignored"); - flags.getList("Unused:exemptNames").ifPresent(exemptNames::addAll); - this.exemptNames = exemptNames.build(); - - ImmutableSet.Builder exemptPrefixes = ImmutableSet.builder().add("unused"); - flags.getSet("Unused:exemptPrefixes").ifPresent(exemptPrefixes::addAll); - this.exemptPrefixes = exemptPrefixes.build(); + this.exemptNames = + ImmutableSet.builder() + .add("ignored") + .addAll(flags.getListOrEmpty("Unused:exemptNames")) + .build(); + + this.exemptPrefixes = + ImmutableSet.builder() + .add("unused") + .addAll(flags.getSetOrEmpty("Unused:exemptPrefixes")) + .build(); } @Override diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java index eee89416e39..7db01019079 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java +++ b/core/src/main/java/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 index 11c29293f11..696bac19f90 100644 --- 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.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/WellKnownThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownThreadSafety.java index 632f57cd62c..63c08dbda4f 100644 --- 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 knownThreadSafe = - flags.getList("ThreadSafe:KnownThreadSafe").orElse(ImmutableList.of()); + ImmutableList knownThreadSafe = flags.getListOrEmpty("ThreadSafe:KnownThreadSafe"); this.knownThreadSafeClasses = buildThreadSafeClasses(knownThreadSafe, wellKnownMutability); this.knownUnsafeClasses = wellKnownMutability.getKnownMutableClasses(); }