From 372bc8f0d5a93eece03e3a37399438976a4b73bd Mon Sep 17 00:00:00 2001 From: Pavel Marek Date: Thu, 8 Jun 2023 17:43:18 +0200 Subject: [PATCH] Improve compiler's diagnostic messages (#6931) Improve and colorize compiler's messages. Heavily inspired by `gcc`. --- CHANGELOG.md | 2 + build.sbt | 4 +- .../org/enso/interpreter/EnsoLanguage.java | 6 +- .../scala/org/enso/compiler/Compiler.scala | 209 +++++++++++++++--- .../compiler/test/CacheInvalidationTest.scala | 8 +- .../interpreter/test/InterpreterTest.scala | 1 + .../test/semantic/ImportsTest.scala | 54 ++--- .../OverloadsResolutionErrorTest.scala | 14 +- .../StrictCompileDiagnosticsTest.scala | 17 +- 9 files changed, 234 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5e44de3a65b..8f7098c523cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -815,6 +815,7 @@ - [Add project creation time to project metadata][6780] - [Upgrade GraalVM to 22.3.1 JDK17][6750] - [Ascribed types are checked during runtime][6790] +- [Improve and colorize compiler's diagnostic messages][6931] [3227]: https://github.com/enso-org/enso/pull/3227 [3248]: https://github.com/enso-org/enso/pull/3248 @@ -929,6 +930,7 @@ [6755]: https://github.com/enso-org/enso/pull/6755 [6780]: https://github.com/enso-org/enso/pull/6780 [6790]: https://github.com/enso-org/enso/pull/6790 +[6931]: https://github.com/enso-org/enso/pull/6931 # Enso 2.0.0-alpha.18 (2021-10-12) diff --git a/build.sbt b/build.sbt index 51f4d4394fe4..fa2689662255 100644 --- a/build.sbt +++ b/build.sbt @@ -485,6 +485,7 @@ val typesafeConfigVersion = "1.4.2" val junitVersion = "4.13.2" val junitIfVersion = "0.11" val netbeansApiVersion = "RELEASE140" +val fansiVersion = "0.4.0" // ============================================================================ // === Internal Libraries ===================================================== @@ -1339,7 +1340,8 @@ lazy val runtime = (project in file("engine/runtime")) "org.graalvm.truffle" % "truffle-api" % graalVersion % Benchmark, "org.typelevel" %% "cats-core" % catsVersion, "junit" % "junit" % junitVersion % Test, - "com.novocode" % "junit-interface" % junitIfVersion % Test exclude ("junit", "junit-dep") + "com.novocode" % "junit-interface" % junitIfVersion % Test exclude ("junit", "junit-dep"), + "com.lihaoyi" %% "fansi" % fansiVersion % "provided" ), Compile / compile / compileInputs := (Compile / compile / compileInputs) .dependsOn(CopyTruffleJAR.preCompileTask) diff --git a/engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java b/engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java index 113dd56a16f2..60ced6351790 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java @@ -16,7 +16,6 @@ import java.util.List; import java.util.Objects; import java.util.function.Consumer; -import java.util.stream.Collectors; import org.enso.compiler.Compiler; import org.enso.compiler.context.InlineContext; import org.enso.compiler.data.CompilerConfig; @@ -265,10 +264,7 @@ protected ExecutableNode parse(InlineParsingRequest request) throws Exception { throw new InlineParsingException("Unhandled entity: " + e.entity(), e); } catch (CompilationAbortedException e) { assert outputRedirect.toString().lines().count() > 1 : "Expected a header line from the compiler"; - String compilerErrOutput = outputRedirect.toString() - .lines() - .skip(1) - .collect(Collectors.joining(";")); + String compilerErrOutput = outputRedirect.toString(); throw new InlineParsingException(compilerErrOutput, e); } finally { silentCompiler.shutdown(false); diff --git a/engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala b/engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala index a951ca805b6b..434a0d86dace 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala @@ -1,7 +1,7 @@ package org.enso.compiler import com.oracle.truffle.api.TruffleLogger -import com.oracle.truffle.api.source.Source +import com.oracle.truffle.api.source.{Source, SourceSection} import org.enso.compiler.codegen.{IrToTruffle, RuntimeStubsGenerator} import org.enso.compiler.context.{FreshNameSupply, InlineContext, ModuleContext} import org.enso.compiler.core.IR @@ -21,22 +21,21 @@ import org.enso.interpreter.runtime.scope.{LocalScope, ModuleScope} import org.enso.interpreter.runtime.{EnsoContext, Module} import org.enso.pkg.QualifiedName import org.enso.polyglot.{LanguageInfo, RuntimeOptions} -import org.enso.syntax.text.Parser.IDMap import org.enso.syntax.text.Parser +import org.enso.syntax.text.Parser.IDMap import org.enso.syntax2.Tree import java.io.{PrintStream, StringReader} import java.util.concurrent.{ CompletableFuture, ExecutorService, + Future, LinkedBlockingDeque, ThreadPoolExecutor, TimeUnit } import java.util.logging.Level - import scala.jdk.OptionConverters._ -import java.util.concurrent.Future /** This class encapsulates the static transformation processes that take place * on source code, including parsing, desugaring, type-checking, static @@ -966,7 +965,6 @@ class Compiler( diagnostics .foldLeft(false) { case (result, (mod, diags)) => if (diags.nonEmpty) { - output.println(s"In module ${mod.getName}:") reportDiagnostics(diags, mod.getSource) || result } else { result @@ -985,41 +983,184 @@ class Compiler( diagnostics: List[IR.Diagnostic], source: Source ): Boolean = { - val errors = diagnostics.collect { case e: IR.Error => e } - val warnings = diagnostics.collect { case w: IR.Warning => w } + diagnostics.foreach(diag => + output.println(new DiagnosticFormatter(diag, source).format()) + ) + diagnostics.exists(_.isInstanceOf[IR.Error]) + } - if (warnings.nonEmpty) { - output.println("Compiler encountered warnings:") - warnings.foreach { warning => - output.println(formatDiagnostic(warning, source)) + /** Formatter of IR diagnostics. Heavily inspired by GCC. Can format one-line as well as multiline + * diagnostics. The output is colorized if the output stream supports ANSI colors. + * Also prints the offending lines from the source along with line number - the same way as + * GCC does. + * @param diagnostic the diagnostic to pretty print + * @param source the original source code + */ + private class DiagnosticFormatter( + private val diagnostic: IR.Diagnostic, + private val source: Source + ) { + private val maxLineNum = 99999 + private val blankLinePrefix = " | " + private val maxSourceLinesToPrint = 3 + private val linePrefixSize = blankLinePrefix.length + private val outSupportsAnsiColors: Boolean = outSupportsColors + private val (textAttrs: fansi.Attrs, subject: String) = diagnostic match { + case _: IR.Error => (fansi.Color.Red ++ fansi.Bold.On, "error: ") + case _: IR.Warning => (fansi.Color.Yellow ++ fansi.Bold.On, "warning: ") + case _ => throw new IllegalStateException("Unexpected diagnostic type") + } + private val sourceSection: Option[SourceSection] = + diagnostic.location match { + case Some(location) => + Some(source.createSection(location.start, location.length)) + case None => None } + private val shouldPrintLineNumber = sourceSection match { + case Some(section) => + section.getStartLine <= maxLineNum && section.getEndLine <= maxLineNum + case None => false } - if (errors.nonEmpty) { - output.println("Compiler encountered errors:") - errors.foreach { error => - output.println(formatDiagnostic(error, source)) + def format(): String = { + sourceSection match { + case Some(section) => + val isOneLine = section.getStartLine == section.getEndLine + val srcPath: String = + if (source.getPath == null && source.getName == null) { + "" + } else if (source.getPath != null) { + source.getPath + } else { + source.getName + } + if (isOneLine) { + val lineNumber = section.getStartLine + val startColumn = section.getStartColumn + val endColumn = section.getEndColumn + var str = fansi.Str() + str ++= fansi + .Str(srcPath + ":" + lineNumber + ":" + startColumn + ": ") + .overlay(fansi.Bold.On) + str ++= fansi.Str(subject).overlay(textAttrs) + str ++= diagnostic.formattedMessage + str ++= "\n" + str ++= oneLineFromSourceColored(lineNumber, startColumn, endColumn) + str ++= "\n" + str ++= underline(startColumn, endColumn) + if (outSupportsAnsiColors) { + str.render.stripLineEnd + } else { + str.plainText.stripLineEnd + } + } else { + var str = fansi.Str() + str ++= fansi + .Str( + srcPath + ":[" + section.getStartLine + ":" + section.getStartColumn + "-" + section.getEndLine + ":" + section.getEndColumn + "]: " + ) + .overlay(fansi.Bold.On) + str ++= fansi.Str(subject).overlay(textAttrs) + str ++= diagnostic.formattedMessage + str ++= "\n" + val printAllSourceLines = + section.getEndLine - section.getStartLine <= maxSourceLinesToPrint + val endLine = + if (printAllSourceLines) section.getEndLine + else section.getStartLine + maxSourceLinesToPrint + for (lineNum <- section.getStartLine to endLine) { + str ++= oneLineFromSource(lineNum) + str ++= "\n" + } + if (!printAllSourceLines) { + val restLineCount = + section.getEndLine - section.getStartLine - maxSourceLinesToPrint + str ++= blankLinePrefix + "... and " + restLineCount + " more lines ..." + str ++= "\n" + } + if (outSupportsAnsiColors) { + str.render.stripLineEnd + } else { + str.plainText.stripLineEnd + } + } + case None => + // We dont have location information, so we just print the message + var str = fansi.Str() + str ++= fansi + .Str(fileLocationFromSection(diagnostic.location, source)) + .overlay(fansi.Bold.On) + str ++= ": " + str ++= fansi.Str(subject).overlay(textAttrs) + str ++= diagnostic.formattedMessage + if (outSupportsAnsiColors) { + str.render.stripLineEnd + } else { + str.plainText.stripLineEnd + } } - true - } else { - false } - } - /** Pretty prints compiler diagnostics. - * - * @param diagnostic the diagnostic to pretty print - * @param source the original source code - * @return the result of pretty printing `diagnostic` - */ - private def formatDiagnostic( - diagnostic: IR.Diagnostic, - source: Source - ): String = { - fileLocationFromSection( - diagnostic.location, - source - ) + ": " + diagnostic.formattedMessage + /** @see https://github.com/termstandard/colors/ + * @see https://no-color.org/ + * @return + */ + private def outSupportsColors: Boolean = { + if (System.console() == null) { + // Non-interactive output is always without color support + return false + } + if (System.getenv("NO_COLOR") != null) { + return false + } + if (config.outputRedirect.isDefined) { + return false + } + if (System.getenv("COLORTERM") != null) { + return true + } + if (System.getenv("TERM") != null) { + val termEnv = System.getenv("TERM").toLowerCase + return termEnv.split("-").contains("color") || termEnv + .split("-") + .contains("256color") + } + return false + } + + private def oneLineFromSource(lineNum: Int): String = { + val line = source.createSection(lineNum).getCharacters.toString + linePrefix(lineNum) + line + } + + private def oneLineFromSourceColored( + lineNum: Int, + startCol: Int, + endCol: Int + ): String = { + val line = source.createSection(lineNum).getCharacters.toString + linePrefix(lineNum) + fansi + .Str(line) + .overlay(textAttrs, startCol - 1, endCol) + } + + private def linePrefix(lineNum: Int): String = { + if (shouldPrintLineNumber) { + val pipeSymbol = " | " + val prefixWhitespaces = + linePrefixSize - lineNum.toString.length - pipeSymbol.length + " " * prefixWhitespaces + lineNum + pipeSymbol + } else { + blankLinePrefix + } + } + + private def underline(startColumn: Int, endColumn: Int): String = { + val sectionLen = endColumn - startColumn + blankLinePrefix + + " " * (startColumn - 1) + + fansi.Str("^" + ("~" * sectionLen)).overlay(textAttrs) + } } private def fileLocationFromSection( @@ -1038,7 +1179,7 @@ class Compiler( "[" + locStr + "]" } .getOrElse("") - source.getName + srcLocation + source.getPath + ":" + srcLocation } /** Generates code for the truffle interpreter. diff --git a/engine/runtime/src/test/scala/org/enso/compiler/test/CacheInvalidationTest.scala b/engine/runtime/src/test/scala/org/enso/compiler/test/CacheInvalidationTest.scala index 946241122672..10fb87347a28 100644 --- a/engine/runtime/src/test/scala/org/enso/compiler/test/CacheInvalidationTest.scala +++ b/engine/runtime/src/test/scala/org/enso/compiler/test/CacheInvalidationTest.scala @@ -4,6 +4,10 @@ import org.apache.commons.lang3.SystemUtils import org.enso.interpreter.test.InterpreterException class CacheInvalidationTest extends ModifiedTest { + private def isDiagnosticLine(line: String): Boolean = { + line.contains(" | ") + } + "IR caching" should "should propagate invalidation" in { assume(!SystemUtils.IS_OS_WINDOWS) evalTestProjectIteration("Test_Caching_Invalidation", iteration = 1) @@ -18,7 +22,7 @@ class CacheInvalidationTest extends ModifiedTest { "Test_Caching_Invalidation", iteration = 3 ) should have message "Compilation aborted due to errors." - val outLines3 = consumeOut - outLines3(2) should endWith("The name `foo` could not be found.") + val outLines3 = consumeOut.filterNot(isDiagnosticLine) + outLines3.head should endWith("The name `foo` could not be found.") } } diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/InterpreterTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/InterpreterTest.scala index 7d96aef4f3a0..9a3b8c896740 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/InterpreterTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/InterpreterTest.scala @@ -118,6 +118,7 @@ class InterpreterContext( .err(err) .option(RuntimeOptions.LOG_LEVEL, "WARNING") .option(RuntimeOptions.DISABLE_IR_CACHES, "true") + .environment("NO_COLOR", "true") .logHandler(System.err) .in(in) .option( diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala index b344371b55c1..462111adff54 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala @@ -10,6 +10,10 @@ class ImportsTest extends PackageTest { exception.getLocalizedMessage } + private def isDiagnosticLine(line: String): Boolean = { + line.contains(" | ") + } + "Atoms and methods" should "be available for import" in { evalTestProject("TestSimpleImports") shouldEqual 20 } @@ -28,13 +32,14 @@ class ImportsTest extends PackageTest { the[InterpreterException] thrownBy evalTestProject( "Test_Bad_Imports" ) should have message "Compilation aborted due to errors." - val outLines = consumeOut - outLines(2) should include( + val outLines = consumeOut.filterNot(isDiagnosticLine) + outLines should have size 2 + outLines(0) should include( "Package containing the module Surely_This.Does_Not_Exist.My_Module " + "could not be loaded: The package could not be resolved: The library " + "`Surely_This.Does_Not_Exist` is not defined within the edition." ) - outLines(3) should include( + outLines(1) should include( "The module Enso_Test.Test_Bad_Imports.Oopsie does not exist." ) } @@ -43,9 +48,9 @@ class ImportsTest extends PackageTest { the[InterpreterException] thrownBy evalTestProject( "Test_Qualified_Error" ) should have message "Compilation aborted due to errors." - consumeOut - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + val outLines = consumeOut + outLines + .filterNot(isDiagnosticLine) .head should include("The name `Mk_X` could not be found.") } @@ -54,8 +59,7 @@ class ImportsTest extends PackageTest { "Test_Hiding_Error" ) should have message "Compilation aborted due to errors." consumeOut - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + .filterNot(isDiagnosticLine) .head should include("The name `X` could not be found.") } @@ -72,8 +76,7 @@ class ImportsTest extends PackageTest { "Test_Rename_Error" ) should have message "Compilation aborted due to errors." consumeOut - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + .filterNot(isDiagnosticLine) .head should include("The name `Atom` could not be found.") } @@ -124,7 +127,7 @@ class ImportsTest extends PackageTest { "TestSubmodulesNameConflict" ) should have message "Method `c_mod_method` of type C.type could not be found." val outLines = consumeOut - outLines(2) should include + outLines(1) should include "Declaration of type C shadows module local.TestSubmodulesNameConflict.A.B.C making it inaccessible via a qualified name." } @@ -159,11 +162,11 @@ class ImportsTest extends PackageTest { the[InterpreterException] thrownBy evalTestProject( "Test_Polyglot_Exports" ) should have message "Compilation aborted due to errors." - val outLines = consumeOut - outLines should have length 3 - outLines( - 2 - ) shouldEqual "Main.enso[5:16-5:19]: The name `Long` could not be found." + val outLines = consumeOut.filterNot(isDiagnosticLine) + outLines should have length 1 + outLines.head should include( + "Main.enso:5:16: error: The name `Long` could not be found." + ) } "Constructors" should "be importable" in { @@ -179,11 +182,11 @@ class ImportsTest extends PackageTest { "Test_Fully_Qualified_Name_Failure" ) should have message "Compilation aborted due to errors." - val outLines = consumeOut - outLines should have length 3 - outLines( - 2 - ) shouldEqual "Main.enso[2:14-2:17]: Fully qualified name references a library Standard.Base but an import statement for it is missing." + val outLines = consumeOut.filterNot(isDiagnosticLine) + outLines should have length 1 + outLines.head should include( + "Main.enso:2:14: error: Fully qualified name references a library Standard.Base but an import statement for it is missing." + ) } "Fully qualified names" should "be resolved when library has already been loaded" in { @@ -199,11 +202,10 @@ class ImportsTest extends PackageTest { the[InterpreterException] thrownBy evalTestProject( "Test_Fully_Qualified_Name_Conflict" ) should have message "Method `Foo` of type Atom.type could not be found." - val outLines = consumeOut - outLines should have length 3 - outLines( - 2 - ) shouldEqual "Main.enso[2:1-2:57]: The exported type `Atom` in `local.Test_Fully_Qualified_Name_Conflict.Atom` module will cause name conflict when attempting to use a fully qualified name of the `local.Test_Fully_Qualified_Name_Conflict.Atom.Foo` module." + val outLines = consumeOut.filterNot(isDiagnosticLine) + outLines.head should include( + "Main.enso:2:1: warning: The exported type `Atom` in `local.Test_Fully_Qualified_Name_Conflict.Atom` module will cause name conflict when attempting to use a fully qualified name of the `local.Test_Fully_Qualified_Name_Conflict.Atom.Foo` module." + ) } "Deeply nested modules" should "infer correct synthetic modules" in { diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala index b2be6cb808d9..f2c0a5736c9a 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala @@ -14,6 +14,10 @@ class OverloadsResolutionErrorTest extends InterpreterTest { override def contextModifiers: Option[Context#Builder => Context#Builder] = Some(_.option(RuntimeOptions.STRICT_ERRORS, "true")) + private def isDiagnosticLine(line: String): Boolean = { + line.contains(" | ") + } + override def specify(implicit interpreterContext: InterpreterContext ): Unit = { @@ -31,10 +35,9 @@ class OverloadsResolutionErrorTest extends InterpreterTest { val diagnostics = consumeOut diagnostics - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + .filterNot(isDiagnosticLine) .toSet shouldEqual Set( - "Test[4:1-4:16]: Method overloads are not supported: Nothing.foo is defined multiple times in this module." + "Test:4:1: error: Method overloads are not supported: Nothing.foo is defined multiple times in this module." ) } @@ -50,10 +53,9 @@ class OverloadsResolutionErrorTest extends InterpreterTest { val diagnostics = consumeOut diagnostics - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + .filterNot(isDiagnosticLine) .toSet shouldEqual Set( - "Test[3:1-3:11]: Redefining atoms is not supported: MyAtom is defined multiple times in this module." + "Test:3:1: error: Redefining atoms is not supported: MyAtom is defined multiple times in this module." ) } diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/StrictCompileDiagnosticsTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/StrictCompileDiagnosticsTest.scala index 4f9def1556ee..0bc74cf187c3 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/StrictCompileDiagnosticsTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/StrictCompileDiagnosticsTest.scala @@ -14,6 +14,10 @@ class StrictCompileDiagnosticsTest extends InterpreterTest { override def contextModifiers: Option[Context#Builder => Context#Builder] = Some(_.option(RuntimeOptions.STRICT_ERRORS, "true")) + private def isDiagnosticLine(line: String): Boolean = { + line.contains(" | ") + } + override def specify(implicit interpreterContext: InterpreterContext ): Unit = { @@ -30,14 +34,13 @@ class StrictCompileDiagnosticsTest extends InterpreterTest { val errors = consumeOut errors - .filterNot(_.contains("Compiler encountered")) - .filterNot(_.contains("In module")) + .filterNot(isDiagnosticLine) .toSet shouldEqual Set( - "Test[2:9-2:10]: Parentheses can't be empty.", - "Test[3:5-3:9]: Variable x is being redefined.", - "Test[4:9-4:9]: Unexpected expression.", - "Test[4:5-4:5]: Unused variable y.", - "Test[2:5-2:5]: Unused variable x." + "Test:2:9: error: Parentheses can't be empty.", + "Test:3:5: error: Variable x is being redefined.", + "Test:4:9: error: Unexpected expression.", + "Test:4:5: warning: Unused variable y.", + "Test:2:5: warning: Unused variable x." ) } }