Fix javac-turbine diagnostic formatting to include source position

PiperOrigin-RevId: 188799934

6 files changed, 99 insertions, 17 deletions

diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
index 8391655d25..f2eda5f9e6 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
@@ -4,12 +4,12 @@ java_library(
     name = "javac_turbine",
     srcs = ["JavacTurbine.java"],
     deps = [
+        ":formatted_diagnostic",
         ":javac_transitive",
         ":javac_turbine_compile_request",
         ":javac_turbine_compile_result",
         ":javac_turbine_compiler",
         ":zip_util",
-        "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:JarOwner",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:javac_options",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:dependency",
         "//third_party:asm",
@@ -24,6 +24,7 @@ java_library(
     name = "javac_turbine_compile_request",
     srcs = ["JavacTurbineCompileRequest.java"],
     deps = [
+        ":formatted_diagnostic",
         ":javac_transitive",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:dependency",
         "//third_party:guava",
@@ -36,6 +37,7 @@ java_library(
     name = "javac_turbine_compile_result",
     srcs = ["JavacTurbineCompileResult.java"],
     deps = [
+        ":formatted_diagnostic",
         "//third_party:guava",
         "//third_party:jsr305",
         "//third_party/java/jdk/langtools:javac",
@@ -46,6 +48,7 @@ java_library(
     name = "javac_turbine_compiler",
     srcs = ["JavacTurbineCompiler.java"],
     deps = [
+        ":formatted_diagnostic",
         ":javac_transitive",
         ":javac_turbine_compile_request",
         ":javac_turbine_compile_result",
@@ -101,6 +104,14 @@ java_library(
     ],
 )
 
+java_library(
+    name = "formatted_diagnostic",
+    srcs = ["FormattedDiagnostic.java"],
+    deps = [
+        "//third_party/java/jdk/langtools:javac",
+    ],
+)
+
 filegroup(
     name = "srcs",
     srcs = glob(["**/*.java"]) + [
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/FormattedDiagnostic.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/FormattedDiagnostic.java
new file mode 100644
index 0000000000..51784beafb
--- /dev/null
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/FormattedDiagnostic.java
@@ -0,0 +1,41 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.java.turbine.javac;
+
+import javax.tools.Diagnostic;
+import javax.tools.JavaFileObject;
+
+/**
+ * A wrapper for a {@link Diagnostic<JavaFileObject>} that includes the full formatted message
+ * produced by javac, which relies on compilation internals and can't be reproduced after the
+ * compilation is complete.
+ */
+class FormattedDiagnostic {
+  private final Diagnostic<? extends JavaFileObject> diagnostic;
+  private final String message;
+
+  FormattedDiagnostic(Diagnostic<? extends JavaFileObject> diagnostic, String message) {
+    this.diagnostic = diagnostic;
+    this.message = message;
+  }
+
+  Diagnostic<? extends JavaFileObject> diagnostic() {
+    return diagnostic;
+  }
+
+  String message() {
+    return message;
+  }
+}
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
index c9160fc705..082cc8812c 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
@@ -46,14 +46,11 @@ import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.Locale;
 import java.util.Map;
 import java.util.jar.Attributes;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 import java.util.zip.ZipOutputStream;
-import javax.tools.Diagnostic;
-import javax.tools.JavaFileObject;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
@@ -250,8 +247,8 @@ public class JavacTurbine implements AutoCloseable {
           turbineOptions, compileResult.files(), transitive.collectTransitiveDependencies());
       dependencyModule.emitDependencyInformation(actualClasspath, compileResult.success());
     } else {
-      for (Diagnostic<? extends JavaFileObject> diagnostic : compileResult.diagnostics()) {
-        out.println(diagnostic.getMessage(Locale.getDefault()));
+      for (FormattedDiagnostic diagnostic : compileResult.diagnostics()) {
+        out.println(diagnostic.message());
       }
       out.print(compileResult.output());
     }
@@ -444,8 +441,8 @@ public class JavacTurbine implements AutoCloseable {
     if (result.success()) {
       return false;
     }
-    for (Diagnostic<? extends JavaFileObject> diagnostic : result.diagnostics()) {
-      String code = diagnostic.getCode();
+    for (FormattedDiagnostic diagnostic : result.diagnostics()) {
+      String code = diagnostic.diagnostic().getCode();
       if (code.contains("doesnt.exist")
           || code.contains("cant.resolve")
           || code.contains("cant.access")) {
@@ -453,7 +450,7 @@ public class JavacTurbine implements AutoCloseable {
       }
       // handle -Xdoclint:reference errors, which don't have a diagnostic code
       // TODO(cushon): this is locale-dependent
-      if (diagnostic.getMessage(Locale.getDefault()).contains("error: reference not found")) {
+      if (diagnostic.message().contains("error: reference not found")) {
         return true;
       }
     }
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileResult.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileResult.java
index b066537b9e..ef40960937 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileResult.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileResult.java
@@ -18,8 +18,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.sun.tools.javac.util.Context;
-import javax.tools.Diagnostic;
-import javax.tools.JavaFileObject;
 
 /** The output from a {@link JavacTurbineCompiler} compilation. */
 class JavacTurbineCompileResult {
@@ -32,14 +30,14 @@ class JavacTurbineCompileResult {
   private final ImmutableMap<String, byte[]> files;
   private final Status status;
   private final String output;
-  private final ImmutableList<Diagnostic<? extends JavaFileObject>> diagnostics;
+  private final ImmutableList<FormattedDiagnostic> diagnostics;
   private final Context context;
 
   JavacTurbineCompileResult(
       ImmutableMap<String, byte[]> files,
       Status status,
       String output,
-      ImmutableList<Diagnostic<? extends JavaFileObject>> diagnostics,
+      ImmutableList<FormattedDiagnostic> diagnostics,
       Context context) {
     this.files = files;
     this.status = status;
@@ -59,7 +57,7 @@ class JavacTurbineCompileResult {
   }
 
   /** The diagnostics from the compilation. */
-  ImmutableList<Diagnostic<? extends JavaFileObject>> diagnostics() {
+  ImmutableList<FormattedDiagnostic> diagnostics() {
     return diagnostics;
   }
 
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
index 58417f1fbf..f88b1a71f0 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
@@ -42,6 +42,7 @@ import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import javax.annotation.Nullable;
@@ -57,8 +58,7 @@ public class JavacTurbineCompiler {
     Map<String, byte[]> files = new LinkedHashMap<>();
     Status status;
     StringWriter sw = new StringWriter();
-    ImmutableList.Builder<Diagnostic<? extends JavaFileObject>> diagnostics =
-        ImmutableList.builder();
+    ImmutableList.Builder<FormattedDiagnostic> diagnostics = ImmutableList.builder();
     Context context = new Context();
 
     try (PrintWriter pw = new PrintWriter(sw)) {
@@ -71,7 +71,7 @@ public class JavacTurbineCompiler {
                 .getTask(
                     pw,
                     fm,
-                    diagnostics::add,
+                    diagnostic -> diagnostics.add(formatDiagnostic(diagnostic)),
                     request.javacOptions(),
                     ImmutableList.of() /*classes*/,
                     fm.getJavaFileObjectsFromPaths(request.sources()),
@@ -115,6 +115,21 @@ public class JavacTurbineCompiler {
         ImmutableMap.copyOf(files), status, sw.toString(), diagnostics.build(), context);
   }
 
+  private static FormattedDiagnostic formatDiagnostic(
+      Diagnostic<? extends JavaFileObject> diagnostic) {
+    StringBuilder message = new StringBuilder();
+    if (diagnostic.getSource() != null) {
+      message.append(diagnostic.getSource().getName());
+      if (diagnostic.getLineNumber() != -1) {
+        message.append(':').append(diagnostic.getLineNumber());
+      }
+      message.append(": ");
+    }
+    message.append(diagnostic.getKind().toString().toLowerCase(Locale.getDefault()));
+    message.append(": ").append(diagnostic.getMessage(Locale.getDefault()));
+    return new FormattedDiagnostic(diagnostic, message.toString());
+  }
+
   /** Mask the annotation processor classpath to avoid version skew. */
   @Trusted
   private static class ClassloaderMaskingFileManager extends JavacFileManager {
diff --git a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
index faa0cc6032..ac285d04ca 100644
--- a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
+++ b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
@@ -1350,5 +1350,25 @@ public class JavacTurbineTest extends AbstractJavacTurbineCompilationTest {
                     || name.equals(JavacTurbine.MANIFEST_NAME)))
         .collect(toSet());
   }
+
+  @Test
+  public void diagnosticFormattingTest() throws Exception {
+    addSourceLines(
+        "A.java", //
+        "class A {",
+        "}}");
+
+    optionsBuilder.addSources(ImmutableList.copyOf(Iterables.transform(sources, TO_STRING)));
+
+    StringWriter output = new StringWriter();
+    Result result;
+    try (JavacTurbine turbine =
+        new JavacTurbine(new PrintWriter(output, true), optionsBuilder.build())) {
+      result = turbine.compile();
+    }
+
+    assertThat(result).isEqualTo(Result.ERROR);
+    assertThat(output.toString()).contains("A.java:2: error: class, interface, or enum expected");
+  }
 }