[PATCH] D158519: [clangd] Move diagnostics from modules to main sources file

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin created this revision.
DmitryPolukhin added reviewers: adamcz, sammccall, kadircet.
DmitryPolukhin added a project: clang.
Herald added subscribers: arphaman, javed.absar.
Herald added a project: All.
DmitryPolukhin requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

After https://reviews.llvm.org/D85753 these diagnostics silently dropped
so the user may not have an idea what is wrong. This diff moves
diagnostics to the start of the main file same as it happens with
errors in the command line. Also added prefix give to make it clear that the
location is not original.

Test Plan: check-clangd


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158519

Files:
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/unittests/ModulesTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp


Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1079,7 +1079,8 @@
       ElementsAre(AllOf(
           Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
           Field(&Diag::Name, Eq("drv_unknown_argument")),
-          Field(&Diag::Message, "unknown argument: '-fsome-unknown-flag'"))));
+          Field(&Diag::Message,
+                "in command line: unknown argument: '-fsome-unknown-flag'"))));
 }
 
 TEST_F(TUSchedulerTests, CommandLineWarnings) {
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -18,6 +18,12 @@
 namespace clangd {
 namespace {
 
+using ::testing::ElementsAre;
+
+MATCHER_P(diag, Desc, "") {
+  return llvm::StringRef(arg.Message).contains(Desc);
+}
+
 TEST(Modules, TextualIncludeInPreamble) {
   TestTU TU = TestTU::withCode(R"cpp(
     #include "Textual.h"
@@ -88,7 +94,13 @@
 )modulemap";
 
   // Test that we do not crash.
-  TU.build();
+  auto AST = TU.build();
+
+  // Test expected diagnostics.
+  EXPECT_THAT(AST.getDiagnostics(),
+              ElementsAre(diag("in implicitly built module: module M does not "
+                               "depend on a module exporting 'non-modular.h'"),
+                          diag("could not build module 'M'")));
 }
 
 // Unknown module formats are a fatal failure for clang. Ensure we don't crash.
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -672,14 +672,14 @@
 
 void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
-  // If the diagnostic was generated for a different SourceManager, skip it.
+  bool FromModule = false;
+  // If the diagnostic was generated for a different SourceManager.
   // This happens when a module is imported and needs to be implicitly built.
   // The compilation of that module will use the same StoreDiags, but different
-  // SourceManager.
+  // SourceManager. Treat them as coming from command line.
   if (OrigSrcMgr && Info.hasSourceManager() &&
       OrigSrcMgr != &Info.getSourceManager()) {
-    IgnoreDiagnostics::log(DiagLevel, Info);
-    return;
+    FromModule = true;
   }
 
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
@@ -687,7 +687,7 @@
       Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
           Info.getID());
 
-  if (Info.getLocation().isInvalid()) {
+  if (Info.getLocation().isInvalid() || FromModule) {
     // Handle diagnostics coming from command-line arguments. The source manager
     // is *not* available at this point, so we cannot use it.
     if (!OriginallyError) {
@@ -702,6 +702,11 @@
     LastDiagOriginallyError = OriginallyError;
     LastDiag->ID = Info.getID();
     fillNonLocationData(DiagLevel, Info, *LastDiag);
+    // Update message to mention original location.
+    const char *Prefix =
+        FromModule ? "in implicitly built module" : "in command line";
+    LastDiag->Message = llvm::formatv("{0}: {1}", Prefix, LastDiag->Message);
+
     LastDiag->InsideMainFile = true;
     // Put it at the start of the main file, for a lack of a better place.
     LastDiag->Range.start = Position{0, 0};


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D158519.552378.patch
Type: text/x-patch
Size: 3654 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230822/bc89dcf4/attachment.bin>