An update on StringEquality.

It seems redundant in the face of ReferenceEquality as a general check. The value it _could_ have is turning this into an error, except it doesn't. PiperOrigin-RevId: 438536211
google · Mar 31, 2022 · 62a2d27 · 62a2d27
1 parent ce2e0f7
commit 62a2d27
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 208 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReferenceEquality.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReferenceEquality.java
@@ -45,8 +45,7 @@
  * Abstract implementation of a BugPattern that detects the use of reference equality to compare
  * classes with value semantics.
  *
- * <p>See e.g. {@link OptionalEquality}, {@link ProtoStringFieldReferenceEquality}, and {@link
- * StringEquality}.
+ * <p>See e.g. {@link OptionalEquality}, {@link ProtoStringFieldReferenceEquality}.
  *
  * @author [email protected] (Liam Miller-Cushon)
  */

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringEquality.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringEquality.java
diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -316,7 +316,6 @@
 import com.google.errorprone.bugpatterns.StreamToIterable;
 import com.google.errorprone.bugpatterns.StreamToString;
 import com.google.errorprone.bugpatterns.StringBuilderInitWithChar;
-import com.google.errorprone.bugpatterns.StringEquality;
 import com.google.errorprone.bugpatterns.StringSplitter;
 import com.google.errorprone.bugpatterns.StronglyTypeByteString;
 import com.google.errorprone.bugpatterns.SubstringOfZero;
@@ -1090,7 +1089,6 @@ public static ScannerSupplier errorChecks() {
           ScopeOrQualifierAnnotationRetention.class,
           StaticOrDefaultInterfaceMethod.class,
           StaticQualifiedUsingExpression.class,
-          StringEquality.class,
           StronglyTypeByteString.class,
           StronglyTypeTime.class,
           SuppressWarningsWithoutExplanation.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringEqualityTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringEqualityTest.java
diff --git a/...src/test/java/com/google/errorprone/bugpatterns/testdata/StringEqualityNegativeCases.java b/...src/test/java/com/google/errorprone/bugpatterns/testdata/StringEqualityNegativeCases.java
diff --git a/...src/test/java/com/google/errorprone/bugpatterns/testdata/StringEqualityPositiveCases.java b/...src/test/java/com/google/errorprone/bugpatterns/testdata/StringEqualityPositiveCases.java
diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java
@@ -48,8 +48,8 @@
 import com.google.errorprone.bugpatterns.EqualsIncompatibleType;
 import com.google.errorprone.bugpatterns.LongLiteralLowerCaseSuffix;
 import com.google.errorprone.bugpatterns.PackageLocation;
+import com.google.errorprone.bugpatterns.ReferenceEquality;
 import com.google.errorprone.bugpatterns.StaticQualifiedUsingExpression;
-import com.google.errorprone.bugpatterns.StringEquality;
 import com.google.errorprone.bugpatterns.nullness.UnnecessaryCheckNotNull;
 import com.sun.source.util.JavacTask;
 import com.sun.tools.javac.api.JavacTool;
@@ -331,10 +331,10 @@ public void applyOverridesEnableAllChecks() {
 
   @Test
   public void applyOverridesDisableErrors() {
-    // BadShiftAmount (error), ArrayEquals (unsuppressible error), StringEquality (warning)
+    // BadShiftAmount (error), ArrayEquals (unsuppressible error), ReferenceEquality (warning)
     ScannerSupplier ss =
         ScannerSupplier.fromBugCheckerClasses(
-            BadShiftAmount.class, UnsuppressibleArrayEquals.class, StringEquality.class);
+            BadShiftAmount.class, UnsuppressibleArrayEquals.class, ReferenceEquality.class);
 
     ErrorProneOptions epOptions =
         ErrorProneOptions.processArgs(ImmutableList.of("-XepAllErrorsAsWarnings"));
@@ -344,31 +344,31 @@ public void applyOverridesDisableErrors() {
             ImmutableMap.of(
                 "ArrayEquals", SeverityLevel.ERROR, // Unsuppressible, not demoted
                 "BadShiftAmount", SeverityLevel.WARNING, // Demoted from error to warning
-                "StringEquality", SeverityLevel.WARNING)); // Already warning, unaffected
+                "ReferenceEquality", SeverityLevel.WARNING)); // Already warning, unaffected
 
     // Flags after AllErrorsAsWarnings flag should override it.
     epOptions =
         ErrorProneOptions.processArgs(
-            ImmutableList.of("-XepAllErrorsAsWarnings", "-Xep:StringEquality:ERROR"));
+            ImmutableList.of("-XepAllErrorsAsWarnings", "-Xep:ReferenceEquality:ERROR"));
 
     assertScanner(ss.applyOverrides(epOptions))
         .hasSeverities(
             ImmutableMap.of(
                 "ArrayEquals", SeverityLevel.ERROR,
                 "BadShiftAmount", SeverityLevel.WARNING,
-                "StringEquality", SeverityLevel.ERROR));
+                "ReferenceEquality", SeverityLevel.ERROR));
 
     // AllErrorsAsWarnings flag should override all error-level severity flags that come before it.
     epOptions =
         ErrorProneOptions.processArgs(
-            ImmutableList.of("-Xep:StringEquality:ERROR", "-XepAllErrorsAsWarnings"));
+            ImmutableList.of("-Xep:ReferenceEquality:ERROR", "-XepAllErrorsAsWarnings"));
 
     assertScanner(ss.applyOverrides(epOptions))
         .hasSeverities(
             ImmutableMap.of(
                 "ArrayEquals", SeverityLevel.ERROR,
                 "BadShiftAmount", SeverityLevel.WARNING,
-                "StringEquality", SeverityLevel.WARNING));
+                "ReferenceEquality", SeverityLevel.WARNING));
 
     // AllErrorsAsWarnings only overrides error-level severity flags.
     // That is, checks disabled before the flag are left disabled, not promoted to warnings.
@@ -380,9 +380,9 @@ public void applyOverridesDisableErrors() {
         .hasSeverities(
             ImmutableMap.of(
                 "ArrayEquals", SeverityLevel.ERROR,
-                "StringEquality", SeverityLevel.WARNING));
+                "ReferenceEquality", SeverityLevel.WARNING));
     assertScanner(ss.applyOverrides(epOptions))
-        .hasEnabledChecks(UnsuppressibleArrayEquals.class, StringEquality.class);
+        .hasEnabledChecks(UnsuppressibleArrayEquals.class, ReferenceEquality.class);
   }
 
   @Test
@@ -465,18 +465,20 @@ public void applyOverridesSucceedsWhenDisablingUnknownCheckAndIgnoreUnknownCheck
   public void applyOverridesSetsSeverity() {
     ScannerSupplier ss =
         ScannerSupplier.fromBugCheckerClasses(
-            BadShiftAmount.class, ChainingConstructorIgnoresParameter.class, StringEquality.class);
+            BadShiftAmount.class,
+            ChainingConstructorIgnoresParameter.class,
+            ReferenceEquality.class);
     ErrorProneOptions epOptions =
         ErrorProneOptions.processArgs(
             ImmutableList.of(
-                "-Xep:ChainingConstructorIgnoresParameter:WARN", "-Xep:StringEquality:ERROR"));
+                "-Xep:ChainingConstructorIgnoresParameter:WARN", "-Xep:ReferenceEquality:ERROR"));
     ScannerSupplier overriddenScannerSupplier = ss.applyOverrides(epOptions);
 
     ImmutableMap<String, SeverityLevel> expected =
         ImmutableMap.of(
             "BadShiftAmount", SeverityLevel.ERROR,
             "ChainingConstructorIgnoresParameter", SeverityLevel.WARNING,
-            "StringEquality", SeverityLevel.ERROR);
+            "ReferenceEquality", SeverityLevel.ERROR);
 
     assertScanner(overriddenScannerSupplier).hasSeverities(expected);
   }
@@ -507,35 +509,37 @@ public void allChecksAsWarningsWorks() {
         ScannerSupplier.fromBugCheckerClasses(
                 BadShiftAmount.class,
                 ChainingConstructorIgnoresParameter.class,
-                StringEquality.class)
+                ReferenceEquality.class)
             .filter(Predicates.alwaysFalse());
     assertScanner(ss).hasEnabledChecks(); // Everything's off
 
     ErrorProneOptions epOptions =
         ErrorProneOptions.processArgs(
-            ImmutableList.of("-Xep:StringEquality:OFF", "-XepAllDisabledChecksAsWarnings"));
+            ImmutableList.of("-Xep:ReferenceEquality:OFF", "-XepAllDisabledChecksAsWarnings"));
 
     ScannerSupplier withOverrides = ss.applyOverrides(epOptions);
     assertScanner(withOverrides)
         .hasEnabledChecks(
-            BadShiftAmount.class, ChainingConstructorIgnoresParameter.class, StringEquality.class);
+            BadShiftAmount.class,
+            ChainingConstructorIgnoresParameter.class,
+            ReferenceEquality.class);
 
     ImmutableMap<String, SeverityLevel> expectedSeverities =
         ImmutableMap.of(
             "BadShiftAmount",
             SeverityLevel.WARNING,
             "ChainingConstructorIgnoresParameter",
             SeverityLevel.WARNING,
-            "StringEquality",
+            "ReferenceEquality",
             SeverityLevel.WARNING);
     assertScanner(withOverrides).hasSeverities(expectedSeverities);
 
     epOptions =
         ErrorProneOptions.processArgs(
             ImmutableList.of(
-                "-Xep:StringEquality:OFF",
+                "-Xep:ReferenceEquality:OFF",
                 "-XepAllDisabledChecksAsWarnings",
-                "-Xep:StringEquality:OFF"));
+                "-Xep:ReferenceEquality:OFF"));
 
     withOverrides = ss.applyOverrides(epOptions);
     assertScanner(withOverrides)

diff --git a/docs/bugpattern/StringEquality.md b/docs/bugpattern/StringEquality.md