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 @@ -789,5 +789,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" @@ -31,10 +37,18 @@ #include #include +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 { @@ -197,9 +211,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 = std::move(AdditionalFiles); + auto BaselinePreamble = PreambleTU.preamble(); if (!BaselinePreamble) { ADD_FAILURE() << "Failed to build baseline preamble"; return std::nullopt; @@ -207,7 +224,8 @@ IgnoreDiagnostics Diags; MockFS FS; - auto TU = TestTU::withCode(Modified); + auto &TU = PreambleTU; + TU.Code = Modified.str(); auto CI = buildCompilerInvocation(TU.inputs(FS), Diags); if (!CI) { ADD_FAILURE() << "Failed to build compiler invocation"; @@ -586,6 +604,161 @@ TU.inputs(FS), *BaselinePreamble); EXPECT_TRUE(PP.text().empty()); } + +::testing::Matcher +withNote(::testing::Matcher 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 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 [[]]"); + Annotations NewCode(R"( +#define BAR +#include [[]])"); + 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 [[]])"); + Annotations NewCode("#include [[]]"); + 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 [[]]"); + 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 [[]]"); + Annotations NewCode(R"( +#define BAR +#include [[]] +#define BAR +#include )"); + 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 [[]]"); + Annotations NewCode(" # include "); + 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