Declare "redundant" overrides of all our package-private `writeReplac…

…e` methods.

If users request for an optimizer to keep one of our classes in `com.google.common.collect` but permit it to move supertypes or subtypes of that class into other packages, then those `writeReplace` methods are no longer treated as overrides, and serialization breaks.

Normally, optimizers are smart enough not to break overrides. However, in the case of serialization, `writeReplace` is invoked reflectively inside the serialization system, which also performs its own access checks. This means that some optimizers don't know that `writeReplace` is called. So, even if they are instructed to keep all methods of that name, they don't know to preserve their override-ness.

In the particular case we were seeing, `ImmutableCollection` and `RegularImmutableList` were ending up in the default package, while `ImmutableList` was staying in `common.collect`. That might sound like it should still work: Serializing an `RegularImmutableList` instance should invoke the package-private `ImmutableCollection.writeReplace` method. However, if I'm understanding correctly, Java serialization is buggy (or at least surprising relative to normal inheritance): `ObjectStreamClass.getInheritableMethod`, upon finding the package-private `ImmutableList.writeReplace` method (which is _not_ relevant to `RegularImmutableList` anymore because it now appears in a different package), [gives up on looking for `writeReplace`](https://github.com/openjdk/jdk/blob/570dffb104fc37f053fcdf38a24aa2cabdc921c0/src/java.base/share/classes/java/io/ObjectStreamClass.java#L1518) instead of continuing to scan for `writeReplace` in any grandparent+ types that live in the same package.

RELNOTES=n/a
PiperOrigin-RevId: 586777576

android/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java

@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2023 The Guava Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.common.collect;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+import static java.lang.reflect.Modifier.PRIVATE;
+import static java.lang.reflect.Modifier.PROTECTED;
+import static java.lang.reflect.Modifier.PUBLIC;
+import static java.util.Arrays.asList;
+
+import com.google.common.base.Optional;
+import com.google.common.reflect.ClassPath;
+import com.google.common.reflect.ClassPath.ClassInfo;
+import com.google.common.reflect.TypeToken;
+import java.lang.reflect.Method;
+import junit.framework.TestCase;
+
+/**
+ * Tests that all package-private {@code writeReplace} methods are overridden in any existing
+ * subclasses. Without such overrides, optimizers might put a {@code writeReplace}-containing class
+ * and its subclass in different packages, causing the serialization system to fail to invoke {@code
+ * writeReplace} when serializing an instance of the subclass. For an example of this problem, see
+ * b/310253115.
+ */
+public class WriteReplaceOverridesTest extends TestCase {
+  private static final ImmutableSet<String> GUAVA_PACKAGES =
+      FluentIterable.of(
+              "base",
+              "cache",
+              "collect",
+              "escape",
+              "eventbus",
+              "graph",
+              "hash",
+              "html",
+              "io",
+              "math",
+              "net",
+              "primitives",
+              "reflect",
+              "util.concurrent",
+              "xml")
+          .transform("com.google.common."::concat)
+          .toSet();
+
+  public void testClassesHaveOverrides() throws Exception {
+    for (ClassInfo info : ClassPath.from(getClass().getClassLoader()).getAllClasses()) {
+      if (!GUAVA_PACKAGES.contains(info.getPackageName())) {
+        continue;
+      }
+      if (info.getName().endsWith("GwtSerializationDependencies")) {
+        continue; // These classes exist only for the GWT compiler, not to be used.
+      }
+      if (
+      /*
+       * At least one of the classes nested inside TypeResolverTest triggers a bug under older JDKs:
+       * https://bugs.openjdk.org/browse/JDK-8215328 -> https://bugs.openjdk.org/browse/JDK-8215470
+       * https://github.com/google/guava/blob/4f12c5891a7adedbaa1d99fc9f77d8cc4e9da206/guava-tests/test/com/google/common/reflect/TypeResolverTest.java#L201
+       */
+      info.getName().contains("TypeResolverTest")
+          /*
+           * And at least one of the classes inside TypeTokenTest ends up with a null value in
+           * TypeMappingIntrospector.mappings. That happens only under older JDKs, too, so it may
+           * well be a JDK bug.
+           */
+          || info.getName().contains("TypeTokenTest")
+      /*
+       * Luckily, we don't care about analyzing tests at all. We'd skip them all if we could do so
+       * trivially, but it's enough to skip these ones.
+       */
+      ) {
+        continue;
+      }
+      Class<?> clazz = info.load();
+      try {
+        Method unused = clazz.getDeclaredMethod("writeReplace");
+        continue; // It overrides writeReplace, so it's safe.
+      } catch (NoSuchMethodException e) {
+        // This is a class whose supertypes we want to examine. We'll do that below.
+      }
+      Optional<Class<?>> supersWithPackagePrivateWriteReplace =
+          FluentIterable.from(TypeToken.of(clazz).getTypes())
+              .transform(TypeToken::getRawType)
+              .transformAndConcat(c -> asList(c.getDeclaredMethods()))
+              .firstMatch(
+                  m ->
+                      m.getName().equals("writeReplace")
+                          && m.getParameterTypes().length == 0
+                          // Only package-private methods are a problem.
+                          && (m.getModifiers() & (PUBLIC | PROTECTED | PRIVATE)) == 0)
+              .transform(Method::getDeclaringClass);
+      if (!supersWithPackagePrivateWriteReplace.isPresent()) {
+        continue;
+      }
+      assertWithMessage(
+              "To help optimizers, any class that inherits a package-private writeReplace() method"
+                  + " should override that method.\n"
+                  + "(An override that delegates to the supermethod is fine.)\n"
+                  + "%s has no such override despite inheriting writeReplace() from %s",
+              clazz.getName(), supersWithPackagePrivateWriteReplace.get().getName())
+          .fail();
+    }
+  }
+}

android/guava/src/com/google/common/collect/CartesianList.java

@@ -17,6 +17,8 @@
 import static com.google.common.base.Preconditions.checkElementIndex;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.common.annotations.GwtIncompatible;
+import com.google.common.annotations.J2ktIncompatible;
 import com.google.common.math.IntMath;
 import java.util.AbstractList;
 import java.util.List;
@@ -132,6 +134,15 @@ public E get(int axis) {
       boolean isPartialView() {
         return true;
       }
+
+      // redeclare to help optimizers with b/310253115
+      @SuppressWarnings("RedundantOverride")
+      @J2ktIncompatible // serialization
+      @Override
+      @GwtIncompatible // serialization
+      Object writeReplace() {
+        return super.writeReplace();
+      }
     };
   }
 

android/guava/src/com/google/common/collect/ContiguousSet.java

@@ -20,6 +20,7 @@
 
 import com.google.common.annotations.GwtCompatible;
 import com.google.common.annotations.GwtIncompatible;
+import com.google.common.annotations.J2ktIncompatible;
 import com.google.errorprone.annotations.DoNotCall;
 import java.util.Collections;
 import java.util.NoSuchElementException;
@@ -259,4 +260,13 @@ public String toString() {
   public static <E> ImmutableSortedSet.Builder<E> builder() {
     throw new UnsupportedOperationException();
   }
+
+  // redeclare to help optimizers with b/310253115
+  @SuppressWarnings("RedundantOverride")
+  @J2ktIncompatible // serialization
+  @Override
+  @GwtIncompatible // serialization
+  Object writeReplace() {
+    return super.writeReplace();
+  }
 }

android/guava/src/com/google/common/collect/DenseImmutableTable.java

@@ -17,6 +17,8 @@
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.common.annotations.GwtIncompatible;
+import com.google.common.annotations.J2ktIncompatible;
 import com.google.common.collect.ImmutableMap.IteratorBasedImmutableMap;
 import com.google.errorprone.annotations.Immutable;
 import com.google.j2objc.annotations.WeakOuter;
@@ -144,6 +146,15 @@ protected Entry<K, V> computeNext() {
         }
       };
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @J2ktIncompatible // serialization
+    @Override
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   private final class Row extends ImmutableArrayMap<C, V> {
@@ -169,6 +180,15 @@ V getValue(int keyIndex) {
     boolean isPartialView() {
       return true;
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   private final class Column extends ImmutableArrayMap<R, V> {
@@ -194,6 +214,15 @@ V getValue(int keyIndex) {
     boolean isPartialView() {
       return true;
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   @WeakOuter
@@ -216,6 +245,15 @@ ImmutableMap<C, V> getValue(int keyIndex) {
     boolean isPartialView() {
       return false;
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   @WeakOuter
@@ -238,6 +276,15 @@ ImmutableMap<R, V> getValue(int keyIndex) {
     boolean isPartialView() {
       return false;
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   @Override
@@ -285,7 +332,9 @@ V getValue(int index) {
   }
 
   @Override
-  SerializedForm createSerializedForm() {
+  @J2ktIncompatible // serialization
+  @GwtIncompatible // serialization
+  Object writeReplace() {
     return SerializedForm.create(this, cellRowIndices, cellColumnIndices);
   }
 }

android/guava/src/com/google/common/collect/DescendingImmutableSortedMultiset.java

@@ -15,6 +15,7 @@
 package com.google.common.collect;
 
 import com.google.common.annotations.GwtIncompatible;
+import com.google.common.annotations.J2ktIncompatible;
 import javax.annotation.CheckForNull;
 
 /**
@@ -83,4 +84,12 @@ public ImmutableSortedMultiset<E> tailMultiset(E lowerBound, BoundType boundType
   boolean isPartialView() {
     return forward.isPartialView();
   }
+
+  // redeclare to help optimizers with b/310253115
+  @SuppressWarnings("RedundantOverride")
+  @Override
+  @J2ktIncompatible // serialization
+  Object writeReplace() {
+    return super.writeReplace();
+  }
 }

android/guava/src/com/google/common/collect/DescendingImmutableSortedSet.java

@@ -17,6 +17,7 @@
 package com.google.common.collect;
 
 import com.google.common.annotations.GwtIncompatible;
+import com.google.common.annotations.J2ktIncompatible;
 import javax.annotation.CheckForNull;
 
 /**
@@ -121,4 +122,12 @@ int indexOf(@CheckForNull Object target) {
   boolean isPartialView() {
     return forward.isPartialView();
   }
+
+  // redeclare to help optimizers with b/310253115
+  @SuppressWarnings("RedundantOverride")
+  @Override
+  @J2ktIncompatible // serialization
+  Object writeReplace() {
+    return super.writeReplace();
+  }
 }

android/guava/src/com/google/common/collect/ImmutableCollection.java

@@ -21,6 +21,7 @@
 import static com.google.common.collect.ObjectArrays.checkElementsNotNull;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.common.annotations.GwtIncompatible;
 import com.google.common.annotations.J2ktIncompatible;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.DoNotCall;
@@ -381,6 +382,7 @@ int copyIntoArray(@Nullable Object[] dst, int offset) {
   }
 
   @J2ktIncompatible // serialization
+  @GwtIncompatible // serialization
   Object writeReplace() {
     // We serialize by default to ImmutableList, the simplest thing that works.
     return new ImmutableList.SerializedForm(toArray());

android/guava/src/com/google/common/collect/ImmutableList.java

@@ -26,6 +26,7 @@
 import static com.google.common.collect.RegularImmutableList.EMPTY;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.common.annotations.GwtIncompatible;
 import com.google.common.annotations.J2ktIncompatible;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.DoNotCall;
@@ -489,6 +490,15 @@ public ImmutableList<E> subList(int fromIndex, int toIndex) {
     boolean isPartialView() {
       return true;
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   /**
@@ -638,6 +648,15 @@ public int size() {
     boolean isPartialView() {
       return forwardList.isPartialView();
     }
+
+    // redeclare to help optimizers with b/310253115
+    @SuppressWarnings("RedundantOverride")
+    @Override
+    @J2ktIncompatible // serialization
+    @GwtIncompatible // serialization
+    Object writeReplace() {
+      return super.writeReplace();
+    }
   }
 
   @Override
@@ -684,6 +703,7 @@ private void readObject(ObjectInputStream stream) throws InvalidObjectException
 
   @Override
   @J2ktIncompatible // serialization
+  @GwtIncompatible // serialization
   Object writeReplace() {
     return new SerializedForm(toArray());
   }