kadircet created this revision.
kadircet added reviewers: sammccall, hokein.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Diagnostics emitted while parsing main-file is easy, as they already
see new source locations. We just need to make sure the ones that can be
attributed to preamble section of the file is patched. That's achieved in two
ways:
- New include is actually patched with a #line-directive, so most clang
features already point at the right line.
- Patching of main-file-includes inside the ParsedAST, which is used by replay
preamble and include-cleaner analysis. This patch fixes/improve patching in
this case.
The second problem is patching includes that were published when building the
preamble. Unfortunately because we don't have the full edit history in place,
it's hard to perform an exact translation. This patch tries to just preserve
diagnostics that are contained to lines containing #include or #define
directives and moves them around with the new file contents. It's granularity is
per-line, it doesn't handle column-wide changes.
Depends on D142890 <https://reviews.llvm.org/D142890>.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D142892
Files:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
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,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 <memory>
@@ -30,9 +33,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 +198,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 +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<std::string> 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 [[<foo>]]\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 <bar>\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 <bar>\n#include <baz>\n");
+ auto PatchedDiags =
+ *createPatchedAST(Code.code(), NewCode.code())->getDiagnostics();
+ EXPECT_THAT(PatchedDiags, IsEmpty());
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ 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<Diag> 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<Inclusion> PreambleIncludes;
+ // Diags that were attached to an #include or a #define directive.
+ std::vector<Diag> PatchedDiags;
PreambleBounds ModifiedBounds = {0, false};
};
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ 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 <iterator>
+#include <map>
#include <memory>
#include <string>
#include <system_error>
@@ -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<const PreambleData>
@@ -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<int, llvm::SmallVector<const Diag *>>
+mapDiagsToLines(llvm::ArrayRef<Diag> Diags) {
+ llvm::DenseMap<int, llvm::SmallVector<const Diag *>> LineToDiags;
+ for (auto &D : Diags) {
+ if (diagReferencesMultipleLines(D))
+ continue;
+ LineToDiags[D.Range.start.line].emplace_back(&D);
+ }
+ return LineToDiags;
+}
+
+static std::map<IncludeKey, const Inclusion *>
+getExistingIncludes(const ScannedPreamble &BaselineScan,
+ llvm::ArrayRef<Inclusion> MFI) {
+ std::map<IncludeKey, const Inclusion *> 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<IncludeKey, int> IncludeToPatchedLine;
if (IncludesChanged && PatchType == PatchType::All) {
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
- llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
- /*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<int> 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<Diag> PreamblePatch::patchedDiags() const { return PatchedDiags; }
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ 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<Diag> D = ASTDiags.take(&*CTContext);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
