diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index bf639a6fb58e7..54a0f979c7303 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -675,8 +675,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, Diags = CompilerInvocationDiags; // Add diagnostics from the preamble, if any. if (Preamble) - Diags->insert(Diags->end(), Preamble->Diags.begin(), - Preamble->Diags.end()); + llvm::append_range(*Diags, Patch->patchedDiags()); // Finally, add diagnostics coming from the AST. { std::vector D = ASTDiags.take(&*CTContext); diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 6a7efcd255d2e..e97564497954c 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -9,7 +9,9 @@ #include "Preamble.h" #include "Compiler.h" #include "Config.h" +#include "Diagnostics.h" #include "Headers.h" +#include "Protocol.h" #include "SourceCode.h" #include "clang-include-cleaner/Record.h" #include "support/Logger.h" @@ -35,6 +37,9 @@ #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/ErrorHandling.h" @@ -43,8 +48,9 @@ #include "llvm/Support/Path.h" #include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" -#include +#include #include +#include #include #include #include @@ -306,6 +312,8 @@ struct DirectiveCollector : public PPCallbacks { struct ScannedPreamble { std::vector Includes; std::vector TextualDirectives; + // Literal lines of the preamble contents. + std::vector Lines; PreambleBounds Bounds = {0, false}; }; @@ -332,7 +340,7 @@ scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) { if (!CI) return error("failed to create compiler invocation"); CI->getDiagnosticOpts().IgnoreWarnings = true; - auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents); + auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(PI.Contents); // This means we're scanning (though not preprocessing) the preamble section // twice. However, it's important to precisely follow the preamble bounds used // elsewhere. @@ -363,6 +371,7 @@ scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) { return std::move(Err); Action.EndSourceFile(); SP.Includes = std::move(Includes.MainFileIncludes); + llvm::append_range(SP.Lines, llvm::split(Contents, "\n")); return SP; } @@ -465,6 +474,93 @@ class TimerFS : public llvm::vfs::ProxyFileSystem { WallTimer Timer; }; +// Helpers for patching diagnostics between two versions of file contents. +class DiagPatcher { + llvm::ArrayRef OldLines; + llvm::ArrayRef CurrentLines; + llvm::StringMap> CurrentContentsToLine; + + // Translates a range from old lines to current lines. + // Finds the consecutive set of lines that corresponds to the same contents in + // old and current, and applies the same translation to the range. + // Returns true if translation succeeded. + bool translateRange(Range &R) { + int OldStart = R.start.line; + int OldEnd = R.end.line; + assert(OldStart <= OldEnd); + + size_t RangeLen = OldEnd - OldStart + 1; + auto RangeContents = OldLines.slice(OldStart).take_front(RangeLen); + // Make sure the whole range is covered in old contents. + if (RangeContents.size() < RangeLen) + return false; + + std::optional Closest; + for (int AlternateLine : CurrentContentsToLine.lookup(RangeContents[0])) { + // Check if AlternateLine matches all lines in the range. + if (RangeContents != + CurrentLines.slice(AlternateLine).take_front(RangeLen)) + continue; + int Delta = AlternateLine - OldStart; + if (!Closest.has_value() || abs(Delta) < abs(*Closest)) + Closest = Delta; + } + // Couldn't find any viable matches in the current contents. + if (!Closest.has_value()) + return false; + R.start.line += *Closest; + R.end.line += *Closest; + return true; + } + + // Translates a Note by patching its range when inside main file. Returns true + // on success. + bool translateNote(Note &N) { + if (!N.InsideMainFile) + return true; + if (translateRange(N.Range)) + return true; + return false; + } + + // Tries to translate all the edit ranges inside the fix. Returns true on + // success. On failure fixes might be in an invalid state. + bool translateFix(Fix &F) { + return llvm::all_of( + F.Edits, [this](TextEdit &E) { return translateRange(E.range); }); + } + +public: + DiagPatcher(llvm::ArrayRef OldLines, + llvm::ArrayRef CurrentLines) { + this->OldLines = OldLines; + this->CurrentLines = CurrentLines; + for (int Line = 0, E = CurrentLines.size(); Line != E; ++Line) { + llvm::StringRef Contents = CurrentLines[Line]; + CurrentContentsToLine[Contents].push_back(Line); + } + } + // Translate diagnostic by moving its main range to new location (if inside + // the main file). Preserve all the notes and fixes that can be translated to + // new contents. + // Drops the whole diagnostic if main range can't be patched. + std::optional translateDiag(const Diag &D) { + Range NewRange = D.Range; + // Patch range if it's inside main file. + if (D.InsideMainFile && !translateRange(NewRange)) { + // Drop the diagnostic if we couldn't patch the range. + return std::nullopt; + } + + Diag NewD = D; + NewD.Range = NewRange; + // Translate ranges inside notes and fixes too, dropping the ones that are + // no longer relevant. + llvm::erase_if(NewD.Notes, [this](Note &N) { return !translateNote(N); }); + llvm::erase_if(NewD.Fixes, [this](Fix &F) { return !translateFix(F); }); + return NewD; + } +}; } // namespace std::shared_ptr @@ -612,6 +708,22 @@ void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) { } } +// Translate diagnostics from baseline into modified for the lines that have the +// same spelling. +static std::vector patchDiags(llvm::ArrayRef BaselineDiags, + const ScannedPreamble &BaselineScan, + const ScannedPreamble &ModifiedScan) { + std::vector PatchedDiags; + if (BaselineDiags.empty()) + return PatchedDiags; + DiagPatcher Patcher(BaselineScan.Lines, ModifiedScan.Lines); + for (auto &D : BaselineDiags) { + if (auto NewD = Patcher.translateDiag(D)) + PatchedDiags.emplace_back(std::move(*NewD)); + } + return PatchedDiags; +} + PreamblePatch PreamblePatch::create(llvm::StringRef FileName, const ParseInputs &Modified, const PreambleData &Baseline, @@ -629,7 +741,7 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, // there's nothing to do but generate an empty patch. auto BaselineScan = scanPreamble( // Contents needs to be null-terminated. - Baseline.Preamble.getContents().str(), Modified.CompileCommand); + Baseline.Preamble.getContents(), Modified.CompileCommand); if (!BaselineScan) { elog("Failed to scan baseline of {0}: {1}", FileName, BaselineScan.takeError()); @@ -730,6 +842,8 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, Patch << TD.Text << '\n'; } } + + PP.PatchedDiags = patchDiags(Baseline.Diags, *BaselineScan, *ModifiedScan); dlog("Created preamble patch: {0}", Patch.str()); Patch.flush(); return PP; @@ -771,6 +885,7 @@ PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) { PreamblePatch PP; PP.PreambleIncludes = Preamble.Includes.MainFileIncludes; PP.ModifiedBounds = Preamble.Preamble.getBounds(); + PP.PatchedDiags = Preamble.Diags; return PP; } diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h index 8bc65331f3e07..b5a5c107ab48a 100644 --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -34,6 +34,7 @@ #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Lex/Lexer.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/StringRef.h" #include @@ -157,6 +158,9 @@ class PreamblePatch { /// Whether diagnostics generated using this patch are trustable. bool preserveDiagnostics() const; + /// Returns diag locations for Modified contents. + llvm::ArrayRef patchedDiags() const { return PatchedDiags; } + static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h"; private: @@ -168,9 +172,11 @@ class PreamblePatch { 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 Baseline and Modified. Used for + // patching includes of baseline preamble. std::vector PreambleIncludes; + // Diags that were attached to a line preserved in Modified contents. + std::vector PatchedDiags; PreambleBounds ModifiedBounds = {0, false}; }; diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp index 2425e430ae6e0..94870abf1569b 100644 --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -723,9 +723,8 @@ TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) { #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"))); + ElementsAre(Diag(NewCode.range(), "pp_file_not_found"))); } { // Check with removals from preamble. @@ -734,18 +733,15 @@ TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) { #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"))); + ElementsAre(Diag(NewCode.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"))); + EXPECT_THAT(*AST->getDiagnostics(), IsEmpty()); } { // Picks closest line in case of multiple alternatives. @@ -756,18 +752,79 @@ TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) { #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"))); + ElementsAre(Diag(NewCode.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(), IsEmpty()); + } + { + // Multiple lines. + Annotations Code(R"( +#define BAR +#include [[]])"); + Annotations NewCode(R"(#include [[]])"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); EXPECT_THAT(*AST->getDiagnostics(), - ElementsAre(Diag(Code.range(), "pp_file_not_found"))); + ElementsAre(Diag(NewCode.range(), "pp_file_not_found"))); + } + { + // Multiple lines with change. + Annotations Code(R"( +#define BAR +#include +#include [[]])"); + Annotations NewCode(R"(#include )"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + EXPECT_THAT(*AST->getDiagnostics(), IsEmpty()); + } + { + // Preserves notes. + Annotations Code(R"( +#define $note[[BAR]] 1 +#define $main[[BAR]] 2)"); + Annotations NewCode(R"( +#define BAZ 0 +#define $note[[BAR]] 1 +#define BAZ 0 +#define $main[[BAR]] 2)"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + EXPECT_THAT( + *AST->getDiagnostics(), + ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"), + withNote(Diag(NewCode.range("note")))))); + } + { + // Preserves diag without note. + Annotations Code(R"( +#define $note[[BAR]] 1 +#define $main[[BAR]] 2)"); + Annotations NewCode(R"( +#define $main[[BAR]] 2)"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + EXPECT_THAT( + *AST->getDiagnostics(), + ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"), + Field(&Diag::Notes, IsEmpty())))); + } + { + // Make sure orphaned notes are not promoted to diags. + Annotations Code(R"( +#define $note[[BAR]] 1 +#define $main[[BAR]] 2)"); + Annotations NewCode(R"( +#define BAZ 0 +#define BAR 1)"); + auto AST = createPatchedAST(Code.code(), NewCode.code()); + EXPECT_THAT(*AST->getDiagnostics(), IsEmpty()); } } } // namespace