[clangd] Replay preamble #includes to clang-tidy checks.
Summary: This is needed to correctly handle checks that use IncludeInserter, which is very common. I couldn't find a totally safe example of a check to enable for testing, I picked modernize-deprecated-headers which some will probably hate. We should get configuration working... This depends on D54691 which ensures our calls to getFile(open=false) don't break subsequent accesses via the FileManager. Reviewers: ilya-biryukov Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D54694 llvm-svn: 347298
This commit is contained in:
parent
2bebc3d060
commit
991e316126
|
@ -119,6 +119,105 @@ private:
|
|||
SourceManager *SourceMgr = nullptr;
|
||||
};
|
||||
|
||||
// When using a preamble, only preprocessor events outside its bounds are seen.
|
||||
// This is almost what we want: replaying transitive preprocessing wastes time.
|
||||
// However this confuses clang-tidy checks: they don't see any #includes!
|
||||
// So we replay the *non-transitive* #includes that appear in the main-file.
|
||||
// It would be nice to replay other events (macro definitions, ifdefs etc) but
|
||||
// this addresses the most common cases fairly cheaply.
|
||||
class ReplayPreamble : private PPCallbacks {
|
||||
public:
|
||||
// Attach preprocessor hooks such that preamble events will be injected at
|
||||
// the appropriate time.
|
||||
// Events will be delivered to the *currently registered* PP callbacks.
|
||||
static void attach(const IncludeStructure &Includes,
|
||||
CompilerInstance &Clang) {
|
||||
auto &PP = Clang.getPreprocessor();
|
||||
auto *ExistingCallbacks = PP.getPPCallbacks();
|
||||
PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(
|
||||
new ReplayPreamble(Includes, ExistingCallbacks,
|
||||
Clang.getSourceManager(), PP, Clang.getLangOpts())));
|
||||
// We're relying on the fact that addPPCallbacks keeps the old PPCallbacks
|
||||
// around, creating a chaining wrapper. Guard against other implementations.
|
||||
assert(PP.getPPCallbacks() != ExistingCallbacks &&
|
||||
"Expected chaining implementation");
|
||||
}
|
||||
|
||||
private:
|
||||
ReplayPreamble(const IncludeStructure &Includes, PPCallbacks *Delegate,
|
||||
const SourceManager &SM, Preprocessor &PP,
|
||||
const LangOptions &LangOpts)
|
||||
: Includes(Includes), Delegate(Delegate), SM(SM), PP(PP),
|
||||
LangOpts(LangOpts) {}
|
||||
|
||||
// In a normal compile, the preamble traverses the following structure:
|
||||
//
|
||||
// mainfile.cpp
|
||||
// <built-in>
|
||||
// ... macro definitions like __cplusplus ...
|
||||
// <command-line>
|
||||
// ... macro definitions for args like -Dfoo=bar ...
|
||||
// "header1.h"
|
||||
// ... header file contents ...
|
||||
// "header2.h"
|
||||
// ... header file contents ...
|
||||
// ... main file contents ...
|
||||
//
|
||||
// When using a preamble, the "header1" and "header2" subtrees get skipped.
|
||||
// We insert them right after the built-in header, which still appears.
|
||||
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
|
||||
SrcMgr::CharacteristicKind Kind, FileID PrevFID) override {
|
||||
// It'd be nice if there was a better way to identify built-in headers...
|
||||
if (Reason == FileChangeReason::ExitFile &&
|
||||
SM.getBuffer(PrevFID)->getBufferIdentifier() == "<built-in>")
|
||||
replay();
|
||||
}
|
||||
|
||||
void replay() {
|
||||
for (const auto &Inc : Includes.MainFileIncludes) {
|
||||
const FileEntry *File = nullptr;
|
||||
if (Inc.Resolved != "")
|
||||
File = SM.getFileManager().getFile(Inc.Resolved);
|
||||
|
||||
StringRef WrittenFilename =
|
||||
StringRef(Inc.Written).drop_front().drop_back();
|
||||
bool Angled = StringRef(Inc.Written).startswith("<");
|
||||
|
||||
// Re-lex the #include directive to find its interesting parts.
|
||||
StringRef Src = SM.getBufferData(SM.getMainFileID());
|
||||
Lexer RawLexer(SM.getLocForStartOfFile(SM.getMainFileID()), LangOpts,
|
||||
Src.begin(), Src.begin() + Inc.HashOffset, Src.end());
|
||||
Token HashTok, IncludeTok, FilenameTok;
|
||||
RawLexer.LexFromRawLexer(HashTok);
|
||||
assert(HashTok.getKind() == tok::hash);
|
||||
RawLexer.setParsingPreprocessorDirective(true);
|
||||
RawLexer.LexFromRawLexer(IncludeTok);
|
||||
IdentifierInfo *II = PP.getIdentifierInfo(IncludeTok.getRawIdentifier());
|
||||
IncludeTok.setIdentifierInfo(II);
|
||||
IncludeTok.setKind(II->getTokenID());
|
||||
RawLexer.LexIncludeFilename(FilenameTok);
|
||||
|
||||
Delegate->InclusionDirective(
|
||||
HashTok.getLocation(), IncludeTok, WrittenFilename, Angled,
|
||||
CharSourceRange::getCharRange(FilenameTok.getLocation(),
|
||||
FilenameTok.getEndLoc()),
|
||||
File, "SearchPath", "RelPath", /*Imported=*/nullptr, Inc.FileKind);
|
||||
if (File)
|
||||
Delegate->FileSkipped(*File, FilenameTok, Inc.FileKind);
|
||||
else {
|
||||
SmallString<1> UnusedRecovery;
|
||||
Delegate->FileNotFound(WrittenFilename, UnusedRecovery);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const IncludeStructure &Includes;
|
||||
PPCallbacks *Delegate;
|
||||
const SourceManager &SM;
|
||||
Preprocessor &PP;
|
||||
const LangOptions &LangOpts;
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
void dumpAST(ParsedAST &AST, raw_ostream &OS) {
|
||||
|
@ -171,8 +270,9 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
|
|||
// FIXME: this needs to be configurable, and we need to support .clang-tidy
|
||||
// files and other options providers.
|
||||
// These checks exercise the matcher- and preprocessor-based hooks.
|
||||
CTOpts.Checks =
|
||||
"bugprone-sizeof-expression,bugprone-macro-repeated-side-effects";
|
||||
CTOpts.Checks = "bugprone-sizeof-expression,"
|
||||
"bugprone-macro-repeated-side-effects,"
|
||||
"modernize-deprecated-headers";
|
||||
CTContext.emplace(llvm::make_unique<tidy::DefaultOptionsProvider>(
|
||||
tidy::ClangTidyGlobalOptions(), CTOpts));
|
||||
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
|
||||
|
@ -190,6 +290,12 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
|
|||
// Copy over the includes from the preamble, then combine with the
|
||||
// non-preamble includes below.
|
||||
auto Includes = Preamble ? Preamble->Includes : IncludeStructure{};
|
||||
// Replay the preamble includes so that clang-tidy checks can see them.
|
||||
if (Preamble)
|
||||
ReplayPreamble::attach(Includes, *Clang);
|
||||
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
|
||||
// Otherwise we would collect the replayed includes again...
|
||||
// (We can't *just* use the replayed includes, they don't have Resolved path).
|
||||
Clang->getPreprocessor().addPPCallbacks(
|
||||
collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
|
||||
|
||||
|
|
|
@ -190,8 +190,11 @@ std::string noteMessage(const Diag &Main, const DiagBase &Note) {
|
|||
} // namespace
|
||||
|
||||
raw_ostream &operator<<(raw_ostream &OS, const DiagBase &D) {
|
||||
OS << "[";
|
||||
if (!D.InsideMainFile)
|
||||
OS << "[in " << D.File << "] ";
|
||||
OS << D.File << ":";
|
||||
OS << D.Range.start << "-" << D.Range.end << "] ";
|
||||
|
||||
return OS << D.Message;
|
||||
}
|
||||
|
||||
|
|
|
@ -34,13 +34,17 @@ public:
|
|||
CharSourceRange FilenameRange, const FileEntry *File,
|
||||
StringRef /*SearchPath*/, StringRef /*RelativePath*/,
|
||||
const Module * /*Imported*/,
|
||||
SrcMgr::CharacteristicKind /*FileType*/) override {
|
||||
if (SM.isInMainFile(HashLoc))
|
||||
Out->MainFileIncludes.push_back({
|
||||
halfOpenToRange(SM, FilenameRange),
|
||||
(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str(),
|
||||
File ? File->tryGetRealPathName() : "",
|
||||
});
|
||||
SrcMgr::CharacteristicKind FileKind) override {
|
||||
if (SM.isWrittenInMainFile(HashLoc)) {
|
||||
Out->MainFileIncludes.emplace_back();
|
||||
auto &Inc = Out->MainFileIncludes.back();
|
||||
Inc.R = halfOpenToRange(SM, FilenameRange);
|
||||
Inc.Written =
|
||||
(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
|
||||
Inc.Resolved = File ? File->tryGetRealPathName() : "";
|
||||
Inc.HashOffset = SM.getFileOffset(HashLoc);
|
||||
Inc.FileKind = FileKind;
|
||||
}
|
||||
if (File) {
|
||||
auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
|
||||
if (!IncludingFileEntry) {
|
||||
|
@ -168,5 +172,11 @@ Optional<TextEdit> IncludeInserter::insert(StringRef VerbatimHeader) const {
|
|||
return Edit;
|
||||
}
|
||||
|
||||
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Inclusion &Inc) {
|
||||
return OS << Inc.Written << " = "
|
||||
<< (Inc.Resolved.empty() ? Inc.Resolved : "[unresolved]") << " at "
|
||||
<< Inc.R;
|
||||
}
|
||||
|
||||
} // namespace clangd
|
||||
} // namespace clang
|
||||
|
|
|
@ -43,7 +43,10 @@ struct Inclusion {
|
|||
Range R; // Inclusion range.
|
||||
std::string Written; // Inclusion name as written e.g. <vector>.
|
||||
Path Resolved; // Resolved path of included file. Empty if not resolved.
|
||||
unsigned HashOffset = 0; // Byte offset from start of file to #.
|
||||
SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
|
||||
};
|
||||
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion&);
|
||||
|
||||
// Information captured about the inclusion graph in a translation unit.
|
||||
// This includes detailed information about the direct #includes, and summary
|
||||
|
|
|
@ -131,18 +131,27 @@ TEST(DiagnosticsTest, FlagsMatter) {
|
|||
|
||||
TEST(DiagnosticsTest, ClangTidy) {
|
||||
Annotations Test(R"cpp(
|
||||
#include $deprecated[["assert.h"]]
|
||||
|
||||
#define $macrodef[[SQUARE]](X) (X)*(X)
|
||||
int main() {
|
||||
return [[sizeof]](sizeof(int));
|
||||
return $doubled[[sizeof]](sizeof(int));
|
||||
int y = 4;
|
||||
return SQUARE($macroarg[[++]]y);
|
||||
}
|
||||
)cpp");
|
||||
auto TU = TestTU::withCode(Test.code());
|
||||
TU.HeaderFilename = "assert.h"; // Suppress "not found" error.
|
||||
EXPECT_THAT(
|
||||
TU.build().getDiagnostics(),
|
||||
UnorderedElementsAre(
|
||||
Diag(Test.range(), "suspicious usage of 'sizeof(sizeof(...))' "
|
||||
AllOf(Diag(Test.range("deprecated"),
|
||||
"inclusion of deprecated C++ header 'assert.h'; consider "
|
||||
"using 'cassert' instead [modernize-deprecated-headers]"),
|
||||
WithFix(Fix(Test.range("deprecated"), "<cassert>",
|
||||
"change '\"assert.h\"' to '<cassert>'"))),
|
||||
Diag(Test.range("doubled"),
|
||||
"suspicious usage of 'sizeof(sizeof(...))' "
|
||||
"[bugprone-sizeof-expression]"),
|
||||
AllOf(
|
||||
Diag(Test.range("macroarg"),
|
||||
|
|
|
@ -11,6 +11,7 @@
|
|||
|
||||
#include "Compiler.h"
|
||||
#include "TestFS.h"
|
||||
#include "TestTU.h"
|
||||
#include "clang/Frontend/CompilerInstance.h"
|
||||
#include "clang/Frontend/CompilerInvocation.h"
|
||||
#include "clang/Frontend/FrontendActions.h"
|
||||
|
@ -24,6 +25,7 @@ namespace clangd {
|
|||
namespace {
|
||||
|
||||
using ::testing::AllOf;
|
||||
using ::testing::ElementsAre;
|
||||
using ::testing::UnorderedElementsAre;
|
||||
|
||||
class HeadersTest : public ::testing::Test {
|
||||
|
@ -132,6 +134,7 @@ protected:
|
|||
|
||||
MATCHER_P(Written, Name, "") { return arg.Written == Name; }
|
||||
MATCHER_P(Resolved, Name, "") { return arg.Resolved == Name; }
|
||||
MATCHER_P(IncludeLine, N, "") { return arg.R.start.line == N; }
|
||||
|
||||
MATCHER_P2(Distance, File, D, "") {
|
||||
if (arg.getKey() != File)
|
||||
|
@ -179,6 +182,21 @@ TEST_F(HeadersTest, OnlyCollectInclusionsInMain) {
|
|||
Distance(testPath("sub/baz.h"), 1u)));
|
||||
}
|
||||
|
||||
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
|
||||
// We use TestTU here, to ensure we use the preamble replay logic.
|
||||
// We're testing that the logic doesn't crash, and doesn't result in duplicate
|
||||
// includes. (We'd test more directly, but it's pretty well encapsulated!)
|
||||
auto TU = TestTU::withCode(R"cpp(
|
||||
#include "a.h"
|
||||
#include "a.h"
|
||||
void foo();
|
||||
#include "a.h"
|
||||
)cpp");
|
||||
TU.HeaderFilename = "a.h"; // suppress "not found".
|
||||
EXPECT_THAT(TU.build().getIncludeStructure().MainFileIncludes,
|
||||
ElementsAre(IncludeLine(1), IncludeLine(2), IncludeLine(4)));
|
||||
}
|
||||
|
||||
TEST_F(HeadersTest, UnResolvedInclusion) {
|
||||
FS.Files[MainFile] = R"cpp(
|
||||
#include "foo.h"
|
||||
|
@ -220,19 +238,20 @@ TEST_F(HeadersTest, PreferredHeader) {
|
|||
}
|
||||
|
||||
TEST_F(HeadersTest, DontInsertDuplicatePreferred) {
|
||||
std::vector<Inclusion> Inclusions = {
|
||||
{Range(), /*Written*/ "\"bar.h\"", /*Resolved*/ ""}};
|
||||
EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", Inclusions), "");
|
||||
EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", Inclusions), "");
|
||||
Inclusion Inc;
|
||||
Inc.Written = "\"bar.h\"";
|
||||
Inc.Resolved = "";
|
||||
EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", {Inc}), "");
|
||||
EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", {Inc}), "");
|
||||
}
|
||||
|
||||
TEST_F(HeadersTest, DontInsertDuplicateResolved) {
|
||||
std::string BarHeader = testPath("sub/bar.h");
|
||||
std::vector<Inclusion> Inclusions = {
|
||||
{Range(), /*Written*/ "fake-bar.h", /*Resolved*/ BarHeader}};
|
||||
EXPECT_EQ(calculate(BarHeader, "", Inclusions), "");
|
||||
Inclusion Inc;
|
||||
Inc.Written = "fake-bar.h";
|
||||
Inc.Resolved = testPath("sub/bar.h");
|
||||
EXPECT_EQ(calculate(Inc.Resolved, "", {Inc}), "");
|
||||
// Do not insert preferred.
|
||||
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
|
||||
EXPECT_EQ(calculate(Inc.Resolved, "\"BAR.h\"", {Inc}), "");
|
||||
}
|
||||
|
||||
TEST_F(HeadersTest, PreferInserted) {
|
||||
|
|
Loading…
Reference in New Issue