Skip to content

Commit

Permalink
Add a sourceGenerators task to help out IDE config generation (#4621)
Browse files Browse the repository at this point in the history
Context : #4530

* Adds a `DeferredGeneratedSourcesModule` that extends `JavaModule`,
providing a `def deferredGeneratedSourceTasks` alternative to
`generatedSources`. This allows BSP/GenIdea/Bloop to predict source
roots without actually running the code generation logic.
* Adds a `def out` to the `Ctx` class, allowing to query the location
`out` folder within tasks.
* Adds unit tests / integration tests where relevant 

#### Previous description (kept for the record) : 

@lihaoyi, I want to open this PR to continue the discussion. At the time
of writing this, this adds a `generatedSourceRoots` parameter for tasks,
as you've described, and uses it to compute the source directories for
the bloop configuration, as opposed to actually running the `allSources`
task. A similar change would need to be achieved in `GenIdea` and `BSP`.

So now, the problem I see is as follows : in order for this to be
useful, it needs to be applied to all source generators provided OOTB by
the mill codebase, but also third party providers of source-generator
(like smithy4s), as the ability to jump to the sources is paramount for
the user experience in IDEs.

Applying this change is a fair bit of work in this codebase, and a big
ask for the larger ecosystem, and although I think SBT's approach of
treating all source generators lazily is preferable to mill's current
approach of running all source generators eagerly, I don't think I can
weigh the benefits of changing the approach against the cost induced by
breaking IDE behaviour for existing plugins.

With that in mind here's what I think would be a good mitigation : 

```scala
// Rather than traversing the transitive inputs of `allSources`, we define this setting as a way to register lazy 
// source generators. We force the contract to return `Unit` to prevent the assumption that a returned `PathRef` would be
// added to the sources. 
def lazySourceGenerators = Seq[T[Unit]] 
 
 // Runs the lazy source generators, returning path refs computed from their `generatedSourceRoots`. 
 // When the tasks don't have `generatedSourceRoots` set, we assume that the `T.dest` is to be added as source root. 
 final def runLazySourceGenerators : T[Seq[PathRef]] = ???
 
// Does not run the lazy source generators, but rather computes the source roots they'll generate code in. 
// Wrapped in T because the evaluator is needed to get the T.dest of the lazy source generators. I presume that is 
// something we could thread in through the ctx (like `def destOf(task: T[_]) : Path`)  
final def lazyGeneratedSourceRoots: T[Seq[Path]] = ???
 
 // Keeps the current behaviour intact 
def allSources = T {
    sources() ++ generatedSources() ++ runLazySourceGenerators()
} 
```

The `Bloop`, `GenIdea` and `BSP` then get amended to call `sources()`,
`generatedSources()` and `lazyGeneratedSourceRoots()`, but avoids
`runLazySourceGenerators`.

This approach would allow to alleviate the pain point raised in the
discussion above, whilst retaining the current behaviour of the
ecosystem. Mill plugin providers could be encouraged to use
`runLazySourceGenerators` instead of `generatedSources`, but users would
have a recourse to turn eager source generators into lazy ones if they
need it to be (provided a `withGeneratedSourceRoots` is provided to
them, that is)

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
  • Loading branch information
Baccata and autofix-ci[bot] authored Mar 6, 2025
1 parent 6f069b0 commit 95f1fe3
Show file tree
Hide file tree
Showing 20 changed files with 407 additions and 34 deletions.
7 changes: 4 additions & 3 deletions bsp/worker/src/mill/bsp/worker/MillBuildServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import mill.eval.Evaluator.TaskResult
import mill.main.MainModule
import mill.runner.MillBuildRootModule
import mill.scalalib.bsp.{BspModule, JvmBuildTarget, ScalaBuildTarget}
import mill.scalalib.{JavaModule, SemanticDbJavaModule, TestModule}
import mill.scalalib.{JavaModule, DeferredGeneratedSourcesModule, SemanticDbJavaModule, TestModule}
import mill.util.ColorLogger
import mill.given

Expand Down Expand Up @@ -231,10 +231,11 @@ private class MillBuildServer(
hint = s"buildTargetSources ${sourcesParams}",
targetIds = _ => sourcesParams.getTargets.asScala.toSeq,
tasks = {
case module: MillBuildRootModule =>
case module: DeferredGeneratedSourcesModule =>
Task.Anon {
module.sources().map(p => sourceItem(p.path, false)) ++
module.generatedSources().map(p => sourceItem(p.path, true))
module.generatedSources().map(p => sourceItem(p.path, true)) ++
module.predictedGeneratedSources().map(p => sourceItem(p, true))
}
case module: JavaModule =>
Task.Anon {
Expand Down
14 changes: 10 additions & 4 deletions contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,16 @@ class BloopImpl(
* from module#sources in bloopInstall
*/
def moduleSourceMap = Task.Input {
val sources = Task.traverse(computeModules) { m =>
m.allSources.map { paths =>
name(m) -> paths.map(_.path)
}
val sources = Task.traverse(computeModules) {
case m: DeferredGeneratedSourcesModule =>
// We're not using `allSources` here, one purpose. See https://github.com/com-lihaoyi/mill/discussions/4530
m.ideSources.map { paths =>
name(m) -> paths
}
case other =>
other.allSources.map { pathRefs =>
name(other) -> pathRefs.map(_.path)
}
}()
Result.Success(sources.toMap)
}
Expand Down
32 changes: 30 additions & 2 deletions contrib/bloop/test/src/mill/contrib/bloop/BloopTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ object BloopTests extends TestSuite {
)

object build extends TestBaseModule {
object scalaModule extends scalalib.ScalaModule with testBloop.Module {
object scalaModule extends scalalib.ScalaModule with testBloop.Module
with DeferredGeneratedSourcesModule {
def scalaVersion = "2.12.8"
val bloopVersion = mill.contrib.bloop.Versions.bloop
override def mainClass = Some("foo.bar.Main")
Expand All @@ -45,6 +46,26 @@ object BloopTests extends TestSuite {
ivy"org.postgresql:postgresql:42.3.3"
)

def someGeneratedSource = Task(deferredGeneratedSourceRoots = Seq(os.SubPath("scala"))) {
val contents = """|package foo
|
|case class GeneratedFoo()
|""".stripMargin
os.write(T.dest / "scala" / "GeneratedFoo.scala", contents)
}

/**
* We're adding a buggy task to check its generated source folders
* will be configured for Bloop even if the task crashes
*/
def someBuggyGeneratedSource: T[Unit] = T {
def crash(): Unit = throw new Exception("Boom")
crash()
}

override def deferredGeneratedSourceTasks =
List(someGeneratedSource, someBuggyGeneratedSource)

object test extends ScalaTests with TestModule.Utest
}

Expand Down Expand Up @@ -130,7 +151,14 @@ object BloopTests extends TestSuite {

assert(name == "scalaModule")
assert(workspaceDir == Some(workdir.wrapped))
assert(sources == List(workdir / "scalaModule/src"))
assert(sources == List(
workdir / "scalaModule/src",
unitTester.outPath / "scalaModule" / "someGeneratedSource.dest" / "scala",
// Despite the task being buggy, the Bloop configuration is still produced.
// The task is not setting `deferredGeneratedSourceRoots` explicitly, which is interpreted
// as the corresponding T.dest being the source root.
unitTester.outPath / "scalaModule" / "someBuggyGeneratedSource.dest"
))
assert(options.contains("-language:higherKinds"))
assert(version == "2.12.8")
assert(
Expand Down
14 changes: 9 additions & 5 deletions idea/src/mill/idea/GenIdeaImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -500,10 +500,14 @@ case class GenIdeaImpl(
evaluator,
scalaVersion
) =>
val ideSources = mod match {
case deferred: DeferredGeneratedSourcesModule => deferred.ideSources
case other => other.allSources.map(_.map(_.path))
}
val Seq(
resourcesPathRefs: Seq[PathRef],
generatedSourcePathRefs: Seq[PathRef],
allSourcesPathRefs: Seq[PathRef]
ideSourcesPaths: Seq[os.Path]
) = evaluator.evalOrThrow(
exceptionFactory = r =>
GenIdeaException(
Expand All @@ -512,13 +516,13 @@ case class GenIdeaImpl(
)(Seq(
mod.resources,
mod.generatedSources,
mod.allSources
// Using `ideSources` instead of `allSources` to prevent the whole process crashing
// in case of mis-configured (or buggy) source-generators.
ideSources
))

val generatedSourcePaths = generatedSourcePathRefs.map(_.path)
val normalSourcePaths = (allSourcesPathRefs
.map(_.path)
.toSet -- generatedSourcePaths.toSet).toSeq
val normalSourcePaths = (ideSourcesPaths.toSet -- generatedSourcePaths.toSet).toSeq

val sanizedDeps: Seq[ScopedOrd[String]] = {
resolvedDeps
Expand Down
24 changes: 24 additions & 0 deletions integration/ide/bsp-server/resources/project/build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,38 @@ object `hello-java` extends scalalib.JavaModule

// testing a simple Scala module
object `hello-scala` extends scalalib.ScalaModule {

def scalaVersion = Option(System.getenv("TEST_SCALA_2_13_VERSION")).getOrElse(???)

}

// testing a simple Kotlin module
object `hello-kotlin` extends kotlinlib.KotlinModule {
def kotlinVersion = Option(System.getenv("TEST_KOTLIN_VERSION")).getOrElse(???)
}

// testing a java module with source generators
object `hello-sourcegen` extends scalalib.DeferredGeneratedSourcesModule {
def someGeneratedSource = Task(deferredGeneratedSourceRoots = Seq(os.SubPath("java"))) {
val contents = """|package foo;
|
|class GeneratedFoo {
|}
|""".stripMargin
os.write(T.dest / "java" / "GeneratedFoo.java", contents)
}

/**
* We're adding a buggy task to check its generated source folders
* will be configured for BSP even if the task crashes
*/
def someBuggyGeneratedSource: T[Unit] = T {
def crash(): Unit = throw new Exception("Boom")
crash()
}
override def deferredGeneratedSourceTasks = List(someGeneratedSource, someBuggyGeneratedSource)
}

// testing a two-module setup, with one module depending on the other,
// involving ivyDeps, but also compile-time only and runtime only dependencies
object lib extends scalalib.JavaModule {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@
"file:///coursier-cache/https/repo1.maven.org/maven2/org/scala-lang/scala-library/<scala-version>/scala-library-<scala-version>.jar",
"file:///workspace/hello-scala/compile-resources"
]
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"classpath": [
"file:///workspace/hello-sourcegen/compile-resources"
]
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@
"version": "<scala-version>"
}
]
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"modules": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@
"sources": [
"file:///coursier-cache/https/repo1.maven.org/maven2/org/scala-lang/scala-library/<scala-version>/scala-library-<scala-version>-sources.jar"
]
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"sources": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@
"file:///workspace/hello-scala/compile-resources"
],
"classDirectory": "file:///workspace/out/hello-scala/compile.dest/classes"
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"options": [],
"classpath": [
"file:///workspace/hello-sourcegen/compile-resources"
],
"classDirectory": "file:///workspace/out/hello-sourcegen/compile.dest/classes"
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@
},
"outputPaths": []
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"outputPaths": []
},
{
"target": {
"uri": "file:///workspace/mill-build"
Expand Down Expand Up @@ -60,4 +66,4 @@
]
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,17 @@
},
"resources": []
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"resources": []
},
{
"target": {
"uri": "file:///workspace/mill-build"
},
"resources": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@
"file:///workspace/hello-scala/compile-resources"
],
"classDirectory": "file:///workspace/out/hello-scala/compile.dest/classes"
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"options": [],
"classpath": [
"file:///workspace/hello-sourcegen/compile-resources"
],
"classDirectory": "file:///workspace/out/hello-sourcegen/compile.dest/classes"
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,28 @@
}
]
},
{
"target": {
"uri": "file:///workspace/hello-sourcegen"
},
"sources": [
{
"uri": "file:///workspace/hello-sourcegen/src",
"kind": 2,
"generated": false
},
{
"uri": "file:///workspace/out/hello-sourcegen/someGeneratedSource.dest/java",
"kind": 2,
"generated": true
},
{
"uri": "file:///workspace/out/hello-sourcegen/someBuggyGeneratedSource.dest",
"kind": 2,
"generated": true
}
]
},
{
"target": {
"uri": "file:///workspace/mill-build"
Expand Down Expand Up @@ -90,4 +112,4 @@
]
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,32 @@
}
}
},
{
"id": {
"uri": "file:///workspace/hello-sourcegen"
},
"displayName": "hello-sourcegen",
"baseDirectory": "file:///workspace/hello-sourcegen",
"tags": [
"library",
"application"
],
"languageIds": [
"java"
],
"dependencies": [],
"capabilities": {
"canCompile": true,
"canTest": false,
"canRun": true,
"canDebug": false
},
"dataKind": "jvm",
"data": {
"javaHome": "java-home",
"javaVersion": "<java-version>"
}
},
{
"id": {
"uri": "file:///workspace/mill-build"
Expand Down Expand Up @@ -207,4 +233,4 @@
}
}
]
}
}
Loading

0 comments on commit 95f1fe3

Please sign in to comment.