[torque-ls] Send lint warnings to the client after compilation

This CL refactors and extends the infrastructure around sending
diagnostic notifications. This enables publishing lint errors as
warnings after a compilation run.

R=sigurds@chromium.org, tebbi@chromium.org

Bug: v8:8880
Change-Id: Ia64d2d490c1449021c92f5dc45eb7f8dab21e60a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1582405
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61003}

src/torque/ls/message-handler.cc

@@ -76,33 +76,90 @@ void ResetCompilationErrorDiagnostics(MessageWriter writer) {
 
     writer(notification.GetJsonValue());
   }
+  DiagnosticsFiles::Get() = {};
 }
 
-void SendCompilationErrorDiagnostics(const TorqueError& error,
-                                     MessageWriter writer) {
-  PublishDiagnosticsNotification notification;
-  notification.set_method("textDocument/publishDiagnostics");
-
-  std::string error_file =
-      error.position ? SourceFileMap::GetSource(error.position->source)
-                     : "<unknown>";
-  notification.params().set_uri(error_file);
-
-  Diagnostic diagnostic = notification.params().add_diagnostics();
-  diagnostic.set_severity(Diagnostic::kError);
-  diagnostic.set_message(error.message);
-  diagnostic.set_source("Torque Compiler");
-
-  if (error.position) {
-    Range range = diagnostic.range();
-    range.start().set_line(error.position->start.line);
-    range.start().set_character(error.position->start.column);
-    range.end().set_line(error.position->end.line);
-    range.end().set_character(error.position->end.column);
+// Each notification must contain all diagnostics for a specific file,
+// because sending multiple notifications per file resets previously sent
+// diagnostics. Thus, two steps are needed:
+//   1) collect all notifications in this class.
+//   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();
+    auto& notification = GetOrCreateNotificationForSource(id);
+
+    Diagnostic diagnostic = notification.params().add_diagnostics();
+    diagnostic.set_severity(Diagnostic::kError);
+    diagnostic.set_message(error.message);
+    diagnostic.set_source("Torque Compiler");
+
+    if (error.position) {
+      PopulateRangeFromSourcePosition(diagnostic.range(), *error.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_;
+  }
+
+ private:
+  PublishDiagnosticsNotification& GetOrCreateNotificationForSource(
+      SourceId id) {
+    auto iter = notifications_.find(id);
+    if (iter != notifications_.end()) return iter->second;
+
+    PublishDiagnosticsNotification& notification = notifications_[id];
+    notification.set_method("textDocument/publishDiagnostics");
+
+    std::string file =
+        id.IsValid() ? SourceFileMap::GetSource(id) : "<unknown>";
+    notification.params().set_uri(file);
+    return notification;
+  }
+
+  void PopulateRangeFromSourcePosition(Range range,
+                                       const SourcePosition& position) {
+    range.start().set_line(position.start.line);
+    range.start().set_character(position.start.column);
+    range.end().set_line(position.end.line);
+    range.end().set_character(position.end.column);
   }
-  writer(notification.GetJsonValue());
 
-  if (error.position) DiagnosticsFiles::Get().push_back(error.position->source);
+  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);
+  }
+
+  for (auto& pair : collector.notifications()) {
+    PublishDiagnosticsNotification& notification = pair.second;
+    writer(notification.GetJsonValue());
+
+    // Record all source files for which notifications are sent, so they
+    // can be reset before the next compiler run.
+    const SourceId& source = pair.first;
+    if (source.IsValid()) DiagnosticsFiles::Get().push_back(source);
+  }
 }
 
 }  // namespace
@@ -111,9 +168,7 @@ void CompilationFinished(TorqueCompilerResult result, MessageWriter writer) {
   LanguageServerData::Get() = result.language_server_data;
   SourceFileMap::Get() = result.source_file_map;
 
-  if (result.error) {
-    SendCompilationErrorDiagnostics(*result.error, writer);
-  }
+  SendCompilationDiagnostics(result, writer);
 }
 
 namespace {
@@ -194,8 +249,8 @@ void HandleTorqueFileListNotification(TorqueFileListNotification notification,
   // we need to order them in the correct way.
   // TODO(szuend): Remove this, once the compiler doesn't require the input
   //               files to be in a specific order.
-  std::vector<std::string> sort_to_front = {"base.tq", "frames.tq",
-                                            "arguments.tq", "array.tq"};
+  std::vector<std::string> sort_to_front = {
+      "base.tq", "frames.tq", "arguments.tq", "array.tq", "typed_array.tq"};
   std::sort(files.begin(), files.end(), [&](std::string a, std::string b) {
     for (const std::string& fixed_file : sort_to_front) {
       if (a.find(fixed_file) != std::string::npos) return true;

src/torque/utils.cc

@@ -138,7 +138,7 @@ void ReportLintError(const std::string& error) {
 void NamingConventionError(const std::string& type, const std::string& name,
                            const std::string& convention) {
   std::stringstream sstream;
-  sstream << type << " \"" << name << "\" doesn't follow \"" << convention
+  sstream << type << " \"" << name << "\" does not follow \"" << convention
           << "\" naming convention.";
   ReportLintError(sstream.str());
 }

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

@@ -112,6 +112,7 @@ TEST(LanguageServerMessage, GotoDefinition) {
 }
 
 TEST(LanguageServerMessage, CompilationErrorSendsDiagnostics) {
+  DiagnosticsFiles::Scope diagnostic_files_scope;
   LanguageServerData::Scope server_data_scope;
   SourceFileMap::Scope source_file_map_scope;
 
@@ -133,6 +134,51 @@ TEST(LanguageServerMessage, CompilationErrorSendsDiagnostics) {
   });
 }
 
+TEST(LanguageServerMessage, LintErrorSendsDiagnostics) {
+  DiagnosticsFiles::Scope diagnostic_files_scope;
+  LintErrors::Scope lint_errors_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.
+  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.source_file_map = SourceFileMap::Get();
+
+  CompilationFinished(result, [](JsonValue& raw_response) {
+    PublishDiagnosticsNotification notification(raw_response);
+
+    EXPECT_EQ(notification.method(), "textDocument/publishDiagnostics");
+    ASSERT_FALSE(notification.IsNull("params"));
+    EXPECT_EQ(notification.params().uri(), "test.tq");
+
+    ASSERT_EQ(notification.params().diagnostics_size(), static_cast<size_t>(2));
+    Diagnostic diagnostic1 = notification.params().diagnostics(0);
+    EXPECT_EQ(diagnostic1.severity(), Diagnostic::kWarning);
+    EXPECT_EQ(diagnostic1.message(), "lint error 1");
+
+    Diagnostic diagnostic2 = notification.params().diagnostics(1);
+    EXPECT_EQ(diagnostic2.severity(), Diagnostic::kWarning);
+    EXPECT_EQ(diagnostic2.message(), "lint error 2");
+  });
+}
+
+TEST(LanguageServerMessage, CleanCompileSendsNoDiagnostics) {
+  LanguageServerData::Scope server_data_scope;
+  SourceFileMap::Scope sourc_file_map_scope;
+
+  TorqueCompilerResult result;
+  result.source_file_map = SourceFileMap::Get();
+
+  CompilationFinished(result, [](JsonValue& raw_response) {
+    FAIL() << "Sending unexpected response!";
+  });
+}
+
 }  // namespace ls
 }  // namespace torque
 }  // namespace internal