From 42b1c49188b410ea3b81ae54a6de086b27f8144e Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Mon, 18 Apr 2022 12:23:06 -0700
Subject: [PATCH] Resolve javadocAll errors (#2195)

Motivation:
The release.sh script runs `javadocAll` task on JDK17 which
found a few errors.

Motivation:
- JDK8 javadoc generation has an JDK8 erroneous javadoc
  `warning - Tag @link: can't find` from generated code.
  Workaround the issue by using canonical type name in the link.
- Remove `h3` usage in Test sources
- Remove exclude of generated code that is linked in javadocs
  from non-generated code.
---
 .../concurrent/api/TestCompletable.java       |  4 +-
 .../concurrent/api/TestPublisher.java         |  3 +-
 .../concurrent/api/TestSingle.java            |  4 +-
 servicetalk-grpc-health/build.gradle          |  4 --
 .../grpc/protoc/FileDescriptor.java           | 14 +++--
 .../grpc/protoc/GenerationContext.java        | 32 ++++++++---
 .../io/servicetalk/grpc/protoc/Generator.java | 57 ++++++++++---------
 .../grpc/protoc/ServiceClassBuilder.java      | 28 +++++++++
 .../io/servicetalk/grpc/protoc/Words.java     |  1 +
 9 files changed, 99 insertions(+), 48 deletions(-)
 create mode 100644 servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/ServiceClassBuilder.java

diff --git a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestCompletable.java b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestCompletable.java
index 996b6b753f..badce3f177 100644
--- a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestCompletable.java
+++ b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestCompletable.java
@@ -42,8 +42,8 @@
  * invoked for every {@link #subscribe(Subscriber)} invocation, and the result is used as the delegate for subsequent
  * {@link #onSubscribe(Cancellable)}, {@link #onComplete()}, and
  * {@link #onError(Throwable)} calls. See {@link Builder} for more information.
- *
- * <h3>Defaults</h3>
+ * <p>
+ * Defaults:
  * <ul>
  * <li>Allows sequential but not concurrent subscribers.</li>
  * <li>Sends {@link #onSubscribe(Cancellable)} automatically when subscribed to.</li>
diff --git a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestPublisher.java b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestPublisher.java
index 236e45f8a8..916ceb847c 100644
--- a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestPublisher.java
+++ b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestPublisher.java
@@ -41,7 +41,8 @@
  * invoked for every {@link #subscribe(Subscriber)} invocation, and the result is used as the delegate for subsequent
  * {@link #onSubscribe(Subscription)}, {@link #onNext(Object[])}, {@link #onComplete()}, and
  * {@link #onError(Throwable)} calls. See {@link Builder} for more information.
- * <h3>Defaults</h3>
+ * <p>
+ * Defaults:
  * <ul>
  *     <li>Allows sequential but not concurrent subscribers.</li>
  *     <li>Asserts that {@link #onNext(Object[])} is not called without sufficient demand.</li>
diff --git a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestSingle.java b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestSingle.java
index 7ad16082a4..d16613b645 100644
--- a/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestSingle.java
+++ b/servicetalk-concurrent-api/src/testFixtures/java/io/servicetalk/concurrent/api/TestSingle.java
@@ -41,8 +41,8 @@
  * invoked for every {@link #subscribe(Subscriber)} invocation, and the result is used as the delegate for subsequent
  * {@link #onSubscribe(Cancellable)}, {@link #onSuccess(Object)}, and
  * {@link #onError(Throwable)} calls. See {@link Builder} for more information.
- *
- * <h3>Defaults</h3>
+ * <p>
+ * Defaults:
  * <ul>
  *     <li>Allows sequential but not concurrent subscribers.</li>
  *     <li>Sends {@link #onSubscribe(Cancellable)} automatically when subscribed to.</li>
diff --git a/servicetalk-grpc-health/build.gradle b/servicetalk-grpc-health/build.gradle
index 607681f121..7dbc374e57 100644
--- a/servicetalk-grpc-health/build.gradle
+++ b/servicetalk-grpc-health/build.gradle
@@ -82,10 +82,6 @@ sourceSets {
   }
 }
 
-javadoc {
-  exclude "**/${generatedJavaPkg.replace('.', '/')}/**"
-}
-
 protobuf {
   protoc {
     artifact = "com.google.protobuf:protoc:$protobufVersion"
diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
index a6c60812dd..ac91951800 100644
--- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
@@ -62,6 +62,7 @@ final class FileDescriptor implements GenerationContext {
     private final boolean multipleClassFiles;
     private final String javaPackageName;
     private final String outerClassName;
+    private final String javaOuterScope;
     @Nullable
     private final String typeNameSuffix;
     private final List<TypeSpec.Builder> serviceClassBuilders;
@@ -96,6 +97,7 @@ final class FileDescriptor implements GenerationContext {
             javaPackageName = inferJavaPackageName(protoPackageName, sanitizedProtoFileName);
             outerClassName = inferOuterClassName(sanitizedProtoFileName, protoFile);
         }
+        javaOuterScope = multipleClassFiles ? javaPackageName() : javaPackageName() + '.' + outerJavaClassName();
         reservedJavaTypeName.add(outerClassName);
 
         serviceClassBuilders = new ArrayList<>(protoFile.getServiceCount());
@@ -118,8 +120,7 @@ List<ServiceDescriptorProto> protoServices() {
     Map<String, ClassName> messageTypesMap() {
         final Map<String, ClassName> messageTypesMap = new HashMap<>(protoFile.getMessageTypeCount());
         addMessageTypes(protoFile.getMessageTypeList(), protoPackageName != null ? '.' + protoPackageName : null,
-                multipleClassFiles ? javaPackageName() : javaPackageName() + '.' + outerJavaClassName(),
-                messageTypesMap);
+                javaOuterScope, messageTypesMap);
         return messageTypesMap;
     }
 
@@ -158,7 +159,12 @@ public String deconflictJavaTypeName(final String name) {
     }
 
     @Override
-    public TypeSpec.Builder newServiceClassBuilder(final ServiceDescriptorProto serviceProto) {
+    public String deconflictJavaTypeName(final String outerClassName, final String name) {
+        return javaOuterScope + '.' + outerClassName + '.' + deconflictJavaTypeName(sanitizeClassName(name));
+    }
+
+    @Override
+    public ServiceClassBuilder newServiceClassBuilder(final ServiceDescriptorProto serviceProto) {
         final String rawClassName = typeNameSuffix == null ? serviceProto.getName() :
                 serviceProto.getName() + typeNameSuffix;
         final String className = deconflictJavaTypeName(sanitizeClassName(rawClassName));
@@ -178,7 +184,7 @@ public TypeSpec.Builder newServiceClassBuilder(final ServiceDescriptorProto serv
 
         serviceClassBuilders.add(builder);
         protoForServiceBuilder.put(builder, serviceProto);
-        return builder;
+        return new ServiceClassBuilder(builder, className);
     }
 
     @Override
diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/GenerationContext.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/GenerationContext.java
index ab2d172392..fea28e8559 100644
--- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/GenerationContext.java
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/GenerationContext.java
@@ -17,13 +17,11 @@
 
 import com.google.protobuf.DescriptorProtos.MethodDescriptorProto;
 import com.google.protobuf.DescriptorProtos.ServiceDescriptorProto;
-import com.squareup.javapoet.TypeSpec;
 
 interface GenerationContext {
     /**
-     * Return a canonical and unique type name based upon the provided type name.
-     * The returned name will not conflict with any other identically named types
-     * in the same context.
+     * Return a canonical and unique type name based upon the provided type name. The returned name will not conflict
+     * with any other identically named types in the same context.
      *
      * @param name The Java type name to deconflict
      * @return The deconflicted, possibly unchanged, Java type name.
@@ -31,14 +29,32 @@ interface GenerationContext {
     String deconflictJavaTypeName(String name);
 
     /**
-     * Create and return the builder for a service class described by the provided
-     * descriptor. The builder will be registered for the destination file and the
-     * generator responsible for filling in the builder will
+     * Return a qualified, canonical, and unique type name based upon the provided type name. The returned name will not
+     * conflict with any other identically named types in the same context.
+     *
+     * @param outerClassName The outer class name that contains this type. This will be used to generate a
+     * more fully qualified return value.
+     * @param name The Java type name to deconflict
+     * @return Deconflicted, possibly unchanged, Java type name that is qualified with the outer java
+     * package+typename+{@code outerClassName} scope.
+     */
+    String deconflictJavaTypeName(String outerClassName, String name);
+
+    /**
+     * Create and return the builder for a service class described by the provided descriptor. The builder will be
+     * registered for the destination file and the generator responsible for filling in the builder will
      *
      * @param serviceProto The service descriptor being created.
      * @return a new builder for the service class
      */
-    TypeSpec.Builder newServiceClassBuilder(ServiceDescriptorProto serviceProto);
+    ServiceClassBuilder newServiceClassBuilder(ServiceDescriptorProto serviceProto);
 
+    /**
+     * Get the <a href="https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md">gRPC H2 path</a> for a method.
+     * @param serviceProto protobuf descriptor for the service.
+     * @param methodProto protobuf descriptor for the method.
+     * @return the <a href="https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md">gRPC H2 path</a> for a
+     * method.
+     */
     String methodPath(ServiceDescriptorProto serviceProto, MethodDescriptorProto methodProto);
 }
diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java
index 9ab5f8a71d..a837705e69 100644
--- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java
@@ -108,6 +108,7 @@
 import static io.servicetalk.grpc.protoc.Words.COMMENT_POST_TAG;
 import static io.servicetalk.grpc.protoc.Words.COMMENT_PRE_TAG;
 import static io.servicetalk.grpc.protoc.Words.Call;
+import static io.servicetalk.grpc.protoc.Words.Client;
 import static io.servicetalk.grpc.protoc.Words.Default;
 import static io.servicetalk.grpc.protoc.Words.Factory;
 import static io.servicetalk.grpc.protoc.Words.INSTANCE;
@@ -209,19 +210,24 @@ private static final class State {
         final ClassName clientClass;
         final ClassName blockingClientClass;
 
-        private State(final ServiceDescriptorProto serviceProto, String name, int serviceIndex) {
+        private State(ServiceDescriptorProto serviceProto, GenerationContext context, String outerClassName,
+                      int serviceIndex) {
             this.serviceProto = serviceProto;
             this.serviceIndex = serviceIndex;
 
+            final String sanitizedProtoName = sanitizeIdentifier(serviceProto.getName(), false);
+
             // Filled in during addServiceRpcInterfaces()
             serviceRpcInterfaces = new ArrayList<>(2 * serviceProto.getMethodCount());
-            serviceClass = ClassName.bestGuess(name);
-            blockingServiceClass = ClassName.bestGuess(Blocking + name);
+            serviceClass = ClassName.bestGuess(context.deconflictJavaTypeName(outerClassName,
+                    sanitizedProtoName + Service));
+            blockingServiceClass = serviceClass.peerClass(Blocking + serviceClass.simpleName());
             serviceFactoryClass = serviceClass.peerClass(Service + Factory);
 
             // Filled in during addClientMetadata()
             clientMetaDatas = new ArrayList<>(serviceProto.getMethodCount());
-            clientClass = ClassName.bestGuess(sanitizeIdentifier(serviceProto.getName(), false) + "Client");
+            clientClass = ClassName.bestGuess(context.deconflictJavaTypeName(outerClassName,
+                    sanitizedProtoName + Client));
             blockingClientClass = clientClass.peerClass(Blocking + clientClass.simpleName());
         }
     }
@@ -242,34 +248,30 @@ private State(final ServiceDescriptorProto serviceProto, String name, int servic
     /**
      * Generate Service class for the provided proto service descriptor.
      *
-     *
      * @param serviceProto The service descriptor.
      * @param serviceIndex The index of the service within the current file (0 based).
      * @return The service class builder
      */
     TypeSpec.Builder generate(FileDescriptor f, final ServiceDescriptorProto serviceProto, final int serviceIndex) {
-        final String name = context.deconflictJavaTypeName(
-                sanitizeIdentifier(serviceProto.getName(), false) + Service);
-        final State state = new State(serviceProto, name, serviceIndex);
-
-        final TypeSpec.Builder serviceClassBuilder = context.newServiceClassBuilder(serviceProto);
+        final ServiceClassBuilder container = context.newServiceClassBuilder(serviceProto);
+        final State state = new State(serviceProto, context, container.className, serviceIndex);
         if (printJavaDocs) {
-            serviceClassBuilder.addJavadoc("Class for $L Service", serviceProto.getName());
+            container.builder.addJavadoc("Class for $L Service", serviceProto.getName());
         }
 
-        addSerializationProviderInit(state, serviceClassBuilder);
+        addSerializationProviderInit(state, container.builder);
 
-        addServiceRpcInterfaces(state, serviceClassBuilder);
-        addServiceInterfaces(state, serviceClassBuilder);
-        addServiceFactory(state, serviceClassBuilder);
+        addServiceRpcInterfaces(state, container.builder);
+        addServiceInterfaces(state, container.builder);
+        addServiceFactory(state, container.builder);
 
-        addClientMetadata(state, serviceClassBuilder);
-        addClientInterfaces(state, serviceClassBuilder);
-        addClientFactory(state, serviceClassBuilder);
+        addClientMetadata(state, container.builder);
+        addClientInterfaces(state, container.builder);
+        addClientFactory(state, container.builder);
         // this empty class is a placeholder and get replaced with insertion point comment
-        serviceClassBuilder.addType(TypeSpec.classBuilder("__" + serviceFQN(f, serviceProto)).build());
+        container.builder.addType(TypeSpec.classBuilder("__" + serviceFQN(f, serviceProto)).build());
 
-        return serviceClassBuilder;
+        return container.builder;
     }
 
     private String serviceFQN(FileDescriptor f, ServiceDescriptorProto serviceDescriptorProto) {
@@ -615,8 +617,9 @@ private TypeSpec.Builder addServiceFactory(final State state, final TypeSpec.Bui
                 .addJavadoc(JAVADOC_PARAM + service + " the {@link $T} implementation to add." + lineSeparator(),
                         state.blockingServiceClass)
                 .addJavadoc(JAVADOC_RETURN + "this." + lineSeparator())
-                .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L($T)}." + lineSeparator(), addBlockingService,
-                        state.blockingServiceClass)
+                .addJavadoc(JAVADOC_DEPRECATED + "Use {@link #$L($L)}." + lineSeparator(),
+                        // force canonicalName to work around JDK8 erroneous javadoc `warning - Tag @link: can't find`
+                        addBlockingService, state.blockingServiceClass.canonicalName())
                 .returns(builderClass)
                 .addParameter(state.blockingServiceClass, service, FINAL)
                 .addStatement("return $L($L)", addBlockingService, service)
@@ -953,12 +956,12 @@ private TypeSpec.Builder addClientInterfaces(final State state, final TypeSpec.B
     }
 
     private TypeSpec.Builder addClientFactory(final State state, final TypeSpec.Builder serviceClassBuilder) {
-        final ClassName clientFactoryClass = state.clientClass.peerClass("Client" + Factory);
-        final ClassName defaultClientClass = clientFactoryClass.peerClass(Default + state.clientClass.simpleName());
-        final ClassName defaultBlockingClientClass = clientFactoryClass.peerClass(Default +
-                state.blockingClientClass.simpleName());
-        final ClassName clientToBlockingClientClass = clientFactoryClass.peerClass(state.clientClass.simpleName() + To +
+        final ClassName clientFactoryClass = state.clientClass.peerClass(Client + Factory);
+        final ClassName defaultClientClass = clientFactoryClass.nestedClass(Default + state.clientClass.simpleName());
+        final ClassName defaultBlockingClientClass = clientFactoryClass.nestedClass(Default +
                 state.blockingClientClass.simpleName());
+        final ClassName clientToBlockingClientClass = clientFactoryClass.nestedClass(state.clientClass.simpleName() + To
+                + state.blockingClientClass.simpleName());
 
         final TypeSpec.Builder clientFactorySpecBuilder = classBuilder(clientFactoryClass)
                 .addModifiers(PUBLIC, STATIC)
diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/ServiceClassBuilder.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/ServiceClassBuilder.java
new file mode 100644
index 0000000000..5daa024405
--- /dev/null
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/ServiceClassBuilder.java
@@ -0,0 +1,28 @@
+/*
+ * Copyright © 2022 Apple Inc. and the ServiceTalk project 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 io.servicetalk.grpc.protoc;
+
+import com.squareup.javapoet.TypeSpec;
+
+final class ServiceClassBuilder {
+    final TypeSpec.Builder builder;
+    final String className;
+
+    ServiceClassBuilder(final TypeSpec.Builder builder, final String className) {
+        this.builder = builder;
+        this.className = className;
+    }
+}
diff --git a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java
index 61bd6499be..1eb52fe8a0 100644
--- a/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java
+++ b/servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Words.java
@@ -51,6 +51,7 @@ final class Words {
     static final String methodDescriptor = "methodDescriptor";
     static final String methodDescriptors = methodDescriptor + "s";
     static final String Service = "Service";
+    static final String Client = "Client";
     static final String Blocking = "Blocking";
     static final String Builder = "Builder";
     static final String Call = "Call";