From 8d158e5ab6bc391357d2a451c13793f48ad83568 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Thu, 30 Oct 2014 13:28:41 -0400 Subject: [PATCH] TODO cleanups based on @havocp's comments. --- .../compiler/javac/DiagnosticsReporter.scala | 17 +++++++++++------ .../compiler/javac/JavaCompilerAdapter.scala | 9 +++++---- .../sbt/compiler/javac/JavacProcessLogger.scala | 11 +++++++++-- .../scala/sbt/compiler/javac/LocalJava.scala | 2 ++ .../compiler/javac/ProcessLoggerWriter.scala | 9 --------- 5 files changed, 27 insertions(+), 21 deletions(-) diff --git a/compile/src/main/scala/sbt/compiler/javac/DiagnosticsReporter.scala b/compile/src/main/scala/sbt/compiler/javac/DiagnosticsReporter.scala index cd908bc63..d898fbac2 100644 --- a/compile/src/main/scala/sbt/compiler/javac/DiagnosticsReporter.scala +++ b/compile/src/main/scala/sbt/compiler/javac/DiagnosticsReporter.scala @@ -11,14 +11,16 @@ import xsbti.{ Severity, Reporter } * @param reporter */ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[JavaFileObject] { - + val END_OF_LINE_MATCHER = "[\r\n]|[\r]|[\n]" + val EOL = System.getProperty("line.separator") private def fixedDiagnosticMessage(d: Diagnostic[_ <: JavaFileObject]): String = { def getRawMessage = d.getMessage(null) def fixWarnOrErrorMessage = { val tmp = getRawMessage // we fragment off the line/source/type report from the message. + // NOTE - End of line handling may be off. val lines: Seq[String] = - tmp.split("[\r\n]") match { + tmp.split(END_OF_LINE_MATCHER) match { case Array(head, tail @ _*) => val newHead = head.split(":").last newHead +: tail @@ -26,8 +28,7 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J head.split(":").last :: Nil case Array() => Seq.empty[String] } - // TODO - Real EOL - lines.mkString("\n") + lines.mkString(EOL) } d.getKind match { case Diagnostic.Kind.ERROR | Diagnostic.Kind.WARNING | Diagnostic.Kind.MANDATORY_WARNING => fixWarnOrErrorMessage @@ -38,7 +39,9 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J 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 + // 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) } } @@ -56,7 +59,9 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J Logger.o2m(if (d.getLineNumber == -1) None else Option(new Integer(d.getLineNumber.toInt))) override def lineContent = { - // TODO - Is this pulling error correctly? Is null an ok return value? + // TODO - Is this pulling contents of the line correctly? + // Would be ok to just return null if this version of the JDK doesn't support grabbing + // source lines? Option(d.getSource). flatMap(s => Option(s.getCharContent(true))). map(_.subSequence(d.getStartPosition.intValue, d.getEndPosition.intValue).toString). diff --git a/compile/src/main/scala/sbt/compiler/javac/JavaCompilerAdapter.scala b/compile/src/main/scala/sbt/compiler/javac/JavaCompilerAdapter.scala index fb090770e..cf581679f 100644 --- a/compile/src/main/scala/sbt/compiler/javac/JavaCompilerAdapter.scala +++ b/compile/src/main/scala/sbt/compiler/javac/JavaCompilerAdapter.scala @@ -16,7 +16,8 @@ import xsbti.compile.{ MultipleOutput, SingleOutput, Output } */ class JavaCompilerAdapter(delegate: NewJavaTool, scalaInstance: xsbti.compile.ScalaInstance, cpOptions: xsbti.compile.ClasspathOptions) extends xsbti.compile.JavaCompiler { override final def compile(sources: Array[File], classpath: Array[File], output: Output, options: Array[String], log: xsbti.Logger): Unit = { - // TODO - 5 max errors ok? + // TODO - 5 max errors ok? We're not expecting this code path to be called, ever. This is only for clients who try to use the xsbti.compile.JavaCompiler interface + // outside of the incremental compiler, for some reason. val reporter = new LoggerReporter(5, log) compileWithReporter(sources, classpath, output, options, reporter, log) } @@ -26,11 +27,11 @@ class JavaCompilerAdapter(delegate: NewJavaTool, scalaInstance: xsbti.compile.Sc case mo: MultipleOutput => throw new RuntimeException("Javac doesn't support multiple output directories") } val args = commandArguments(Seq(), classpath, target, options, log) - // TODO - is sorting the sources correct here? + // We sort the sources for deterministic results. val success = delegate.run(sources.sortBy(_.getAbsolutePath), args)(log, reporter) - // TODO - What should this error message be, and how do we ensure things are correct here? if (!success) { - // TODO - Should we track only the problems from this javac? + // TODO - Will the reporter have problems from Scalac? It appears like it does not, only from the most recent run. + // This is because the incremental compiler will not run javac if scalac fails. throw new CompileFailed(args.toArray, "javac returned nonzero exit code", reporter.problems()) } } diff --git a/compile/src/main/scala/sbt/compiler/javac/JavacProcessLogger.scala b/compile/src/main/scala/sbt/compiler/javac/JavacProcessLogger.scala index ea4a179dc..b16fbd813 100644 --- a/compile/src/main/scala/sbt/compiler/javac/JavacProcessLogger.scala +++ b/compile/src/main/scala/sbt/compiler/javac/JavacProcessLogger.scala @@ -89,7 +89,7 @@ object JavaNoPosition extends Position { /** A wrapper around xsbti.Problem with java-specific options. */ final case class JavaProblem(val position: Position, val severity: Severity, val message: String) extends xsbti.Problem { - override def category: String = null // TODO - what is this even supposed to be? + override def category: String = "javac" // TODO - what is this even supposed to be? For now it appears unused. override def toString = s"$severity @ $position - $message" } @@ -98,6 +98,7 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa // Here we track special handlers to catch "Note:" and "Warning:" lines. private val NOTE_LINE_PREFIXES = Array("Note: ", "\u6ce8: ", "\u6ce8\u610f\uff1a ") private val WARNING_PREFIXES = Array("warning", "\u8b66\u544a", "\u8b66\u544a\uff1a") + private val END_OF_LINE = System.getProperty("line.separator") override val skipWhitespace = false @@ -110,6 +111,7 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa } // Parses the rest of an input line. val restOfLine: Parser[String] = + // TODO - Can we use END_OF_LINE here without issues? allUntilChars(Array('\n', '\r')) ~ "[\r]?[\n]?".r ^^ { case msg ~ _ => msg } @@ -151,9 +153,14 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa } catch { case e: NumberFormatException => false } + + // Helper to extract an integer from a string + private object ParsedInteger { + def unapply(s: String): Option[Int] = try Some(Integer.parseInt(s)) catch { case e: NumberFormatException => None } + } // Parses a line number val line: Parser[Int] = allUntilChar(':') ^? { - case x if isInteger(x) => Integer.parseInt(x) + case ParsedInteger(x) => x } val allUntilCharat: Parser[String] = allUntilChar('^') diff --git a/compile/src/main/scala/sbt/compiler/javac/LocalJava.scala b/compile/src/main/scala/sbt/compiler/javac/LocalJava.scala index 2db9ca63a..406c49c3a 100644 --- a/compile/src/main/scala/sbt/compiler/javac/LocalJava.scala +++ b/compile/src/main/scala/sbt/compiler/javac/LocalJava.scala @@ -47,6 +47,8 @@ final class LocalJavadoc() extends NewJavadoc { try { exitCode = LocalJava.unsafeJavadoc(allArguments.toArray, warnOrError, warnOrError, infoWriter) } finally { + warnOrError.close() + infoWriter.close() javacLogger.flush(exitCode) } // We return true or false, depending on success. diff --git a/compile/src/main/scala/sbt/compiler/javac/ProcessLoggerWriter.scala b/compile/src/main/scala/sbt/compiler/javac/ProcessLoggerWriter.scala index c058155f9..eb64bfdae 100644 --- a/compile/src/main/scala/sbt/compiler/javac/ProcessLoggerWriter.scala +++ b/compile/src/main/scala/sbt/compiler/javac/ProcessLoggerWriter.scala @@ -4,10 +4,7 @@ import sbt.{ Level, ProcessLogger } /** Delegates a stream into a process logger. Mimics LoggerWriter, but for the ProcessLogger interface which differs. */ private class ProcessLoggerWriter(delegate: ProcessLogger, level: Level.Value, nl: String = System.getProperty("line.separator")) extends java.io.Writer { - private[this] val buffer = new StringBuilder - private[this] val lines = new collection.mutable.ListBuffer[String] - override def close() = flush() override def flush(): Unit = synchronized { @@ -16,12 +13,6 @@ private class ProcessLoggerWriter(delegate: ProcessLogger, level: Level.Value, n buffer.clear() } } - def flushLines(level: Level.Value): Unit = - synchronized { - for (line <- lines) - log(line) - lines.clear() - } override def write(content: Array[Char], offset: Int, length: Int): Unit = synchronized { buffer.appendAll(content, offset, length)