Handle incorrect Scalac options and prevent printing ScalacWorker stacktraces (#1606)

* Improve handling of invalid settings passed to Scala compiler. Throw known exception instead of allowing to None.get (Scala3) or NullPointerException (Scala2)

* Prevent printing stack trace of ScalacInvoker on known compilation errors

* Run reporter.flush in Scala3

* Restore previous error messages

* Addi tests to ensure corect error message and no stack traces are shown

* Remove unused code from test_invalid_scalacopts.sh

* Print exception message only once. Previously printed in both e.getMessage and e.printStackTrace

* Ident with spaces instead of tabs

Author: WojciechMazur

src/java/io/bazel/rulesscala/scalac/ScalacInvoker.java

@@ -24,11 +24,11 @@ public static ScalacInvokerResults invokeCompiler(CompileOptions ops, String[] c
       comp.process(compilerArgs);
     } catch (Throwable ex) {
       if (ex.toString().contains("scala.reflect.internal.Types$TypeError")) {
-        throw new RuntimeException("Build failure with type error", ex);
+        throw new ScalacWorker.CompilationFailed("with type error", ex);
       } else if (ex.toString().contains("java.lang.StackOverflowError")) {
-        throw new RuntimeException("Build failure with StackOverflowError", ex);
+        throw new ScalacWorker.CompilationFailed("with StackOverflowError", ex);
       } else if (isMacroException(ex)) {
-        throw new RuntimeException("Build failure during macro expansion", ex);
+        throw new ScalacWorker.CompilationFailed("during macro expansion", ex);
       } else {
         throw ex;
       }
@@ -39,6 +39,12 @@ public static ScalacInvokerResults invokeCompiler(CompileOptions ops, String[] c
     results.stopTime = System.currentTimeMillis();
 
     ConsoleReporter reporter = (ConsoleReporter) comp.getReporter();
+    if (reporter == null) {
+      // Can happen only when `ReportableMainClass::newCompiler` was not invoked,
+      // typically due to invalid settings
+      throw new ScalacWorker.InvalidSettings();
+    }
+
     if (reporter instanceof ProtoReporter) {
       ProtoReporter protoReporter = (ProtoReporter) reporter;
       protoReporter.writeTo(Paths.get(ops.diagnosticsFile));
@@ -52,7 +58,7 @@ public static ScalacInvokerResults invokeCompiler(CompileOptions ops, String[] c
 
     if (reporter.hasErrors()) {
       reporter.flush();
-      throw new RuntimeException("Build failed");
+      throw new ScalacWorker.CompilationFailed("with errors.");
     }
 
     return results;

src/java/io/bazel/rulesscala/scalac/ScalacInvoker3.java

@@ -22,7 +22,11 @@ public static ScalacInvokerResults invokeCompiler(CompileOptions ops, String[] c
     Driver driver = new dotty.tools.dotc.Driver();
     Contexts.Context ctx = driver.initCtx().fresh();
 
-    Tuple2<scala.collection.immutable.List<AbstractFile>, Contexts.Context> r = driver.setup(compilerArgs, ctx).get();
+    Tuple2<scala.collection.immutable.List<AbstractFile>, Contexts.Context> r = 
+      driver.setup(compilerArgs, ctx)
+        .getOrElse(() -> {
+          throw new ScalacWorker.InvalidSettings();
+        });
 
     Compiler compiler = driver.newCompiler(r._2);
 
@@ -39,8 +43,8 @@ public static ScalacInvokerResults invokeCompiler(CompileOptions ops, String[] c
 
 
     if (reporter.hasErrors()) {
-//      reporter.flush();
-      throw new RuntimeException("Build failed");
+      reporter.flush(ctx);
+      throw new ScalacWorker.CompilationFailed("with errors.");
     }
 
     return results;

src/java/io/bazel/rulesscala/scalac/ScalacWorker.java

@@ -26,6 +26,21 @@
 
 class ScalacWorker implements Worker.Interface {
 
+  public static class InvalidSettings extends WorkerException {
+    public InvalidSettings() {
+      super("Failed to invoke Scala compiler, ensure passed options are valid");
+    }
+  }
+
+  public static class CompilationFailed extends WorkerException {
+    public CompilationFailed(String reason, Throwable cause) {
+      super("Build failure " + reason, cause);
+    }
+    public CompilationFailed(String reason) {
+      this(reason, null);
+    }
+  }
+
   private static final boolean isWindows =
       System.getProperty("os.name").toLowerCase().contains("windows");
 

src/java/io/bazel/rulesscala/worker/Worker.java

@@ -27,6 +27,16 @@ public final class Worker {
 
   public static interface Interface {
     public void work(String[] args) throws Exception;
+
+
+    public abstract class WorkerException extends RuntimeException {
+      public WorkerException(String message) {
+        super(message);
+      }
+      public WorkerException(String message, Throwable cause) {
+        super(message, cause);
+      }
+    }
   }
 
   /**
@@ -87,8 +97,8 @@ public void checkPermission(Permission permission) {
           } catch (ExitTrapped e) {
             code = e.code;
           } catch (Exception e) {
-            System.err.println(e.getMessage());
-            e.printStackTrace();
+            if (e instanceof Interface.WorkerException) System.err.println(e.getMessage());
+            else e.printStackTrace();
             code = 1;
           }
 

test/shell/test_invalid_scalacopts.sh

@@ -0,0 +1,36 @@
+# shellcheck source=./test_runner.sh
+
+dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+. "${dir}"/test_runner.sh
+. "${dir}"/test_helper.sh
+runner=$(get_test_runner "${1:-local}")
+
+test_logs_contains() {
+  scalaVersion=$1
+  expected=$2
+  
+  bazel build \
+   --repo_env=SCALA_VERSION=${scalaVersion} \
+   //test_expect_failure/scalacopts_invalid:empty \
+   2>&1 | grep "$expected"
+}
+
+test_logs_not_contains() {
+  scalaVersion=$1
+  expected=$2
+
+  bazel build \
+   --repo_env=SCALA_VERSION=${scalaVersion} \
+   //test_expect_failure/scalacopts_invalid:empty \
+   2>&1 | grep -v "$expected"
+}
+
+for scalaVersion in 2.12.19 2.13.14 3.3.3; do
+  if [[ "$scalaVersion" == 3.* ]]; then
+    $runner test_logs_contains $scalaVersion "not-existing is not a valid choice for -source"
+  else
+    $runner test_logs_contains $scalaVersion "bad option: '-source:not-existing'"
+  fi
+  $runner test_logs_contains $scalaVersion 'Failed to invoke Scala compiler, ensure passed options are valid'
+  $runner test_logs_not_contains $scalaVersion 'at io.bazel.rulesscala.scalac.ScalacWorker.main'
+done

test_expect_failure/scalacopts_invalid/BUILD

@@ -0,0 +1,7 @@
+load("//scala:scala.bzl", "scala_library")
+
+scala_library(
+    name = "empty",
+    srcs = ["Empty.scala"],
+    scalacopts = ["-source:not-existing"],
+)

test_expect_failure/scalacopts_invalid/Empty.scala

@@ -0,0 +1,3 @@
+package test_expect_failure.scalacopts_invalid
+
+class Empty

test_rules_scala.sh

@@ -58,3 +58,4 @@ $runner bazel build //test_statsfile:SimpleNoStatsFile_statsfile --extra_toolcha
 . "${test_dir}"/test_persistent_worker.sh
 . "${test_dir}"/test_semanticdb.sh
 . "${test_dir}"/test_scaladoc.sh
+. "${test_dir}"/test_invalid_scalacopts.sh