Restrict filesystem reads during resolution phase (#5043)

Follow up to #5037, but for
resolution phase rather than execution. We allow filesystem reads within
`interp.watchValue` blocks, since those get properly handled w.r.t.
`--watch` and other things


Covered by updated integration tests. There are a few awkward existing
violations that I grandfathered in for now

Author: lihaoyi

contrib/jmh/src/mill/contrib/jmh/JmhModule.scala

@@ -42,7 +42,7 @@ trait JmhModule extends JavaModule {
       Jvm.callProcess(
         mainClass = "org.openjdk.jmh.Main",
         classPath = (runClasspath() ++ generatorDeps()).map(_.path) ++
-          Seq(compileGeneratedSources().path, resources),
+          Seq(compileGeneratedSources().path, resources.path),
         mainArgs = args,
         cwd = Task.ctx().dest,
         javaHome = jvmWorker().javaHome().map(_.path),
@@ -58,7 +58,7 @@ trait JmhModule extends JavaModule {
     Task {
       val dest = Task.ctx().dest
       val (sourcesDir, _) = generateBenchmarkSources()
-      val sources = os.walk(sourcesDir).filter(os.isFile)
+      val sources = os.walk(sourcesDir.path).filter(os.isFile)
 
       os.proc(
         Jvm.jdkTool("javac"),
@@ -101,7 +101,7 @@ trait JmhModule extends JavaModule {
         stdout = os.Inherit
       )
 
-      (sourcesDir, resourcesDir)
+      (PathRef(sourcesDir), PathRef(resourcesDir))
     }
 
   def generatorDeps = Task {

core/constants/src/mill/constants/Util.java

@@ -124,7 +124,6 @@ public static String interpolateEnvVars(
         matcher.appendReplacement(result, "\\$");
       } else {
         String envVarValue;
-        mill.constants.DebugLog.println("MATCH " + match);
         envVarValue = env.containsKey(match) ? env.get(match) : onMissing.apply(match);
         matcher.appendReplacement(result, envVarValue);
       }

core/define/src/mill/define/internal/ResolveChecker.scala

@@ -1,7 +1,13 @@
 package mill.define.internal
 
 class ResolveChecker(workspace: os.Path) extends os.Checker {
-  def onRead(path: os.ReadablePath): Unit = ()
+  def onRead(path: os.ReadablePath): Unit = {
+    path match {
+      case path: os.Path =>
+        sys.error(s"Reading from ${path.relativeTo(workspace)} not allowed during resolution phase")
+      case _ =>
+    }
+  }
 
   def onWrite(path: os.Path): Unit = {
     sys.error(s"Writing to ${path.relativeTo(workspace)} not allowed during resolution phase")

core/util/src/mill/util/Jvm.scala

@@ -576,9 +576,11 @@ object Jvm {
       artifactTypes,
       resolutionParams
     ).map { res =>
-      res.files
-        .map(os.Path(_))
-        .map(PathRef(_, quick = true))
+      os.checker.withValue(os.Checker.Nop) {
+        res.files
+          .map(os.Path(_))
+          .map(PathRef(_, quick = true))
+      }
     }
 
   def jvmIndex(
@@ -682,7 +684,6 @@ object Jvm {
       case cp =>
         cp.split(';').map { s =>
           val url = os.Path(s).toNIO.toUri.toURL
-          mill.constants.DebugLog.println("MILL_LOCAL_TEST_OVERRIDE_CLASSPATH " + url)
           new java.net.URLClassLoader(Array(url))
         }.toList
     }

integration/failure/os-checker/src/OsCheckerTests.scala

@@ -10,7 +10,6 @@ object OsCheckerTests extends UtestIntegrationTestSuite {
       import tester._
       val res = tester.eval("foo.bar")
 
-      val sep = if (mill.constants.Util.isWindows) "\\" else "/"
       assert(res.isSuccess == false)
       assert(res.err.contains(
         s"Writing to foo not allowed during resolution phase"
@@ -31,6 +30,18 @@ object OsCheckerTests extends UtestIntegrationTestSuite {
         s"Writing to  not allowed during resolution phase"
       ))
 
+      tester.modifyFile(workspacePath / "build.mill", _.replace("if (true)", "if (false)"))
+      tester.modifyFile(
+        workspacePath / "build.mill",
+        _ + "\nprintln(os.read(mill.define.WorkspaceRoot.workspaceRoot / \"build.mill\"))"
+      )
+      val res4 = tester.eval("baz")
+
+      assert(res4.isSuccess == false)
+      assert(res4.err.contains(
+        s"Reading from build.mill not allowed during resolution phase"
+      ))
+
     }
   }
 }

integration/invalidation/watch-source-input/resources/build.mill

@@ -43,8 +43,9 @@ def writeCompletionMarker(name: String) = {
 }
 
 writeCompletionMarker("initialized")
-
-if (os.read(moduleDir / "watchValue.txt").contains("exit")) {
-  Thread.sleep(1000)
-  System.exit(0)
+os.checker.withValue(os.Checker.Nop) {
+  if (os.read(moduleDir / "watchValue.txt").contains("exit")) {
+    Thread.sleep(1000)
+    System.exit(0)
+  }
 }

libs/javascriptlib/src/mill/javascriptlib/TypeScriptModule.scala

@@ -152,21 +152,21 @@ trait TypeScriptModule extends Module { outer =>
     if (!os.exists(T.dest)) os.makeDir.all(T.dest)
 
     // Copy everything except "build.mill" and the "/out" directory from Task.workspace
-    os.walk(moduleDir, skip = _.last == "out")
-      .filter(_.last != "build.mill")
-      .filter(_.last != "mill")
-      .filter(_.last != "package.json")
-      .filter(_.last != "package-lock.json")
-      .filter(_.last != "tsconfig.json")
-      .foreach { path =>
-        val relativePath = path.relativeTo(moduleDir)
-        val destination = T.dest / relativePath
-
-        if (os.isDir(path)) os.makeDir.all(destination)
-        else os.checker.withValue(os.Checker.Nop) {
-          os.copy.over(path, destination)
+    os.checker.withValue(os.Checker.Nop) {
+      os.walk(moduleDir, skip = _.last == "out")
+        .filter(_.last != "build.mill")
+        .filter(_.last != "mill")
+        .filter(_.last != "package.json")
+        .filter(_.last != "package-lock.json")
+        .filter(_.last != "tsconfig.json")
+        .foreach { path =>
+          val relativePath = path.relativeTo(moduleDir)
+          val destination = T.dest / relativePath
+
+          if (os.isDir(path)) os.makeDir.all(destination)
+          else os.copy.over(path, destination)
         }
-      }
+    }
 
     object IsSrcDirectory {
       def unapply(path: Path): Option[Path] =

libs/main/src/mill/main/MainModule.scala

@@ -26,14 +26,16 @@ trait MainModule extends BaseModule with MainModuleApi {
   protected[mill] val evalWatchedValues: mutable.Buffer[Watchable] = mutable.Buffer.empty[Watchable]
   object interp {
     def watchValue[T](v0: => T)(implicit fn: sourcecode.FileName, ln: sourcecode.Line): T = {
-      val v = v0
-      val watchable = Watchable.Value(
-        () => v0.hashCode,
-        v.hashCode(),
-        fn.value + ":" + ln.value
-      )
-      watchedValues.append(watchable)
-      v
+      os.checker.withValue(os.Checker.Nop) {
+        val v = v0
+        val watchable = Watchable.Value(
+          () => v0.hashCode,
+          v.hashCode(),
+          fn.value + ":" + ln.value
+        )
+        watchedValues.append(watchable)
+        v
+      }
     }
 
     def watch(p: os.Path): os.Path = {

runner/meta/src/mill/runner/meta/MillBuildRootModule.scala

@@ -38,9 +38,11 @@ class MillBuildRootModule()(implicit
 
   override def scalaVersion: T[String] = BuildInfo.scalaVersion
 
-  val scriptSourcesPaths = FileImportGraph
-    .walkBuildFiles(rootModuleInfo.projectRoot / os.up, rootModuleInfo.output)
-    .sorted
+  val scriptSourcesPaths = os.checker.withValue(os.Checker.Nop) {
+    FileImportGraph
+      .walkBuildFiles(rootModuleInfo.projectRoot / os.up, rootModuleInfo.output)
+      .sorted
+  }
 
   /**
    * All script files (that will get wrapped later)
@@ -52,7 +54,9 @@ class MillBuildRootModule()(implicit
 
   def parseBuildFiles: T[FileImportGraph] = Task {
     scriptSources()
-    MillBuildRootModule.parseBuildFiles(compilerWorker(), rootModuleInfo)
+    os.checker.withValue(os.Checker.Nop) {
+      MillBuildRootModule.parseBuildFiles(compilerWorker(), rootModuleInfo)
+    }
   }
 
   private[runner] def compilerWorker: Worker[ScalaCompilerWorkerApi] = Task.Worker {