Module collision check on generated item only (#15440)

* do the module collision check on generated item only

* Revert "do the module collision check on generated item only"

This reverts commit a6a38425ab.

* added test for detectModuleCollisions

* added test to reproduce issue #15341

* Revert "Revert "do the module collision check on generated item only""

This reverts commit cd4c5a24fd.

* fix test case

* use `daml.lf.language.Reference.Module` instead

* add change log

CHANGELOG_BEGIN
Bug fix: Module name collision check performed on modules which is not to be generated #15340 #15341
CHANGELOG_END

* remove debug log

* Minor renaming

* refine name test case name

* add comment

* format

Co-authored-by: Fayi Femi-Balogun <fayimora.femibalogun@digitalasset.com>

language-support/java/codegen/BUILD.bazel

@@ -56,6 +56,7 @@ da_scala_library(
         "//daml-lf/archive:daml_lf_1.dev_archive_proto_java",
         "//daml-lf/archive:daml_lf_archive_reader",
         "//daml-lf/data",
+        "//daml-lf/language",
         "//language-support/codegen-common",
         "//language-support/java/bindings:bindings-java",
         "@maven//:ch_qos_logback_logback_classic",
@@ -75,6 +76,7 @@ da_scala_test(
     data = [
         ":test-daml.dar",
         ":test-daml-with-same-dependencies.dar",
+        ":test-daml-with-same-dependencies-but-different-target-version.dar",
     ],
     resource_strip_prefix = "language-support/java/codegen/src/test/resources/",
     resources = glob(["src/test/resources/**/*"]),
@@ -94,6 +96,7 @@ da_scala_test(
         "//daml-lf/api-type-signature",
         "//daml-lf/archive:daml_lf_archive_reader",
         "//daml-lf/data",
+        "//daml-lf/language",
         "//language-support/codegen-common",
         "//language-support/java/bindings:bindings-java",
         "@maven//:com_google_api_grpc_proto_google_common_protos",
@@ -114,6 +117,12 @@ daml_compile(
     srcs = ["src/test/daml/Foo2.daml"],
 )
 
+daml_compile(
+    name = "test-daml-with-same-dependencies-but-different-target-version",
+    srcs = ["src/test/daml/Foo2.daml"],
+    target = "1.13",
+)
+
 daml_compile(
     name = "test-contract-id-daml",
     srcs = [

language-support/java/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/CodeGenRunner.scala

@@ -6,22 +6,22 @@ package com.daml.lf.codegen
 import java.nio.file.{Files, Path}
 import java.util.concurrent.atomic.AtomicInteger
 import java.util.concurrent.{Executors, ThreadFactory}
-
 import com.daml.lf.archive.DarParser
 import com.daml.lf.codegen.backend.java.inner.{ClassForType, DecoderClass, fullyQualifiedName}
 import com.daml.lf.codegen.conf.{Conf, PackageReference}
 import com.daml.lf.codegen.dependencygraph.DependencyGraph
-import com.daml.lf.data.Ref.{PackageId, Identifier}
-import com.daml.lf.typesig.reader.{SignatureReader, Errors}
+import com.daml.lf.data.Ref.{Identifier, PackageId}
+import com.daml.lf.typesig.reader.{Errors, SignatureReader}
 import com.daml.lf.typesig.{EnvironmentSignature, PackageSignature}
 import PackageSignature.TypeDecl
-import com.squareup.javapoet.{JavaFile, ClassName}
+import com.daml.lf.language.Reference
+import com.squareup.javapoet.{ClassName, JavaFile}
 import com.typesafe.scalalogging.StrictLogging
-import org.slf4j.{LoggerFactory, Logger, MDC}
+import org.slf4j.{Logger, LoggerFactory, MDC}
 
 import scala.collection.immutable.Map
 import scala.concurrent.duration.DurationInt
-import scala.concurrent.{ExecutionContextExecutorService, Await, ExecutionContext, Future}
+import scala.concurrent.{Await, ExecutionContext, ExecutionContextExecutorService, Future}
 
 private final class CodeGenRunner(
     scope: CodeGenRunner.Scope,
@@ -164,11 +164,22 @@ object CodeGenRunner extends StrictLogging {
     for (error <- transitiveClosure.errors) {
       logger.error(error.msg)
     }
+    val generatedModuleIds: Set[Reference.Module] = (
+      transitiveClosure.serializableTypes.map(_._1) ++
+        transitiveClosure.interfaces.map(_._1)
+    ).toSet.map { id: Identifier =>
+      Reference.Module(id.packageId, id.qualifiedName.module)
+    }
 
     val resolvedSignatures = resolveRetroInterfaces(signatures)
 
     val resolvedPrefixes =
-      resolvePackagePrefixes(packagePrefixes, modulePrefixes, resolvedSignatures)
+      resolvePackagePrefixes(
+        packagePrefixes,
+        modulePrefixes,
+        resolvedSignatures,
+        generatedModuleIds,
+      )
 
     new CodeGenRunner.Scope(
       resolvedSignatures,
@@ -216,6 +227,7 @@ object CodeGenRunner extends StrictLogging {
       packagePrefixes: Map[PackageId, String],
       modulePrefixes: Map[PackageReference, String],
       signatures: Seq[PackageSignature],
+      generatedModules: Set[Reference.Module],
   ): Map[PackageId, String] = {
     val metadata: Map[PackageReference.NameVersion, PackageId] = signatures.view
       .flatMap(iface =>
@@ -250,7 +262,7 @@ object CodeGenRunner extends StrictLogging {
         }
         k -> prefix
       }.toMap
-    detectModuleCollisions(resolvedPackagePrefixes, signatures)
+    detectModuleCollisions(resolvedPackagePrefixes, signatures, generatedModules)
     resolvedPackagePrefixes
   }
 
@@ -260,6 +272,7 @@ object CodeGenRunner extends StrictLogging {
   private[codegen] def detectModuleCollisions(
       pkgPrefixes: Map[PackageId, String],
       interfaces: Seq[PackageSignature],
+      generatedModules: Set[Reference.Module],
   ): Unit = {
     val allModules: Seq[(String, PackageId)] =
       for {
@@ -268,6 +281,7 @@ object CodeGenRunner extends StrictLogging {
         module <- modules
         maybePrefix = pkgPrefixes.get(interface.packageId)
         prefixedName = maybePrefix.fold(module.toString)(prefix => s"$prefix.$module")
+        if generatedModules.contains(Reference.Module(interface.packageId, module))
       } yield prefixedName -> interface.packageId
     allModules.groupBy(_._1).foreach { case (m, grouped) =>
       if (grouped.length > 1) {

language-support/java/codegen/src/test/scala/com/digitalasset/daml/lf/codegen/CodeGenRunnerTests.scala

@@ -4,13 +4,13 @@
 package com.daml.lf.codegen
 
 import java.nio.file.Path
-
 import com.daml.bazeltools.BazelRunfiles
 import com.daml.lf.archive.DarReader
 import com.daml.lf.data.ImmArray.ImmArraySeq
 import com.daml.lf.data.Ref._
 import com.daml.lf.typesig._
 import com.daml.lf.codegen.conf.PackageReference
+import com.daml.lf.language.Reference
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.flatspec.AnyFlatSpec
 
@@ -42,6 +42,20 @@ final class CodeGenRunnerTests extends AnyFlatSpec with Matchers {
     assert(scope.toBeGenerated === Set.empty)
   }
 
+  // Test case reproducing #15341
+  it should "read interfaces from 2 DAR files with same dependencies but one with different daml compiler version" in {
+
+    val scope =
+      CodeGenRunner.configureCodeGenScope(
+        Map(testDar -> None, testDarWithSameDependenciesButDifferentTargetVersion -> None),
+        Map.empty,
+      )
+
+    assert(scope.signatures.length === 28)
+    assert(scope.packagePrefixes === Map.empty)
+    assert(scope.toBeGenerated === Set.empty)
+  }
+
   it should "read interfaces from a single DAR file with a prefix" in {
 
     val scope = CodeGenRunner.configureCodeGenScope(Map(testDar -> Some("PREFIX")), Map.empty)
@@ -54,38 +68,75 @@ final class CodeGenRunnerTests extends AnyFlatSpec with Matchers {
 
   behavior of "detectModuleCollisions"
 
+  private def moduleIdSet(signatures: Seq[PackageSignature]): Set[Reference.Module] = {
+    (for {
+      s <- signatures
+      module <- s.typeDecls.keySet.map(_.module)
+    } yield Reference.Module(s.packageId, module)).toSet
+  }
+
   it should "succeed if there are no collisions" in {
+    val signatures = Seq(interface("pkg1", "A", "A.B"), interface("pkg2", "B", "A.B.C"))
     assert(
       CodeGenRunner.detectModuleCollisions(
         Map.empty,
-        Seq(interface("pkg1", "A", "A.B"), interface("pkg2", "B", "A.B.C")),
+        signatures,
+        moduleIdSet(signatures),
       ) === ()
     )
   }
 
   it should "fail if there is a collision" in {
+    val signatures = Seq(interface("pkg1", "A"), interface("pkg2", "A"))
     assertThrows[IllegalArgumentException] {
       CodeGenRunner.detectModuleCollisions(
         Map.empty,
-        Seq(interface("pkg1", "A"), interface("pkg2", "A")),
+        signatures,
+        moduleIdSet(signatures),
       )
     }
   }
 
   it should "fail if there is a collision caused by prefixing" in {
+    val signatures = Seq(interface("pkg1", "A.B"), interface("pkg2", "B"))
     assertThrows[IllegalArgumentException] {
       CodeGenRunner.detectModuleCollisions(
         Map(PackageId.assertFromString("pkg2") -> "A"),
         Seq(interface("pkg1", "A.B"), interface("pkg2", "B")),
+        moduleIdSet(signatures),
       )
     }
   }
 
   it should "succeed if collision is resolved by prefixing" in {
+    val signatures = Seq(interface("pkg1", "A"), interface("pkg2", "A"))
     assert(
       CodeGenRunner.detectModuleCollisions(
         Map(PackageId.assertFromString("pkg2") -> "Pkg2"),
-        Seq(interface("pkg1", "A"), interface("pkg2", "A")),
+        signatures,
+        moduleIdSet(signatures),
+      ) === ()
+    )
+  }
+
+  it should "succeed if there is a collisions on modules which are not to be generated" in {
+    val signatures = Seq(interface("pkg1", "A"), interface("pkg2", "A"))
+    assert(
+      CodeGenRunner.detectModuleCollisions(
+        Map.empty,
+        signatures,
+        Set.empty,
+      ) === ()
+    )
+  }
+
+  it should "succeed if same module name between a module not to be generated and a module to be generated " in {
+    val signatures = Seq(interface("pkg1", "A"), interface("pkg2", "A"))
+    assert(
+      CodeGenRunner.detectModuleCollisions(
+        Map.empty,
+        signatures,
+        Set(Reference.Module(PackageId.assertFromString("pkg1"), ModuleName.assertFromString("A"))),
       ) === ()
     )
   }
@@ -112,6 +163,7 @@ final class CodeGenRunnerTests extends AnyFlatSpec with Matchers {
         pkgPrefixes,
         modulePrefixes,
         Seq(interface1, interface2, interface3),
+        moduleIdSet(Seq(interface1, interface2, interface3)),
       ) ===
         Map(pkg1 -> "com.pkg1", pkg2 -> "com.pkg2.a.b", pkg3 -> "c.d")
     )
@@ -122,7 +174,7 @@ final class CodeGenRunnerTests extends AnyFlatSpec with Matchers {
     val modulePrefixes =
       Map[PackageReference, String](PackageReference.NameVersion(name2, version) -> "A.B")
     assertThrows[IllegalArgumentException] {
-      CodeGenRunner.resolvePackagePrefixes(Map.empty, modulePrefixes, Seq.empty)
+      CodeGenRunner.resolvePackagePrefixes(Map.empty, modulePrefixes, Seq.empty, Set.empty)
     }
   }
 }
@@ -132,9 +184,13 @@ object CodeGenRunnerTests {
   private[this] val testDarPath = "language-support/java/codegen/test-daml.dar"
   private[this] val testDarWithSameDependenciesPath =
     "language-support/java/codegen/test-daml-with-same-dependencies.dar"
+  private[this] val testDarWithSameDependenciesButDifferentTargetVersionPath =
+    "language-support/java/codegen/test-daml-with-same-dependencies-but-different-target-version.dar"
   private val testDar = Path.of(BazelRunfiles.rlocation(testDarPath))
   private val testDarWithSameDependencies =
     Path.of(BazelRunfiles.rlocation(testDarWithSameDependenciesPath))
+  private val testDarWithSameDependenciesButDifferentTargetVersion =
+    Path.of(BazelRunfiles.rlocation(testDarWithSameDependenciesButDifferentTargetVersionPath))
   private val dar = DarReader.assertReadArchiveFromFile(testDar.toFile)
 
   private def interface(pkgId: String, modNames: String*): PackageSignature =