kadircet updated this revision to Diff 494003.
kadircet added a comment.
Wire up the feature and add some test cases
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142890/new/
https://reviews.llvm.org/D142890
Files:
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
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ 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"
@@ -18,10 +22,12 @@
#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/VirtualFileSystem.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <memory>
@@ -30,9 +36,14 @@
#include <vector>
using testing::Contains;
+using testing::ElementsAre;
using testing::Field;
+using testing::IsEmpty;
using testing::Matcher;
using testing::MatchesRegex;
+using testing::Not;
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
namespace clang {
namespace clangd {
@@ -190,9 +201,12 @@
Field(&Inclusion::Resolved, testPath("a.h")))));
}
-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 PreambleTU = TestTU::withCode(Baseline);
+ PreambleTU.AdditionalFiles = AdditionalFiles;
+ auto BaselinePreamble = PreambleTU.preamble();
if (!BaselinePreamble) {
ADD_FAILURE() << "Failed to build baseline preamble";
return std::nullopt;
@@ -201,6 +215,7 @@
IgnoreDiagnostics Diags;
MockFS FS;
auto TU = TestTU::withCode(Modified);
+ TU.AdditionalFiles = std::move(AdditionalFiles);
auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
if (!CI) {
ADD_FAILURE() << "Failed to build compiler invocation";
@@ -565,6 +580,150 @@
TU.inputs(FS), *BaselinePreamble);
EXPECT_TRUE(PP.text().empty());
}
+
+std::vector<clangd::Range> mainFileDiagRanges(const ParsedAST &AST) {
+ std::vector<clangd::Range> Result;
+ auto AddRangeIfInMainfile = [&Result](const DiagBase &DB) {
+ if (DB.InsideMainFile)
+ Result.emplace_back(DB.Range);
+ };
+ for (auto &D : *AST.getDiagnostics()) {
+ AddRangeIfInMainfile(D);
+ for (auto &N : D.Notes)
+ AddRangeIfInMainfile(N);
+ }
+ return Result;
+}
+
+TEST(PreamblePatch, DiagnosticsFromMainASTAreInRightPlace) {
+ Config Cfg;
+ Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble = "#define FOO\n";
+ {
+ // Check with removals from preamble.
+ Annotations Code("[[x]];/* error-ok */");
+ auto AST = createPatchedAST(BaselinePreamble, Code.code());
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with additions to preamble.
+ Annotations Code(
+ (BaselinePreamble + "#define BAR\n[[x]];/* error-ok */").str());
+ auto AST = createPatchedAST(BaselinePreamble, Code.code());
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
+
+TEST(PreamblePatch, DiagnosticsToPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
+ 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";
+ llvm::StringLiteral BaselinePreamble("#define FOO 1\n[[#include \"foo.h\"]]");
+ {
+ // Check with removals from preamble.
+ Annotations NewCode("[[# include \"foo.h\"]]");
+ auto AST = createPatchedAST(Annotations(BaselinePreamble).code(),
+ NewCode.code(), AdditionalFiles);
+ // FIXME: Should be pointing at the range inside the Newcode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), IsEmpty());
+ }
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("[[#include \"bar.h\"]]\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+ // FIXME: This is just all sorts of wrong. We point at the FOO from baseline
+ // claiming it's redefined and not point at the new include of "bar.h".
+ }
+ {
+ llvm::StringLiteral BaselinePreamble = "#define [[FOO]] 1\n";
+ Annotations Code(BaselinePreamble);
+ // Check ranges for notes.
+ Annotations NewCode(("#define [[BARXYZ]] 1\n" + BaselinePreamble +
+ "void foo();\n#define [[FOO]] 2")
+ .str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+ // FIXME: We shouldn't be warning for BARXYZ, and pointing at the FOO inside
+ // the baselinepreamble.
+ EXPECT_THAT(mainFileDiagRanges(*AST),
+ UnorderedElementsAre(NewCode.ranges()[0], NewCode.ranges()[2]));
+ }
+}
+
+TEST(PreamblePatch, TranslatesIncludeDiagnosticsInPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble("#include [[<foo>]]\n");
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("#define BAR\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with removals from preamble.
+ Annotations Code(("#define BAR\n" + BaselinePreamble).str());
+ Annotations NewCode(BaselinePreamble);
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Drop line with diags.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode("#define BAR\n#include [[<bar>]]\n");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
+
+TEST(PreamblePatch, TranslatesNonIncludeDiagnosticsInPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble("#define [[__LINE__]]\n");
+ ASSERT_THAT(TestTU::withCode(Annotations(BaselinePreamble).code())
+ .build()
+ .getDiagnostics(),
+ Not(llvm::ValueIs(IsEmpty())));
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("#define BAR\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with removals from preamble.
+ Annotations Code(("#define BAR\n" + BaselinePreamble).str());
+ Annotations NewCode(BaselinePreamble);
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Drop line with diags.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode("#define BAR\n#include [[<bar>]]\n#include <baz>\n");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -538,7 +538,7 @@
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 @@
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()));
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -273,6 +273,31 @@
EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
ElementsAre(val("foo"), val("bar")));
}
+
+TEST(ParseYAML, DiagnosticsMode) {
+ CapturedDiags Diags;
+ {
+ Annotations YAML(R"yaml(
+Diagnostics:
+ Mode: Fast)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.Mode, llvm::ValueIs(val("Fast")));
+ }
+
+ {
+ Annotations YAML(R"yaml(
+Diagnostics:
+ Mode: Strict)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.Mode, llvm::ValueIs(val("Strict")));
+ }
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -543,6 +543,23 @@
EXPECT_TRUE(compileAndApply());
EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
}
+
+TEST_F(ConfigCompileTests, DiagnosticsMode) {
+ Frag = {};
+ EXPECT_TRUE(compileAndApply());
+ // Defaults to Strict.
+ EXPECT_EQ(Conf.Diagnostics.Mode, Config::DiagnosticsMode::Strict);
+
+ Frag.Diagnostics.Mode.emplace("Fast");
+ EXPECT_TRUE(compileAndApply());
+ // Defaults to Strict.
+ EXPECT_EQ(Conf.Diagnostics.Mode, Config::DiagnosticsMode::Fast);
+
+ Frag.Diagnostics.Mode.emplace("Strict");
+ EXPECT_TRUE(compileAndApply());
+ // Defaults to Strict.
+ EXPECT_EQ(Conf.Diagnostics.Mode, Config::DiagnosticsMode::Strict);
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -155,7 +155,7 @@
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,
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -779,5 +779,9 @@
return Loc;
}
+bool PreamblePatch::preserveDiagnostics() const {
+ return PatchContents.empty() ||
+ Config::current().Diagnostics.Mode != Config::DiagnosticsMode::Strict;
+}
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -130,6 +130,7 @@
});
Dict.handle("Includes", [&](Node &N) { parse(F.Includes, N); });
Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+ Dict.handle("Mode", [&](Node &N) { F.Mode = scalarValue(N, "Mode"); });
Dict.parse(N);
}
@@ -268,7 +269,7 @@
// 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));
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -232,9 +232,16 @@
///
/// Valid values are:
/// - Strict
+ /// - Experiment
/// - None
std::optional<Located<std::string>> UnusedIncludes;
+ /// Controls whether clangd should emit fast but loose diagnostics.
+ /// Valid values are:
+ /// - Fast
+ /// - Strict
+ std::optional<Located<std::string>> Mode;
+
/// Controls IncludeCleaner diagnostics.
struct IncludesBlock {
/// Regexes that will be used to avoid diagnosing certain includes as
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -441,8 +441,16 @@
Out.Apply.push_back([Val](const Params &, Config &C) {
C.Diagnostics.UnusedIncludes = *Val;
});
- compile(std::move(F.Includes));
+ if (F.Mode) {
+ if (auto Val = compileEnum<Config::DiagnosticsMode>("Mode", **F.Mode)
+ .map("Strict", Config::DiagnosticsMode::Strict)
+ .map("Fast", Config::DiagnosticsMode::Fast)
+ .value())
+ Out.Apply.push_back(
+ [Val](const Params &, Config &C) { C.Diagnostics.Mode = *Val; });
+ }
+ compile(std::move(F.Includes));
compile(std::move(F.ClangTidy));
}
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -88,13 +88,21 @@
bool StandardLibrary = true;
} Index;
- enum UnusedIncludesPolicy {
+ enum class UnusedIncludesPolicy {
/// Diagnose unused includes.
Strict,
None,
/// The same as Strict, but using the include-cleaner library.
Experiment,
};
+ /// Configures whether clangd can emit fast but possibly inaccurate
+ /// diagnostics.
+ enum class DiagnosticsMode {
+ /// Only emit diagnostics with up-to-date analysis.
+ Strict,
+ /// Can emit diagnostics with stale analysis.
+ Fast,
+ };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -107,7 +115,9 @@
llvm::StringMap<std::string> CheckOptions;
} ClangTidy;
- UnusedIncludesPolicy UnusedIncludes = None;
+ UnusedIncludesPolicy UnusedIncludes = UnusedIncludesPolicy::None;
+
+ DiagnosticsMode Mode = DiagnosticsMode::Strict;
/// IncludeCleaner will not diagnose usages of these headers matched by
/// these regexes.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
