Improve the init buildgen code resolving some TODOs

libs/init/buildgen/src/mill/main/buildgen/BuildGenBase.scala

@@ -4,22 +4,21 @@ import mill.main.buildgen.BuildGenUtil.{compactBuildTree, writeBuildObject}
 
 import scala.collection.immutable.{SortedMap, SortedSet}
 
-/*
-TODO Can we just convert all generic type parameters to abstract type members?
- See https://stackoverflow.com/a/1154727/5082913.
- I think abstract type members are preferred in this case.
- */
-trait BuildGenBase[M, D, I] {
+trait BuildGenBase {
+  type Module
+  type Input
+  // not renamed to `Config` as it would otherwise conflict with the concrete nested `Config` classes inside
   type C
-  def convertWriteOut(cfg: C, shared: BuildGenUtil.BasicConfig, input: I): Unit = {
+
+  def convertWriteOut(cfg: C, shared: BuildGenUtil.BasicConfig, input: Input): Unit = {
     val output = convert(input, cfg, shared)
     writeBuildObject(
       if (shared.merge.value) compactBuildTree(output) else output,
       shared.jvmId
     )
   }
 
-  def getModuleTree(input: I): Tree[Node[Option[M]]]
+  def getModuleTree(input: Input): Tree[Node[Option[Module]]]
 
   /**
    * A [[Map]] mapping from a key retrieved from the original build tool
@@ -29,25 +28,26 @@ trait BuildGenBase[M, D, I] {
    * If there is no need for such a map, override it with [[Unit]].
    */
   type ModuleFqnMap
-  def getModuleFqnMap(moduleNodes: Seq[Node[M]]): ModuleFqnMap
+
+  def getModuleFqnMap(moduleNodes: Seq[Node[Module]]): ModuleFqnMap
 
   def convert(
-      input: I,
+      input: Input,
       cfg: C,
       shared: BuildGenUtil.BasicConfig
   ): Tree[Node[BuildObject]] = {
     val moduleTree = getModuleTree(input)
-    val moduleOptionTree = moduleTree.map(node => node.copy(value = node.value))
 
     // for resolving moduleDeps
     val moduleNodes =
-      moduleOptionTree.nodes().flatMap(node => node.value.map(m => node.copy(value = m))).toSeq
+      moduleTree.nodes().flatMap(node => node.value.map(m => node.copy(value = m))).toSeq
     val moduleRefMap = getModuleFqnMap(moduleNodes)
 
-    val baseInfo =
-      shared.baseModule.fold(IrBaseInfo()) { getBaseInfo(input, cfg, _, moduleNodes.size) }
+    val baseInfo = shared.baseModule.map {
+      getBaseInfo(input, cfg, _, moduleNodes.size)
+    }
 
-    moduleOptionTree.map(optionalBuild =>
+    moduleTree.map(optionalBuild =>
       optionalBuild.copy(value =
         optionalBuild.value.fold(
           BuildObject(SortedSet("mill._"), SortedMap.empty, Seq("Module"), "", "")
@@ -56,7 +56,7 @@ trait BuildGenBase[M, D, I] {
           println(s"converting module $name")
 
           val build = optionalBuild.copy(value = moduleModel)
-          val inner = extractIrBuild(cfg, build, moduleRefMap)
+          val inner = extractIrModuleBuild(cfg, build, moduleRefMap)
 
           val isNested = optionalBuild.dirs.nonEmpty
           BuildObject(
@@ -71,10 +71,10 @@ trait BuildGenBase[M, D, I] {
                 SortedMap((name, SortedMap(inner.scopedDeps.namedMvnDeps.toSeq*)))
               ),
             supertypes = getSupertypes(cfg, baseInfo, build),
-            inner = BuildGenUtil.renderIrBuild(inner, baseInfo),
+            inner = BuildGenUtil.renderIrModuleBuild(inner, baseInfo),
             outer =
-              if (isNested || baseInfo.moduleTypedef == null) ""
-              else BuildGenUtil.renderIrTrait(baseInfo.moduleTypedef)
+              if (isNested || baseInfo.isEmpty) ""
+              else BuildGenUtil.renderIrBaseInfo(baseInfo.get)
           )
         })
       )
@@ -83,29 +83,30 @@ trait BuildGenBase[M, D, I] {
 
   def extraImports: Seq[String]
 
-  def getSupertypes(cfg: C, baseInfo: IrBaseInfo, build: Node[M]): Seq[String]
+  def getSupertypes(cfg: C, baseInfo: Option[IrBaseInfo], build: Node[Module]): Seq[String]
 
   def getBaseInfo(
-      input: I,
+      input: Input,
       cfg: C,
       baseModule: String,
       packagesSize: Int
   ): IrBaseInfo
 
-  def getArtifactId(moduleModel: M): String
+  def getArtifactId(moduleModel: Module): String
 
-  def extractIrBuild(
+  def extractIrModuleBuild(
       cfg: C,
-      // baseInfo: IrBaseInfo, // `baseInfo` is no longer needed as we compare the `IrBuild` with `IrBaseInfo`/`IrTrait` in common code now.
-      build: Node[M],
+      // baseInfo: IrBaseInfo, // `baseInfo` is no longer needed as we compare the `IrModuleBuild` with `IrBaseInfo` in common code now.
+      build: Node[Module],
       moduleFqnMap: ModuleFqnMap
-  ): IrBuild
+  ): IrModuleBuild
 }
 
 object BuildGenBase {
-  trait MavenAndGradle[M, D] extends BuildGenBase[M, D, Tree[Node[M]]] {
-    override def getModuleTree(input: Tree[Node[M]]): Tree[Node[Option[M]]] =
-      // TODO consider filtering out projects without the `java` plugin applied in Gradle too
+  trait MavenAndGradle extends BuildGenBase {
+    type Input = Tree[Node[Module]]
+
+    override def getModuleTree(input: Tree[Node[Module]]): Tree[Node[Option[Module]]] =
       input.map(node => node.copy(value = Some(node.value)))
     override def extraImports: Seq[String] = Seq()
   }

libs/init/buildgen/src/mill/main/buildgen/BuildGenUtil.scala

@@ -14,7 +14,7 @@ import scala.util.boundary
 @internal
 object BuildGenUtil {
 
-  def renderIrTrait(value: IrTrait): String = {
+  def renderIrBaseInfo(value: IrBaseInfo): String = {
     import value.*
 
     s"""trait $baseModule ${renderExtends(moduleSupertypes)} {
@@ -49,8 +49,7 @@ object BuildGenUtil {
   /**
    * @param baseInfo to compare with [[build]] and render the values only if they are different.
    */
-  def renderIrBuild(build: IrBuild, baseInfo: IrBaseInfo): String = {
-    val baseTrait = baseInfo.moduleTypedef
+  def renderIrModuleBuild(build: IrModuleBuild, baseInfo: Option[IrBaseInfo]): String = {
     import build.*
     val testModuleTypedef =
       if (!hasTest) ""
@@ -82,22 +81,13 @@ object BuildGenUtil {
 
     s"""${renderArtifactName(projectName, dirs)}
        |
-       |${renderJavacOptions(
-        javacOptions,
-        if (baseTrait != null) baseTrait.javacOptions else Seq.empty
-      )}
+       |${renderJavacOptions(javacOptions, baseInfo.fold(Seq.empty)(_.javacOptions))}
        |
-       |${renderScalaVersion(scalaVersion, if (baseTrait != null) baseTrait.scalaVersion else None)}
+       |${renderScalaVersion(scalaVersion, baseInfo.flatMap(_.scalaVersion))}
        |
-       |${renderScalacOptions(
-        scalacOptions,
-        if (baseTrait != null) baseTrait.scalacOptions else None
-      )}
+       |${renderScalacOptions(scalacOptions, baseInfo.flatMap(_.scalacOptions))}
        |
-       |${renderRepositories(
-        repositories,
-        if (baseTrait != null) baseTrait.repositories else Seq.empty
-      )}
+       |${renderRepositories(repositories, baseInfo.fold(Seq.empty)(_.repositories))}
        |
        |${renderBomMvnDeps(scopedDeps.mainBomMvnDeps)}
        |
@@ -114,15 +104,12 @@ object BuildGenUtil {
        |${renderRunModuleDeps(scopedDeps.mainRunModuleDeps)}
        |
        |${
-        if (pomSettings != (if (baseTrait != null) baseTrait.pomSettings else null))
+        if (pomSettings != baseInfo.map(_.pomSettings).orNull)
           renderPomSettings(renderIrPom(pomSettings))
         else ""
       }
        |
-       |${renderPublishVersion(
-        publishVersion,
-        if (baseTrait != null) baseTrait.publishVersion else null
-      )}
+       |${renderPublishVersion(publishVersion, baseInfo.map(_.publishVersion).orNull)}
        |
        |${renderPomPackaging(packaging)}
        |
@@ -219,7 +206,7 @@ object BuildGenUtil {
 
       childCompanions.foreach { case entry @ (objectName, childConstants) =>
         val parentConstants = mergedParentCompanions.getOrElse(objectName, null)
-        if (null == parentConstants) mergedParentCompanions += entry
+        if (parentConstants == null) mergedParentCompanions += entry
         else {
           if (childConstants.exists { case (k, v) => v != parentConstants.getOrElse(k, v) })
             boundary.break(null)
@@ -237,7 +224,7 @@ object BuildGenUtil {
       children.iterator.foreach {
         case child @ Tree(Node(_ :+ dir, nested), Seq()) if nested.outer.isEmpty =>
           val mergedCompanions = merge(module.companions, nested.companions)
-          if (null == mergedCompanions) unmerged += child
+          if (mergedCompanions == null) unmerged += child
           else {
             val mergedImports = module.imports ++ nested.imports
             val mergedInner = {
@@ -271,7 +258,7 @@ object BuildGenUtil {
     pprint.Util.literalize(if (value == null) "" else value)
 
   def escapeOption(value: String): String =
-    if (null == value) "None" else s"Some(\"$value\")"
+    if (value == null) "None" else s"Some(\"$value\")"
 
   def renderMvnString(
       group: String,
@@ -291,7 +278,7 @@ object BuildGenUtil {
         }
     }
     val sepVersion =
-      if (null == version) {
+      if (version == null) {
         println(
           s"assuming $group:$artifact is a BOM dependency; if not, please specify version in the generated build file"
         )
@@ -319,7 +306,7 @@ object BuildGenUtil {
     groupArtifactVersion._2.endsWith("-bom")
 
   def isNullOrEmpty(value: String | Null): Boolean =
-    null == value || value.isEmpty
+    value == null || value.isEmpty
 
   val linebreak: String =
     """
@@ -356,11 +343,16 @@ object BuildGenUtil {
   def renderVersionControl(vc: IrVersionControl): String =
     s"VersionControl(${escapeOption(vc.url)}, ${escapeOption(vc.connection)}, ${escapeOption(vc.devConnection)}, ${escapeOption(vc.tag)})"
 
-  // TODO consider renaming to `renderOptionalDef` or `renderIfArgsNonEmpty`?
-  def optional(construct: String, args: IterableOnce[String]): String =
-    optional(construct + "(", args, ",", ")")
+  // TODO more alternative names: `renderOptional(Def)` or `render(Def)IfArgsNonEmpty`?
+  def renderIfArgsNonEmpty(construct: String, args: IterableOnce[String]): String =
+    renderIfArgsNonEmpty(construct + "(", args, ",", ")")
 
-  def optional(start: String, args: IterableOnce[String], sep: String, end: String): String = {
+  def renderIfArgsNonEmpty(
+      start: String,
+      args: IterableOnce[String],
+      sep: String,
+      end: String
+  ): String = {
     val itr = args.iterator
     if (itr.isEmpty) ""
     else itr.mkString(start, sep, end)
@@ -406,25 +398,25 @@ object BuildGenUtil {
     else s"def artifactName = ${escape(name)}"
 
   def renderBomMvnDeps(args: IterableOnce[String]): String =
-    optional("def bomMvnDeps = super.bomMvnDeps() ++ Seq", args)
+    renderIfArgsNonEmpty("def bomMvnDeps = super.bomMvnDeps() ++ Seq", args)
 
   def renderMvnDeps(args: IterableOnce[String]): String =
-    optional("def mvnDeps = Seq", args)
+    renderIfArgsNonEmpty("def mvnDeps = Seq", args)
 
   def renderModuleDeps(args: IterableOnce[String]): String =
-    optional("def moduleDeps = super.moduleDeps ++ Seq", args)
+    renderIfArgsNonEmpty("def moduleDeps = super.moduleDeps ++ Seq", args)
 
   def renderCompileMvnDeps(args: IterableOnce[String]): String =
-    optional("def compileMvnDeps = Seq", args)
+    renderIfArgsNonEmpty("def compileMvnDeps = Seq", args)
 
   def renderCompileModuleDeps(args: IterableOnce[String]): String =
-    optional("def compileModuleDeps = super.compileModuleDeps ++ Seq", args)
+    renderIfArgsNonEmpty("def compileModuleDeps = super.compileModuleDeps ++ Seq", args)
 
   def renderRunMvnDeps(args: IterableOnce[String]): String =
-    optional("def runMvnDeps = Seq", args)
+    renderIfArgsNonEmpty("def runMvnDeps = Seq", args)
 
   def renderRunModuleDeps(args: IterableOnce[String]): String =
-    optional("def runModuleDeps = super.runModuleDeps ++ Seq", args)
+    renderIfArgsNonEmpty("def runModuleDeps = super.runModuleDeps ++ Seq", args)
 
   def renderJavacOptions(args: Seq[String], superArgs: Seq[String] = Seq.empty): String =
     renderSeqTaskDefWithSuper("javacOptions", args, superArgs, "String", escape).getOrElse("")
@@ -455,7 +447,7 @@ object BuildGenUtil {
     ).getOrElse("")
 
   def renderResources(args: IterableOnce[os.SubPath]): String =
-    optional(
+    renderIfArgsNonEmpty(
       """def resources = Task { super.resources() ++ customResources() }
         |def customResources = Task.Sources(""".stripMargin,
       args.iterator.map(sub => escape(sub.toString())),
@@ -464,9 +456,9 @@ object BuildGenUtil {
     )
 
   def renderPomPackaging(packaging: String): String =
-    if (isNullOrEmpty(packaging) || "jar" == packaging) "" // skip default
+    if (isNullOrEmpty(packaging) || packaging == "jar") "" // skip default
     else {
-      val pkg = if ("pom" == packaging) "PackagingType.Pom" else escape(packaging)
+      val pkg = if (packaging == "pom") "PackagingType.Pom" else escape(packaging)
       s"def pomPackagingType = $pkg"
     }
 
@@ -491,7 +483,7 @@ object BuildGenUtil {
       args: Seq[(String, String)]
   ): String = {
     val tuples = args.iterator.map { case (k, v) => s"(${escape(k)}, ${escape(v)})" }
-    optional("def publishProperties = super.publishProperties() ++ Map", tuples)
+    renderIfArgsNonEmpty("def publishProperties = super.publishProperties() ++ Map", tuples)
   }
 
   def renderJvmWorker(moduleName: String): String =
@@ -557,15 +549,16 @@ object BuildGenUtil {
   object BasicConfig {
     implicit def parser: mainargs.ParserForClass[BasicConfig] = mainargs.ParserForClass[BasicConfig]
   }
-  // TODO alternative names: `MavenAndGradleConfig`, `MavenAndGradleSharedConfig`
+
+  // TODO other alternative names to consider: `MavenAndGradleConfig`, `MavenAndGradleSharedConfig`
   @mainargs.main
-  case class Config(
+  case class MavenAndGradleCommonConfig(
       basicConfig: BasicConfig,
       @arg(doc = "capture Maven publish properties", short = 'p')
       publishProperties: Flag = Flag()
   )
-
-  object Config {
-    implicit def configParser: mainargs.ParserForClass[Config] = mainargs.ParserForClass[Config]
+  object MavenAndGradleCommonConfig {
+    implicit def configParser: mainargs.ParserForClass[MavenAndGradleCommonConfig] =
+      mainargs.ParserForClass[MavenAndGradleCommonConfig]
   }
 }

libs/init/buildgen/src/mill/main/buildgen/ir.scala

@@ -36,7 +36,7 @@ object BuildObject {
 @mill.api.experimental
 case class Node[T](dirs: Seq[String], value: T)
 
-case class IrTrait(
+case class IrBaseInfo(
     jvmId: Option[String],
     baseModule: String,
     moduleSupertypes: Seq[String],
@@ -77,9 +77,8 @@ case class IrLicense(
     distribution: String = "repo"
 )
 
-// TODO Consider renaming to `IrModule(Build)` to disambiguate? `sbt`, for example, uses `ThisBuild` and `buildSettings` to refer to the whole build.
-// TODO reuse the members in `IrTrait`?
-case class IrBuild(
+// TODO reuse the members in `IrBaseInfo`?
+case class IrModuleBuild(
     scopedDeps: IrScopedDeps,
     testModule: String,
     testModuleMainType: String,
@@ -101,9 +100,9 @@ case class IrBuild(
     testForkDir: Option[String]
 )
 
-object IrBuild {
-  // TODO not used
-  def empty(dirs: Seq[String]) = IrBuild(
+object IrModuleBuild {
+  // This function is not used currently but temporarily kept for possible use cases. Remove if it's ensured that it's not going to be used.
+  def empty(dirs: Seq[String]) = IrModuleBuild(
     IrScopedDeps(),
     null,
     null,
@@ -144,21 +143,6 @@ case class IrScopedDeps(
     testCompileModuleDeps: SortedSet[String] = SortedSet()
 )
 
-// TODO remove `IrBaseInfo` and just use `IrTrait` directly?
-case class IrBaseInfo(
-    /*
-    javacOptions: Seq[String] = Nil,
-    scalaVersion: Option[String] = None,
-    scalacOptions: Option[Seq[String]] = None,
-    repositories: Seq[String] = Nil,
-    noPom: Boolean = true,
-    publishVersion: String = "",
-    publishProperties: Seq[(String, String)] = Nil,
-     */
-    // TODO consider renaming directly to `trait` or `baseTrait`?
-    moduleTypedef: IrTrait | Null = null
-)
-
 sealed class IrDependencyType
 object IrDependencyType {
   case object Default extends IrDependencyType