Skip to content

Commit

Permalink
[libTooling] Move RewriteRule include edits to ASTEdit granularity.
Browse files Browse the repository at this point in the history 
Currently, changes to includes are applied to an entire rule. However,
include changes may be specific to particular edits within a rule (for example,
they may apply to one file but not another). Also, include changes may need to
carry metadata, just like other changes. So, we make include changes first-class
edits.

Reviewed By: tdl-g

Differential Revision: https://reviews.llvm.org/D85734
  • Loading branch information
@ymand
ymand committed Aug 11, 2020
1 parent d110d4a commit d8c1f43
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 50 deletions.
27 changes: 14 additions & 13 deletions clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
View file
Expand Up @@ -53,12 +53,7 @@ TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,

void TransformerClangTidyCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
// Only register the IncludeInsert when some `Case` will add
// includes.
if (Rule && llvm::any_of(Rule->Cases, [](const RewriteRule::Case &C) {
return !C.AddedIncludes.empty();
}))
Inserter.registerPreprocessor(PP);
Inserter.registerPreprocessor(PP);
}

void TransformerClangTidyCheck::registerMatchers(
Expand Down Expand Up @@ -96,13 +91,19 @@ void TransformerClangTidyCheck::check(
// Associate the diagnostic with the location of the first change.
DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
for (const auto &T : *Edits)
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);

for (const auto &I : Case.AddedIncludes) {
Diag << Inserter.createMainFileIncludeInsertion(
I.first,
/*IsAngled=*/I.second == transformer::IncludeFormat::Angled);
}
switch (T.Kind) {
case transformer::EditKind::Range:
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
break;
case transformer::EditKind::AddInclude: {
StringRef FileName = T.Replacement;
bool IsAngled = FileName.startswith("<") && FileName.endswith(">");
Diag << Inserter.createMainFileIncludeInsertion(
IsAngled ? FileName.substr(1, FileName.size() - 2) : FileName,
IsAngled);
break;
}
}
}

void TransformerClangTidyCheck::storeOptions(
Expand Down
39 changes: 29 additions & 10 deletions clang/include/clang/Tooling/Transformer/RewriteRule.h
View file
Expand Up @@ -31,14 +31,30 @@

namespace clang {
namespace transformer {
// Specifies how to interpret an edit.
enum class EditKind {
// Edits a source range in the file.
Range,
// Inserts an include in the file. The `Replacement` field is the name of the
// newly included file.
AddInclude,
};

/// A concrete description of a source edit, represented by a character range in
/// the source to be replaced and a corresponding replacement string.
struct Edit {
EditKind Kind = EditKind::Range;
CharSourceRange Range;
std::string Replacement;
llvm::Any Metadata;
};

/// Format of the path in an include directive -- angle brackets or quotes.
enum class IncludeFormat {
Quoted,
Angled,
};

/// Maps a match result to a list of concrete edits (with possible
/// failure). This type is a building block of rewrite rules, but users will
/// generally work in terms of `ASTEdit`s (below) rather than directly in terms
Expand Down Expand Up @@ -86,6 +102,7 @@ using AnyGenerator = MatchConsumer<llvm::Any>;
// changeTo(cat("different_expr"))
// \endcode
struct ASTEdit {
EditKind Kind = EditKind::Range;
RangeSelector TargetRange;
TextGenerator Replacement;
TextGenerator Note;
Expand Down Expand Up @@ -185,6 +202,18 @@ inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
/// Removes the source selected by \p S.
ASTEdit remove(RangeSelector S);

/// Adds an include directive for the given header to the file of `Target`. The
/// particular location specified by `Target` is ignored.
ASTEdit addInclude(RangeSelector Target, StringRef Header,
IncludeFormat Format = IncludeFormat::Quoted);

/// Adds an include directive for the given header to the file associated with
/// `RootID`.
inline ASTEdit addInclude(StringRef Header,
IncludeFormat Format = IncludeFormat::Quoted) {
return addInclude(node(RootID), Header, Format);
}

// FIXME: If `Metadata` returns an `llvm::Expected<T>` the `AnyGenerator` will
// construct an `llvm::Expected<llvm::Any>` where no error is present but the
// `llvm::Any` holds the error. This is unlikely but potentially surprising.
Expand Down Expand Up @@ -216,12 +245,6 @@ inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) {
remove(enclose(after(inner), after(outer)))});
}

/// Format of the path in an include directive -- angle brackets or quotes.
enum class IncludeFormat {
Quoted,
Angled,
};

/// Description of a source-code transformation.
//
// A *rewrite rule* describes a transformation of source code. A simple rule
Expand Down Expand Up @@ -250,10 +273,6 @@ struct RewriteRule {
ast_matchers::internal::DynTypedMatcher Matcher;
EditGenerator Edits;
TextGenerator Explanation;
/// Include paths to add to the file affected by this case. These are
/// bundled with the `Case`, rather than the `RewriteRule`, because each
/// case might have different associated changes to the includes.
std::vector<std::pair<std::string, IncludeFormat>> AddedIncludes;
};
// We expect RewriteRules will most commonly include only one case.
SmallVector<Case, 1> Cases;
Expand Down
31 changes: 27 additions & 4 deletions clang/lib/Tooling/Transformer/RewriteRule.cpp
View file
Expand Up @@ -52,6 +52,7 @@ translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
if (!Metadata)
return Metadata.takeError();
transformer::Edit T;
T.Kind = E.Kind;
T.Range = *EditRange;
T.Replacement = std::move(*Replacement);
T.Metadata = std::move(*Metadata);
Expand Down Expand Up @@ -115,14 +116,36 @@ class SimpleTextGenerator : public MatchComputation<std::string> {
};
} // namespace

static TextGenerator makeText(std::string S) {
return std::make_shared<SimpleTextGenerator>(std::move(S));
}

ASTEdit transformer::remove(RangeSelector S) {
return change(std::move(S), std::make_shared<SimpleTextGenerator>(""));
return change(std::move(S), makeText(""));
}

static std::string formatHeaderPath(StringRef Header, IncludeFormat Format) {
switch (Format) {
case transformer::IncludeFormat::Quoted:
return Header.str();
case transformer::IncludeFormat::Angled:
return ("<" + Header + ">").str();
}
}

ASTEdit transformer::addInclude(RangeSelector Target, StringRef Header,
IncludeFormat Format) {
ASTEdit E;
E.Kind = EditKind::AddInclude;
E.TargetRange = Target;
E.Replacement = makeText(formatHeaderPath(Header, Format));
return E;
}

RewriteRule transformer::makeRule(DynTypedMatcher M, EditGenerator Edits,
TextGenerator Explanation) {
return RewriteRule{{RewriteRule::Case{
std::move(M), std::move(Edits), std::move(Explanation), {}}}};
return RewriteRule{{RewriteRule::Case{std::move(M), std::move(Edits),
std::move(Explanation)}}};
}

namespace {
Expand Down Expand Up @@ -258,7 +281,7 @@ EditGenerator transformer::rewriteDescendants(std::string NodeId,
void transformer::addInclude(RewriteRule &Rule, StringRef Header,
IncludeFormat Format) {
for (auto &Case : Rule.Cases)
Case.AddedIncludes.emplace_back(Header.str(), Format);
Case.Edits = flatten(std::move(Case.Edits), addInclude(Header, Format));
}

#ifndef NDEBUG
Expand Down
35 changes: 13 additions & 22 deletions clang/lib/Tooling/Transformer/Transformer.cpp
View file
Expand Up @@ -51,29 +51,20 @@ void Transformer::run(const MatchFinder::MatchResult &Result) {
T.Range.getBegin(), T.Metadata))
.first;
auto &AC = Iter->second;
if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Consumer(std::move(Err));
return;
}
}

for (auto &IDChangePair : ChangesByFileID) {
auto &AC = IDChangePair.second;
// FIXME: this will add includes to *all* changed files, which may not be
// the intent. We should upgrade the representation to allow associating
// headers with specific edits.
for (const auto &I : Case.AddedIncludes) {
auto &Header = I.first;
switch (I.second) {
case transformer::IncludeFormat::Quoted:
AC.addHeader(Header);
break;
case transformer::IncludeFormat::Angled:
AC.addHeader((llvm::Twine("<") + Header + ">").str());
break;
switch (T.Kind) {
case transformer::EditKind::Range:
if (auto Err =
AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Consumer(std::move(Err));
return;
}
break;
case transformer::EditKind::AddInclude:
AC.addHeader(T.Replacement);
break;
}

Consumer(std::move(AC));
}

for (auto &IDChangePair : ChangesByFileID)
Consumer(std::move(IDChangePair.second));
}
68 changes: 67 additions & 1 deletion clang/unittests/Tooling/TransformerTest.cpp
View file
Expand Up @@ -21,6 +21,7 @@ using namespace clang;
using namespace tooling;
using namespace ast_matchers;
namespace {
using ::testing::ElementsAre;
using ::testing::IsEmpty;
using transformer::cat;
using transformer::changeTo;
Expand Down Expand Up @@ -194,6 +195,43 @@ TEST_F(TransformerTest, Flag) {
}

TEST_F(TransformerTest, AddIncludeQuoted) {
RewriteRule Rule =
makeRule(callExpr(callee(functionDecl(hasName("f")))),
{addInclude("clang/OtherLib.h"), changeTo(cat("other()"))});

std::string Input = R"cc(
int f(int x);
int h(int x) { return f(x); }
)cc";
std::string Expected = R"cc(#include "clang/OtherLib.h"
int f(int x);
int h(int x) { return other(); }
)cc";

testRule(Rule, Input, Expected);
}

TEST_F(TransformerTest, AddIncludeAngled) {
RewriteRule Rule = makeRule(
callExpr(callee(functionDecl(hasName("f")))),
{addInclude("clang/OtherLib.h", transformer::IncludeFormat::Angled),
changeTo(cat("other()"))});

std::string Input = R"cc(
int f(int x);
int h(int x) { return f(x); }
)cc";
std::string Expected = R"cc(#include <clang/OtherLib.h>
int f(int x);
int h(int x) { return other(); }
)cc";

testRule(Rule, Input, Expected);
}

TEST_F(TransformerTest, AddIncludeQuotedForRule) {
RewriteRule Rule = makeRule(callExpr(callee(functionDecl(hasName("f")))),
changeTo(cat("other()")));
addInclude(Rule, "clang/OtherLib.h");
Expand All @@ -211,7 +249,7 @@ TEST_F(TransformerTest, AddIncludeQuoted) {
testRule(Rule, Input, Expected);
}

TEST_F(TransformerTest, AddIncludeAngled) {
TEST_F(TransformerTest, AddIncludeAngledForRule) {
RewriteRule Rule = makeRule(callExpr(callee(functionDecl(hasName("f")))),
changeTo(cat("other()")));
addInclude(Rule, "clang/OtherLib.h", transformer::IncludeFormat::Angled);
Expand Down Expand Up @@ -1180,4 +1218,32 @@ TEST_F(TransformerTest, MultipleFiles) {
EXPECT_EQ(format(*UpdatedCode), format(R"cc(#include "input.h"
;)cc"));
}

TEST_F(TransformerTest, AddIncludeMultipleFiles) {
std::string Header = R"cc(void RemoveThisFunction();)cc";
std::string Source = R"cc(#include "input.h"
void Foo() {RemoveThisFunction();})cc";
Transformer T(
makeRule(callExpr(callee(
functionDecl(hasName("RemoveThisFunction")).bind("fun"))),
addInclude(node("fun"), "header.h")),
consumer());
T.registerMatchers(&MatchFinder);
auto Factory = newFrontendActionFactory(&MatchFinder);
EXPECT_TRUE(runToolOnCodeWithArgs(
Factory->create(), Source, std::vector<std::string>(), "input.cc",
"clang-tool", std::make_shared<PCHContainerOperations>(),
{{"input.h", Header}}));

ASSERT_EQ(Changes.size(), 1U);
ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
EXPECT_THAT(Changes[0].getInsertedHeaders(), ElementsAre("header.h"));
EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
llvm::Expected<std::string> UpdatedCode =
clang::tooling::applyAllReplacements(Header,
Changes[0].getReplacements());
ASSERT_TRUE(static_cast<bool>(UpdatedCode))
<< "Could not update code: " << llvm::toString(UpdatedCode.takeError());
EXPECT_EQ(format(*UpdatedCode), format(Header));
}
} // namespace

0 comments on commit d8c1f43

Please sign in to comment.