Merge pull request #2943 from square/bquenaudon.2024-05-15.claiming

Improve claimedPath and claimedDefinitions
square · May 16, 2024 · b20024e · b20024e
2 parents 9fca82c + 6020f06
commit b20024e
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 12 deletions.
diff --git a/wire-compiler/src/test/java/com/squareup/wire/schema/WireRunTest.kt b/wire-compiler/src/test/java/com/squareup/wire/schema/WireRunTest.kt
@@ -1200,6 +1200,78 @@ class WireRunTest {
     }
   }
 
+  @Test fun crashWhenExtendGenerationConflicts() {
+    fs.add(
+      "protos/zero/zero.proto",
+      """
+          |package zero;
+          |option java_package = "same.package";
+          |import "google/protobuf/descriptor.proto";
+          |extend google.protobuf.FieldOptions {
+          |  optional string documentation_url = 60001;
+          |}
+          |extend google.protobuf.MessageOptions {
+          |  optional string documentation_url = 60002;
+          |}
+          |
+      """.trimMargin(),
+    )
+    val wireRun = WireRun(
+      sourcePath = listOf(Location.get("protos")),
+      targets = listOf(
+        JavaTarget(
+          outDirectory = "generated/java",
+          emitDeclaredOptions = true,
+          emitAppliedOptions = true,
+        ),
+      ),
+    )
+
+    try {
+      wireRun.execute(fs, logger)
+      fail()
+    } catch (expected: IllegalStateException) {
+      assertThat(expected).hasMessage(
+        "Same file generated/java/same/package/DocumentationUrlOption.java is getting generated for different extends:\n" +
+          "  FieldOptions.documentation_url at protos/zero/zero.proto:4:1\n" +
+          "  MessageOptions.documentation_url at protos/zero/zero.proto:7:1",
+      )
+    }
+  }
+
+  @Test fun javaDoesNotClaimServices() {
+    writeRedProto()
+    fs.add(
+      "routes/src/main/proto/squareup/routes1/route.proto",
+      """
+          |syntax = "proto2";
+          |package squareup.routes;
+          |option java_package = "same.package";
+          |import "squareup/colors/red.proto";
+          |service Route {
+          |  rpc GetUpdatedRed(squareup.colors.Red) returns (squareup.colors.Red) {}
+          |}
+      """.trimMargin(),
+    )
+    val wireRun = WireRun(
+      sourcePath = listOf(Location.get("routes/src/main/proto")),
+      protoPath = listOf(Location.get("colors/src/main/proto")),
+      targets = listOf(
+        JavaTarget(outDirectory = "generate/java"),
+        KotlinTarget(outDirectory = "generated/kt"),
+      ),
+    )
+
+    wireRun.execute(fs, logger)
+    // We had a bug where the Java target, even though it doesn't generate them, would claim the
+    // services and prevent the Kotlin target to generate them. Asserting that we have services
+    // generated in Kotlin confirms the fix.
+    assertThat(fs.findFiles("generated")).containsExactlyInAnyOrderAsRelativePaths(
+      "generated/kt/same/package/GrpcRouteClient.kt",
+      "generated/kt/same/package/RouteClient.kt",
+    )
+  }
+
   @Test fun crashWhenServiceGenerationConflicts() {
     writeRedProto()
     fs.add(

diff --git a/wire-schema/api/wire-schema.api b/wire-schema/api/wire-schema.api
@@ -50,6 +50,7 @@ public final class com/squareup/wire/schema/ClaimedDefinitions {
 
 public final class com/squareup/wire/schema/ClaimedPaths {
 	public fun <init> ()V
+	public final fun claim (Lokio/Path;Lcom/squareup/wire/schema/Extend;)V
 	public final fun claim (Lokio/Path;Lcom/squareup/wire/schema/Service;)V
 	public final fun claim (Lokio/Path;Lcom/squareup/wire/schema/Type;)V
 }

diff --git a/wire-schema/api/wire-schema.klib.api b/wire-schema/api/wire-schema.klib.api
@@ -659,6 +659,7 @@ final class com.squareup.wire.schema/ClaimedDefinitions { // com.squareup.wire.s
 }
 final class com.squareup.wire.schema/ClaimedPaths { // com.squareup.wire.schema/ClaimedPaths|null[0]
     constructor <init>() // com.squareup.wire.schema/ClaimedPaths.<init>|<init>(){}[0]
+    final fun claim(okio/Path, com.squareup.wire.schema/Extend) // com.squareup.wire.schema/ClaimedPaths.claim|claim(okio.Path;com.squareup.wire.schema.Extend){}[0]
     final fun claim(okio/Path, com.squareup.wire.schema/Service) // com.squareup.wire.schema/ClaimedPaths.claim|claim(okio.Path;com.squareup.wire.schema.Service){}[0]
     final fun claim(okio/Path, com.squareup.wire.schema/Type) // com.squareup.wire.schema/ClaimedPaths.claim|claim(okio.Path;com.squareup.wire.schema.Type){}[0]
 }

diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ClaimedPaths.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ClaimedPaths.kt
@@ -49,6 +49,19 @@ class ClaimedPaths {
     paths[path] = errorMessage
   }
 
+  /** Tracks that [extend] has been generated to [path]. */
+  fun claim(path: Path, extend: Extend) {
+    val errorMessage = extend.asErrorMessage()
+    val existingEntry = paths[path]
+    check(existingEntry == null) {
+      "Same file $path is getting generated for different extends:\n" +
+        "  ${existingEntry}\n" +
+        "  ${extend.asErrorMessage()}"
+    }
+    paths[path] = errorMessage
+  }
+
+  private fun Extend.asErrorMessage(): String = "${type!!.simpleName}.${fields.joinToString()} at $location"
   private fun Type.asErrorMessage(): String = "${type.simpleName} at $location"
   private fun Service.asErrorMessage(): String = "${type.simpleName} at $location"
 }
diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SchemaHandler.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SchemaHandler.kt
@@ -153,10 +153,9 @@ abstract class SchemaHandler {
 
       if (generatedFilePath != null) {
         claimedPaths.claim(generatedFilePath, type)
+        // We don't let other targets handle this one.
+        claimedDefinitions?.claim(type)
       }
-
-      // We don't let other targets handle this one.
-      claimedDefinitions?.claim(type)
     }
 
     val services = protoFile.services
@@ -166,12 +165,13 @@ abstract class SchemaHandler {
     for (service in services) {
       val generatedFilePaths = handle(service, context)
 
-      for (generatedFilePath in generatedFilePaths) {
-        claimedPaths.claim(generatedFilePath, service)
+      if (generatedFilePaths.isNotEmpty()) {
+        for (generatedFilePath in generatedFilePaths) {
+          claimedPaths.claim(generatedFilePath, service)
+        }
+        // We don't let other targets handle this one.
+        claimedDefinitions?.claim(service)
       }
-
-      // We don't let other targets handle this one.
-      claimedDefinitions?.claim(service)
     }
 
     // TODO(jwilson): extend emitting rules to support include/exclude of extension fields.
@@ -181,11 +181,13 @@ abstract class SchemaHandler {
         claimedDefinitions == null || extend.member(field) !in claimedDefinitions
       }
       .forEach { (extend, field) ->
-        // TODO(Benoît) claim path.
-        handle(extend, field, context)
+        val generatedFilePath = handle(extend, field, context)
 
-        // We don't let other targets handle this one.
-        claimedDefinitions?.claim(extend.member(field))
+        if (generatedFilePath != null) {
+          claimedPaths.claim(generatedFilePath, extend)
+          // We don't let other targets handle this one.
+          claimedDefinitions?.claim(extend.member(field))
+        }
       }
   }