diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -674,9 +674,10 @@ if (PreserveDiags) { Diags = CompilerInvocationDiags; // Add diagnostics from the preamble, if any. - if (Preamble) - Diags->insert(Diags->end(), Preamble->Diags.begin(), - Preamble->Diags.end()); + if (Preamble) { + auto PDiags = Patch->patchedDiags(); + Diags->insert(Diags->end(), PDiags.begin(), PDiags.end()); + } // Finally, add diagnostics coming from the AST. { std::vector D = ASTDiags.take(&*CTContext); 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,11 @@ llvm::StringRef text() const { return PatchContents; } /// Whether diagnostics generated using this patch are trustable. - bool preserveDiagnostics() const { return PatchContents.empty(); } + bool preserveDiagnostics() const; + + /// Returns diag locations for Modified contents, only contains diags attached + /// to an #include or #define directive. + std::vector patchedDiags() const; private: static PreamblePatch create(llvm::StringRef FileName, @@ -166,9 +170,11 @@ PreamblePatch() = default; std::string PatchContents; std::string PatchFileName; - /// Includes that are present in both \p Baseline and \p Modified. Used for - /// patching includes of baseline preamble. + // Includes that are present in both \p Baseline and \p Modified. Used for + // patching includes of baseline preamble. std::vector PreambleIncludes; + // Diags that were attached to an #include or a #define directive. + std::vector PatchedDiags; PreambleBounds ModifiedBounds = {0, false}; }; 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 @@ -9,6 +9,7 @@ #include "Preamble.h" #include "Compiler.h" #include "Config.h" +#include "Diagnostics.h" #include "Headers.h" #include "SourceCode.h" #include "clang-include-cleaner/Record.h" @@ -35,6 +36,8 @@ #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/ErrorHandling.h" @@ -43,7 +46,7 @@ #include "llvm/Support/Path.h" #include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" -#include +#include #include #include #include @@ -467,6 +470,19 @@ WallTimer Timer; }; +// Represents an include as spelled in the source code, without taking resolved +// path or location into account. +struct IncludeKey { + tok::PPKeywordKind Directive; + llvm::StringRef Written; + + bool operator<(const IncludeKey &RHS) const { + return std::tie(Directive, Written) < std::tie(RHS.Directive, RHS.Written); + } + bool operator==(const IncludeKey &RHS) const { + return std::tie(Directive, Written) == std::tie(RHS.Directive, RHS.Written); + } +}; } // namespace std::shared_ptr @@ -614,6 +630,70 @@ } } +// Checks if all pointers in \p D are for the same line of the main file. +static bool diagReferencesMultipleLines(const Diag &D) { + int Line = D.Range.start.line; + if (D.Range.end.line != Line) + return true; + bool NotePointsToOutside = llvm::any_of(D.Notes, [&](const Note &N) { + return N.File == D.File && + (N.Range.start.line != Line || N.Range.end.line != Line); + }); + if (NotePointsToOutside) + return true; + bool FixPointsToOutside = llvm::any_of(D.Fixes, [&](const Fix &F) { + for (auto &E : F.Edits) + if (E.range.start.line != Line || E.range.end.line != Line) + return true; + return false; + }); + if (FixPointsToOutside) + return true; + return false; +} + +// Move all references in \p D to \p NewLine. This assumes none of the +// references to the main file are multi-line. +static Diag translateDiag(const Diag &D, int NewLine) { + assert(!diagReferencesMultipleLines(D)); + Diag Translated = D; + Translated.Range.start.line = Translated.Range.end.line = NewLine; + for (auto &N : Translated.Notes) { + if (N.File != D.File) + continue; + N.Range.start.line = N.Range.end.line = NewLine; + } + for (auto &F : Translated.Fixes) { + for (auto &E : F.Edits) + E.range.start.line = E.range.end.line = NewLine; + } + return Translated; +} + +static llvm::DenseMap> +mapDiagsToLines(llvm::ArrayRef Diags) { + llvm::DenseMap> LineToDiags; + for (auto &D : Diags) { + if (diagReferencesMultipleLines(D)) + continue; + LineToDiags[D.Range.start.line].emplace_back(&D); + } + return LineToDiags; +} + +static std::map +getExistingIncludes(const ScannedPreamble &BaselineScan, + llvm::ArrayRef MFI) { + std::map ExistingIncludes; + // There might be includes coming from disabled regions, record these for + // exclusion. note that we don't have resolved paths for those. + for (const auto &Inc : BaselineScan.Includes) + ExistingIncludes[{Inc.Directive, Inc.Written}] = nullptr; + for (const auto &Inc : MFI) + ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc; + return ExistingIncludes; +} + PreamblePatch PreamblePatch::create(llvm::StringRef FileName, const ParseInputs &Modified, const PreambleData &Baseline, @@ -667,26 +747,37 @@ escapeBackslashAndQuotes(FileName, Patch); Patch << "\"\n"; + // List of diagnostics associated to a particular line in the baseline. + auto LineToDiags = mapDiagsToLines(Baseline.Diags); + auto MoveDiagsBetweenLines = [&LineToDiags, &PP](int OldLine, int NewLine) { + auto DiagsForInclude = LineToDiags.find(OldLine); + if (DiagsForInclude == LineToDiags.end()) + return; + for (auto *D : DiagsForInclude->second) + PP.PatchedDiags.emplace_back(translateDiag(*D, NewLine)); + }; + // Map from an include to its line in the Modified contents. + std::map IncludeToPatchedLine; if (IncludesChanged && PatchType == PatchType::All) { // We are only interested in newly added includes, record the ones in // Baseline for exclusion. - llvm::DenseMap, - /*Resolved=*/llvm::StringRef> - ExistingIncludes; - for (const auto &Inc : Baseline.Includes.MainFileIncludes) - ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved; - // There might be includes coming from disabled regions, record these for - // exclusion too. note that we don't have resolved paths for those. - for (const auto &Inc : BaselineScan->Includes) - ExistingIncludes.try_emplace({Inc.Directive, Inc.Written}); + auto ExistingIncludes = + getExistingIncludes(*BaselineScan, Baseline.Includes.MainFileIncludes); // Calculate extra includes that needs to be inserted. for (auto &Inc : ModifiedScan->Includes) { auto It = ExistingIncludes.find({Inc.Directive, Inc.Written}); // Include already present in the baseline preamble. Set resolved path and // put into preamble includes. if (It != ExistingIncludes.end()) { - Inc.Resolved = It->second.str(); + if (It->second) { + // Copy everything from existing include, apart from the location. + Inc.Resolved = It->second->Resolved; + Inc.HeaderID = It->second->HeaderID; + Inc.BehindPragmaKeep = It->second->BehindPragmaKeep; + Inc.FileKind = It->second->FileKind; + } PP.PreambleIncludes.push_back(Inc); + IncludeToPatchedLine[It->first] = Inc.HashLine; continue; } // Include is new in the modified preamble. Inject it into the patch and @@ -696,8 +787,29 @@ Patch << llvm::formatv( "#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written); } + } else { + // Copy include information from baseline preamble if nothing has changed. + PP.PreambleIncludes = Baseline.Includes.MainFileIncludes; + } + + // Patch locations for diagnostics on baseline includes. + for (const auto &Inc : Baseline.Includes.MainFileIncludes) { + // Use existing locations if includes haven't changed. + size_t NewLine = Inc.HashLine; + if (IncludesChanged) { + auto PatchedLine = + IncludeToPatchedLine.find({Inc.Directive, Inc.Written}); + // Ignore diags for deleted includes. + if (PatchedLine == IncludeToPatchedLine.end()) + continue; + NewLine = PatchedLine->second; + } + MoveDiagsBetweenLines(Inc.HashLine, NewLine); } + // Maps a directive from its spelling to its location in the Modified + // contents. + llvm::StringMap DirectiveToPatchedLine; if (DirectivesChanged) { // We need to patch all the directives, since they are order dependent. e.g: // #define BAR(X) NEW(X) // Newly introduced in Modified @@ -713,8 +825,23 @@ for (const auto &TD : ModifiedScan->TextualDirectives) { Patch << "#line " << TD.DirectiveLine << '\n'; Patch << TD.Text << '\n'; + DirectiveToPatchedLine[TD.Text] = TD.DirectiveLine; } + } else { + // Take existing directives as-is, if they're the same. + for (const auto &TD : BaselineScan->TextualDirectives) + DirectiveToPatchedLine[TD.Text] = TD.DirectiveLine; } + + // Patch locations for diagnositcs on baseline macros. + for (const auto &TD : BaselineScan->TextualDirectives) { + // Textual directive lines are 1-based. + auto NewLine = DirectiveToPatchedLine.find(TD.Text); + if (NewLine == DirectiveToPatchedLine.end()) + continue; + MoveDiagsBetweenLines(TD.DirectiveLine - 1, NewLine->second - 1); + } + dlog("Created preamble patch: {0}", Patch.str()); Patch.flush(); return PP; @@ -756,6 +883,7 @@ PreamblePatch PP; PP.PreambleIncludes = Preamble.Includes.MainFileIncludes; PP.ModifiedBounds = Preamble.Preamble.getBounds(); + PP.PatchedDiags = Preamble.Diags; return PP; } @@ -779,5 +907,10 @@ return Loc; } +bool PreamblePatch::preserveDiagnostics() const { + return PatchContents.empty() || + Config::current().Diagnostics.Mode != Config::DiagnosticsMode::Strict; +} +std::vector PreamblePatch::patchedDiags() const { return PatchedDiags; } } // namespace clangd } // namespace clang 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,6 +8,7 @@ #include "Annotations.h" #include "Compiler.h" +#include "Config.h" #include "Headers.h" #include "Hover.h" #include "Preamble.h" @@ -18,10 +19,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 @@ -30,9 +33,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 { @@ -190,9 +198,12 @@ Field(&Inclusion::Resolved, testPath("a.h"))))); } -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; @@ -201,6 +212,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 +577,135 @@ TU.inputs(FS), *BaselinePreamble); EXPECT_TRUE(PP.text().empty()); } + +MATCHER_P(DiagWithRange, Range, "") { return arg.Range == Range; } + +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 PatchedDiags = + *createPatchedAST(BaselinePreamble, Code.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, + testing::ElementsAre(DiagWithRange(Code.range()))); + } + { + // Check with additions to preamble. + Annotations Code( + (BaselinePreamble + "#define BAR\n[[x]];/* error-ok */").str()); + auto PatchedDiags = + *createPatchedAST(BaselinePreamble, Code.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, + testing::ElementsAre(DiagWithRange(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 AdditionalFiles; + AdditionalFiles["foo.h"] = "#pragma once"; + AdditionalFiles["bar.h"] = "#pragma once"; + llvm::StringLiteral BaselinePreamble("#define FOO\n[[#include \"foo.h\"]]"); + { + // Check with removals from preamble. + Annotations NewCode("[[# include \"foo.h\"]]"); + auto PatchedDiags = *createPatchedAST(Annotations(BaselinePreamble).code(), + NewCode.code(), AdditionalFiles) + ->getDiagnostics(); + EXPECT_THAT(PatchedDiags, ElementsAre(DiagWithRange(NewCode.range()))); + } + { + // Check with additions to preamble. + Annotations NewCode( + ("[[#include \"bar.h\"]]\n" + BaselinePreamble + "\n#define BAR") + .str()); + auto PatchedDiags = *createPatchedAST(Annotations(BaselinePreamble).code(), + NewCode.code(), AdditionalFiles) + ->getDiagnostics(); + auto ExpectedRanges = NewCode.ranges(); + decltype(ExpectedRanges) ActualRanges; + for (auto &D : PatchedDiags) + ActualRanges.push_back(D.Range); + EXPECT_THAT(ExpectedRanges, UnorderedElementsAreArray(ActualRanges)); + } +} + +TEST(PreamblePatch, TranslatesIncludeDiagnosticsInPreamble) { + Config Cfg; + Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast; + 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 PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, ElementsAre(DiagWithRange(NewCode.range()))); + } + { + // Check with removals from preamble. + Annotations Code(("#define BAR\n" + BaselinePreamble).str()); + Annotations NewCode(BaselinePreamble); + auto PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, ElementsAre(DiagWithRange(NewCode.range()))); + } + { + // Drop line with diags. + Annotations Code(BaselinePreamble); + Annotations NewCode("#define BAR\n#include \n"); + auto PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, IsEmpty()); + } +} + +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 PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, ElementsAre(DiagWithRange(NewCode.range()))); + } + { + // Check with removals from preamble. + Annotations Code(("#define BAR\n" + BaselinePreamble).str()); + Annotations NewCode(BaselinePreamble); + auto PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, ElementsAre(DiagWithRange(NewCode.range()))); + } + { + // Drop line with diags. + Annotations Code(BaselinePreamble); + Annotations NewCode("#define BAR\n#include \n#include \n"); + auto PatchedDiags = + *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics(); + EXPECT_THAT(PatchedDiags, IsEmpty()); + } +} } // namespace } // namespace clangd } // namespace clang