[clang-tools-extra] 7177a23 - [clangd] Add config option for fast diagnostics mode
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 22 06:54:40 PST 2023
Author: Kadir Cetinkaya
Date: 2023-02-22T15:54:14+01:00
New Revision: 7177a237b68f32befcecedba78a875f1bbc4a609
URL: https://github.com/llvm/llvm-project/commit/7177a237b68f32befcecedba78a875f1bbc4a609
DIFF: https://github.com/llvm/llvm-project/commit/7177a237b68f32befcecedba78a875f1bbc4a609.diff
LOG: [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
Added:
Modified:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index f41906b2f0faf..dffd54b01c459 100644
--- 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 b1876e21ee30d..18de6e4d5c3b6 100644
--- 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 bcd1a05b4a999..a5597596196fa 100644
--- 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 ee91753dd88dd..0ec0239fc71e6 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -130,6 +130,9 @@ class Parser {
});
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 @@ class Parser {
// 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 f99c4142a4640..f442c852c571a 100644
--- 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 bd5fefcf8f0bc..438a4561cac1c 100644
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -155,7 +155,7 @@ class PreamblePatch {
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 427409bfe6341..014db131c6224 100644
--- 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 6f12bc830767a..7ed66c67d99f7 100644
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -273,6 +273,33 @@ TEST(ParseYAML, 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 d2dce9105cd81..01b8e6a05dab5 100644
--- 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 ae353f94ce067..0bd5a9f806411 100644
--- 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
More information about the cfe-commits
mailing list