[clangd] Add config option for fast diagnostics mode

Also wire it up for use with patched preambles and introduce test cases
for behaviour we'd like to improve.

Differential Revision: https://reviews.llvm.org/D142890

diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index f41906b..dffd54b 100644 (file)
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -88,7 +88,7 @@ struct Config {
     bool StandardLibrary = true;
   } Index;
 
-  enum UnusedIncludesPolicy {
+  enum class UnusedIncludesPolicy {
     /// Diagnose unused includes.
     Strict,
     None,
@@ -107,7 +107,10 @@ struct Config {
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
 
-    UnusedIncludesPolicy UnusedIncludes = None;
+    UnusedIncludesPolicy UnusedIncludes = UnusedIncludesPolicy::None;
+
+    /// Enable emitting diagnostics using stale preambles.
+    bool AllowStalePreamble = false;
 
     /// IncludeCleaner will not diagnose usages of these headers matched by
     /// these regexes.

diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index b1876e2..18de6e4 100644 (file)
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -441,8 +441,14 @@ struct FragmentCompiler {
         Out.Apply.push_back([Val](const Params &, Config &C) {
           C.Diagnostics.UnusedIncludes = *Val;
         });
-    compile(std::move(F.Includes));
+    if (F.AllowStalePreamble) {
+      if (auto Val = F.AllowStalePreamble)
+        Out.Apply.push_back([Val](const Params &, Config &C) {
+          C.Diagnostics.AllowStalePreamble = **Val;
+        });
+    }
 
+    compile(std::move(F.Includes));
     compile(std::move(F.ClangTidy));
   }
 

diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index bcd1a05..a559759 100644 (file)
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -232,9 +232,13 @@ struct Fragment {
     ///
     /// Valid values are:
     /// - Strict
+    /// - Experiment
     /// - None
     std::optional<Located<std::string>> UnusedIncludes;
 
+    /// Enable emitting diagnostics using stale preambles.
+    std::optional<Located<bool>> AllowStalePreamble;
+
     /// Controls IncludeCleaner diagnostics.
     struct IncludesBlock {
       /// Regexes that will be used to avoid diagnosing certain includes as

diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index ee91753..0ec0239 100644 (file)
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -130,6 +130,9 @@ private:
     });
     Dict.handle("Includes", [&](Node &N) { parse(F.Includes, N); });
     Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+    Dict.handle("AllowStalePreamble", [&](Node &N) {
+      F.AllowStalePreamble = boolValue(N, "AllowStalePreamble");
+    });
     Dict.parse(N);
   }
 
@@ -268,7 +271,7 @@ private:
     // If Key is seen twice, Parse runs only once and an error is reported.
     void handle(llvm::StringLiteral Key, std::function<void(Node &)> Parse) {
       for (const auto &Entry : Keys) {
-        (void) Entry;
+        (void)Entry;
         assert(Entry.first != Key && "duplicate key handler");
       }
       Keys.emplace_back(Key, std::move(Parse));

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index f99c414..f442c85 100644 (file)
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -792,5 +792,9 @@ SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
   return Loc;
 }
 
+bool PreamblePatch::preserveDiagnostics() const {
+  return PatchContents.empty() ||
+         Config::current().Diagnostics.AllowStalePreamble;
+}
 } // namespace clangd
 } // namespace clang

diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index bd5fefc..438a456 100644 (file)
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -155,7 +155,7 @@ public:
   llvm::StringRef text() const { return PatchContents; }
 
   /// Whether diagnostics generated using this patch are trustable.
-  bool preserveDiagnostics() const { return PatchContents.empty(); }
+  bool preserveDiagnostics() const;
 
 private:
   static PreamblePatch create(llvm::StringRef FileName,

diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 427409b..014db13 100644 (file)
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -543,6 +543,21 @@ TEST_F(ConfigCompileTests, Style) {
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
 }
+
+TEST_F(ConfigCompileTests, AllowDiagsFromStalePreamble) {
+  Frag = {};
+  EXPECT_TRUE(compileAndApply());
+  // Off by default.
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+
+  Frag.Diagnostics.AllowStalePreamble.emplace(true);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, true);
+
+  Frag.Diagnostics.AllowStalePreamble.emplace(false);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+}
 } // namespace
 } // namespace config
 } // namespace clangd

diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index 6f12bc8..7ed66c6 100644 (file)
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -273,6 +273,33 @@ Style:
   EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
               ElementsAre(val("foo"), val("bar")));
 }
+
+TEST(ParseYAML, DiagnosticsMode) {
+  CapturedDiags Diags;
+  {
+    Annotations YAML(R"yaml(
+Diagnostics:
+  AllowStalePreamble: Yes)yaml");
+    auto Results =
+        Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+    ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+    ASSERT_EQ(Results.size(), 1u);
+    EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+                llvm::ValueIs(val(true)));
+  }
+
+  {
+    Annotations YAML(R"yaml(
+Diagnostics:
+  AllowStalePreamble: No)yaml");
+    auto Results =
+        Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+    ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+    ASSERT_EQ(Results.size(), 1u);
+    EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+                llvm::ValueIs(val(false)));
+  }
+}
 } // namespace
 } // namespace config
 } // namespace clangd

diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index d2dce91..01b8e6a 100644 (file)
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -538,7 +538,7 @@ TEST(IncludeCleaner, IWYUPragmas) {
     void foo() {}
   )cpp");
   Config Cfg;
-  Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Experiment;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
@@ -627,7 +627,7 @@ TEST(IncludeCleaner, NoDiagsForObjC) {
   TU.ExtraArgs.emplace_back("-xobjective-c");
 
   Config Cfg;
-  Cfg.Diagnostics.UnusedIncludes = Config::Strict;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));

diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
index ae353f9..0bd5a9f 100644 (file)
--- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,9 +8,13 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Config.h"
+#include "Diagnostics.h"
 #include "Headers.h"
 #include "Hover.h"
+#include "ParsedAST.h"
 #include "Preamble.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -19,10 +23,13 @@
 #include "clang/Format/Format.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest-matchers.h"
 #include "gtest/gtest.h"
@@ -31,10 +38,18 @@
 #include <string>
 #include <vector>
 
+using testing::AllOf;
 using testing::Contains;
+using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
+using testing::HasSubstr;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::MatchesRegex;
+using testing::Not;
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
 
 namespace clang {
 namespace clangd {
@@ -204,9 +219,12 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) {
           Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User))));
 }
 
-std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
-                                          llvm::StringRef Modified) {
-  auto BaselinePreamble = TestTU::withCode(Baseline).preamble();
+std::optional<ParsedAST>
+createPatchedAST(llvm::StringRef Baseline, llvm::StringRef Modified,
+                 llvm::StringMap<std::string> AdditionalFiles = {}) {
+  auto TU = TestTU::withCode(Baseline);
+  TU.AdditionalFiles = std::move(AdditionalFiles);
+  auto BaselinePreamble = TU.preamble();
   if (!BaselinePreamble) {
     ADD_FAILURE() << "Failed to build baseline preamble";
     return std::nullopt;
@@ -214,7 +232,7 @@ std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
 
   IgnoreDiagnostics Diags;
   MockFS FS;
-  auto TU = TestTU::withCode(Modified);
+  TU.Code = Modified.str();
   auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
   if (!CI) {
     ADD_FAILURE() << "Failed to build compiler invocation";
@@ -599,6 +617,161 @@ TEST(PreamblePatch, NoopWhenNotRequested) {
                                             TU.inputs(FS), *BaselinePreamble);
   EXPECT_TRUE(PP.text().empty());
 }
+
+::testing::Matcher<const Diag &>
+withNote(::testing::Matcher<Note> NoteMatcher) {
+  return Field(&Diag::Notes, ElementsAre(NoteMatcher));
+}
+MATCHER_P(Diag, Range, "Diag at " + llvm::to_string(Range)) {
+  return arg.Range == Range;
+}
+MATCHER_P2(Diag, Range, Name,
+           "Diag at " + llvm::to_string(Range) + " = [" + Name + "]") {
+  return arg.Range == Range && arg.Name == Name;
+}
+
+TEST(PreamblePatch, DiagnosticsFromMainASTAreInRightPlace) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  {
+    Annotations Code("#define FOO");
+    // Check with removals from preamble.
+    Annotations NewCode("[[x]];/* error-ok */");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "missing_type_specifier")));
+  }
+  {
+    // Check with additions to preamble.
+    Annotations Code("#define FOO");
+    Annotations NewCode(R"(
+#define FOO
+#define BAR
+[[x]];/* error-ok */)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "missing_type_specifier")));
+  }
+}
+
+TEST(PreamblePatch, DiagnosticsToPreamble) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  llvm::StringMap<std::string> AdditionalFiles;
+  AdditionalFiles["foo.h"] = "#pragma once";
+  AdditionalFiles["bar.h"] = "#pragma once";
+  {
+    Annotations Code(R"(
+// Test comment
+[[#include "foo.h"]])");
+    // Check with removals from preamble.
+    Annotations NewCode(R"([[#  include "foo.h"]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "unused-includes")));
+  }
+  {
+    // Check with additions to preamble.
+    Annotations Code(R"(
+// Test comment
+[[#include "foo.h"]])");
+    Annotations NewCode(R"(
+$bar[[#include "bar.h"]]
+// Test comment
+$foo[[#include "foo.h"]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        UnorderedElementsAre(Diag(NewCode.range("bar"), "unused-includes"),
+                             Diag(NewCode.range("foo"), "unused-includes")));
+  }
+  {
+    Annotations Code("#define [[FOO]] 1\n");
+    // Check ranges for notes.
+    Annotations NewCode(R"(#define $barxyz[[BARXYZ]] 1
+#define $foo1[[FOO]] 1
+void foo();
+#define $foo2[[FOO]] 2)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        ElementsAre(
+            // FIXME: This diagnostics shouldn't exist. It's emitted from the
+            // preamble patch to the stale location inside preamble.
+            AllOf(Field(&Diag::Name, Eq("-Wmacro-redefined")),
+                  Field(&Diag::File, HasSubstr("_preamble_patch_")),
+                  withNote(Diag(NewCode.range("barxyz")))),
+            AllOf(
+                Diag(NewCode.range("foo2"), "-Wmacro-redefined"),
+                // FIXME: This should be translated into main file.
+                withNote(Field(&Note::File, HasSubstr("_preamble_patch_"))))));
+  }
+}
+
+TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  {
+    // Check with additions to preamble.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(R"(
+#define BAR
+#include [[<foo>]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Check with removals from preamble.
+    Annotations Code(R"(
+#define BAR
+#include [[<foo>]])");
+    Annotations NewCode("#include [[<foo>]]");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Drop line with diags.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode("#define BAR\n#define BAZ\n");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: No diagnostics.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Picks closest line in case of multiple alternatives.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(R"(
+#define BAR
+#include [[<foo>]]
+#define BAR
+#include <foo>)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Drop diag if line spelling has changed.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(" # include <foo>");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: No diags.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang