Make PositionImpl thread-safe

Fixes sbt#464 sbt/sbt#3623

Currently compiling a lot of Java code in CI environment causes NullPointerException, which is
suspected of race condition around `javax.tools.Diagnostics[S]`, which is held by `PositionImpl` and then later accessed by async logging.

This change makes the `PositionImpl` strict and immutable, and extracts it from a Java Diagnostics object. No other observable changes are introduced besides, hopefully the lack of NPE. Credit on the detective work goes to Lightbend Akka team ktoso, 2m, and jrudolph.

internal/zinc-compile-core/src/main/mima-filters/1.0.0.backwards.excludes

@@ -0,0 +1,3 @@
+# PositionImpl is a private class only invoked in the same source.
+ProblemFilters.exclude[FinalClassProblem]("sbt.internal.inc.javac.DiagnosticsReporter$PositionImpl")
+ProblemFilters.exclude[DirectMissingMethodProblem]("sbt.internal.inc.javac.DiagnosticsReporter#PositionImpl.this")

internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/DiagnosticsReporter.scala

@@ -23,6 +23,8 @@ import javax.tools.Diagnostic.NOPOS
  * @param reporter
  */
 final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[JavaFileObject] {
+  import DiagnosticsReporter._
+
   val END_OF_LINE_MATCHER = "(\r\n)|[\r]|[\n]"
   val EOL = System.getProperty("line.separator")
 
@@ -64,85 +66,118 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J
 
     import sbt.util.InterfaceUtil.problem
     val msg = fixedDiagnosticMessage(d)
-    val pos: xsbti.Position = new PositionImpl(d)
+    val pos: xsbti.Position = PositionImpl(d)
     if (severity == Severity.Error) errorEncountered = true
     reporter.log(problem("", pos, msg, severity))
   }
+}
 
-  private class PositionImpl(d: Diagnostic[_ <: JavaFileObject]) extends xsbti.Position {
-    // https://docs.oracle.com/javase/7/docs/api/javax/tools/Diagnostic.html
-    // Negative values (except NOPOS) and 0 are not valid line or column numbers,
-    // except that you can cause this number to occur by putting "abc {}" in A.java.
-    // This will cause Invalid position: 0 masking the actual error message
-    //     a/A.java:1: class, interface, or enum expected
-    private[this] def checkNoPos(n: Long): Option[Long] =
-      n match {
-        case NOPOS       => None
-        case x if x <= 0 => None
-        case x           => Option(x)
-      }
-
-    override val line: Optional[Integer] = o2jo(checkNoPos(d.getLineNumber) map { x =>
-      new Integer(x.toInt)
-    })
-    def startPosition: Option[Long] = checkNoPos(d.getStartPosition)
-    def endPosition: Option[Long] = checkNoPos(d.getEndPosition)
-    override val offset: Optional[Integer] = o2jo(checkNoPos(d.getPosition) map { x =>
-      new Integer(x.toInt)
-    })
-    override def lineContent: String = {
-      def getDiagnosticLine: Option[String] =
-        try {
-          // See com.sun.tools.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper
-          val diagnostic = d.getClass.getField("d").get(d)
-          // See com.sun.tools.javac.util.JCDiagnostic#getDiagnosticSource
-          val getDiagnosticSourceMethod =
-            diagnostic.getClass.getDeclaredMethod("getDiagnosticSource")
-          val getPositionMethod = diagnostic.getClass.getDeclaredMethod("getPosition")
-          (Option(getDiagnosticSourceMethod.invoke(diagnostic)),
-           Option(getPositionMethod.invoke(diagnostic))) match {
-            case (Some(diagnosticSource), Some(position: java.lang.Long)) =>
-              // See com.sun.tools.javac.util.DiagnosticSource
-              val getLineMethod = diagnosticSource.getClass.getMethod("getLine", Integer.TYPE)
-              Option(getLineMethod.invoke(diagnosticSource, new Integer(position.intValue())))
-                .map(_.toString)
-            case _ => None
-          }
-        } catch {
-          // TODO - catch ReflectiveOperationException once sbt is migrated to JDK7
-          case ignored: Throwable => None
-        }
-
-      def getExpression: String =
-        Option(d.getSource) match {
-          case Some(source: JavaFileObject) =>
-            (Option(source.getCharContent(true)), startPosition, endPosition) match {
-              case (Some(cc), Some(start), Some(end)) =>
-                cc.subSequence(start.toInt, end.toInt).toString
-              case _ => ""
-            }
-          case _ => ""
-        }
+object DiagnosticsReporter {
 
-      getDiagnosticLine.getOrElse(getExpression)
-    }
-    private val sourceUri: Option[String] = fixSource(d.getSource)
+  /**
+   * Strict and immutable implementation of Position.
+   */
+  private[sbt] final class PositionImpl(
+      sourceUri: Option[String],
+      override val line: Optional[Integer],
+      override val lineContent: String,
+      override val offset: Optional[Integer],
+      val startPosition: Option[Long],
+      val endPosition: Option[Long]
+  ) extends xsbti.Position {
     override val sourcePath: Optional[String] = o2jo(sourceUri)
     override val sourceFile: Optional[File] = o2jo(sourceUri.map(new File(_)))
     override val pointer: Optional[Integer] = o2jo(Option.empty[Integer])
     override val pointerSpace: Optional[String] = o2jo(Option.empty[String])
+
     override def toString: String =
       if (sourceUri.isDefined) s"${sourceUri.get}:${if (line.isPresent) line.get else -1}"
       else ""
-    private def fixSource[T <: JavaFileObject](source: T): Option[String] = {
-      try Option(source).map(_.toUri.normalize).map(new File(_)).map(_.getAbsolutePath)
-      catch {
-        case t: IllegalArgumentException =>
-          // Oracle JDK6 has a super dumb notion of what a URI is.  In fact, it's not even a legimitate URL, but a dump
-          // of the filename in a "I hope this works to toString it" kind of way.  This appears to work in practice
-          // but we may need to re-evaluate.
-          Option(source).map(_.toUri.toString)
+  }
+
+  private[sbt] object PositionImpl {
+
+    /**
+     * Extracts PositionImpl from a Java Diagnostic.
+     * The previous implementation of PositionImpl held on to the Diagostic object as a wrapper,
+     * and calculated the lineContent on the fly.
+     * This caused race condition on the Diagnostic object, resulting to NullPointerException.
+     * See https://github.com/sbt/sbt/issues/3623
+     */
+    def apply(d: Diagnostic[_ <: JavaFileObject]): PositionImpl = {
+      // https://docs.oracle.com/javase/7/docs/api/javax/tools/Diagnostic.html
+      // Negative values (except NOPOS) and 0 are not valid line or column numbers,
+      // except that you can cause this number to occur by putting "abc {}" in A.java.
+      // This will cause Invalid position: 0 masking the actual error message
+      //     a/A.java:1: class, interface, or enum expected
+      def checkNoPos(n: Long): Option[Long] =
+        n match {
+          case NOPOS       => None
+          case x if x <= 0 => None
+          case x           => Option(x)
+        }
+
+      val line: Optional[Integer] = o2jo(checkNoPos(d.getLineNumber) map { x =>
+        new Integer(x.toInt)
+      })
+      val startPosition: Option[Long] = checkNoPos(d.getStartPosition)
+      val endPosition: Option[Long] = checkNoPos(d.getEndPosition)
+      val offset: Optional[Integer] = o2jo(checkNoPos(d.getPosition) map { x =>
+        new Integer(x.toInt)
+      })
+      def lineContent: String = {
+        def getDiagnosticLine: Option[String] =
+          try {
+            // See com.sun.tools.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper
+            val diagnostic = d.getClass.getField("d").get(d)
+            // See com.sun.tools.javac.util.JCDiagnostic#getDiagnosticSource
+            val getDiagnosticSourceMethod =
+              diagnostic.getClass.getDeclaredMethod("getDiagnosticSource")
+            val getPositionMethod = diagnostic.getClass.getDeclaredMethod("getPosition")
+            (Option(getDiagnosticSourceMethod.invoke(diagnostic)),
+             Option(getPositionMethod.invoke(diagnostic))) match {
+              case (Some(diagnosticSource), Some(position: java.lang.Long)) =>
+                // See com.sun.tools.javac.util.DiagnosticSource
+                val getLineMethod = diagnosticSource.getClass.getMethod("getLine", Integer.TYPE)
+                Option(getLineMethod.invoke(diagnosticSource, new Integer(position.intValue())))
+                  .map(_.toString)
+              case _ => None
+            }
+          } catch {
+            // TODO - catch ReflectiveOperationException once sbt is migrated to JDK7
+            case _: Throwable => None
+          }
+
+        def getExpression: String =
+          Option(d.getSource) match {
+            case Some(source: JavaFileObject) =>
+              (Option(source.getCharContent(true)), startPosition, endPosition) match {
+                case (Some(cc), Some(start), Some(end)) =>
+                  cc.subSequence(start.toInt, end.toInt).toString
+                case _ => ""
+              }
+            case _ => ""
+          }
+
+        getDiagnosticLine.getOrElse(getExpression)
+      }
+
+      def fixSource[T <: JavaFileObject](source: T): Option[String] = {
+        try Option(source).map(_.toUri.normalize).map(new File(_)).map(_.getAbsolutePath)
+        catch {
+          case t: IllegalArgumentException =>
+            // Oracle JDK6 has a super dumb notion of what a URI is.  In fact, it's not even a legimitate URL, but a dump
+            // of the filename in a "I hope this works to toString it" kind of way.  This appears to work in practice
+            // but we may need to re-evaluate.
+            Option(source).map(_.toUri.toString)
+        }
       }
+      new PositionImpl(fixSource(d.getSource),
+                       line,
+                       lineContent,
+                       offset,
+                       startPosition,
+                       endPosition)
     }
   }
 }