core: Remove builder class signature rewriter

api/src/main/java/io/grpc/ForwardingChannelBuilder.java

@@ -16,8 +16,6 @@
 
 package io.grpc;
 
-import com.google.common.base.MoreObjects;
-import com.google.errorprone.annotations.DoNotCall;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
@@ -28,40 +26,27 @@
  * A {@link ManagedChannelBuilder} that delegates all its builder methods to another builder by
  * default.
  *
- * @param <T> The type of the subclass extending this abstract class.
+ * <p>Important! Use {@link ForwardingChannelBuilder2} instead!
+ *
+ * <p>This class mistakenly used {@code <T extends ForwardingChannelBuilder<T>>} which causes
+ * return types to be {@link ForwardingChannelBuilder} instead of {@link ManagedChannelBuilder}.
+ * This pollutes the ABI of its subclasses with undesired method signatures.
+ * {@link ForwardingChannelBuilder2} generates correct return types; use it instead.
  *
+ * @param <T> The type of the subclass extending this abstract class.
  * @since 1.7.0
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/3363")
 public abstract class ForwardingChannelBuilder<T extends ForwardingChannelBuilder<T>>
-    extends ManagedChannelBuilder<T> {
+    extends ForwardingChannelBuilder2<T> {
+  // TODO(sergiitk): deprecate after stabilizing
 
   /**
    * The default constructor.
    */
-  protected ForwardingChannelBuilder() {}
-
-  /**
-   * This method serves to force sub classes to "hide" this static factory.
-   */
-  @DoNotCall("Unsupported")
-  public static ManagedChannelBuilder<?> forAddress(String name, int port) {
-    throw new UnsupportedOperationException("Subclass failed to hide static factory");
+  protected ForwardingChannelBuilder() {
   }
 
-  /**
-   * This method serves to force sub classes to "hide" this static factory.
-   */
-  @DoNotCall("Unsupported")
-  public static ManagedChannelBuilder<?> forTarget(String target) {
-    throw new UnsupportedOperationException("Subclass failed to hide static factory");
-  }
-
-  /**
-   * Returns the delegated {@code ManagedChannelBuilder}.
-   */
-  protected abstract ManagedChannelBuilder<?> delegate();
-
   @Override
   public T directExecutor() {
     delegate().directExecutor();
@@ -249,24 +234,11 @@ public T disableServiceConfigLookUp() {
     return thisT();
   }
 
-  /**
-   * Returns the {@link ManagedChannel} built by the delegate by default. Overriding method can
-   * return different value.
-   */
-  @Override
-  public ManagedChannel build() {
-    return delegate().build();
-  }
-
-  @Override
-  public String toString() {
-    return MoreObjects.toStringHelper(this).add("delegate", delegate()).toString();
-  }
-
   /**
    * Returns the correctly typed version of the builder.
    */
   protected final T thisT() {
+    // TODO(sergiitk): make private when stabilizing.
     @SuppressWarnings("unchecked")
     T thisT = (T) this;
     return thisT;

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java → api/src/main/java/io/grpc/ForwardingChannelBuilder2.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,58 +14,46 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
 import com.google.errorprone.annotations.DoNotCall;
-import io.grpc.BinaryLog;
-import io.grpc.ClientInterceptor;
-import io.grpc.CompressorRegistry;
-import io.grpc.DecompressorRegistry;
-import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
-import io.grpc.NameResolver;
-import io.grpc.ProxyDetector;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 
 /**
- * Temporarily duplicates {@link io.grpc.ForwardingChannelBuilder} to fix ABI backward
- * compatibility.
+ * A {@link ManagedChannelBuilder} that delegates all its builder methods to another builder by
+ * default.
  *
- * @param <T> The concrete type of this builder.
- * @see <a href="https://github.com/grpc/grpc-java/issues/7211">grpc/grpc-java#7211</a>
+ * <p>Always choose this over {@link ForwardingChannelBuilder}, because
+ * {@link ForwardingChannelBuilder2} is ABI-safe.
+ *
+ * @param <T> The type of the subclass extending this abstract class.
+ * @since 1.59.0
  */
-public abstract class AbstractManagedChannelImplBuilder
-    <T extends AbstractManagedChannelImplBuilder<T>> extends ManagedChannelBuilder<T> {
-
-  /**
-   * Added for ABI compatibility.
-   *
-   * <p>See details in {@link #maxInboundMessageSize(int)}.
-   * TODO(sergiitk): move back to concrete classes as a private field, when this class is removed.
-   */
-  protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
+@ExperimentalApi("https://github.com/grpc/grpc-java/issues/10585")
+public abstract class ForwardingChannelBuilder2<T extends ManagedChannelBuilder<T>>
+    extends ManagedChannelBuilder<T> {
 
   /**
    * The default constructor.
    */
-  protected AbstractManagedChannelImplBuilder() {}
+  protected ForwardingChannelBuilder2() {
+  }
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forAddress(String name, int port) {
     throw new UnsupportedOperationException("Subclass failed to hide static factory");
   }
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forTarget(String target) {
@@ -170,31 +158,7 @@ public T idleTimeout(long value, TimeUnit unit) {
 
   @Override
   public T maxInboundMessageSize(int max) {
-    /*
-     Why this method is not delegating, as the rest of the methods?
-
-     In refactoring described in #7211, the implementation of #maxInboundMessageSize(int)
-     (and its corresponding field) was pulled down from internal AbstractManagedChannelImplBuilder
-     to concrete classes that actually enforce this setting. For the same reason, it wasn't ported
-     to ManagedChannelImplBuilder (the #delegate()).
-
-     Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility,
-     and temporarily turned into a ForwardingChannelBuilder, ref PR #7564. Eventually it will
-     be deleted, after a period with "bridge" ABI solution introduced in #7834.
-
-     However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of
-     #maxInboundMessageSize(int) implemented by the concrete classes backward incompatible:
-     pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder,
-     and not concrete classes, ref #8313.
-
-     The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it.
-     To fix method's ABI, we temporary reintroduce it to the original layer it was removed from:
-     AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term
-     ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer
-     necessary, and both will perish with removing AbstractManagedChannelImplBuilder.
-    */
-    Preconditions.checkArgument(max >= 0, "negative max");
-    maxInboundMessageSize = max;
+    delegate().maxInboundMessageSize(max);
     return thisT();
   }
 
@@ -305,7 +269,7 @@ public String toString() {
   /**
    * Returns the correctly typed version of the builder.
    */
-  protected final T thisT() {
+  private T thisT() {
     @SuppressWarnings("unchecked")
     T thisT = (T) this;
     return thisT;

core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java → api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,33 +14,30 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 
 import com.google.common.base.Defaults;
-import com.google.common.collect.ImmutableSet;
-import io.grpc.ForwardingTestUtil;
-import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.Collections;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /**
- * Unit tests for {@link AbstractManagedChannelImplBuilder}.
+ * Unit tests for {@link ForwardingChannelBuilder2}.
  */
 @RunWith(JUnit4.class)
-public class AbstractManagedChannelImplBuilderTest {
+public class ForwardingChannelBuilder2Test {
   private final ManagedChannelBuilder<?> mockDelegate = mock(ManagedChannelBuilder.class);
 
-  private final AbstractManagedChannelImplBuilder<?> testChannelBuilder = new TestBuilder();
+  private final ForwardingChannelBuilder2<?> testChannelBuilder = new TestBuilder();
 
-  private final class TestBuilder extends AbstractManagedChannelImplBuilder<TestBuilder> {
+  private final class TestBuilder extends ForwardingChannelBuilder2<TestBuilder> {
     @Override
     protected ManagedChannelBuilder<?> delegate() {
       return mockDelegate;
@@ -53,8 +50,7 @@ public void allMethodsForwarded() throws Exception {
         ManagedChannelBuilder.class,
         mockDelegate,
         testChannelBuilder,
-        // maxInboundMessageSize is the only method that shouldn't forward.
-        ImmutableSet.of(ManagedChannelBuilder.class.getMethod("maxInboundMessageSize", int.class)),
+        Collections.emptyList(),
         new ForwardingTestUtil.ArgumentProvider() {
           @Override
           public Object get(Method method, int argPos, Class<?> clazz) {
@@ -67,15 +63,6 @@ public Object get(Method method, int argPos, Class<?> clazz) {
         });
   }
 
-  @Test
-  public void testMaxInboundMessageSize() {
-    assertThat(testChannelBuilder.maxInboundMessageSize)
-        .isEqualTo(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE);
-
-    testChannelBuilder.maxInboundMessageSize(42);
-    assertThat(testChannelBuilder.maxInboundMessageSize).isEqualTo(42);
-  }
-
   @Test
   public void allBuilderMethodsReturnThis() throws Exception {
     for (Method method : ManagedChannelBuilder.class.getDeclaredMethods()) {

core/build.gradle

@@ -14,10 +14,6 @@ plugins {
     id "ru.vyarus.animalsniffer"
 }
 
-import static java.nio.charset.StandardCharsets.US_ASCII;
-
-import com.google.common.primitives.Bytes;
-
 description = 'gRPC: Core'
 
 dependencies {
@@ -73,55 +69,6 @@ animalsniffer {
 components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) { skip() }
 components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() }
 
-import net.ltgt.gradle.errorprone.CheckSeverity
-
-def replaceBytes(byte[] haystack, byte[] needle, byte[] replacement) {
-    int i = Bytes.indexOf(haystack, needle);
-    assert i != -1;
-    byte[] result = new byte[haystack.length - needle.length + replacement.length];
-    System.arraycopy(haystack, 0, result, 0, i);
-    System.arraycopy(replacement, 0, result, i, replacement.length);
-    System.arraycopy(haystack, i + needle.length, result, i + replacement.length, haystack.length - i - needle.length);
-    return result;
-}
-
-def bigEndianShortBytes(int value) {
-    return [value >> 8, value & 0xFF] as byte[];
-}
-
-def replaceConstant(File file, String needle, String replacement) {
-    // CONSTANT_Utf8_info. https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4.7
-    byte[] needleBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(needle.length()), needle.getBytes(US_ASCII));
-    byte[] replacementBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(replacement.length()), replacement.getBytes(US_ASCII));
-    file.setBytes(replaceBytes(file.getBytes(), needleBytes, replacementBytes));
-}
-
-plugins.withId("java") {
-    tasks.named("compileJava").configure {
-        doLast {
-            // Replace value of Signature Attribute.
-            // https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.9
-	    //
-	    // Have new compilations compile against a public class, without breaking the internal
-	    // ABI for the moment. After giving users some time to recompile, this should be removed
-	    // and #7211 can continue. When this is removed, at the very least the generics need to
-	    // be changed to avoid <? extends io.grpc.internal.*>.
-            project.replaceConstant(
-                destinationDirectory.file(
-                    "io/grpc/internal/AbstractManagedChannelImplBuilder.class").get().getAsFile(),
-                "<T:Lio/grpc/internal/AbstractManagedChannelImplBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;",
-                "<T:Lio/grpc/ManagedChannelBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;");
-            project.replaceConstant(
-                destinationDirectory.file(
-                    "io/grpc/internal/AbstractServerImplBuilder.class").get().getAsFile(),
-                "<T:Lio/grpc/internal/AbstractServerImplBuilder<TT;>;>Lio/grpc/ServerBuilder<TT;>;",
-                "<T:Lio/grpc/ServerBuilder<TT;>;>Lio/grpc/ServerBuilder<TT;>;");
-        }
-    }
-}
-
 tasks.register("versionFile") {
     doLast {
         new File(buildDir, "version").write("${project.version}\n")