diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -88,7 +88,7 @@ bool StandardLibrary = true; } Index; - enum UnusedIncludesPolicy { + enum class UnusedIncludesPolicy { /// Diagnose unused includes. Strict, None, @@ -107,7 +107,10 @@ llvm::StringMap 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 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -441,8 +441,14 @@ 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 --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -232,9 +232,13 @@ /// /// Valid values are: /// - Strict + /// - Experiment /// - None std::optional> UnusedIncludes; + /// Enable emitting diagnostics using stale preambles. + std::optional> 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 --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -130,6 +130,9 @@ }); 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 @@ // If Key is seen twice, Parse runs only once and an error is reported. void handle(llvm::StringLiteral Key, std::function 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.h b/clang-tools-extra/clangd/Preamble.h --- a/clang-tools-extra/clangd/Preamble.h +++ b/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, diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -790,5 +790,9 @@ return Loc; } +bool PreamblePatch::preserveDiagnostics() const { + return PatchContents.empty() || + Config::current().Diagnostics.AllowStalePreamble; +} } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -543,6 +543,21 @@ 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 --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp @@ -273,6 +273,33 @@ 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 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/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())); diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp --- 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,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-matchers.h" #include "gtest/gtest.h" @@ -32,9 +38,14 @@ #include 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 { @@ -197,9 +208,12 @@ Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User)))); } -std::optional createPatchedAST(llvm::StringRef Baseline, - llvm::StringRef Modified) { - auto BaselinePreamble = TestTU::withCode(Baseline).preamble(); +std::optional +createPatchedAST(llvm::StringRef Baseline, llvm::StringRef Modified, + llvm::StringMap 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; @@ -208,6 +222,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"; @@ -586,6 +601,137 @@ TU.inputs(FS), *BaselinePreamble); EXPECT_TRUE(PP.text().empty()); } + +std::vector mainFileDiagRanges(const ParsedAST &AST) { + std::vector 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.AllowStalePreamble = true; + 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 + R"( + #define BAR + [[x]];/* error-ok */)") + .str()); + auto AST = createPatchedAST(BaselinePreamble, Code.code()); + EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range())); + } +} + +TEST(PreamblePatch, DiagnosticsToPreamble) { + Config Cfg; + Cfg.Diagnostics.AllowStalePreamble = true; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict; + WithContextValue WithCfg(Config::Key, std::move(Cfg)); + + llvm::StringMap AdditionalFiles; + AdditionalFiles["foo.h"] = "#pragma once"; + AdditionalFiles["bar.h"] = "#pragma once"; + llvm::StringLiteral BaselinePreamble(R"( + // Test comment + [[#include "foo.h"]])"); + { + // Check with removals from preamble. + Annotations NewCode("[[# include \"foo.h\"]]"); + auto AST = createPatchedAST(Annotations(BaselinePreamble).code(), + NewCode.code(), AdditionalFiles); + EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(NewCode.range())); + } + { + // Check with additions to preamble. + Annotations Code(BaselinePreamble); + Annotations NewCode(("[[#include \"bar.h\"]]\n" + BaselinePreamble).str()); + auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles); + EXPECT_THAT(mainFileDiagRanges(*AST), + UnorderedElementsAreArray(NewCode.ranges())); + } + { + 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, TranslatesDiagnosticsInPreamble) { + Config Cfg; + Cfg.Diagnostics.AllowStalePreamble = true; + WithContextValue WithCfg(Config::Key, std::move(Cfg)); + + llvm::StringLiteral BaselinePreamble("#include [[]]\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#define BAZ\n"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + // FIXME: No diagnostics. + EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range())); + } + { + // Picks closest line in case of multiple alternatives. + Annotations Code(BaselinePreamble); + Annotations NewCode(R"( +#define BAR +#include [[]] +#define BAR +#include )"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + // FIXME: Should point at ranges in NewCode. + EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range())); + } + { + // Drop diag if line spelling has changed. + Annotations Code(BaselinePreamble); + Annotations NewCode(" # include "); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + // FIXME: No diags. + EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range())); + } +} } // namespace } // namespace clangd } // namespace clang