[torque] More flexibel and uniform error reporting

This CL changes the existing TorqueError struct into a more general
TorqueMessage by adding a "kind" enum. The contextual for lint errors
is removed and replaced by a list of TorqueMessages.

A MessageBuilder is introduced to help with the different
combinations of present information and method of reporting. A lint
error with custom SourcePosition can be reported like this:

Lint("naming convention error").Position(<src_pos_var>);

While a fatal error, with CurrentSourcePosition can be thrown
like this:

Error("something went horrible wrong").Throw();

This approach is both backwards compatible and should prove flexible
enough to add more information to messages or add other message kinds.

Bug: v8:7793
Change-Id: Ib04fa188e34b3e8e9a6526a086f80da8f690a6f5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1617245
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61696}

src/torque/contextual.h

@@ -67,6 +67,9 @@ class ContextualVariable {
 
  private:
   V8_EXPORT_PRIVATE static VarType*& Top();
+
+  static bool HasScope() { return Top() != nullptr; }
+  friend class MessageBuilder;
 };
 
 // Usage: DECLARE_CONTEXTUAL_VARIABLE(VarName, VarType)

src/torque/ls/json-parser.cc

@@ -184,13 +184,15 @@ JsonParserResult ParseJson(const std::string& input) {
   // Torque needs a CurrentSourceFile scope during parsing.
   // As JSON lives in memory only, a unknown file scope is created.
   SourceFileMap::Scope source_map_scope;
+  TorqueMessages::Scope messages_scope;
   CurrentSourceFile::Scope unkown_file(SourceFileMap::AddSource("<json>"));
 
   JsonParserResult result;
   try {
     result.value = (*JsonGrammar().Parse(input)).Cast<JsonValue>();
-  } catch (TorqueError& error) {
-    result.error = error;
+  } catch (TorqueAbortCompilation&) {
+    CHECK(!TorqueMessages::Get().empty());
+    result.error = TorqueMessages::Get().front();
   }
   return result;
 }

src/torque/ls/json-parser.h

@@ -17,7 +17,7 @@ namespace ls {
 
 struct JsonParserResult {
   JsonValue value;
-  base::Optional<TorqueError> error;
+  base::Optional<TorqueMessage> error;
 };
 
 V8_EXPORT_PRIVATE JsonParserResult ParseJson(const std::string& input);

src/torque/ls/message-handler.cc

@@ -86,32 +86,21 @@ void ResetCompilationErrorDiagnostics(MessageWriter writer) {
 //   2) send one notification per entry (per file).
 class DiagnosticCollector {
  public:
-  void AddTorqueError(const TorqueError& error) {
-    SourceId id = error.position ? error.position->source : SourceId::Invalid();
+  void AddTorqueMessage(const TorqueMessage& message) {
+    SourceId id =
+        message.position ? message.position->source : SourceId::Invalid();
     auto& notification = GetOrCreateNotificationForSource(id);
 
     Diagnostic diagnostic = notification.params().add_diagnostics();
-    diagnostic.set_severity(Diagnostic::kError);
-    diagnostic.set_message(error.message);
+    diagnostic.set_severity(ServerityFor(message.kind));
+    diagnostic.set_message(message.message);
     diagnostic.set_source("Torque Compiler");
 
-    if (error.position) {
-      PopulateRangeFromSourcePosition(diagnostic.range(), *error.position);
+    if (message.position) {
+      PopulateRangeFromSourcePosition(diagnostic.range(), *message.position);
     }
   }
 
-  void AddLintError(const LintError& error) {
-    auto& notification =
-        GetOrCreateNotificationForSource(error.position.source);
-
-    Diagnostic diagnostic = notification.params().add_diagnostics();
-    diagnostic.set_severity(Diagnostic::kWarning);
-    diagnostic.set_message(error.message);
-    diagnostic.set_source("Torque Compiler");
-
-    PopulateRangeFromSourcePosition(diagnostic.range(), error.position);
-  }
-
   std::map<SourceId, PublishDiagnosticsNotification>& notifications() {
     return notifications_;
   }
@@ -139,16 +128,25 @@ class DiagnosticCollector {
     range.end().set_character(position.end.column);
   }
 
+  Diagnostic::DiagnosticSeverity ServerityFor(TorqueMessage::Kind kind) {
+    switch (kind) {
+      case TorqueMessage::Kind::kError:
+        return Diagnostic::kError;
+      case TorqueMessage::Kind::kLint:
+        return Diagnostic::kWarning;
+    }
+  }
+
   std::map<SourceId, PublishDiagnosticsNotification> notifications_;
 };
 
 void SendCompilationDiagnostics(const TorqueCompilerResult& result,
                                 MessageWriter writer) {
   DiagnosticCollector collector;
-  if (result.error) collector.AddTorqueError(*result.error);
 
-  for (const LintError& error : result.lint_errors) {
-    collector.AddLintError(error);
+  // TODO(szuend): Split up messages by SourceId and sort them by line number.
+  for (const TorqueMessage& message : result.messages) {
+    collector.AddTorqueMessage(message);
   }
 
   for (auto& pair : collector.notifications()) {

src/torque/torque-compiler.cc

@@ -39,7 +39,7 @@ void ReadAndParseTorqueFile(const std::string& path) {
   }
 
   if (!maybe_content) {
-    ReportErrorWithoutPosition("Cannot open file path/uri: ", path);
+    Error("Cannot open file path/uri: ", path).Throw();
   }
 
   ParseTorque(*maybe_content);
@@ -102,20 +102,21 @@ TorqueCompilerResult CompileTorque(const std::string& source,
   SourceFileMap::Scope source_map_scope;
   CurrentSourceFile::Scope no_file_scope(SourceFileMap::AddSource("<torque>"));
   CurrentAst::Scope ast_scope;
-  LintErrors::Scope lint_errors_scope;
+  TorqueMessages::Scope messages_scope;
   LanguageServerData::Scope server_data_scope;
 
   TorqueCompilerResult result;
   try {
     ParseTorque(source);
     CompileCurrentAst(options);
-  } catch (TorqueError& error) {
-    result.error = error;
+  } catch (TorqueAbortCompilation&) {
+    // Do nothing. The relevant TorqueMessage is part of the
+    // TorqueMessages contextual.
   }
 
   result.source_file_map = SourceFileMap::Get();
   result.language_server_data = std::move(LanguageServerData::Get());
-  result.lint_errors = LintErrors::Get();
+  result.messages = std::move(TorqueMessages::Get());
 
   return result;
 }
@@ -125,20 +126,21 @@ TorqueCompilerResult CompileTorque(std::vector<std::string> files,
   SourceFileMap::Scope source_map_scope;
   CurrentSourceFile::Scope unknown_source_file_scope(SourceId::Invalid());
   CurrentAst::Scope ast_scope;
-  LintErrors::Scope lint_errors_scope;
+  TorqueMessages::Scope messages_scope;
   LanguageServerData::Scope server_data_scope;
 
   TorqueCompilerResult result;
   try {
     for (const auto& path : files) ReadAndParseTorqueFile(path);
     CompileCurrentAst(options);
-  } catch (TorqueError& error) {
-    result.error = error;
+  } catch (TorqueAbortCompilation&) {
+    // Do nothing. The relevant TorqueMessage is part of the
+    // TorqueMessages contextual.
   }
 
   result.source_file_map = SourceFileMap::Get();
   result.language_server_data = std::move(LanguageServerData::Get());
-  result.lint_errors = LintErrors::Get();
+  result.messages = std::move(TorqueMessages::Get());
 
   return result;
 }

src/torque/torque-compiler.h

@@ -35,13 +35,8 @@ struct TorqueCompilerResult {
   // Set the corresponding options flag to enable.
   LanguageServerData language_server_data;
 
-  // Lint errors collected during compilation. These are
-  // mainly naming convention violations.
-  std::vector<LintError> lint_errors;
-
-  // If any error occurred during either parsing or compilation,
-  // this field will be set.
-  base::Optional<TorqueError> error;
+  // Errors collected during compilation.
+  std::vector<TorqueMessage> messages;
 };
 
 V8_EXPORT_PRIVATE TorqueCompilerResult

src/torque/torque-parser.cc

@@ -249,7 +249,7 @@ void CheckNotDeferredStatement(Statement* statement) {
   CurrentSourcePosition::Scope source_position(statement->pos);
   if (BlockStatement* block = BlockStatement::DynamicCast(statement)) {
     if (block->deferred) {
-      ReportLintError(
+      Lint(
           "cannot use deferred with a statement block here, it will have no "
           "effect");
     }
@@ -657,14 +657,10 @@ class AnnotationSet {
     auto list = iter->NextAs<std::vector<Identifier*>>();
     for (const Identifier* i : list) {
       if (allowed.find(i->value) == allowed.end()) {
-        std::stringstream s;
-        s << "Annotation " << i->value << " is not allowed here";
-        ReportLintError(s.str(), i->pos);
+        Lint("Annotation ", i->value, " is not allowed here").Position(i->pos);
       }
       if (!set_.insert(i->value).second) {
-        std::stringstream s;
-        s << "Duplicate annotation " << i->value;
-        ReportLintError(s.str(), i->pos);
+        Lint("Duplicate annotation ", i->value).Position(i->pos);
       }
     }
   }

src/torque/torque.cc

@@ -9,6 +9,15 @@ namespace v8 {
 namespace internal {
 namespace torque {
 
+std::string ErrorPrefixFor(TorqueMessage::Kind kind) {
+  switch (kind) {
+    case TorqueMessage::Kind::kError:
+      return "Torque Error";
+    case TorqueMessage::Kind::kLint:
+      return "Lint error";
+  }
+}
+
 int WrappedMain(int argc, const char** argv) {
   std::string output_directory;
   std::vector<std::string> files;
@@ -35,19 +44,16 @@ int WrappedMain(int argc, const char** argv) {
   // resolve the file name. Needed to report errors and lint warnings.
   SourceFileMap::Scope source_file_map_scope(result.source_file_map);
 
-  if (result.error) {
-    TorqueError& error = *result.error;
-    if (error.position) std::cerr << PositionAsString(*error.position) << ": ";
-    std::cerr << "Torque error: " << error.message << "\n";
-    v8::base::OS::Abort();
+  for (const TorqueMessage& message : result.messages) {
+    if (message.position) {
+      std::cerr << *message.position << ": ";
+    }
+
+    std::cerr << ErrorPrefixFor(message.kind) << ": " << message.message
+              << "\n";
   }
 
-  for (const LintError& error : result.lint_errors) {
-    std::cerr << PositionAsString(error.position)
-              << ": Lint error: " << error.message << "\n";
-  }
-
-  if (!result.lint_errors.empty()) v8::base::OS::Abort();
+  if (!result.messages.empty()) v8::base::OS::Abort();
 
   return 0;
 }

src/torque/types.cc

@@ -341,12 +341,12 @@ void ClassType::Finalize() const {
     if (const ClassType* super_class = ClassType::DynamicCast(parent())) {
       if (super_class->HasIndexedField()) flags_ |= ClassFlag::kHasIndexedField;
       if (!super_class->IsAbstract() && !HasSameInstanceTypeAsParent()) {
-        ReportLintError(
+        Lint(
             "Super class must either be abstract (annotate super class with "
             "@abstract) "
             "or this class must have the same instance type as the super class "
-            "(annotate this class with @hasSameInstanceTypeAsParent).",
-            this->decl_->name->pos);
+            "(annotate this class with @hasSameInstanceTypeAsParent).")
+            .Position(this->decl_->name->pos);
       }
     }
   }
@@ -359,11 +359,10 @@ void ClassType::Finalize() const {
       if (!field_type->IsSubtypeOf(TypeOracle::GetObjectType()) ||
           field_type->IsSubtypeOf(TypeOracle::GetSmiType()) ||
           field_type->IsSubtypeOf(TypeOracle::GetNumberType())) {
-        std::stringstream s;
-        s << "Generation of C++ class for Torque class " << name()
-          << " is not supported yet, because the type of field "
-          << f.name_and_type.name << " cannot be handled yet";
-        ReportLintError(s.str(), f.pos);
+        Lint("Generation of C++ class for Torque class ", name(),
+             " is not supported yet, because the type of field ",
+             f.name_and_type.name, " cannot be handled yet")
+            .Position(f.pos);
       }
     }
   }

src/torque/utils.cc

@@ -15,7 +15,7 @@ namespace v8 {
 namespace internal {
 namespace torque {
 
-DEFINE_CONTEXTUAL_VARIABLE(LintErrors)
+DEFINE_CONTEXTUAL_VARIABLE(TorqueMessages)
 
 std::string StringLiteralUnquote(const std::string& s) {
   DCHECK(('"' == s.front() && '"' == s.back()) ||
@@ -124,23 +124,27 @@ std::string CurrentPositionAsString() {
   return PositionAsString(CurrentSourcePosition::Get());
 }
 
-[[noreturn]] void ThrowTorqueError(const std::string& message,
-                                   bool include_position) {
-  TorqueError error(message);
-  if (include_position) error.position = CurrentSourcePosition::Get();
-  throw error;
-}
-
-void ReportLintError(const std::string& error, SourcePosition pos) {
-  LintErrors::Get().push_back({error, pos});
-}
-
 void NamingConventionError(const std::string& type, const std::string& name,
                            const std::string& convention) {
-  std::stringstream sstream;
-  sstream << type << " \"" << name << "\" does not follow \"" << convention
-          << "\" naming convention.";
-  ReportLintError(sstream.str());
+  Lint(type, " \"", name, "\" does not follow \"", convention,
+       "\" naming convention.");
+}
+
+MessageBuilder::MessageBuilder(const std::string& message,
+                               TorqueMessage::Kind kind) {
+  base::Optional<SourcePosition> position;
+  if (CurrentSourcePosition::HasScope()) {
+    position = CurrentSourcePosition::Get();
+  }
+  message_ = TorqueMessage{message, position, kind};
+}
+
+void MessageBuilder::Report() const {
+  TorqueMessages::Get().push_back(message_);
+}
+
+[[noreturn]] void MessageBuilder::Throw() const {
+  throw TorqueAbortCompilation{};
 }
 
 namespace {

src/torque/utils.h

@@ -28,16 +28,60 @@ std::string StringLiteralQuote(const std::string& s);
 V8_EXPORT_PRIVATE base::Optional<std::string> FileUriDecode(
     const std::string& s);
 
-struct LintError {
+struct TorqueMessage {
+  enum class Kind { kError, kLint };
+
   std::string message;
-  SourcePosition position;
+  base::Optional<SourcePosition> position;
+  Kind kind;
 };
-DECLARE_CONTEXTUAL_VARIABLE(LintErrors, std::vector<LintError>);
 
-void ReportLintError(const std::string& error,
-                     SourcePosition pos = CurrentSourcePosition::Get());
+DECLARE_CONTEXTUAL_VARIABLE(TorqueMessages, std::vector<TorqueMessage>);
 
-// Prints a LintError with the format "{type} '{name}' doesn't follow
+class V8_EXPORT_PRIVATE MessageBuilder {
+ public:
+  MessageBuilder(const std::string& message, TorqueMessage::Kind kind);
+
+  MessageBuilder& Position(SourcePosition position) {
+    message_.position = position;
+    return *this;
+  }
+
+  [[noreturn]] void Throw() const;
+
+  ~MessageBuilder() {
+    // This will also get called in case the error is thrown.
+    Report();
+  }
+
+ private:
+  MessageBuilder() = delete;
+  void Report() const;
+
+  TorqueMessage message_;
+};
+
+// Used for throwing exceptions. Retrieve TorqueMessage from the contextual
+// for specific error information.
+struct TorqueAbortCompilation {};
+
+template <class... Args>
+static MessageBuilder Message(TorqueMessage::Kind kind, Args&&... args) {
+  std::stringstream stream;
+  USE((stream << std::forward<Args>(args))...);
+  return MessageBuilder(stream.str(), kind);
+}
+
+template <class... Args>
+MessageBuilder Error(Args&&... args) {
+  return Message(TorqueMessage::Kind::kError, std::forward<Args>(args)...);
+}
+template <class... Args>
+MessageBuilder Lint(Args&&... args) {
+  return Message(TorqueMessage::Kind::kLint, std::forward<Args>(args)...);
+}
+
+// Report a LintError with the format "{type} '{name}' doesn't follow
 // '{convention}' naming convention".
 void NamingConventionError(const std::string& type, const std::string& name,
                            const std::string& convention);
@@ -48,26 +92,9 @@ bool IsSnakeCase(const std::string& s);
 bool IsValidNamespaceConstName(const std::string& s);
 bool IsValidTypeName(const std::string& s);
 
-struct TorqueError {
-  explicit TorqueError(const std::string& message) : message(message) {}
-
-  std::string message;
-  base::Optional<SourcePosition> position;
-};
-
-[[noreturn]] void ThrowTorqueError(const std::string& error,
-                                   bool include_position);
 template <class... Args>
 [[noreturn]] void ReportError(Args&&... args) {
-  std::stringstream s;
-  USE((s << std::forward<Args>(args))...);
-  ThrowTorqueError(s.str(), true);
-}
-template <class... Args>
-[[noreturn]] void ReportErrorWithoutPosition(Args&&... args) {
-  std::stringstream s;
-  USE((s << std::forward<Args>(args))...);
-  ThrowTorqueError(s.str(), false);
+  Error(std::forward<Args>(args)...).Throw();
 }
 
 std::string CapifyStringWithUnderscores(const std::string& camellified_string);

test/unittests/torque/ls-message-unittest.cc

@@ -115,10 +115,12 @@ TEST(LanguageServerMessage, GotoDefinition) {
 TEST(LanguageServerMessage, CompilationErrorSendsDiagnostics) {
   DiagnosticsFiles::Scope diagnostic_files_scope;
   LanguageServerData::Scope server_data_scope;
+  TorqueMessages::Scope messages_scope;
   SourceFileMap::Scope source_file_map_scope;
 
   TorqueCompilerResult result;
-  result.error = TorqueError("compilation failed somehow");
+  { Error("compilation failed somehow"); }
+  result.messages = std::move(TorqueMessages::Get());
   result.source_file_map = SourceFileMap::Get();
 
   CompilationFinished(std::move(result), [](JsonValue& raw_response) {
@@ -137,17 +139,21 @@ TEST(LanguageServerMessage, CompilationErrorSendsDiagnostics) {
 
 TEST(LanguageServerMessage, LintErrorSendsDiagnostics) {
   DiagnosticsFiles::Scope diagnostic_files_scope;
-  LintErrors::Scope lint_errors_scope;
+  TorqueMessages::Scope messages_scope;
   LanguageServerData::Scope server_data_scope;
   SourceFileMap::Scope sourc_file_map_scope;
   SourceId test_id = SourceFileMap::AddSource("test.tq");
 
   // No compilation errors but two lint warnings.
+  {
+    SourcePosition pos1{test_id, {0, 0}, {0, 1}};
+    SourcePosition pos2{test_id, {1, 0}, {1, 1}};
+    Lint("lint error 1").Position(pos1);
+    Lint("lint error 2").Position(pos2);
+  }
+
   TorqueCompilerResult result;
-  SourcePosition pos1{test_id, {0, 0}, {0, 1}};
-  SourcePosition pos2{test_id, {1, 0}, {1, 1}};
-  result.lint_errors.push_back({"lint error 1", pos1});
-  result.lint_errors.push_back({"lint error 2", pos2});
+  result.messages = std::move(TorqueMessages::Get());
   result.source_file_map = SourceFileMap::Get();
 
   CompilationFinished(std::move(result), [](JsonValue& raw_response) {

test/unittests/torque/torque-unittest.cc

@@ -42,8 +42,8 @@ TEST(Torque, TypeNamingConventionLintError) {
 
   const TorqueCompilerResult result = TestCompileTorque(source);
 
-  ASSERT_EQ(result.lint_errors.size(), static_cast<size_t>(1));
-  EXPECT_THAT(result.lint_errors[0].message, HasSubstr("\"foo\""));
+  ASSERT_EQ(result.messages.size(), static_cast<size_t>(1));
+  EXPECT_THAT(result.messages[0].message, HasSubstr("\"foo\""));
 }
 
 TEST(Torque, StructNamingConventionLintError) {
@@ -56,8 +56,8 @@ TEST(Torque, StructNamingConventionLintError) {
 
   const TorqueCompilerResult result = TestCompileTorque(source);
 
-  ASSERT_EQ(result.lint_errors.size(), static_cast<size_t>(1));
-  EXPECT_THAT(result.lint_errors[0].message, HasSubstr("\"foo\""));
+  ASSERT_EQ(result.messages.size(), static_cast<size_t>(1));
+  EXPECT_THAT(result.messages[0].message, HasSubstr("\"foo\""));
 }
 
 }  // namespace torque